From 7a590550c93ae146cc9549871ac76cb6afd4a366 Mon Sep 17 00:00:00 2001 From: Apoorv Mishra Date: Sun, 25 Sep 2022 02:49:26 +0530 Subject: [PATCH] Sign webhook requests (#4156) Co-authored-by: Tom Moor --- app/models/WebhookSubscription.ts | 4 ++ .../components/WebhookSubscriptionForm.tsx | 11 ++++ .../20220922073737-webhook-signing-secret.js | 14 +++++ server/models/WebhookSubscription.ts | 53 ++++++++++++++++++- server/presenters/webhookSubscription.ts | 1 + .../queues/tasks/DeliverWebhookTask.test.ts | 27 ++++++++++ server/queues/tasks/DeliverWebhookTask.ts | 16 +++++- server/routes/api/webhookSubscriptions.ts | 15 ++++-- shared/i18n/locales/en_US/translation.json | 4 ++ 9 files changed, 139 insertions(+), 6 deletions(-) create mode 100644 server/migrations/20220922073737-webhook-signing-secret.js diff --git a/app/models/WebhookSubscription.ts b/app/models/WebhookSubscription.ts index be7c73320..093df0315 100644 --- a/app/models/WebhookSubscription.ts +++ b/app/models/WebhookSubscription.ts @@ -15,6 +15,10 @@ class WebhookSubscription extends BaseModel { @observable url: string; + @Field + @observable + secret: string; + @Field @observable enabled: boolean; diff --git a/app/scenes/Settings/components/WebhookSubscriptionForm.tsx b/app/scenes/Settings/components/WebhookSubscriptionForm.tsx index fc58ec498..629f514ba 100644 --- a/app/scenes/Settings/components/WebhookSubscriptionForm.tsx +++ b/app/scenes/Settings/components/WebhookSubscriptionForm.tsx @@ -146,6 +146,7 @@ type Props = { interface FormData { name: string; url: string; + secret: string; events: string[]; } @@ -163,6 +164,7 @@ function WebhookSubscriptionForm({ handleSubmit, webhookSubscription }: Props) { events: webhookSubscription ? [...webhookSubscription.events] : [], name: webhookSubscription?.name, url: webhookSubscription?.url, + secret: webhookSubscription?.secret, }, }); @@ -237,6 +239,7 @@ function WebhookSubscriptionForm({ handleSubmit, webhookSubscription }: Props) { autoFocus flex label={t("Name")} + placeholder={t("A memorable identifer")} {...register("name", { required: true, })} @@ -250,6 +253,14 @@ function WebhookSubscriptionForm({ handleSubmit, webhookSubscription }: Props) { label={t("URL")} {...register("url", { required: true })} /> + diff --git a/server/migrations/20220922073737-webhook-signing-secret.js b/server/migrations/20220922073737-webhook-signing-secret.js new file mode 100644 index 000000000..48c54617c --- /dev/null +++ b/server/migrations/20220922073737-webhook-signing-secret.js @@ -0,0 +1,14 @@ +"use strict"; + +module.exports = { + async up (queryInterface, Sequelize) { + return queryInterface.addColumn("webhook_subscriptions", "secret", { + type: Sequelize.BLOB, + allowNull: true, + }); + }, + + async down (queryInterface, Sequelize) { + return queryInterface.removeColumn("webhook_subscriptions", "secret"); + } +}; diff --git a/server/models/WebhookSubscription.ts b/server/models/WebhookSubscription.ts index 148ee7c66..2d51db9f9 100644 --- a/server/models/WebhookSubscription.ts +++ b/server/models/WebhookSubscription.ts @@ -1,4 +1,6 @@ +import crypto from "crypto"; import { bool } from "aws-sdk/clients/signer"; +import { isEmpty } from "lodash"; import { Column, Table, @@ -9,6 +11,7 @@ import { IsUrl, BeforeCreate, DefaultScope, + AllowNull, } from "sequelize-typescript"; import { SaveOptions } from "sequelize/types"; import { WebhookSubscriptionValidation } from "@shared/validations"; @@ -17,6 +20,10 @@ import { Event } from "@server/types"; import Team from "./Team"; import User from "./User"; import ParanoidModel from "./base/ParanoidModel"; +import Encrypted, { + setEncryptedColumn, + getEncryptedColumn, +} from "./decorators/Encrypted"; import Fix from "./decorators/Fix"; import Length from "./validators/Length"; @@ -51,6 +58,21 @@ class WebhookSubscription extends ParanoidModel { @Column(DataType.ARRAY(DataType.STRING)) events: string[]; + @AllowNull + @Encrypted + @Column(DataType.BLOB) + get secret() { + const val = getEncryptedColumn(this, "secret"); + // Turns out that `val` evals to `{}` instead + // of `null` even if secret's value in db is `null`. + // https://github.com/defunctzombie/sequelize-encrypted/blob/c3854e76ae4b80318c8f10f94e6c898c67659ca6/index.js#L30-L33 explains it possibly. + return isEmpty(val) ? "" : val; + } + + set secret(value: string) { + setEncryptedColumn(this, "secret", value); + } + // associations @BelongsTo(() => User, "createdById") @@ -87,12 +109,19 @@ class WebhookSubscription extends ParanoidModel { * Disables the webhook subscription * * @param options Save options - * @returns Promise + * @returns Promise */ public async disable(options?: SaveOptions) { return this.update({ enabled: false }, options); } + /** + * Determines if an event should be processed for this webhook subscription + * based on the event configuration. + * + * @param event Event to ceck + * @returns true if event is valid + */ public validForEvent = (event: Event): bool => { if (this.events.length === 1 && this.events[0] === "*") { return true; @@ -106,6 +135,28 @@ class WebhookSubscription extends ParanoidModel { return false; }; + + /** + * Calculates the signature for a webhook payload if the webhook subscription + * has an associated secret stored. + * + * @param payload The text payload of a webhook delivery + * @returns the signature as a string + */ + public signature = (payload: string) => { + if (isEmpty(this.secret)) { + return; + } + + const signTimestamp = Date.now(); + + const signature = crypto + .createHmac("sha256", this.secret) + .update(`${signTimestamp}.${payload}`) + .digest("hex"); + + return `t=${signTimestamp},s=${signature}`; + }; } export default WebhookSubscription; diff --git a/server/presenters/webhookSubscription.ts b/server/presenters/webhookSubscription.ts index d1cf73995..1b74c83d9 100644 --- a/server/presenters/webhookSubscription.ts +++ b/server/presenters/webhookSubscription.ts @@ -5,6 +5,7 @@ export default function present(webhook: WebhookSubscription) { id: webhook.id, name: webhook.name, url: webhook.url, + secret: webhook.secret, events: webhook.events, enabled: webhook.enabled, createdAt: webhook.createdAt, diff --git a/server/queues/tasks/DeliverWebhookTask.test.ts b/server/queues/tasks/DeliverWebhookTask.test.ts index a9ea6f0bc..f22214892 100644 --- a/server/queues/tasks/DeliverWebhookTask.test.ts +++ b/server/queues/tasks/DeliverWebhookTask.test.ts @@ -70,6 +70,33 @@ describe("DeliverWebhookTask", () => { expect(delivery.responseBody).toEqual("SUCCESS"); }); + test("should hit the subscription url with signature header", async () => { + const subscription = await buildWebhookSubscription({ + url: "http://example.com", + events: ["*"], + secret: "secret", + }); + const signedInUser = await buildUser({ teamId: subscription.teamId }); + const processor = new DeliverWebhookTask(); + + const event: UserEvent = { + name: "users.signin", + userId: signedInUser.id, + teamId: subscription.teamId, + actorId: signedInUser.id, + ip, + }; + await processor.perform({ + subscriptionId: subscription.id, + event, + }); + + const headers = fetchMock.mock.calls[0]![1]!.headers!; + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(headers["Outline-Signature"]).toMatch(/^t=[0-9]+,s=[a-z0-9]+$/); + }); + test("should hit the subscription url when the eventing model doesn't exist", async () => { const subscription = await buildWebhookSubscription({ url: "http://example.com", diff --git a/server/queues/tasks/DeliverWebhookTask.ts b/server/queues/tasks/DeliverWebhookTask.ts index 7a98f0594..49032af4e 100644 --- a/server/queues/tasks/DeliverWebhookTask.ts +++ b/server/queues/tasks/DeliverWebhookTask.ts @@ -209,12 +209,20 @@ export default class DeliverWebhookTask extends BaseTask { paranoid: false, }); + let data = null; + if (model) { + data = { + ...presentWebhookSubscription(model), + secret: undefined, + }; + } + await this.sendWebhook({ event, subscription, payload: { id: event.modelId, - model: model && presentWebhookSubscription(model), + model: data, }, }); } @@ -540,6 +548,12 @@ export default class DeliverWebhookTask extends BaseTask { env.VERSION ? `/${env.VERSION.slice(0, 7)}` : "" }`, }; + + const signature = subscription.signature(JSON.stringify(requestBody)); + if (signature) { + requestHeaders["Outline-Signature"] = signature; + } + response = await fetch(subscription.url, { method: "POST", headers: requestHeaders, diff --git a/server/routes/api/webhookSubscriptions.ts b/server/routes/api/webhookSubscriptions.ts index 71753935e..5cf7883d9 100644 --- a/server/routes/api/webhookSubscriptions.ts +++ b/server/routes/api/webhookSubscriptions.ts @@ -1,5 +1,5 @@ import Router from "koa-router"; -import { compact } from "lodash"; +import { compact, isEmpty } from "lodash"; import { ValidationError } from "@server/errors"; import auth from "@server/middlewares/authentication"; import { WebhookSubscription, Event } from "@server/models"; @@ -41,7 +41,7 @@ router.post( const { user } = ctx.state; authorize(user, "createWebhookSubscription", user.team); - const { name, url } = ctx.request.body; + const { name, url, secret } = ctx.request.body; const events: string[] = compact(ctx.request.body.events); assertPresent(name, "name is required"); assertPresent(url, "url is required"); @@ -57,6 +57,7 @@ router.post( teamId: user.teamId, url, enabled: true, + secret: isEmpty(secret) ? undefined : secret, }); const event: WebhookSubscriptionEvent = { @@ -116,7 +117,7 @@ router.post( assertUuid(id, "id is required"); const { user } = ctx.state; - const { name, url } = ctx.request.body; + const { name, url, secret } = ctx.request.body; const events: string[] = compact(ctx.request.body.events); assertPresent(name, "name is required"); assertPresent(url, "url is required"); @@ -129,7 +130,13 @@ router.post( authorize(user, "update", webhookSubscription); - await webhookSubscription.update({ name, url, events, enabled: true }); + await webhookSubscription.update({ + name, + url, + events, + enabled: true, + secret: isEmpty(secret) ? undefined : secret, + }); const event: WebhookSubscriptionEvent = { name: "webhook_subscriptions.update", diff --git a/shared/i18n/locales/en_US/translation.json b/shared/i18n/locales/en_US/translation.json index 3582ba2ce..a82b5e034 100644 --- a/shared/i18n/locales/en_US/translation.json +++ b/shared/i18n/locales/en_US/translation.json @@ -619,7 +619,11 @@ "Updating": "Updating", "Provide a descriptive name for this webhook and the URL we should send a POST request to when matching events are created.": "Provide a descriptive name for this webhook and the URL we should send a POST request to when matching events are created.", "Subscribe to all events, groups, or individual events. We recommend only subscribing to the minimum amount of events that your application needs to function.": "Subscribe to all events, groups, or individual events. We recommend only subscribing to the minimum amount of events that your application needs to function.", + "A memorable identifer": "A memorable identifer", "URL": "URL", + "Secret": "Secret", + "Optional": "Optional", + "Used to sign payload": "Used to sign payload", "All events": "All events", "All {{ groupName }} events": "All {{ groupName }} events", "Delete webhook": "Delete webhook",