Skip to content

Commit

Permalink
fix: only parse payload once (#921)
Browse files Browse the repository at this point in the history
  • Loading branch information
Uzlopak authored Nov 16, 2023
1 parent cc836d0 commit a12200f
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 15 deletions.
12 changes: 1 addition & 11 deletions src/middleware/node/get-payload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,6 @@ export function getPayload(request: IncomingMessage): Promise<string> {
// 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));
});
}
15 changes: 13 additions & 2 deletions src/verify-and-receive.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
import AggregateError from "aggregate-error";
import { verify } from "@octokit/webhooks-methods";

import type {
EmitterWebhookEvent,
EmitterWebhookEventWithStringPayloadAndSignature,
State,
} from "./types";

export async function verifyAndReceive(
state: State & { secret: string },
event: EmitterWebhookEventWithStringPayloadAndSignature,
): Promise<any> {
): Promise<void> {
// verify will validate that the secret is not undefined
const matchesSignature = await verify(
state.secret,
Expand All @@ -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,
});
}
6 changes: 4 additions & 2 deletions test/integration/node-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
{
Expand All @@ -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,
},
);

Expand Down

0 comments on commit a12200f

Please sign in to comment.