Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle gcf, verify already provided request.body #936

Closed
wants to merge 13 commits into from
18 changes: 7 additions & 11 deletions src/middleware/node/get-payload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,16 @@ import AggregateError from "aggregate-error";
// }
type IncomingMessage = any;

export function getPayload(request: IncomingMessage): Promise<string> {
// 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);

export function getPayload(request: IncomingMessage): Promise<Buffer> {
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", data.push.bind(data));
Uzlopak marked this conversation as resolved.
Show resolved Hide resolved
// istanbul ignore next
request.on("end", () =>
setImmediate(resolve, data.length === 1 ? data[0] : Buffer.concat(data)),
Uzlopak marked this conversation as resolved.
Show resolved Hide resolved
);
});
}
40 changes: 31 additions & 9 deletions src/middleware/node/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Uzlopak marked this conversation as resolved.
Show resolved Hide resolved
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
Expand All @@ -93,13 +98,30 @@ export async function middleware(
}, 9000).unref();

try {
const payload = await getPayload(request);
let payload: Buffer;

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)
payload = request.rawBody;
Uzlopak marked this conversation as resolved.
Show resolved Hide resolved
} 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,
signature: signatureSHA256,
signature,
});
clearTimeout(timeout);

Expand Down
2 changes: 1 addition & 1 deletion src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export type EmitterWebhookEvent<
export type EmitterWebhookEventWithStringPayloadAndSignature = {
id: string;
name: EmitterWebhookEventName;
payload: string;
payload: string | Buffer;
signature: string;
};

Expand Down
7 changes: 6 additions & 1 deletion src/verify-and-receive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not. It's better to fix this at the source

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event.payload,
event.signature,
).catch(() => false);
Expand All @@ -30,8 +31,12 @@ export async function verifyAndReceive(
}

let payload: EmitterWebhookEvent["payload"];

try {
payload = JSON.parse(event.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;
Expand Down
51 changes: 49 additions & 2 deletions test/integration/node-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -372,7 +419,7 @@ describe("createNodeMiddleware(webhooks)", () => {
});

test("Handles timeout", async () => {
jest.useFakeTimers();
jest.useFakeTimers({ doNotFake: ["setImmediate"] });

const webhooks = new Webhooks({
secret: "mySecret",
Expand Down Expand Up @@ -407,7 +454,7 @@ describe("createNodeMiddleware(webhooks)", () => {
});

test("Handles timeout with error", async () => {
jest.useFakeTimers();
jest.useFakeTimers({ doNotFake: ["setImmediate"] });

const webhooks = new Webhooks({
secret: "mySecret",
Expand Down