From 9816f510dc9184b77e467ac648eea07ddb524bde Mon Sep 17 00:00:00 2001 From: Patrick Jungermann Date: Mon, 14 Oct 2024 19:52:54 +0200 Subject: [PATCH] fix(events,github): fixes signature validation by using raw req body Adds raw body information (body as buffer, encoding) to `RequestDetails` to support more request validation use cases. Additionally, uses the raw body to retrieve the transmitted JSON string unparsed/raw to correctly validate the signature. Previously, we re-stringified the parsed JSON payload which could lead to different JSON strings. Those differences can lead to the rejection of requests due to a mismatch in expected signature. Fixes: #26709 Relates-to: PR #26884 Co-authored-by: Christopher Diaz Signed-off-by: Patrick Jungermann --- .changeset/seven-hotels-move.md | 6 + .changeset/shiny-falcons-fly.md | 8 + .../createGithubSignatureValidator.test.ts | 9 +- .../http/createGithubSignatureValidator.ts | 6 +- .../service/eventsModuleGithubWebhook.test.ts | 9 +- .../http/createGitlabTokenValidator.test.ts | 3 +- .../service/eventsModuleGitlabWebhook.test.ts | 3 +- plugins/events-backend/package.json | 2 + .../src/service/EventsPlugin.test.ts | 6 +- .../src/service/EventsPlugin.ts | 24 +-- .../HttpPostIngressEventPublisher.test.ts | 145 ++++++++++++++++-- .../http/HttpPostIngressEventPublisher.ts | 85 ++++++++-- plugins/events-node/report.api.md | 8 +- .../src/api/http/validation/RequestDetails.ts | 16 ++ yarn.lock | 9 ++ 15 files changed, 293 insertions(+), 46 deletions(-) create mode 100644 .changeset/seven-hotels-move.md create mode 100644 .changeset/shiny-falcons-fly.md diff --git a/.changeset/seven-hotels-move.md b/.changeset/seven-hotels-move.md new file mode 100644 index 0000000000..03bb39116a --- /dev/null +++ b/.changeset/seven-hotels-move.md @@ -0,0 +1,6 @@ +--- +'@backstage/plugin-events-backend-module-github': patch +--- + +Fix the event request validation for incoming requests for GitHub webhook events +by using the raw body when verifying the signature. diff --git a/.changeset/shiny-falcons-fly.md b/.changeset/shiny-falcons-fly.md new file mode 100644 index 0000000000..899bbfbda4 --- /dev/null +++ b/.changeset/shiny-falcons-fly.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-events-node': patch +'@backstage/plugin-events-backend-module-github': patch +'@backstage/plugin-events-backend': patch +--- + +Add raw body information to `RequestDetails` +and use the raw body when validating incoming event requests. diff --git a/plugins/events-backend-module-github/src/http/createGithubSignatureValidator.test.ts b/plugins/events-backend-module-github/src/http/createGithubSignatureValidator.test.ts index 32d67437a8..3c96c98996 100644 --- a/plugins/events-backend-module-github/src/http/createGithubSignatureValidator.test.ts +++ b/plugins/events-backend-module-github/src/http/createGithubSignatureValidator.test.ts @@ -47,8 +47,9 @@ describe('createGithubSignatureValidator', () => { }, }, }); - const payload = { test: 'payload' }; - const payloadString = JSON.stringify(payload); + const payloadString = '{"test": "payload", "score": 5.0}'; + const payload = JSON.parse(payloadString); + const payloadBuffer = Buffer.from(payloadString); const validSignature = sign({ secret, algorithm: 'sha256' }, payloadString); const requestWithSignature = async (signature: string | undefined) => { @@ -57,6 +58,10 @@ describe('createGithubSignatureValidator', () => { headers: { 'x-hub-signature-256': signature, }, + raw: { + body: payloadBuffer, + encoding: 'utf-8', + }, } as RequestDetails; }; diff --git a/plugins/events-backend-module-github/src/http/createGithubSignatureValidator.ts b/plugins/events-backend-module-github/src/http/createGithubSignatureValidator.ts index e977640ce8..87cb710697 100644 --- a/plugins/events-backend-module-github/src/http/createGithubSignatureValidator.ts +++ b/plugins/events-backend-module-github/src/http/createGithubSignatureValidator.ts @@ -48,7 +48,11 @@ export function createGithubSignatureValidator( if ( !signature || - !(await verify(secret, JSON.stringify(request.body), signature)) + !(await verify( + secret, + request.raw.body.toString(request.raw.encoding), + signature, + )) ) { context.reject({ status: 403, diff --git a/plugins/events-backend-module-github/src/service/eventsModuleGithubWebhook.test.ts b/plugins/events-backend-module-github/src/service/eventsModuleGithubWebhook.test.ts index 460022b04d..ec5ec66946 100644 --- a/plugins/events-backend-module-github/src/service/eventsModuleGithubWebhook.test.ts +++ b/plugins/events-backend-module-github/src/service/eventsModuleGithubWebhook.test.ts @@ -25,8 +25,9 @@ import { eventsModuleGithubWebhook } from './eventsModuleGithubWebhook'; describe('eventsModuleGithubWebhook', () => { const secret = 'valid-secret'; - const payload = { test: 'payload' }; - const payloadString = JSON.stringify(payload); + const payloadString = '{"test": "payload", "score": 5.0}'; + const payload = JSON.parse(payloadString); + const payloadBuffer = Buffer.from(payloadString); const validSignature = sign({ secret, algorithm: 'sha256' }, payloadString); const requestWithSignature = async (signature?: string) => { return { @@ -34,6 +35,10 @@ describe('eventsModuleGithubWebhook', () => { headers: { 'x-hub-signature-256': signature, }, + raw: { + body: payloadBuffer, + encoding: 'utf-8', + }, } as RequestDetails; }; diff --git a/plugins/events-backend-module-gitlab/src/http/createGitlabTokenValidator.test.ts b/plugins/events-backend-module-gitlab/src/http/createGitlabTokenValidator.test.ts index 33e677450e..72fee279ea 100644 --- a/plugins/events-backend-module-gitlab/src/http/createGitlabTokenValidator.test.ts +++ b/plugins/events-backend-module-gitlab/src/http/createGitlabTokenValidator.test.ts @@ -49,11 +49,10 @@ describe('createGitlabTokenValidator', () => { const requestWithToken = (token: string | undefined) => { return { - body: undefined, headers: { 'x-gitlab-token': token, }, - } as RequestDetails; + } as Partial as unknown as RequestDetails; }; it('no secret configured, throw error', async () => { diff --git a/plugins/events-backend-module-gitlab/src/service/eventsModuleGitlabWebhook.test.ts b/plugins/events-backend-module-gitlab/src/service/eventsModuleGitlabWebhook.test.ts index 9a4f4326da..d3452dd813 100644 --- a/plugins/events-backend-module-gitlab/src/service/eventsModuleGitlabWebhook.test.ts +++ b/plugins/events-backend-module-gitlab/src/service/eventsModuleGitlabWebhook.test.ts @@ -25,11 +25,10 @@ import { eventsModuleGitlabWebhook } from './eventsModuleGitlabWebhook'; describe('gitlabWebhookEventsModule', () => { const requestWithToken = (token?: string) => { return { - body: undefined, headers: { 'x-gitlab-token': token, }, - } as RequestDetails; + } as Partial as unknown as RequestDetails; }; it('should be correctly wired and set up', async () => { diff --git a/plugins/events-backend/package.json b/plugins/events-backend/package.json index e2360e5d36..f44e269900 100644 --- a/plugins/events-backend/package.json +++ b/plugins/events-backend/package.json @@ -61,6 +61,7 @@ "@backstage/plugin-events-node": "workspace:^", "@backstage/types": "workspace:^", "@types/express": "^4.17.6", + "content-type": "^1.0.5", "express": "^4.17.1", "express-promise-router": "^4.1.0", "knex": "^3.0.0", @@ -73,6 +74,7 @@ "@backstage/cli": "workspace:^", "@backstage/plugin-events-backend-test-utils": "workspace:^", "@backstage/repo-tools": "workspace:^", + "@types/content-type": "^1.1.8", "supertest": "^7.0.0" }, "configSchema": "config.d.ts" diff --git a/plugins/events-backend/src/service/EventsPlugin.test.ts b/plugins/events-backend/src/service/EventsPlugin.test.ts index 2c0befe1fd..39962afdcc 100644 --- a/plugins/events-backend/src/service/EventsPlugin.test.ts +++ b/plugins/events-backend/src/service/EventsPlugin.test.ts @@ -83,14 +83,16 @@ describe('eventsPlugin', () => { const response1 = await request(server) .post('/api/events/http/fake') + .type('application/json') .timeout(1000) - .send({ test: 'fake' }); + .send(JSON.stringify({ test: 'fake' })); expect(response1.status).toBe(202); const response2 = await request(server) .post('/api/events/http/fake-ext') + .type('application/json') .timeout(1000) - .send({ test: 'fake-ext' }); + .send(JSON.stringify({ test: 'fake-ext' })); expect(response2.status).toBe(202); expect(eventsService.published).toHaveLength(2); diff --git a/plugins/events-backend/src/service/EventsPlugin.ts b/plugins/events-backend/src/service/EventsPlugin.ts index 10af3798ec..89f50d835b 100644 --- a/plugins/events-backend/src/service/EventsPlugin.ts +++ b/plugins/events-backend/src/service/EventsPlugin.ts @@ -76,21 +76,21 @@ export const eventsPlugin = createBackendPlugin({ config: coreServices.rootConfig, events: eventsServiceRef, database: coreServices.database, + httpAuth: coreServices.httpAuth, + httpRouter: coreServices.httpRouter, + lifecycle: coreServices.lifecycle, logger: coreServices.logger, scheduler: coreServices.scheduler, - lifecycle: coreServices.lifecycle, - httpAuth: coreServices.httpAuth, - router: coreServices.httpRouter, }, async init({ config, events, database, + httpAuth, + httpRouter, + lifecycle, logger, scheduler, - lifecycle, - httpAuth, - router, }) { const ingresses = Object.fromEntries( extensionPoint.httpPostIngresses.map(ingress => [ @@ -108,18 +108,22 @@ export const eventsPlugin = createBackendPlugin({ const eventsRouter = Router(); http.bind(eventsRouter); - router.use( + // MUST be registered *before* the event bus router. + // Otherwise, it would already make use of `express.json()` + // that is used there as part of the middleware stack. + httpRouter.use(eventsRouter); + + httpRouter.use( await createEventBusRouter({ database, + lifecycle, logger, httpAuth, scheduler, - lifecycle, }), ); - router.use(eventsRouter); - router.addAuthPolicy({ + httpRouter.addAuthPolicy({ allow: 'unauthenticated', path: '/http', }); diff --git a/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.test.ts b/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.test.ts index 2bffa4e511..32a24de169 100644 --- a/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.test.ts +++ b/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.test.ts @@ -50,22 +50,25 @@ describe('HttpPostIngressEventPublisher', () => { const notFoundResponse = await request(app) .post('/http/unknown') + .type('application/json') .timeout(1000) - .send({ test: 'data' }); + .send(JSON.stringify({ test: 'data' })); expect(notFoundResponse.status).toBe(404); const response1 = await request(app) .post('/http/testA') + .type('application/json') .set('X-Custom-Header', 'test-value') .timeout(1000) - .send({ testA: 'data' }); + .send(JSON.stringify({ testA: 'data' })); expect(response1.status).toBe(202); const response2 = await request(app) .post('/http/testB') + .type('application/json') .set('X-Custom-Header', 'test-value') .timeout(1000) - .send({ testB: 'data' }); + .send(JSON.stringify({ testB: 'data' })); expect(response2.status).toBe(202); expect(events.published).toHaveLength(2); @@ -87,6 +90,124 @@ describe('HttpPostIngressEventPublisher', () => { ); }); + it('no raw body', async () => { + const config = new ConfigReader({ + events: { + http: { + topics: ['testA'], + }, + }, + }); + + const router = Router(); + router.use(express.json()); // will prevent the raw body from being available + const app = express().use(router); + const events = new TestEventsService(); + + const publisher = HttpPostIngressEventPublisher.fromConfig({ + config, + events, + logger, + }); + publisher.bind(router); + + const response = await request(app) + .post('/http/testA') + .type('application/json; charset=utf-8') + .timeout(1000) + .send(JSON.stringify({ testA: 'data' })); + expect(response.status).toBe(500); + expect(response.body).toEqual( + expect.objectContaining({ + error: { + message: + 'Failed to retrieve raw body from incoming event for topic testA; not a buffer: object', + name: 'Error', + }, + request: { method: 'POST', url: '/testA' }, + response: { statusCode: 500 }, + }), + ); + }); + + it('invalid charset', async () => { + const config = new ConfigReader({ + events: { + http: { + topics: ['testA'], + }, + }, + }); + + const router = Router(); + const app = express().use(router); + const events = new TestEventsService(); + + const publisher = HttpPostIngressEventPublisher.fromConfig({ + config, + events, + logger, + }); + publisher.bind(router); + + const response = await request(app) + .post('/http/testA') + .type('application/json; charset=invalid') + .timeout(1000) + .send(JSON.stringify({ testA: 'data' })); + expect(response.status).toBe(415); + expect(response.body).toEqual( + expect.objectContaining({ + error: { + message: 'Unsupported charset: invalid', + name: 'UnsupportedCharsetError', + statusCode: 415, + }, + request: { method: 'POST', url: '/testA' }, + response: { statusCode: 415 }, + }), + ); + }); + + it('non-JSON media type', async () => { + const config = new ConfigReader({ + events: { + http: { + topics: ['testA'], + }, + }, + }); + + const router = Router(); + const app = express().use(router); + const events = new TestEventsService(); + + const publisher = HttpPostIngressEventPublisher.fromConfig({ + config, + events, + logger, + }); + publisher.bind(router); + + const response = await request(app) + .post('/http/testA') + .type('text/plain') + .timeout(1000) + .send('Textual information'); + expect(response.status).toBe(415); + expect(response.body).toEqual( + expect.objectContaining({ + error: { + message: 'Unsupported media type: text/plain', + name: 'UnsupportedMediaTypeError', + statusCode: 415, + }, + request: { method: 'POST', url: '/testA' }, + response: { statusCode: 415 }, + }), + ); + }); + it('with validator', async () => { const config = new ConfigReader({ events: { @@ -149,43 +270,49 @@ describe('HttpPostIngressEventPublisher', () => { const response1 = await request(app) .post('/http/testA') + .type('application/json') .timeout(1000) - .send({ test: 'data' }); + .send(JSON.stringify({ test: 'data' })); expect(response1.status).toBe(202); const response2 = await request(app) .post('/http/testB') + .type('application/json') .timeout(1000) - .send({ test: 'data' }); + .send(JSON.stringify({ test: 'data' })); expect(response2.status).toBe(400); expect(response2.body).toEqual({ message: 'wrong signature' }); const response3 = await request(app) .post('/http/testB') + .type('application/json') .set('X-Test-Signature', 'wrong') .timeout(1000) - .send({ test: 'data' }); + .send(JSON.stringify({ test: 'data' })); expect(response3.status).toBe(400); expect(response3.body).toEqual({ message: 'wrong signature' }); const response4 = await request(app) .post('/http/testB') + .type('application/json') .set('X-Test-Signature', 'testB-signature') .timeout(1000) - .send({ test: 'data' }); + .send(JSON.stringify({ test: 'data' })); expect(response4.status).toBe(202); const response5 = await request(app) .post('/http/testC') + .type('application/json') .timeout(1000) - .send({ test: 'data' }); + .send(JSON.stringify({ test: 'data' })); expect(response5.status).toBe(404); expect(response5.body).toEqual({}); const response6 = await request(app) .post('/http/testD') + .type('application/json') .timeout(1000) - .send({ test: 'data' }); + .send(JSON.stringify({ test: 'data' })); expect(response6.status).toBe(403); expect(response6.body).toEqual({}); diff --git a/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.ts b/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.ts index 06dc4e463a..fe17186856 100644 --- a/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.ts +++ b/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.ts @@ -17,15 +17,35 @@ import { errorHandler } from '@backstage/backend-common'; import { LoggerService } from '@backstage/backend-plugin-api'; import { Config } from '@backstage/config'; +import { CustomErrorBase } from '@backstage/errors'; import { EventsService, HttpPostIngressOptions, RequestValidator, } from '@backstage/plugin-events-node'; +import contentType from 'content-type'; import express from 'express'; import Router from 'express-promise-router'; import { RequestValidationContextImpl } from './validation'; +class UnsupportedCharsetError extends CustomErrorBase { + name = 'UnsupportedCharsetError' as const; + statusCode = 415 as const; + + constructor(charset: string) { + super(`Unsupported charset: ${charset}`); + } +} + +class UnsupportedMediaTypeError extends CustomErrorBase { + name = 'UnsupportedMediaTypeError' as const; + statusCode = 415 as const; + + constructor(mediaType?: string) { + super(`Unsupported media type: ${mediaType ?? 'unknown'}`); + } +} + /** * Publishes events received from their origin (e.g., webhook events from an SCM system) * via HTTP POST endpoint and passes the request body as event payload to the registered subscribers. @@ -71,7 +91,7 @@ export class HttpPostIngressEventPublisher { [topic: string]: Omit; }): express.Router { const router = Router(); - router.use(express.json()); + router.use(express.raw({ type: '*/*' })); Object.keys(ingresses).forEach(topic => this.addRouteForTopic(router, topic, ingresses[topic].validator), @@ -87,25 +107,60 @@ export class HttpPostIngressEventPublisher { validator?: RequestValidator, ): void { const path = `/${topic}`; + const logger = this.logger; router.post(path, async (request, response) => { - const requestDetails = { - body: request.body, - headers: request.headers, - }; - const context = new RequestValidationContextImpl(); - await validator?.(requestDetails, context); - if (context.wasRejected()) { - response - .status(context.rejectionDetails!.status) - .json(context.rejectionDetails!.payload); - return; + const requestBody = request.body; + if (!Buffer.isBuffer(requestBody)) { + throw new Error( + `Failed to retrieve raw body from incoming event for topic ${topic}; not a buffer: ${typeof requestBody}`, + ); + } + + const bodyBuffer: Buffer = requestBody; + const parsedContentType = contentType.parse(request); + if ( + !parsedContentType.type || + parsedContentType.type !== 'application/json' + ) { + throw new UnsupportedMediaTypeError(parsedContentType.type); + } + + const encoding = parsedContentType.parameters.charset ?? 'utf-8'; + if (!Buffer.isEncoding(encoding)) { + throw new UnsupportedCharsetError(encoding); + } + + const bodyString = bodyBuffer.toString(encoding); + const bodyParsed = + parsedContentType.type === 'application/json' + ? JSON.parse(bodyString) + : bodyString; + + if (validator) { + const requestDetails = { + body: bodyParsed, + headers: request.headers, + raw: { + body: bodyBuffer, + encoding: encoding as BufferEncoding, + }, + }; + + const context = new RequestValidationContextImpl(); + await validator(requestDetails, context); + + if (context.wasRejected()) { + response + .status(context.rejectionDetails!.status) + .json(context.rejectionDetails!.payload); + return; + } } - const eventPayload = request.body; await this.events.publish({ topic, - eventPayload, + eventPayload: bodyParsed, metadata: request.headers, }); @@ -114,6 +169,6 @@ export class HttpPostIngressEventPublisher { // TODO(pjungermann): We don't really know the externally defined path prefix here, // however it is more useful for users to have it. Is there a better way? - this.logger.info(`Registered /api/events/http${path} to receive events`); + logger.info(`Registered /api/events/http${path} to receive events`); } } diff --git a/plugins/events-node/report.api.md b/plugins/events-node/report.api.md index a9e028e049..0b9c31090a 100644 --- a/plugins/events-node/report.api.md +++ b/plugins/events-node/report.api.md @@ -3,6 +3,8 @@ > Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/). ```ts +/// + import { AuthService } from '@backstage/backend-plugin-api'; import { DiscoveryService } from '@backstage/backend-plugin-api'; import { LifecycleService } from '@backstage/backend-plugin-api'; @@ -114,10 +116,14 @@ export interface HttpPostIngressOptions { validator?: RequestValidator; } -// @public (undocumented) +// @public export interface RequestDetails { body: unknown; headers: Record; + raw: { + body: Buffer; + encoding: BufferEncoding; + }; } // @public diff --git a/plugins/events-node/src/api/http/validation/RequestDetails.ts b/plugins/events-node/src/api/http/validation/RequestDetails.ts index 83c2669503..fc99b6a228 100644 --- a/plugins/events-node/src/api/http/validation/RequestDetails.ts +++ b/plugins/events-node/src/api/http/validation/RequestDetails.ts @@ -15,6 +15,8 @@ */ /** + * View on an incoming request that has to be validated. + * * @public */ export interface RequestDetails { @@ -26,4 +28,18 @@ export interface RequestDetails { * Key-value pairs of header names and values. Header names are lower-cased. */ headers: Record; + /** + * Raw request details. + */ + raw: { + /** + * Raw request body (buffer). + */ + body: Buffer; + /** + * Encoding of the raw request body. + * Can be used to decode the raw request body like `raw.body.toString(raw.encoding)`. + */ + encoding: BufferEncoding; + }; } diff --git a/yarn.lock b/yarn.lock index 22aaacbd73..f57d896cba 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6599,7 +6599,9 @@ __metadata: "@backstage/plugin-events-node": "workspace:^" "@backstage/repo-tools": "workspace:^" "@backstage/types": "workspace:^" + "@types/content-type": ^1.1.8 "@types/express": ^4.17.6 + content-type: ^1.0.5 express: ^4.17.1 express-promise-router: ^4.1.0 knex: ^3.0.0 @@ -17843,6 +17845,13 @@ __metadata: languageName: node linkType: hard +"@types/content-type@npm:^1.1.8": + version: 1.1.8 + resolution: "@types/content-type@npm:1.1.8" + checksum: 2dd15e51925db7208b0d989c3a93d805a0e5e0942aa9edd70a1c3520896b772526d8280e344a674ae68a96a24aa8fce290843a07512460176f36a3020d99c792 + languageName: node + linkType: hard + "@types/cookie-parser@npm:^1.4.2": version: 1.4.7 resolution: "@types/cookie-parser@npm:1.4.7"