From a41fbfe73973dc8a53f647966228a691d331d8b6 Mon Sep 17 00:00:00 2001 From: Iain Billett Date: Tue, 14 Dec 2021 09:39:27 +0000 Subject: [PATCH 1/5] Search result location filtering Introduces filtering of unsafe search result locations. Signed-off-by: Iain Billett --- .changeset/sixty-pandas-switch.md | 8 ++ packages/backend/src/plugins/search.ts | 1 + .../search-backend/src/service/router.test.ts | 79 ++++++++++++++++++- plugins/search-backend/src/service/router.ts | 32 +++++++- 4 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 .changeset/sixty-pandas-switch.md diff --git a/.changeset/sixty-pandas-switch.md b/.changeset/sixty-pandas-switch.md new file mode 100644 index 0000000000..91e767e154 --- /dev/null +++ b/.changeset/sixty-pandas-switch.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-search-backend': minor +--- + +Search result location filtering + +This change introduces a filter for search results based on their location protocol. The intention is to filter out unsafe or +malicious values before they can be consumed by the frontend. By default locations must be http/https URLs (or paths). diff --git a/packages/backend/src/plugins/search.ts b/packages/backend/src/plugins/search.ts index 9a8db0f0f9..5659968e1d 100644 --- a/packages/backend/src/plugins/search.ts +++ b/packages/backend/src/plugins/search.ts @@ -96,5 +96,6 @@ export default async function createPlugin({ return await createRouter({ engine: indexBuilder.getSearchEngine(), logger, + discovery, }); } diff --git a/plugins/search-backend/src/service/router.test.ts b/plugins/search-backend/src/service/router.test.ts index 4b3cb30264..aeb5709379 100644 --- a/plugins/search-backend/src/service/router.test.ts +++ b/plugins/search-backend/src/service/router.test.ts @@ -14,10 +14,14 @@ * limitations under the License. */ -import { getVoidLogger } from '@backstage/backend-common'; +import { + getVoidLogger, + PluginEndpointDiscovery, +} from '@backstage/backend-common'; import { IndexBuilder, LunrSearchEngine, + SearchEngine, } from '@backstage/plugin-search-backend-node'; import express from 'express'; import request from 'supertest'; @@ -26,14 +30,22 @@ import { createRouter } from './router'; describe('createRouter', () => { let app: express.Express; + let mockDiscoveryApi: jest.Mocked; + let mockSearchEngine: jest.Mocked; beforeAll(async () => { const logger = getVoidLogger(); const searchEngine = new LunrSearchEngine({ logger }); const indexBuilder = new IndexBuilder({ logger, searchEngine }); + mockDiscoveryApi = { + getBaseUrl: jest.fn(), + getExternalBaseUrl: jest.fn().mockResolvedValue('http://localhost:3000/'), + }; + const router = await createRouter({ engine: indexBuilder.getSearchEngine(), logger, + discovery: mockDiscoveryApi, }); app = express().use(router); }); @@ -49,5 +61,70 @@ describe('createRouter', () => { expect(response.status).toEqual(200); expect(response.body).toMatchObject({ results: [] }); }); + + describe('search result filtering', () => { + beforeAll(async () => { + const logger = getVoidLogger(); + mockDiscoveryApi = { + getBaseUrl: jest.fn(), + getExternalBaseUrl: jest + .fn() + .mockResolvedValue('http://localhost:3000/'), + }; + mockSearchEngine = { + index: jest.fn(), + setTranslator: jest.fn(), + query: jest.fn(), + }; + const indexBuilder = new IndexBuilder({ + logger, + searchEngine: mockSearchEngine, + }); + + const router = await createRouter({ + engine: indexBuilder.getSearchEngine(), + logger, + discovery: mockDiscoveryApi, + }); + app = express().use(router); + }); + + describe('where the search result set includes unsafe results', () => { + const safeResult = { + type: 'software-catalog', + document: { + text: 'safe', + title: 'safe-location', + // eslint-disable-next-line no-script-url + location: '/catalog/default/component/safe', + }, + }; + beforeEach(() => { + mockSearchEngine.query.mockResolvedValue({ + results: [ + { + type: 'software-catalog', + document: { + text: 'unsafe', + title: 'unsafe-location', + // eslint-disable-next-line no-script-url + location: 'javascript:alert("unsafe")', + }, + }, + safeResult, + ], + nextPageCursor: '', + previousPageCursor: '', + }); + }); + + it('removes the unsafe results', async () => { + const response = await request(app).get('/query'); + + expect(response.status).toEqual(200); + expect(response.body).toMatchObject({ results: [safeResult] }); + }); + }); + }); }); }); diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index 5bd99988a7..e13e0cc3bf 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -19,16 +19,42 @@ import Router from 'express-promise-router'; import { Logger } from 'winston'; import { SearchQuery, SearchResultSet } from '@backstage/search-common'; import { SearchEngine } from '@backstage/plugin-search-backend-node'; +import { PluginEndpointDiscovery } from '@backstage/backend-common'; export type RouterOptions = { engine: SearchEngine; logger: Logger; + discovery: PluginEndpointDiscovery; + allowedLocationProtocols?: string[]; }; +const defaultAllowedLocationProtocols = ['http:', 'https:']; + export async function createRouter( options: RouterOptions, ): Promise { - const { engine, logger } = options; + const { + engine, + logger, + discovery, + allowedLocationProtocols = defaultAllowedLocationProtocols, + } = options; + const baseUrl = await discovery.getExternalBaseUrl(''); + + const filterResultSet = ({ results, ...resultSet }: SearchResultSet) => ({ + ...resultSet, + results: results.filter(result => { + const protocol = new URL(result.document.location, baseUrl).protocol; + const isAllowed = allowedLocationProtocols.includes(protocol); + if (!isAllowed) { + logger.info( + `Rejected search result for "${result.document.title}" as location protocol "${protocol}" is unsafe`, + ); + } + return isAllowed; + }), + }); + const router = Router(); router.get( '/query', @@ -46,8 +72,8 @@ export async function createRouter( ); try { - const results = await engine?.query(req.query); - res.send(results); + const resultSet = await engine?.query(req.query); + res.send(filterResultSet(resultSet)); } catch (err) { throw new Error( `There was a problem performing the search query. ${err}`, From 4ce74edaf6857a29126b10596862ecf6d98655ec Mon Sep 17 00:00:00 2001 From: Iain Billett Date: Tue, 14 Dec 2021 16:34:16 +0000 Subject: [PATCH 2/5] Fix standalone server Signed-off-by: Iain Billett --- plugins/search-backend/src/service/standaloneServer.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/plugins/search-backend/src/service/standaloneServer.ts b/plugins/search-backend/src/service/standaloneServer.ts index 9ba9dbd5f7..ce775c205a 100644 --- a/plugins/search-backend/src/service/standaloneServer.ts +++ b/plugins/search-backend/src/service/standaloneServer.ts @@ -14,7 +14,11 @@ * limitations under the License. */ -import { createServiceBuilder } from '@backstage/backend-common'; +import { + createServiceBuilder, + loadBackendConfig, + SingleHostDiscovery, +} from '@backstage/backend-common'; import { Server } from 'http'; import { Logger } from 'winston'; import { createRouter } from './router'; @@ -33,6 +37,8 @@ export async function startStandaloneServer( options: ServerOptions, ): Promise { const logger = options.logger.child({ service: 'search-backend' }); + const config = await loadBackendConfig({ logger, argv: process.argv }); + const discovery = SingleHostDiscovery.fromConfig(config); const searchEngine = new LunrSearchEngine({ logger }); const indexBuilder = new IndexBuilder({ logger, searchEngine }); logger.debug('Starting application server...'); @@ -42,6 +48,7 @@ export async function startStandaloneServer( const router = await createRouter({ engine: indexBuilder.getSearchEngine(), logger, + discovery, }); let service = createServiceBuilder(module) From 801323b9230903d5de701483861e503f8f6996fe Mon Sep 17 00:00:00 2001 From: Iain Billett Date: Tue, 14 Dec 2021 17:47:48 +0000 Subject: [PATCH 3/5] Api Report Signed-off-by: Iain Billett --- plugins/search-backend/api-report.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugins/search-backend/api-report.md b/plugins/search-backend/api-report.md index cac97bd725..c4558f1b59 100644 --- a/plugins/search-backend/api-report.md +++ b/plugins/search-backend/api-report.md @@ -5,6 +5,7 @@ ```ts import express from 'express'; import { Logger as Logger_2 } from 'winston'; +import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { SearchEngine } from '@backstage/plugin-search-backend-node'; // Warning: (ae-missing-release-tag) "createRouter" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) @@ -18,5 +19,7 @@ export function createRouter(options: RouterOptions): Promise; export type RouterOptions = { engine: SearchEngine; logger: Logger_2; + discovery: PluginEndpointDiscovery; + allowedLocationProtocols?: string[]; }; ``` From a91c4dd05a4bd47cc72ddeab21c65b22a0a933ad Mon Sep 17 00:00:00 2001 From: Iain Billett Date: Tue, 14 Dec 2021 18:18:05 +0000 Subject: [PATCH 4/5] Update create-app template Signed-off-by: Iain Billett --- .../templates/default-app/packages/backend/src/plugins/search.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts b/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts index f23b0c7bcf..74ea8f032c 100644 --- a/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts +++ b/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts @@ -50,5 +50,6 @@ export default async function createPlugin({ return await createRouter({ engine: indexBuilder.getSearchEngine(), logger, + discovery, }); } From 651d1e7696f13ccba3178b2a6fb811acfdc6c116 Mon Sep 17 00:00:00 2001 From: Iain Billett Date: Wed, 15 Dec 2021 11:50:44 +0000 Subject: [PATCH 5/5] Don't use discovery API Signed-off-by: Iain Billett --- packages/backend/src/plugins/search.ts | 1 - .../packages/backend/src/plugins/search.ts | 1 - plugins/search-backend/api-report.md | 3 --- .../search-backend/src/service/router.test.ts | 18 +----------------- plugins/search-backend/src/service/router.ts | 16 ++++------------ .../src/service/standaloneServer.ts | 9 +-------- 6 files changed, 6 insertions(+), 42 deletions(-) diff --git a/packages/backend/src/plugins/search.ts b/packages/backend/src/plugins/search.ts index 5659968e1d..9a8db0f0f9 100644 --- a/packages/backend/src/plugins/search.ts +++ b/packages/backend/src/plugins/search.ts @@ -96,6 +96,5 @@ export default async function createPlugin({ return await createRouter({ engine: indexBuilder.getSearchEngine(), logger, - discovery, }); } diff --git a/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts b/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts index 74ea8f032c..f23b0c7bcf 100644 --- a/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts +++ b/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts @@ -50,6 +50,5 @@ export default async function createPlugin({ return await createRouter({ engine: indexBuilder.getSearchEngine(), logger, - discovery, }); } diff --git a/plugins/search-backend/api-report.md b/plugins/search-backend/api-report.md index c4558f1b59..cac97bd725 100644 --- a/plugins/search-backend/api-report.md +++ b/plugins/search-backend/api-report.md @@ -5,7 +5,6 @@ ```ts import express from 'express'; import { Logger as Logger_2 } from 'winston'; -import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { SearchEngine } from '@backstage/plugin-search-backend-node'; // Warning: (ae-missing-release-tag) "createRouter" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) @@ -19,7 +18,5 @@ export function createRouter(options: RouterOptions): Promise; export type RouterOptions = { engine: SearchEngine; logger: Logger_2; - discovery: PluginEndpointDiscovery; - allowedLocationProtocols?: string[]; }; ``` diff --git a/plugins/search-backend/src/service/router.test.ts b/plugins/search-backend/src/service/router.test.ts index aeb5709379..77a1be1eb4 100644 --- a/plugins/search-backend/src/service/router.test.ts +++ b/plugins/search-backend/src/service/router.test.ts @@ -14,10 +14,7 @@ * limitations under the License. */ -import { - getVoidLogger, - PluginEndpointDiscovery, -} from '@backstage/backend-common'; +import { getVoidLogger } from '@backstage/backend-common'; import { IndexBuilder, LunrSearchEngine, @@ -30,22 +27,16 @@ import { createRouter } from './router'; describe('createRouter', () => { let app: express.Express; - let mockDiscoveryApi: jest.Mocked; let mockSearchEngine: jest.Mocked; beforeAll(async () => { const logger = getVoidLogger(); const searchEngine = new LunrSearchEngine({ logger }); const indexBuilder = new IndexBuilder({ logger, searchEngine }); - mockDiscoveryApi = { - getBaseUrl: jest.fn(), - getExternalBaseUrl: jest.fn().mockResolvedValue('http://localhost:3000/'), - }; const router = await createRouter({ engine: indexBuilder.getSearchEngine(), logger, - discovery: mockDiscoveryApi, }); app = express().use(router); }); @@ -65,12 +56,6 @@ describe('createRouter', () => { describe('search result filtering', () => { beforeAll(async () => { const logger = getVoidLogger(); - mockDiscoveryApi = { - getBaseUrl: jest.fn(), - getExternalBaseUrl: jest - .fn() - .mockResolvedValue('http://localhost:3000/'), - }; mockSearchEngine = { index: jest.fn(), setTranslator: jest.fn(), @@ -84,7 +69,6 @@ describe('createRouter', () => { const router = await createRouter({ engine: indexBuilder.getSearchEngine(), logger, - discovery: mockDiscoveryApi, }); app = express().use(router); }); diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index e13e0cc3bf..aae1914fc1 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -19,32 +19,24 @@ import Router from 'express-promise-router'; import { Logger } from 'winston'; import { SearchQuery, SearchResultSet } from '@backstage/search-common'; import { SearchEngine } from '@backstage/plugin-search-backend-node'; -import { PluginEndpointDiscovery } from '@backstage/backend-common'; export type RouterOptions = { engine: SearchEngine; logger: Logger; - discovery: PluginEndpointDiscovery; - allowedLocationProtocols?: string[]; }; -const defaultAllowedLocationProtocols = ['http:', 'https:']; +const allowedLocationProtocols = ['http:', 'https:']; export async function createRouter( options: RouterOptions, ): Promise { - const { - engine, - logger, - discovery, - allowedLocationProtocols = defaultAllowedLocationProtocols, - } = options; - const baseUrl = await discovery.getExternalBaseUrl(''); + const { engine, logger } = options; const filterResultSet = ({ results, ...resultSet }: SearchResultSet) => ({ ...resultSet, results: results.filter(result => { - const protocol = new URL(result.document.location, baseUrl).protocol; + const protocol = new URL(result.document.location, 'https://example.com') + .protocol; const isAllowed = allowedLocationProtocols.includes(protocol); if (!isAllowed) { logger.info( diff --git a/plugins/search-backend/src/service/standaloneServer.ts b/plugins/search-backend/src/service/standaloneServer.ts index ce775c205a..9ba9dbd5f7 100644 --- a/plugins/search-backend/src/service/standaloneServer.ts +++ b/plugins/search-backend/src/service/standaloneServer.ts @@ -14,11 +14,7 @@ * limitations under the License. */ -import { - createServiceBuilder, - loadBackendConfig, - SingleHostDiscovery, -} from '@backstage/backend-common'; +import { createServiceBuilder } from '@backstage/backend-common'; import { Server } from 'http'; import { Logger } from 'winston'; import { createRouter } from './router'; @@ -37,8 +33,6 @@ export async function startStandaloneServer( options: ServerOptions, ): Promise { const logger = options.logger.child({ service: 'search-backend' }); - const config = await loadBackendConfig({ logger, argv: process.argv }); - const discovery = SingleHostDiscovery.fromConfig(config); const searchEngine = new LunrSearchEngine({ logger }); const indexBuilder = new IndexBuilder({ logger, searchEngine }); logger.debug('Starting application server...'); @@ -48,7 +42,6 @@ export async function startStandaloneServer( const router = await createRouter({ engine: indexBuilder.getSearchEngine(), logger, - discovery, }); let service = createServiceBuilder(module)