paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(events,github): fixes signature validation by using raw req body

Browse Source 
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 <cdiaz@rvohealth.com>
Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
This commit is contained in:
Patrick Jungermann
2024-10-14 19:52:54 +02:00
parent b1dbdddc87
commit 9816f510dc
15 changed files with 293 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/seven-hotels-move.md
+6
View File
@@ -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.
.changeset/shiny-falcons-fly.md
+8
View File
@@ -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.
plugins/events-backend-module-github/src/http/createGithubSignatureValidator.test.ts
+7 -2
View File
@@ -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;
};
plugins/events-backend-module-github/src/http/createGithubSignatureValidator.ts
+5 -1
View File
@@ -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,
plugins/events-backend-module-github/src/service/eventsModuleGithubWebhook.test.ts
+7 -2
View File
@@ -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;
};
plugins/events-backend-module-gitlab/src/http/createGitlabTokenValidator.test.ts
+1 -2
View File
@@ -49,11 +49,10 @@ describe('createGitlabTokenValidator', () => {
const requestWithToken = (token: string | undefined) => {
return {
body: undefined,
headers: {
'x-gitlab-token': token,
},
} as RequestDetails;
} as Partial<RequestDetails> as unknown as RequestDetails;
};
it('no secret configured, throw error', async () => {
plugins/events-backend-module-gitlab/src/service/eventsModuleGitlabWebhook.test.ts
+1 -2
View File
@@ -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<RequestDetails> as unknown as RequestDetails;
};
it('should be correctly wired and set up', async () => {
plugins/events-backend/package.json
+2
View File
@@ -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"
plugins/events-backend/src/service/EventsPlugin.test.ts
+4 -2
View File
@@ -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);
plugins/events-backend/src/service/EventsPlugin.ts
+14 -10
View File
@@ -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',
});
plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.test.ts
+136 -9
View File
@@ -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({});
plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.ts
+70 -15
View File
@@ -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<HttpPostIngressOptions, 'topic'>;
}): 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`);
}
}
plugins/events-node/report.api.md
+7 -1
View File
@@ -3,6 +3,8 @@
> Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/).
```ts
/// <reference types="node" />
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<string, string | string[] | undefined>;
raw: {
body: Buffer;
encoding: BufferEncoding;
};
}
// @public
plugins/events-node/src/api/http/validation/RequestDetails.ts
+16
View File
@@ -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<string, string | string[] | undefined>;
/**
* 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;
};
}
yarn.lock
+9
View File
@@ -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"