From 99f265a5fd0f3e9a36f2cf61aca4177e85c4d537 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Wed, 29 Nov 2023 20:09:14 +0100 Subject: [PATCH 01/12] fix: handle gcf, verify already provided request.body --- src/middleware/node/get-payload.ts | 13 ++----- src/middleware/node/middleware.ts | 18 ++++++++- src/types.ts | 3 +- src/verify-and-receive.ts | 28 ++++++++++---- test/integration/node-middleware.test.ts | 47 ++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 19 deletions(-) diff --git a/src/middleware/node/get-payload.ts b/src/middleware/node/get-payload.ts index 9d1d4909..3683d81c 100644 --- a/src/middleware/node/get-payload.ts +++ b/src/middleware/node/get-payload.ts @@ -11,20 +11,15 @@ import AggregateError from "aggregate-error"; // } type IncomingMessage = any; -export function getPayload(request: IncomingMessage): Promise { +export function getPayload(request: IncomingMessage): Promise { // If request.body already exists we can stop here // See https://github.com/octokit/webhooks.js/pull/23 - - if (request.body) return Promise.resolve(request.body); - return new Promise((resolve, reject) => { - let data = ""; - - request.setEncoding("utf8"); + let data: Buffer[] = []; // istanbul ignore next request.on("error", (error: Error) => reject(new AggregateError([error]))); - request.on("data", (chunk: string) => (data += chunk)); - request.on("end", () => resolve(data)); + request.on("data", (chunk: Buffer) => data.push(chunk)); + request.on("end", () => resolve(Buffer.concat(data))); }); } diff --git a/src/middleware/node/middleware.ts b/src/middleware/node/middleware.ts index 4947172d..cdf06703 100644 --- a/src/middleware/node/middleware.ts +++ b/src/middleware/node/middleware.ts @@ -93,12 +93,28 @@ export async function middleware( }, 9000).unref(); try { - const payload = await getPayload(request); + let payload: Buffer; + let body: { [key: string]: any } | undefined; + + const bodyType = typeof request.body; + + // The body is a String (e.g. Lambda) + if (bodyType === "string") { + payload = request.body; + // The body is already an Object and rawBody is a Buffer (e.g. GCF) + } else if (bodyType === "object" && request.rawBody instanceof Buffer) { + body = request.body; + payload = request.rawBody; + // We need to load the payload from the request (normal case of Node.js server) + } else { + payload = await getPayload(request); + } await webhooks.verifyAndReceive({ id: id, name: eventName as any, payload, + body, signature: signatureSHA256, }); clearTimeout(timeout); diff --git a/src/types.ts b/src/types.ts index e64b07ea..ec862288 100644 --- a/src/types.ts +++ b/src/types.ts @@ -18,7 +18,8 @@ export type EmitterWebhookEvent< export type EmitterWebhookEventWithStringPayloadAndSignature = { id: string; name: EmitterWebhookEventName; - payload: string; + body?: { [key: string]: any }; + payload: string | Buffer; signature: string; }; diff --git a/src/verify-and-receive.ts b/src/verify-and-receive.ts index 88f28083..d7646d9c 100644 --- a/src/verify-and-receive.ts +++ b/src/verify-and-receive.ts @@ -15,6 +15,7 @@ export async function verifyAndReceive( // verify will validate that the secret is not undefined const matchesSignature = await verify( state.secret, + // @ts-expect-error verify uses createHmac, which can take Strings and Buffers event.payload, event.signature, ).catch(() => false); @@ -29,18 +30,29 @@ export async function verifyAndReceive( ); } - let payload: EmitterWebhookEvent["payload"]; + // The body is already an Object (e.g. GCF) and can be passed directly to the EventHandler + if (event.body) { + return state.eventHandler.receive({ + id: event.id, + name: event.name, + payload: event.body as EmitterWebhookEvent["payload"], + }); + } + + const payload = + typeof event.payload === "string" + ? event.payload + : event.payload.toString("utf8"); + try { - payload = JSON.parse(event.payload); + return state.eventHandler.receive({ + id: event.id, + name: event.name, + payload: JSON.parse(payload) as EmitterWebhookEvent["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, - }); } diff --git a/test/integration/node-middleware.test.ts b/test/integration/node-middleware.test.ts index a5568a42..bc8f34f8 100644 --- a/test/integration/node-middleware.test.ts +++ b/test/integration/node-middleware.test.ts @@ -107,6 +107,53 @@ describe("createNodeMiddleware(webhooks)", () => { server.close(); }); + test("request.body is already an Object and has request.rawBody as Buffer (e.g. GCF)", async () => { + expect.assertions(3); + + const webhooks = new Webhooks({ + secret: "mySecret", + }); + const dataChunks: any[] = []; + const middleware = createNodeMiddleware(webhooks); + + const server = createServer((req, res) => { + req.once("data", (chunk) => dataChunks.push(chunk)); + req.once("end", () => { + // @ts-expect-error - TS2339: Property 'rawBody' does not exist on type 'IncomingMessage'. + req.rawBody = Buffer.concat(dataChunks); + // @ts-expect-error - TS2339: Property 'body' does not exist on type 'IncomingMessage'. + req.body = JSON.parse(req.rawBody); + middleware(req, res); + }); + }).listen(); + + webhooks.on("push", (event) => { + expect(event.id).toBe("123e4567-e89b-12d3-a456-426655440000"); + }); + + // @ts-expect-error complains about { port } although it's included in returned AddressInfo interface + const { port } = server.address(); + + const response = await fetch( + `http://localhost:${port}/api/github/webhooks`, + { + method: "POST", + headers: { + "Content-Type": "application/json", + "X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000", + "X-GitHub-Event": "push", + "X-Hub-Signature-256": signatureSha256, + }, + body: pushEventPayload, + }, + ); + + expect(response.status).toEqual(200); + expect(await response.text()).toEqual("ok\n"); + + server.close(); + }); + test("Handles invalid Content-Type", async () => { const webhooks = new Webhooks({ secret: "mySecret", From 206484575d3acf255f85746d4c251ea8be3ddc11 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Wed, 29 Nov 2023 20:37:49 +0100 Subject: [PATCH 02/12] fix pr remark --- src/middleware/node/get-payload.ts | 2 -- src/verify-and-receive.ts | 20 +++++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/middleware/node/get-payload.ts b/src/middleware/node/get-payload.ts index cdac1990..61eb28ae 100644 --- a/src/middleware/node/get-payload.ts +++ b/src/middleware/node/get-payload.ts @@ -12,8 +12,6 @@ import AggregateError from "aggregate-error"; type IncomingMessage = any; export function getPayload(request: IncomingMessage): Promise { - // If request.body already exists we can stop here - // See https://github.com/octokit/webhooks.js/pull/23 return new Promise((resolve, reject) => { let data: Buffer[] = []; diff --git a/src/verify-and-receive.ts b/src/verify-and-receive.ts index d7646d9c..1d3749dd 100644 --- a/src/verify-and-receive.ts +++ b/src/verify-and-receive.ts @@ -39,20 +39,22 @@ export async function verifyAndReceive( }); } - const payload = - typeof event.payload === "string" - ? event.payload - : event.payload.toString("utf8"); + let payload; try { - return state.eventHandler.receive({ - id: event.id, - name: event.name, - payload: JSON.parse(payload) as EmitterWebhookEvent["payload"], - }); + payload = + typeof event.payload === "string" + ? JSON.parse(event.payload) + : JSON.parse(event.payload.toString("utf8")); } 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, + }); } From b2685016b67b6a077a2fd7d33c9db2638a4641c5 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Wed, 29 Nov 2023 20:58:26 +0100 Subject: [PATCH 03/12] Update src/verify-and-receive.ts Co-authored-by: wolfy1339 <4595477+wolfy1339@users.noreply.github.com> --- src/verify-and-receive.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/verify-and-receive.ts b/src/verify-and-receive.ts index 1d3749dd..916099c7 100644 --- a/src/verify-and-receive.ts +++ b/src/verify-and-receive.ts @@ -39,7 +39,7 @@ export async function verifyAndReceive( }); } - let payload; + let payload: EmitterWebhookEvent["payload"]; try { payload = From 8fe7aa2925b6cf71f51db77e70c237caec4bd8b8 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 30 Nov 2023 00:09:31 +0100 Subject: [PATCH 04/12] use setImmediate to improve the throughput --- src/middleware/node/get-payload.ts | 7 +++++-- test/integration/node-middleware.test.ts | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/middleware/node/get-payload.ts b/src/middleware/node/get-payload.ts index 61eb28ae..6ca45433 100644 --- a/src/middleware/node/get-payload.ts +++ b/src/middleware/node/get-payload.ts @@ -17,7 +17,10 @@ export function getPayload(request: IncomingMessage): Promise { // istanbul ignore next request.on("error", (error: Error) => reject(new AggregateError([error]))); - request.on("data", (chunk: Buffer) => data.push(chunk)); - request.on("end", () => resolve(Buffer.concat(data))); + request.on("data", data.push.bind(data)); + // istanbul ignore next + request.on("end", () => + setImmediate(resolve, data.length === 1 ? data[0] : Buffer.concat(data)), + ); }); } diff --git a/test/integration/node-middleware.test.ts b/test/integration/node-middleware.test.ts index bc8f34f8..1e7d7708 100644 --- a/test/integration/node-middleware.test.ts +++ b/test/integration/node-middleware.test.ts @@ -419,7 +419,7 @@ describe("createNodeMiddleware(webhooks)", () => { }); test("Handles timeout", async () => { - jest.useFakeTimers(); + jest.useFakeTimers({ doNotFake: ["setImmediate"] }); const webhooks = new Webhooks({ secret: "mySecret", @@ -454,7 +454,7 @@ describe("createNodeMiddleware(webhooks)", () => { }); test("Handles timeout with error", async () => { - jest.useFakeTimers(); + jest.useFakeTimers({ doNotFake: ["setImmediate"] }); const webhooks = new Webhooks({ secret: "mySecret", From d214e24db939f65cdd60100448f4f92fe3a2273a Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 30 Nov 2023 00:28:15 +0100 Subject: [PATCH 05/12] stabilize performance --- src/middleware/node/middleware.ts | 45 ++++++++++++++++++------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/src/middleware/node/middleware.ts b/src/middleware/node/middleware.ts index ac1c02f0..17e97a44 100644 --- a/src/middleware/node/middleware.ts +++ b/src/middleware/node/middleware.ts @@ -77,11 +77,16 @@ export async function middleware( return true; } - const eventName = request.headers["x-github-event"] as WebhookEventName; - const signatureSHA256 = request.headers["x-hub-signature-256"] as string; - const id = request.headers["x-github-delivery"] as string; - - options.log.debug(`${eventName} event received (id: ${id})`); + const { + "x-github-event": name, + "x-hub-signature-256": signature, + "x-github-delivery": id, + } = request.headers as { + 'x-github-event': WebhookEventName; + 'x-hub-signature-256': string; + 'x-github-delivery': string; + } + options.log.debug(`${name} event received (id: ${id})`); // GitHub will abort the request if it does not receive a response within 10s // See https://github.com/octokit/webhooks.js/issues/185 @@ -96,26 +101,30 @@ export async function middleware( let payload: Buffer; let body: { [key: string]: any } | undefined; - const bodyType = typeof request.body; - - // The body is a String (e.g. Lambda) - if (bodyType === "string") { - payload = request.body; - // The body is already an Object and rawBody is a Buffer (e.g. GCF) - } else if (bodyType === "object" && request.rawBody instanceof Buffer) { - body = request.body; - payload = request.rawBody; - // We need to load the payload from the request (normal case of Node.js server) + if ("body" in request) { + if ( + typeof request.body === "object" && + "rawBody" in request && + request.rawBody instanceof Buffer + ) { + // The body is already an Object and rawBody is a Buffer (e.g. GCF) + body = request.body; + payload = request.rawBody; + } else { + // The body is a String (e.g. Lambda) + payload = request.body; + } } else { + // We need to load the payload from the request (normal case of Node.js server) payload = await getPayload(request); } await webhooks.verifyAndReceive({ - id: id, - name: eventName as any, + id, + name, payload, body, - signature: signatureSHA256, + signature, }); clearTimeout(timeout); From 13679ff1c037dd7bd5b179c8ec078f40c6e98b77 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 30 Nov 2023 00:29:34 +0100 Subject: [PATCH 06/12] lint --- src/middleware/node/middleware.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/middleware/node/middleware.ts b/src/middleware/node/middleware.ts index 17e97a44..c3fdc868 100644 --- a/src/middleware/node/middleware.ts +++ b/src/middleware/node/middleware.ts @@ -82,10 +82,10 @@ export async function middleware( "x-hub-signature-256": signature, "x-github-delivery": id, } = request.headers as { - 'x-github-event': WebhookEventName; - 'x-hub-signature-256': string; - 'x-github-delivery': string; - } + "x-github-event": WebhookEventName; + "x-hub-signature-256": string; + "x-github-delivery": string; + }; options.log.debug(`${name} event received (id: ${id})`); // GitHub will abort the request if it does not receive a response within 10s From e043dc33edfc5cb9bad2c4dbc18bae4ce0a309f7 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 30 Nov 2023 01:13:56 +0100 Subject: [PATCH 07/12] standardize --- src/middleware/node/middleware.ts | 3 --- src/types.ts | 1 - src/verify-and-receive.ts | 9 --------- 3 files changed, 13 deletions(-) diff --git a/src/middleware/node/middleware.ts b/src/middleware/node/middleware.ts index c3fdc868..f4c98f33 100644 --- a/src/middleware/node/middleware.ts +++ b/src/middleware/node/middleware.ts @@ -99,7 +99,6 @@ export async function middleware( try { let payload: Buffer; - let body: { [key: string]: any } | undefined; if ("body" in request) { if ( @@ -108,7 +107,6 @@ export async function middleware( request.rawBody instanceof Buffer ) { // The body is already an Object and rawBody is a Buffer (e.g. GCF) - body = request.body; payload = request.rawBody; } else { // The body is a String (e.g. Lambda) @@ -123,7 +121,6 @@ export async function middleware( id, name, payload, - body, signature, }); clearTimeout(timeout); diff --git a/src/types.ts b/src/types.ts index ec862288..8ca25da3 100644 --- a/src/types.ts +++ b/src/types.ts @@ -18,7 +18,6 @@ export type EmitterWebhookEvent< export type EmitterWebhookEventWithStringPayloadAndSignature = { id: string; name: EmitterWebhookEventName; - body?: { [key: string]: any }; payload: string | Buffer; signature: string; }; diff --git a/src/verify-and-receive.ts b/src/verify-and-receive.ts index 916099c7..10adfcd5 100644 --- a/src/verify-and-receive.ts +++ b/src/verify-and-receive.ts @@ -30,15 +30,6 @@ export async function verifyAndReceive( ); } - // The body is already an Object (e.g. GCF) and can be passed directly to the EventHandler - if (event.body) { - return state.eventHandler.receive({ - id: event.id, - name: event.name, - payload: event.body as EmitterWebhookEvent["payload"], - }); - } - let payload: EmitterWebhookEvent["payload"]; try { From 2fbd7a307be22fa79a0d7f6ec5c0d86c86e12a79 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Thu, 30 Nov 2023 01:27:50 +0100 Subject: [PATCH 08/12] Apply suggestions from code review --- src/middleware/node/middleware.ts | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/middleware/node/middleware.ts b/src/middleware/node/middleware.ts index f4c98f33..83816db4 100644 --- a/src/middleware/node/middleware.ts +++ b/src/middleware/node/middleware.ts @@ -77,15 +77,9 @@ export async function middleware( return true; } - const { - "x-github-event": name, - "x-hub-signature-256": signature, - "x-github-delivery": id, - } = request.headers as { - "x-github-event": WebhookEventName; - "x-hub-signature-256": string; - "x-github-delivery": string; - }; + const eventName = request.headers["x-github-event"] as WebhookEventName; + const signatureSHA256 = request.headers["x-hub-signature-256"] as string; + const id = request.headers["x-github-delivery"] as string; options.log.debug(`${name} event received (id: ${id})`); // GitHub will abort the request if it does not receive a response within 10s From ad1ed2d7080f14d1f951b47da7e605f1fe5cfa11 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 30 Nov 2023 01:29:19 +0100 Subject: [PATCH 09/12] fix --- src/middleware/node/middleware.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middleware/node/middleware.ts b/src/middleware/node/middleware.ts index 83816db4..7781457c 100644 --- a/src/middleware/node/middleware.ts +++ b/src/middleware/node/middleware.ts @@ -77,8 +77,8 @@ export async function middleware( return true; } - const eventName = request.headers["x-github-event"] as WebhookEventName; - const signatureSHA256 = request.headers["x-hub-signature-256"] as string; + const name = request.headers["x-github-event"] as WebhookEventName; + const signature = request.headers["x-hub-signature-256"] as string; const id = request.headers["x-github-delivery"] as string; options.log.debug(`${name} event received (id: ${id})`); From dd1d31f2d95458a6cbf8366a34ca8f83c24e560b Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Sun, 3 Dec 2023 00:16:43 +0100 Subject: [PATCH 10/12] Update src/middleware/node/get-payload.ts Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com> --- src/middleware/node/get-payload.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/node/get-payload.ts b/src/middleware/node/get-payload.ts index 6ca45433..42b1a89d 100644 --- a/src/middleware/node/get-payload.ts +++ b/src/middleware/node/get-payload.ts @@ -17,7 +17,7 @@ export function getPayload(request: IncomingMessage): Promise { // istanbul ignore next request.on("error", (error: Error) => reject(new AggregateError([error]))); - request.on("data", data.push.bind(data)); + request.on("data", (chunk: string) => data.push(chunk); // istanbul ignore next request.on("end", () => setImmediate(resolve, data.length === 1 ? data[0] : Buffer.concat(data)), From d82fc69d6feeeb63c4485bc0cbbe90dba8559af5 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Sun, 3 Dec 2023 00:17:42 +0100 Subject: [PATCH 11/12] Update src/middleware/node/get-payload.ts --- src/middleware/node/get-payload.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/middleware/node/get-payload.ts b/src/middleware/node/get-payload.ts index 42b1a89d..abe8acda 100644 --- a/src/middleware/node/get-payload.ts +++ b/src/middleware/node/get-payload.ts @@ -20,6 +20,8 @@ export function getPayload(request: IncomingMessage): Promise { request.on("data", (chunk: string) => data.push(chunk); // istanbul ignore next request.on("end", () => + // setImmediate improves the throughput by reducing the pressure from + // the event loop setImmediate(resolve, data.length === 1 ? data[0] : Buffer.concat(data)), ); }); From a57434ec321fde67cdc04bc12d2305a0f91a183b Mon Sep 17 00:00:00 2001 From: uzlopak Date: Sun, 3 Dec 2023 00:23:41 +0100 Subject: [PATCH 12/12] fix --- src/middleware/node/get-payload.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/node/get-payload.ts b/src/middleware/node/get-payload.ts index abe8acda..36e67a7f 100644 --- a/src/middleware/node/get-payload.ts +++ b/src/middleware/node/get-payload.ts @@ -17,7 +17,7 @@ export function getPayload(request: IncomingMessage): Promise { // istanbul ignore next request.on("error", (error: Error) => reject(new AggregateError([error]))); - request.on("data", (chunk: string) => data.push(chunk); + request.on("data", (chunk: Buffer) => data.push(chunk)); // istanbul ignore next request.on("end", () => // setImmediate improves the throughput by reducing the pressure from