From a12200fbb106aeda7cd11b6b5504cf42c1300971 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Thu, 16 Nov 2023 21:34:07 +0100 Subject: [PATCH] fix: only parse payload once (#921) --- src/middleware/node/get-payload.ts | 12 +----------- src/verify-and-receive.ts | 15 +++++++++++++-- test/integration/node-middleware.test.ts | 6 ++++-- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/middleware/node/get-payload.ts b/src/middleware/node/get-payload.ts index 2856056e..9d1d4909 100644 --- a/src/middleware/node/get-payload.ts +++ b/src/middleware/node/get-payload.ts @@ -25,16 +25,6 @@ export function getPayload(request: IncomingMessage): Promise { // istanbul ignore next request.on("error", (error: Error) => reject(new AggregateError([error]))); request.on("data", (chunk: string) => (data += chunk)); - request.on("end", () => { - try { - // Call JSON.parse() only to check if the payload is valid JSON - JSON.parse(data); - resolve(data); - } catch (error: any) { - error.message = "Invalid JSON"; - error.status = 400; - reject(new AggregateError([error])); - } - }); + request.on("end", () => resolve(data)); }); } diff --git a/src/verify-and-receive.ts b/src/verify-and-receive.ts index 2165ab2b..4a1230ab 100644 --- a/src/verify-and-receive.ts +++ b/src/verify-and-receive.ts @@ -1,6 +1,8 @@ +import AggregateError from "aggregate-error"; import { verify } from "@octokit/webhooks-methods"; import type { + EmitterWebhookEvent, EmitterWebhookEventWithStringPayloadAndSignature, State, } from "./types"; @@ -8,7 +10,7 @@ import type { export async function verifyAndReceive( state: State & { secret: string }, event: EmitterWebhookEventWithStringPayloadAndSignature, -): Promise { +): Promise { // verify will validate that the secret is not undefined const matchesSignature = await verify( state.secret, @@ -26,9 +28,18 @@ export async function verifyAndReceive( ); } + let payload: EmitterWebhookEvent["payload"]; + try { + payload = JSON.parse(event.payload); + } catch (error: any) { + error.message = "Invalid JSON"; + error.status = 400; + throw new AggregateError([error]); + } + return state.eventHandler.receive({ id: event.id, name: event.name, - payload: JSON.parse(event.payload), + payload, }); } diff --git a/test/integration/node-middleware.test.ts b/test/integration/node-middleware.test.ts index 51d7eb13..e2500807 100644 --- a/test/integration/node-middleware.test.ts +++ b/test/integration/node-middleware.test.ts @@ -178,6 +178,8 @@ describe("createNodeMiddleware(webhooks)", () => { // @ts-expect-error complains about { port } although it's included in returned AddressInfo interface const { port } = server.address(); + const payload = '{"name":"invalid"'; + const response = await fetch( `http://localhost:${port}/api/github/webhooks`, { @@ -186,9 +188,9 @@ describe("createNodeMiddleware(webhooks)", () => { "Content-Type": "application/json", "X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000", "X-GitHub-Event": "push", - "X-Hub-Signature-256": signatureSha256, + "X-Hub-Signature-256": await sign("mySecret", payload), }, - body: "invalid", + body: payload, }, );