From feecb21fda8e434ae6853ad1100e782aae8157dc Mon Sep 17 00:00:00 2001 From: Christian Fehmer Date: Wed, 11 Sep 2024 11:15:06 +0300 Subject: [PATCH 01/12] impr: use ts-rest for webhook endpoints (@fehmer) --- .../__tests__/api/controllers/public.spec.ts | 5 +- .../api/controllers/webhooks.spec.ts | 118 ++++++++++++++++++ backend/scripts/openapi.ts | 6 + backend/src/api/controllers/webhooks.ts | 20 ++- backend/src/api/routes/index.ts | 3 +- backend/src/api/routes/webhooks.ts | 25 ++-- packages/contracts/src/index.ts | 2 + packages/contracts/src/schemas/api.ts | 3 +- packages/contracts/src/webhooks.ts | 62 +++++++++ 9 files changed, 218 insertions(+), 26 deletions(-) create mode 100644 backend/__tests__/api/controllers/webhooks.spec.ts create mode 100644 packages/contracts/src/webhooks.ts diff --git a/backend/__tests__/api/controllers/public.spec.ts b/backend/__tests__/api/controllers/public.spec.ts index e5e348d72466..3a77212a5cd5 100644 --- a/backend/__tests__/api/controllers/public.spec.ts +++ b/backend/__tests__/api/controllers/public.spec.ts @@ -18,9 +18,8 @@ describe("PublicController", () => { //WHEN const { body } = await mockApp .get("/public/speedHistogram") - .query({ language: "english", mode: "time", mode2: "60" }); - //.expect(200); - console.log(body); + .query({ language: "english", mode: "time", mode2: "60" }) + .expect(200); //THEN expect(body).toEqual({ diff --git a/backend/__tests__/api/controllers/webhooks.spec.ts b/backend/__tests__/api/controllers/webhooks.spec.ts new file mode 100644 index 000000000000..3e3d11994d64 --- /dev/null +++ b/backend/__tests__/api/controllers/webhooks.spec.ts @@ -0,0 +1,118 @@ +import GeorgeQueue from "../../../src/queues/george-queue"; +import crypto from "crypto"; +import request from "supertest"; +import app from "../../../src/app"; + +const mockApp = request(app); + +describe("WebhooksController", () => { + describe("githubRelease", () => { + const georgeSendReleaseAnnouncementMock = vi.spyOn( + GeorgeQueue, + "sendReleaseAnnouncement" + ); + const timingSafeEqualMock = vi.spyOn(crypto, "timingSafeEqual"); + + beforeEach(() => { + vi.stubEnv("GITHUB_WEBHOOK_SECRET", "GITHUB_WEBHOOK_SECRET"); + + georgeSendReleaseAnnouncementMock.mockReset(); + timingSafeEqualMock.mockReset().mockReturnValue(true); + }); + + it("should announce release", async () => { + //WHEN + const { body } = await mockApp + .post("/webhooks/githubRelease") + .set("x-hub-signature-256", "the-signature") + .send({ action: "published", release: { id: 1 } }) + .expect(200); + + //THEN + expect(body).toEqual({ + message: "Added release announcement task to queue", + data: null, + }); + + expect(georgeSendReleaseAnnouncementMock).toHaveBeenCalledWith("1"); + expect(timingSafeEqualMock).toHaveBeenCalledWith( + Buffer.from( + "sha256=ff0f3080539e9df19153f6b5b5780f66e558d61038e6cf5ecf4efdc7266a7751" + ), + Buffer.from("the-signature") + ); + }); + it("should ignore non-published actions", async () => { + //WHEN + const { body } = await mockApp + .post("/webhooks/githubRelease") + .set("x-hub-signature-256", "the-signature") + .send({ action: "created" }) + .expect(200); + + //THEN + expect(body.message).toEqual("No action taken"); + expect(georgeSendReleaseAnnouncementMock).not.toHaveBeenCalled(); + }); + it("should ignore additional properties", async () => { + //WHEN + await mockApp + .post("/webhooks/githubRelease") + .set("x-hub-signature-256", "the-signature") + .send({ + action: "published", + extra: "value", + release: { id: 1, extra2: "value" }, + }) + .expect(200); + }); + it("should fail with missing releaseId", async () => { + //WHEN + const { body } = await mockApp + .post("/webhooks/githubRelease") + .set("x-hub-signature-256", "the-signature") + .send({ action: "published" }) + .expect(422); + + //THEN + expect(body.message).toEqual('Missing property "release.id".'); + }); + it("should fail with missing GITHUB_WEBHOOK_SECRET", async () => { + //GIVEN + vi.stubEnv("GITHUB_WEBHOOK_SECRET", ""); + //WHEN + const { body } = await mockApp + .post("/webhooks/githubRelease") + .set("x-hub-signature-256", "the-signature") + .send({ action: "published", release: { id: 1 } }) + .expect(500); + + //THEN + expect(body.message).toEqual("Missing Github Webhook Secret"); + }); + it("should fail without signature header", async () => { + //WHEN + const { body } = await mockApp + .post("/webhooks/githubRelease") + .send({ action: "published", release: { id: 1 } }) + .expect(401); + + //THEN + expect(body.message).toEqual("Missing Github signature header"); + }); + it("should fail with mismatched signature", async () => { + //GIVEN + timingSafeEqualMock.mockReturnValue(false); + + //WHEN + const { body } = await mockApp + .post("/webhooks/githubRelease") + .set("x-hub-signature-256", "the-signature") + .send({ action: "published", release: { id: 1 } }) + .expect(401); + + //THEN + expect(body.message).toEqual("Github webhook signature invalid"); + }); + }); +}); diff --git a/backend/scripts/openapi.ts b/backend/scripts/openapi.ts index 83c78277ad2f..5e983d661425 100644 --- a/backend/scripts/openapi.ts +++ b/backend/scripts/openapi.ts @@ -134,6 +134,12 @@ export function getOpenApi(): OpenAPIObject { "x-displayName": "Development", "x-public": "no", }, + { + name: "webhooks", + description: "Endpoints for incoming webhooks.", + "x-displayName": "Webhooks", + "x-public": "yes", + }, ], }, diff --git a/backend/src/api/controllers/webhooks.ts b/backend/src/api/controllers/webhooks.ts index 3bef5bbca04a..f82c58f82419 100644 --- a/backend/src/api/controllers/webhooks.ts +++ b/backend/src/api/controllers/webhooks.ts @@ -1,15 +1,23 @@ -import { MonkeyResponse } from "../../utils/monkey-response"; +import { PostGithubReleaseRequest } from "@monkeytype/contracts/webhooks"; import GeorgeQueue from "../../queues/george-queue"; +import { MonkeyResponse2 } from "../../utils/monkey-response"; +import MonkeyError from "../../utils/error"; export async function githubRelease( - req: MonkeyTypes.Request -): Promise { + req: MonkeyTypes.Request2 +): Promise { const action = req.body.action; if (action === "published") { - const releaseId = req.body.release.id; + const releaseId = req.body.release?.id; + if (releaseId === undefined) + throw new MonkeyError(422, 'Missing property "release.id".'); + await GeorgeQueue.sendReleaseAnnouncement(releaseId); - return new MonkeyResponse("Added release announcement task to queue"); + return new MonkeyResponse2( + "Added release announcement task to queue", + null + ); } - return new MonkeyResponse("No action taken"); + return new MonkeyResponse2("No action taken", null); } diff --git a/backend/src/api/routes/index.ts b/backend/src/api/routes/index.ts index c969eed5972b..d6148cebd7bf 100644 --- a/backend/src/api/routes/index.ts +++ b/backend/src/api/routes/index.ts @@ -42,7 +42,6 @@ const BASE_ROUTE = pathOverride !== undefined ? `/${pathOverride}` : ""; const APP_START_TIME = Date.now(); const API_ROUTE_MAP = { - "/webhooks": webhooks, "/docs": docs, }; @@ -60,6 +59,7 @@ const router = s.router(contract, { dev, users, quotes, + webhooks, }); export function addApiRoutes(app: Application): void { @@ -152,7 +152,6 @@ function applyDevApiRoutes(app: Application): void { function applyApiRoutes(app: Application): void { addSwaggerMiddlewares(app); - //TODO move to globalMiddleware when all endpoints use tsrest app.use( (req: MonkeyTypes.Request, res: Response, next: NextFunction): void => { if (req.path.startsWith("/configuration")) { diff --git a/backend/src/api/routes/webhooks.ts b/backend/src/api/routes/webhooks.ts index d0e774d096bf..1a585b0d4726 100644 --- a/backend/src/api/routes/webhooks.ts +++ b/backend/src/api/routes/webhooks.ts @@ -1,17 +1,14 @@ // import joi from "joi"; -import { Router } from "express"; +import { webhooksContract } from "@monkeytype/contracts/webhooks"; +import { initServer } from "@ts-rest/express"; import { authenticateGithubWebhook } from "../../middlewares/auth"; -import { asyncHandler } from "../../middlewares/utility"; -import { webhookLimit } from "../../middlewares/rate-limit"; -import { githubRelease } from "../controllers/webhooks"; +import * as WebhooksController from "../controllers/webhooks"; +import { callController } from "../ts-rest-adapter"; -const router = Router(); - -router.post( - "/githubRelease", - webhookLimit, - authenticateGithubWebhook(), - asyncHandler(githubRelease) -); - -export default router; +const s = initServer(); +export default s.router(webhooksContract, { + postGithubRelease: { + middleware: [authenticateGithubWebhook()], + handler: async (r) => callController(WebhooksController.githubRelease)(r), + }, +}); diff --git a/packages/contracts/src/index.ts b/packages/contracts/src/index.ts index dc84d01f81f7..75aa486635c3 100644 --- a/packages/contracts/src/index.ts +++ b/packages/contracts/src/index.ts @@ -11,6 +11,7 @@ import { configurationContract } from "./configuration"; import { devContract } from "./dev"; import { usersContract } from "./users"; import { quotesContract } from "./quotes"; +import { webhooksContract } from "./webhooks"; const c = initContract(); @@ -27,4 +28,5 @@ export const contract = c.router({ dev: devContract, users: usersContract, quotes: quotesContract, + webhooks: webhooksContract, }); diff --git a/packages/contracts/src/schemas/api.ts b/packages/contracts/src/schemas/api.ts index d0ce83c124e4..c1e15d05b1bc 100644 --- a/packages/contracts/src/schemas/api.ts +++ b/packages/contracts/src/schemas/api.ts @@ -13,7 +13,8 @@ export type OpenApiTag = | "configuration" | "development" | "users" - | "quotes"; + | "quotes" + | "webhooks"; export type PermissionId = | "quoteMod" diff --git a/packages/contracts/src/webhooks.ts b/packages/contracts/src/webhooks.ts new file mode 100644 index 000000000000..2af215bae2db --- /dev/null +++ b/packages/contracts/src/webhooks.ts @@ -0,0 +1,62 @@ +import { initContract } from "@ts-rest/core"; +import { z } from "zod"; +import { PSASchema } from "./schemas/psas"; + +import { + CommonResponses, + meta, + MonkeyResponseSchema, + responseWithData, +} from "./schemas/api"; + +/** + *Schema: https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=published#release + We only specify the values we read and don't validate any other values. + */ +export const PostGithubReleaseRequestSchema = z.object({ + action: z.literal("published").or(z.string()), + release: z + .object({ + id: z.string().or(z.number().transform(String)), //we use string, github defines this as a number. + }) + .optional(), +}); +export type PostGithubReleaseRequest = z.infer< + typeof PostGithubReleaseRequestSchema +>; + +export const GetPsaResponseSchema = responseWithData(z.array(PSASchema)); +export type GetPsaResponse = z.infer; + +const c = initContract(); +export const webhooksContract = c.router( + { + postGithubRelease: { + summary: "Github release", + description: "Announce github release.", + method: "POST", + path: "/githubRelease", + body: PostGithubReleaseRequestSchema, //don't use strict + headers: z.object({ + "x-hub-signature-256": z.string(), + }), + responses: { + 200: MonkeyResponseSchema, + }, + metadata: meta({ + authenticationOptions: { + isPublic: true, + }, + rateLimit: "webhookLimit", + }), + }, + }, + { + pathPrefix: "/webhooks", + strictStatusCodes: true, + metadata: meta({ + openApiTags: "webhooks", + }), + commonResponses: CommonResponses, + } +); From ca6481552a47ab9b20c56f3324f623ac0675d34d Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 12:04:21 +0200 Subject: [PATCH 02/12] move from middleware to auth setting approach --- backend/src/api/routes/webhooks.ts | 2 - backend/src/middlewares/auth.ts | 88 ++++++++++++++++----------- backend/src/types/types.d.ts | 2 +- backend/src/utils/prometheus.ts | 4 +- packages/contracts/src/schemas/api.ts | 2 + packages/contracts/src/webhooks.ts | 11 ++-- 6 files changed, 62 insertions(+), 47 deletions(-) diff --git a/backend/src/api/routes/webhooks.ts b/backend/src/api/routes/webhooks.ts index 1a585b0d4726..5946d75d0d3f 100644 --- a/backend/src/api/routes/webhooks.ts +++ b/backend/src/api/routes/webhooks.ts @@ -1,14 +1,12 @@ // import joi from "joi"; import { webhooksContract } from "@monkeytype/contracts/webhooks"; import { initServer } from "@ts-rest/express"; -import { authenticateGithubWebhook } from "../../middlewares/auth"; import * as WebhooksController from "../controllers/webhooks"; import { callController } from "../ts-rest-adapter"; const s = initServer(); export default s.router(webhooksContract, { postGithubRelease: { - middleware: [authenticateGithubWebhook()], handler: async (r) => callController(WebhooksController.githubRelease)(r), }, }); diff --git a/backend/src/middlewares/auth.ts b/backend/src/middlewares/auth.ts index 74727f0aab98..4935d656a42a 100644 --- a/backend/src/middlewares/auth.ts +++ b/backend/src/middlewares/auth.ts @@ -18,6 +18,7 @@ import { RequestAuthenticationOptions } from "@monkeytype/contracts/schemas/api" import { Configuration } from "@monkeytype/contracts/schemas/configuration"; const DEFAULT_OPTIONS: RequestAuthenticationOptions = { + isGithubWebhook: false, isPublic: false, acceptApeKeys: false, requireFreshToken: false, @@ -78,10 +79,19 @@ async function _authenticateRequestInternal( const isPublic = options.isPublic || (options.isPublicOnDev && isDevEnvironment()); - const { authorization: authHeader } = req.headers; + const { + authorization: authHeader, + "x-hub-signature-256": githubWebhookHeader, + } = req.headers; try { - if (authHeader !== undefined && authHeader !== "") { + if ( + githubWebhookHeader !== undefined && + typeof githubWebhookHeader === "string" && + options.isGithubWebhook + ) { + token = authenticateGithubWebhook(req, githubWebhookHeader); + } else if (authHeader !== undefined && authHeader !== "") { token = await authenticateWithAuthHeader( authHeader, req.ctx.configuration, @@ -322,44 +332,48 @@ async function authenticateWithUid( }; } -export function authenticateGithubWebhook(): Handler { - return async ( - req: MonkeyTypes.Request, - _res: Response, - next: NextFunction - ): Promise => { - //authorize github webhook - const { "x-hub-signature-256": authHeader } = req.headers; - +export function authenticateGithubWebhook( + req: MonkeyTypes.Request, + authHeader: string +): MonkeyTypes.DecodedToken { + try { const webhookSecret = process.env["GITHUB_WEBHOOK_SECRET"]; - try { - if (webhookSecret === undefined || webhookSecret === "") { - throw new MonkeyError(500, "Missing Github Webhook Secret"); - } else if ( - authHeader === undefined || - authHeader === "" || - authHeader.length === 0 - ) { - throw new MonkeyError(401, "Missing Github signature header"); - } else { - const signature = crypto - .createHmac("sha256", webhookSecret) - .update(JSON.stringify(req.body)) - .digest("hex"); - const trusted = Buffer.from(`sha256=${signature}`, "ascii"); - const untrusted = Buffer.from(authHeader as string, "ascii"); - const isSignatureValid = crypto.timingSafeEqual(trusted, untrusted); - - if (!isSignatureValid) { - throw new MonkeyError(401, "Github webhook signature invalid"); - } + if (webhookSecret === undefined || webhookSecret === "") { + throw new MonkeyError(500, "Missing Github Webhook Secret"); + } else if ( + authHeader === undefined || + authHeader === "" || + authHeader.length === 0 + ) { + throw new MonkeyError(401, "Missing Github signature header"); + } else { + const signature = crypto + .createHmac("sha256", webhookSecret) + .update(JSON.stringify(req.body)) + .digest("hex"); + const trusted = Buffer.from(`sha256=${signature}`, "ascii"); + const untrusted = Buffer.from(authHeader, "ascii"); + const isSignatureValid = crypto.timingSafeEqual(trusted, untrusted); + + if (!isSignatureValid) { + throw new MonkeyError(401, "Github webhook signature invalid"); } - } catch (e) { - next(e); - return; + + return { + type: "GithubWebhook", + uid: "", + email: "", + }; + } + } catch (error) { + if (!(error instanceof MonkeyError)) { + throw new MonkeyError( + 500, + "Failed to authenticate Github webhook: " + (error as Error).message + ); } - next(); - }; + throw error; + } } diff --git a/backend/src/types/types.d.ts b/backend/src/types/types.d.ts index dff561c7d15f..0cc14751c6af 100644 --- a/backend/src/types/types.d.ts +++ b/backend/src/types/types.d.ts @@ -8,7 +8,7 @@ type AppRoute = import("@ts-rest/core").AppRoute; type AppRouter = import("@ts-rest/core").AppRouter; declare namespace MonkeyTypes { export type DecodedToken = { - type: "Bearer" | "ApeKey" | "None"; + type: "Bearer" | "ApeKey" | "None" | "GithubWebhook"; uid: string; email: string; }; diff --git a/backend/src/utils/prometheus.ts b/backend/src/utils/prometheus.ts index 1aaa39f4b1d4..5831acd8a9ef 100644 --- a/backend/src/utils/prometheus.ts +++ b/backend/src/utils/prometheus.ts @@ -74,7 +74,9 @@ const leaderboardUpdate = new Gauge({ labelNames: ["language", "mode", "mode2", "step"], }); -export function incrementAuth(type: "Bearer" | "ApeKey" | "None"): void { +export function incrementAuth( + type: "Bearer" | "ApeKey" | "None" | "GithubWebhook" +): void { auth.inc({ type }); } diff --git a/packages/contracts/src/schemas/api.ts b/packages/contracts/src/schemas/api.ts index c1e15d05b1bc..a5de95727423 100644 --- a/packages/contracts/src/schemas/api.ts +++ b/packages/contracts/src/schemas/api.ts @@ -57,6 +57,8 @@ export type RequestAuthenticationOptions = { noCache?: boolean; /** Allow unauthenticated requests on dev */ isPublicOnDev?: boolean; + + isGithubWebhook?: boolean; }; export const MonkeyResponseSchema = z.object({ diff --git a/packages/contracts/src/webhooks.ts b/packages/contracts/src/webhooks.ts index 2af215bae2db..8da7b7d9ff53 100644 --- a/packages/contracts/src/webhooks.ts +++ b/packages/contracts/src/webhooks.ts @@ -43,12 +43,6 @@ export const webhooksContract = c.router( responses: { 200: MonkeyResponseSchema, }, - metadata: meta({ - authenticationOptions: { - isPublic: true, - }, - rateLimit: "webhookLimit", - }), }, }, { @@ -56,6 +50,11 @@ export const webhooksContract = c.router( strictStatusCodes: true, metadata: meta({ openApiTags: "webhooks", + authenticationOptions: { + isPublic: true, + isGithubWebhook: true, + }, + rateLimit: "webhookLimit", }), commonResponses: CommonResponses, } From 02274409c9dcb5d04c19187353f49aa0bd618f0f Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 12:41:35 +0200 Subject: [PATCH 03/12] fix test --- backend/__tests__/api/controllers/webhooks.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/__tests__/api/controllers/webhooks.spec.ts b/backend/__tests__/api/controllers/webhooks.spec.ts index 3e3d11994d64..247293c2bbb3 100644 --- a/backend/__tests__/api/controllers/webhooks.spec.ts +++ b/backend/__tests__/api/controllers/webhooks.spec.ts @@ -95,10 +95,10 @@ describe("WebhooksController", () => { const { body } = await mockApp .post("/webhooks/githubRelease") .send({ action: "published", release: { id: 1 } }) - .expect(401); + .expect(422); //THEN - expect(body.message).toEqual("Missing Github signature header"); + expect(body.message).toEqual("Invalid header schema"); }); it("should fail with mismatched signature", async () => { //GIVEN From 17c6f853d7ff0b5b0109214472951a26c5407d92 Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 12:42:25 +0200 Subject: [PATCH 04/12] comment --- packages/contracts/src/schemas/api.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/src/schemas/api.ts b/packages/contracts/src/schemas/api.ts index a5de95727423..4d8acb69e37d 100644 --- a/packages/contracts/src/schemas/api.ts +++ b/packages/contracts/src/schemas/api.ts @@ -57,7 +57,7 @@ export type RequestAuthenticationOptions = { noCache?: boolean; /** Allow unauthenticated requests on dev */ isPublicOnDev?: boolean; - + /** Endpoint is a webhook only to be called by Github */ isGithubWebhook?: boolean; }; From 054991a2930a7cc8d987e8d8eefe9355ddd1b6f1 Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 12:43:38 +0200 Subject: [PATCH 05/12] stricter expect --- backend/__tests__/api/controllers/webhooks.spec.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/backend/__tests__/api/controllers/webhooks.spec.ts b/backend/__tests__/api/controllers/webhooks.spec.ts index 247293c2bbb3..7fb1e4815e86 100644 --- a/backend/__tests__/api/controllers/webhooks.spec.ts +++ b/backend/__tests__/api/controllers/webhooks.spec.ts @@ -98,7 +98,10 @@ describe("WebhooksController", () => { .expect(422); //THEN - expect(body.message).toEqual("Invalid header schema"); + expect(body).toEqual({ + message: "Invalid header schema", + validationErrors: ['"x-hub-signature-256" Required'], + }); }); it("should fail with mismatched signature", async () => { //GIVEN From d493bb885d459093a15a2db82a7d2852ca18a8fc Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 12:57:30 +0200 Subject: [PATCH 06/12] webhooks are not public --- packages/contracts/src/webhooks.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/contracts/src/webhooks.ts b/packages/contracts/src/webhooks.ts index 8da7b7d9ff53..2fcd11793f5b 100644 --- a/packages/contracts/src/webhooks.ts +++ b/packages/contracts/src/webhooks.ts @@ -51,7 +51,6 @@ export const webhooksContract = c.router( metadata: meta({ openApiTags: "webhooks", authenticationOptions: { - isPublic: true, isGithubWebhook: true, }, rateLimit: "webhookLimit", From a5ceb27aaf8b6d9e213fd45fe7747c9130dab083 Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 12:59:08 +0200 Subject: [PATCH 07/12] make sure codeflow always enters webhook authentication function if its enabled --- backend/src/middlewares/auth.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/backend/src/middlewares/auth.ts b/backend/src/middlewares/auth.ts index 4935d656a42a..d2a06a0de621 100644 --- a/backend/src/middlewares/auth.ts +++ b/backend/src/middlewares/auth.ts @@ -85,11 +85,7 @@ async function _authenticateRequestInternal( } = req.headers; try { - if ( - githubWebhookHeader !== undefined && - typeof githubWebhookHeader === "string" && - options.isGithubWebhook - ) { + if (options.isGithubWebhook) { token = authenticateGithubWebhook(req, githubWebhookHeader); } else if (authHeader !== undefined && authHeader !== "") { token = await authenticateWithAuthHeader( @@ -334,7 +330,7 @@ async function authenticateWithUid( export function authenticateGithubWebhook( req: MonkeyTypes.Request, - authHeader: string + authHeader: string | string[] | undefined ): MonkeyTypes.DecodedToken { try { const webhookSecret = process.env["GITHUB_WEBHOOK_SECRET"]; @@ -342,9 +338,9 @@ export function authenticateGithubWebhook( if (webhookSecret === undefined || webhookSecret === "") { throw new MonkeyError(500, "Missing Github Webhook Secret"); } else if ( + Array.isArray(authHeader) || authHeader === undefined || - authHeader === "" || - authHeader.length === 0 + authHeader === "" ) { throw new MonkeyError(401, "Missing Github signature header"); } else { From ee0865bfd420802add67e43c2fbef4c520f6597c Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 12:59:16 +0200 Subject: [PATCH 08/12] fix tests --- backend/__tests__/api/controllers/webhooks.spec.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/backend/__tests__/api/controllers/webhooks.spec.ts b/backend/__tests__/api/controllers/webhooks.spec.ts index 7fb1e4815e86..3e3d11994d64 100644 --- a/backend/__tests__/api/controllers/webhooks.spec.ts +++ b/backend/__tests__/api/controllers/webhooks.spec.ts @@ -95,13 +95,10 @@ describe("WebhooksController", () => { const { body } = await mockApp .post("/webhooks/githubRelease") .send({ action: "published", release: { id: 1 } }) - .expect(422); + .expect(401); //THEN - expect(body).toEqual({ - message: "Invalid header schema", - validationErrors: ['"x-hub-signature-256" Required'], - }); + expect(body.message).toEqual("Missing Github signature header"); }); it("should fail with mismatched signature", async () => { //GIVEN From 630245380df8786ebcd13c7464e1140d947710d8 Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 13:01:43 +0200 Subject: [PATCH 09/12] reduce indentation --- backend/src/middlewares/auth.ts | 38 +++++++++++++++++---------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/backend/src/middlewares/auth.ts b/backend/src/middlewares/auth.ts index d2a06a0de621..919330e7063c 100644 --- a/backend/src/middlewares/auth.ts +++ b/backend/src/middlewares/auth.ts @@ -337,31 +337,33 @@ export function authenticateGithubWebhook( if (webhookSecret === undefined || webhookSecret === "") { throw new MonkeyError(500, "Missing Github Webhook Secret"); - } else if ( + } + + if ( Array.isArray(authHeader) || authHeader === undefined || authHeader === "" ) { throw new MonkeyError(401, "Missing Github signature header"); - } else { - const signature = crypto - .createHmac("sha256", webhookSecret) - .update(JSON.stringify(req.body)) - .digest("hex"); - const trusted = Buffer.from(`sha256=${signature}`, "ascii"); - const untrusted = Buffer.from(authHeader, "ascii"); - const isSignatureValid = crypto.timingSafeEqual(trusted, untrusted); - - if (!isSignatureValid) { - throw new MonkeyError(401, "Github webhook signature invalid"); - } + } - return { - type: "GithubWebhook", - uid: "", - email: "", - }; + const signature = crypto + .createHmac("sha256", webhookSecret) + .update(JSON.stringify(req.body)) + .digest("hex"); + const trusted = Buffer.from(`sha256=${signature}`, "ascii"); + const untrusted = Buffer.from(authHeader, "ascii"); + const isSignatureValid = crypto.timingSafeEqual(trusted, untrusted); + + if (!isSignatureValid) { + throw new MonkeyError(401, "Github webhook signature invalid"); } + + return { + type: "GithubWebhook", + uid: "", + email: "", + }; } catch (error) { if (!(error instanceof MonkeyError)) { throw new MonkeyError( From 69a71ac1f2a28f8a79e387d4b1b4ddd16e1650e9 Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 13:16:11 +0200 Subject: [PATCH 10/12] invert if --- backend/src/middlewares/auth.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/backend/src/middlewares/auth.ts b/backend/src/middlewares/auth.ts index 919330e7063c..a6e100c90c9e 100644 --- a/backend/src/middlewares/auth.ts +++ b/backend/src/middlewares/auth.ts @@ -365,13 +365,12 @@ export function authenticateGithubWebhook( email: "", }; } catch (error) { - if (!(error instanceof MonkeyError)) { - throw new MonkeyError( - 500, - "Failed to authenticate Github webhook: " + (error as Error).message - ); + if (error instanceof MonkeyError) { + throw error; } - - throw error; + throw new MonkeyError( + 500, + "Failed to authenticate Github webhook: " + (error as Error).message + ); } } From 87470c41b3613cc9a2e5daa22f4cbd72434d3411 Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 14:07:45 +0200 Subject: [PATCH 11/12] tests --- .../api/controllers/webhooks.spec.ts | 37 ------ backend/__tests__/middlewares/auth.spec.ts | 121 ++++++++++++++++++ 2 files changed, 121 insertions(+), 37 deletions(-) diff --git a/backend/__tests__/api/controllers/webhooks.spec.ts b/backend/__tests__/api/controllers/webhooks.spec.ts index 3e3d11994d64..be54c47c144b 100644 --- a/backend/__tests__/api/controllers/webhooks.spec.ts +++ b/backend/__tests__/api/controllers/webhooks.spec.ts @@ -77,42 +77,5 @@ describe("WebhooksController", () => { //THEN expect(body.message).toEqual('Missing property "release.id".'); }); - it("should fail with missing GITHUB_WEBHOOK_SECRET", async () => { - //GIVEN - vi.stubEnv("GITHUB_WEBHOOK_SECRET", ""); - //WHEN - const { body } = await mockApp - .post("/webhooks/githubRelease") - .set("x-hub-signature-256", "the-signature") - .send({ action: "published", release: { id: 1 } }) - .expect(500); - - //THEN - expect(body.message).toEqual("Missing Github Webhook Secret"); - }); - it("should fail without signature header", async () => { - //WHEN - const { body } = await mockApp - .post("/webhooks/githubRelease") - .send({ action: "published", release: { id: 1 } }) - .expect(401); - - //THEN - expect(body.message).toEqual("Missing Github signature header"); - }); - it("should fail with mismatched signature", async () => { - //GIVEN - timingSafeEqualMock.mockReturnValue(false); - - //WHEN - const { body } = await mockApp - .post("/webhooks/githubRelease") - .set("x-hub-signature-256", "the-signature") - .send({ action: "published", release: { id: 1 } }) - .expect(401); - - //THEN - expect(body.message).toEqual("Github webhook signature invalid"); - }); }); }); diff --git a/backend/__tests__/middlewares/auth.spec.ts b/backend/__tests__/middlewares/auth.spec.ts index 49af130f4557..89cb3d863e54 100644 --- a/backend/__tests__/middlewares/auth.spec.ts +++ b/backend/__tests__/middlewares/auth.spec.ts @@ -8,6 +8,7 @@ import { ObjectId } from "mongodb"; import { hashSync } from "bcrypt"; import MonkeyError from "../../src/utils/error"; import * as Misc from "../../src/utils/misc"; +import crypto from "crypto"; import { EndpointMetadata, RequestAuthenticationOptions, @@ -259,6 +260,8 @@ describe("middlewares/auth", () => { describe("authenticateTsRestRequest", () => { const prometheusRecordAuthTimeMock = vi.spyOn(Prometheus, "recordAuthTime"); const prometheusIncrementAuthMock = vi.spyOn(Prometheus, "incrementAuth"); + const timingSafeEqualMock = vi.spyOn(crypto, "timingSafeEqual"); + timingSafeEqualMock.mockReset().mockReturnValue(true); beforeEach(() => [prometheusIncrementAuthMock, prometheusRecordAuthTimeMock].forEach( @@ -604,6 +607,124 @@ describe("middlewares/auth", () => { expect(prometheusIncrementAuthMock).toHaveBeenCalledWith("ApeKey"); expect(prometheusRecordAuthTimeMock).toHaveBeenCalledOnce(); }); + it("should allow githubwebhook with header", async () => { + vi.stubEnv("GITHUB_WEBHOOK_SECRET", "GITHUB_WEBHOOK_SECRET"); + //WHEN + const result = await authenticate( + { + headers: { "x-hub-signature-256": "the-signature" }, + body: { action: "published", release: { id: 1 } }, + }, + { isGithubWebhook: true } + ); + + //THEN + const decodedToken = result.decodedToken; + expect(decodedToken?.type).toBe("GithubWebhook"); + expect(decodedToken?.email).toBe(""); + expect(decodedToken?.uid).toBe(""); + expect(nextFunction).toHaveBeenCalledTimes(1); + + expect(prometheusIncrementAuthMock).toHaveBeenCalledWith("GithubWebhook"); + expect(prometheusRecordAuthTimeMock).toHaveBeenCalledOnce(); + expect(timingSafeEqualMock).toHaveBeenCalledWith( + Buffer.from( + "sha256=ff0f3080539e9df19153f6b5b5780f66e558d61038e6cf5ecf4efdc7266a7751" + ), + Buffer.from("the-signature") + ); + }); + it("should fail githubwebhook with mismatched signature", async () => { + vi.stubEnv("GITHUB_WEBHOOK_SECRET", "GITHUB_WEBHOOK_SECRET"); + timingSafeEqualMock.mockReturnValue(false); + + await expect(() => + authenticate( + { + headers: { "x-hub-signature-256": "the-signature" }, + body: { action: "published", release: { id: 1 } }, + }, + { isGithubWebhook: true } + ) + ).rejects.toThrowError("Github webhook signature invalid"); + + //THEH + expect(prometheusIncrementAuthMock).not.toHaveBeenCalled(); + expect(prometheusRecordAuthTimeMock).toHaveBeenCalledWith( + "None", + "failure", + expect.anything(), + expect.anything() + ); + }); + it("should fail without header when endpoint is using githubwebhook", async () => { + vi.stubEnv("GITHUB_WEBHOOK_SECRET", "GITHUB_WEBHOOK_SECRET"); + await expect(() => + authenticate( + { + headers: {}, + body: { action: "published", release: { id: 1 } }, + }, + { isGithubWebhook: true } + ) + ).rejects.toThrowError("Missing Github signature header"); + + //THEH + expect(prometheusIncrementAuthMock).not.toHaveBeenCalled(); + expect(prometheusRecordAuthTimeMock).toHaveBeenCalledWith( + "None", + "failure", + expect.anything(), + expect.anything() + ); + }); + it("should fail with missing GITHUB_WEBHOOK_SECRET when endpoint is using githubwebhook", async () => { + vi.stubEnv("GITHUB_WEBHOOK_SECRET", ""); + await expect(() => + authenticate( + { + headers: { "x-hub-signature-256": "the-signature" }, + body: { action: "published", release: { id: 1 } }, + }, + { isGithubWebhook: true } + ) + ).rejects.toThrowError("Missing Github Webhook Secret"); + + //THEH + expect(prometheusIncrementAuthMock).not.toHaveBeenCalled(); + expect(prometheusRecordAuthTimeMock).toHaveBeenCalledWith( + "None", + "failure", + expect.anything(), + expect.anything() + ); + }); + it("should throw 500 if something went wrong when validating the signature when endpoint is using githubwebhook", async () => { + vi.stubEnv("GITHUB_WEBHOOK_SECRET", "GITHUB_WEBHOOK_SECRET"); + timingSafeEqualMock.mockImplementation(() => { + throw new Error("could not validate"); + }); + await expect(() => + authenticate( + { + headers: { "x-hub-signature-256": "the-signature" }, + body: { action: "published", release: { id: 1 } }, + }, + { isGithubWebhook: true } + ) + ).rejects.toThrowError( + "Failed to authenticate Github webhook: could not validate" + ); + + //THEH + expect(prometheusIncrementAuthMock).not.toHaveBeenCalled(); + expect(prometheusRecordAuthTimeMock).toHaveBeenCalledWith( + "None", + "failure", + expect.anything(), + expect.anything() + ); + }); }); }); From b6cbda6a0bbd25c8e70d13083c3916ed697a77d6 Mon Sep 17 00:00:00 2001 From: Miodec Date: Wed, 11 Sep 2024 14:12:26 +0200 Subject: [PATCH 12/12] review --- backend/__tests__/middlewares/auth.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/backend/__tests__/middlewares/auth.spec.ts b/backend/__tests__/middlewares/auth.spec.ts index 89cb3d863e54..b59efef05dde 100644 --- a/backend/__tests__/middlewares/auth.spec.ts +++ b/backend/__tests__/middlewares/auth.spec.ts @@ -261,13 +261,13 @@ describe("middlewares/auth", () => { const prometheusRecordAuthTimeMock = vi.spyOn(Prometheus, "recordAuthTime"); const prometheusIncrementAuthMock = vi.spyOn(Prometheus, "incrementAuth"); const timingSafeEqualMock = vi.spyOn(crypto, "timingSafeEqual"); - timingSafeEqualMock.mockReset().mockReturnValue(true); - beforeEach(() => + beforeEach(() => { + timingSafeEqualMock.mockReset().mockReturnValue(true); [prometheusIncrementAuthMock, prometheusRecordAuthTimeMock].forEach( (it) => it.mockReset() - ) - ); + ); + }); it("should fail if token is not fresh", async () => { //GIVEN