From f1959726fe98fc251c7fac6d81d82bac3126d3fe Mon Sep 17 00:00:00 2001 From: Heikki Hellgren Date: Thu, 15 Aug 2024 08:37:08 +0300 Subject: [PATCH] fix: validate notification link when posting fixes an issue that allows posting notifications containing links with cross-site scripting attack Signed-off-by: Heikki Hellgren --- .changeset/rotten-crabs-hear.md | 5 ++ plugins/notifications-backend/src/plugin.ts | 3 + .../src/service/router.test.ts | 87 ++++++++++++++++++- .../src/service/router.ts | 26 +++++- 4 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 .changeset/rotten-crabs-hear.md diff --git a/.changeset/rotten-crabs-hear.md b/.changeset/rotten-crabs-hear.md new file mode 100644 index 0000000000..69148c59c1 --- /dev/null +++ b/.changeset/rotten-crabs-hear.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-notifications-backend': patch +--- + +Validate notification link when new notification is created diff --git a/plugins/notifications-backend/src/plugin.ts b/plugins/notifications-backend/src/plugin.ts index c0344fb06f..288a0f8507 100644 --- a/plugins/notifications-backend/src/plugin.ts +++ b/plugins/notifications-backend/src/plugin.ts @@ -67,6 +67,7 @@ export const notificationsPlugin = createBackendPlugin({ database: coreServices.database, discovery: coreServices.discovery, signals: signalsServiceRef, + config: coreServices.rootConfig, }, async init({ auth, @@ -77,6 +78,7 @@ export const notificationsPlugin = createBackendPlugin({ database, discovery, signals, + config, }) { httpRouter.use( await createRouter({ @@ -84,6 +86,7 @@ export const notificationsPlugin = createBackendPlugin({ httpAuth, userInfo, logger, + config, database, discovery, signals, diff --git a/plugins/notifications-backend/src/service/router.test.ts b/plugins/notifications-backend/src/service/router.test.ts index 804b0ff088..a3a66636ca 100644 --- a/plugins/notifications-backend/src/service/router.test.ts +++ b/plugins/notifications-backend/src/service/router.test.ts @@ -23,7 +23,8 @@ import request from 'supertest'; import { createRouter } from './router'; import { ConfigReader } from '@backstage/config'; import { SignalsService } from '@backstage/plugin-signals-node'; -import { mockServices } from '@backstage/backend-test-utils'; +import { mockCredentials, mockServices } from '@backstage/backend-test-utils'; +import { NotificationSendOptions } from '@backstage/plugin-notifications-node'; function createDatabase(): PluginDatabaseManager { return DatabaseManager.fromConfig( @@ -47,8 +48,13 @@ describe('createRouter', () => { const discovery = mockServices.discovery(); const userInfo = mockServices.userInfo(); - const httpAuth = mockServices.httpAuth(); + const httpAuth = mockServices.httpAuth({ + defaultCredentials: mockCredentials.service(), + }); const auth = mockServices.auth(); + const config = mockServices.rootConfig({ + data: { app: { baseUrl: 'http://localhost' } }, + }); beforeAll(async () => { const router = await createRouter({ @@ -57,6 +63,7 @@ describe('createRouter', () => { discovery, signals: signalService, userInfo, + config, httpAuth, auth, }); @@ -75,4 +82,80 @@ describe('createRouter', () => { expect(response.body).toEqual({ status: 'ok' }); }); }); + + describe('POST /', () => { + const sendNotification = async (data: NotificationSendOptions) => + request(app) + .post('/') + .send(data) + .set('Content-Type', 'application/json') + .set('Accept', 'application/json'); + + it('returns error on invalid link', async () => { + const javascriptXSS = await sendNotification({ + recipients: { + type: 'broadcast', + }, + payload: { + title: 'test notification', + // eslint-disable-next-line no-script-url + link: 'javascript:alert(document.domain)', + }, + }); + + expect(javascriptXSS.status).toEqual(400); + + const ftpLink = await sendNotification({ + recipients: { + type: 'broadcast', + }, + payload: { + title: 'test notification', + link: 'ftp://example.com', + }, + }); + + expect(ftpLink.status).toEqual(400); + }); + + it('should accept absolute http links', async () => { + const httpLink = await sendNotification({ + recipients: { + type: 'broadcast', + }, + payload: { + title: 'test notification', + link: 'http://localhost/test', + }, + }); + + expect(httpLink.status).toEqual(200); + + const httpsLink = await sendNotification({ + recipients: { + type: 'broadcast', + }, + payload: { + title: 'test notification', + link: 'https://example.com', + }, + }); + + expect(httpsLink.status).toEqual(200); + }); + + it('should accept relative links', async () => { + const catalogLink = await sendNotification({ + recipients: { + type: 'broadcast', + }, + payload: { + title: 'test notification', + link: '/catalog', + }, + }); + + expect(catalogLink.status).toEqual(200); + }); + }); }); diff --git a/plugins/notifications-backend/src/service/router.ts b/plugins/notifications-backend/src/service/router.ts index abd7e2dc13..95b9467f4a 100644 --- a/plugins/notifications-backend/src/service/router.ts +++ b/plugins/notifications-backend/src/service/router.ts @@ -46,10 +46,12 @@ import { } from '@backstage/plugin-notifications-common'; import { parseEntityOrderFieldParams } from './parseEntityOrderFieldParams'; import { getUsersForEntityRef } from './getUsersForEntityRef'; +import { Config } from '@backstage/config'; /** @internal */ export interface RouterOptions { logger: LoggerService; + config: Config; database: PluginDatabaseManager; discovery: DiscoveryService; auth: AuthService; @@ -65,6 +67,7 @@ export async function createRouter( options: RouterOptions, ): Promise { const { + config, logger, database, auth, @@ -79,6 +82,7 @@ export async function createRouter( const catalogClient = catalog ?? new CatalogClient({ discoveryApi: discovery }); const store = await DatabaseNotificationsStore.create({ database }); + const frontendBaseUrl = config.getString('app.baseUrl'); const getUser = async (req: Request) => { const credentials = await httpAuth.credentials(req, { allow: ['user'] }); @@ -177,6 +181,18 @@ export async function createRouter( } }; + const validateLink = (link: string) => { + const stripLeadingSlash = (s: string) => s.replace(/^\//, ''); + const ensureTrailingSlash = (s: string) => s.replace(/\/?$/, '/'); + const url = new URL( + stripLeadingSlash(link), + ensureTrailingSlash(frontendBaseUrl), + ); + if (url.protocol !== 'https:' && url.protocol !== 'http:') { + throw new Error('Only HTTP/HTTPS links are allowed'); + } + }; + // TODO: Move to use OpenAPI router instead const router = Router(); router.use(express.json()); @@ -419,13 +435,21 @@ export async function createRouter( allow: ['service'], }); - const { title } = payload; + const { title, link } = payload; if (!recipients || !title) { logger.error(`Invalid notification request received`); throw new InputError(`Invalid notification request received`); } + if (link) { + try { + validateLink(link); + } catch (e) { + throw new InputError('Invalid link provided', e); + } + } + const origin = credentials.principal.subject; const baseNotification = { payload: {