From f2b339a30cc3f0343e8bd366f5e33b4cc9f3b371 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Fri, 28 May 2021 14:02:15 +0200 Subject: [PATCH 1/9] Fix existing GCS tests to match others. Signed-off-by: Eric Peterson --- packages/techdocs-common/__mocks__/@google-cloud/storage.ts | 3 +++ .../techdocs-common/src/stages/publish/googleStorage.test.ts | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/techdocs-common/__mocks__/@google-cloud/storage.ts b/packages/techdocs-common/__mocks__/@google-cloud/storage.ts index 5e9890cd98..684c4023d7 100644 --- a/packages/techdocs-common/__mocks__/@google-cloud/storage.ts +++ b/packages/techdocs-common/__mocks__/@google-cloud/storage.ts @@ -63,6 +63,9 @@ class GCSFile { process.nextTick(() => { if (fs.existsSync(this.localFilePath)) { + if (readable.eventNames().includes('pipe')) { + readable.emit('pipe'); + } readable.emit('data', fs.readFileSync(this.localFilePath)); readable.emit('end'); } else { diff --git a/packages/techdocs-common/src/stages/publish/googleStorage.test.ts b/packages/techdocs-common/src/stages/publish/googleStorage.test.ts index f29e1f10be..c4c032015e 100644 --- a/packages/techdocs-common/src/stages/publish/googleStorage.test.ts +++ b/packages/techdocs-common/src/stages/publish/googleStorage.test.ts @@ -309,7 +309,9 @@ describe('GoogleGCSPublish', () => { const pngResponse = await request(app).get( `/${namespace}/${kind}/${name}/img/with%20spaces.png`, ); - expect(pngResponse.text).toEqual('found it'); + expect(Buffer.from(pngResponse.body).toString('utf8')).toEqual( + 'found it', + ); const jsResponse = await request(app).get( `/${namespace}/${kind}/${name}/some%20folder/also%20with%20spaces.js`, ); From dc6cf3b14f81212fc8fbd4d3ba8a9d11d1b4887d Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Fri, 28 May 2021 14:06:44 +0200 Subject: [PATCH 2/9] Test for sanitization bypass Signed-off-by: Eric Peterson --- .../src/stages/publish/awsS3.test.ts | 18 +++++++ .../stages/publish/azureBlobStorage.test.ts | 18 +++++++ .../src/stages/publish/googleStorage.test.ts | 19 ++++++- .../src/stages/publish/local.test.ts | 49 ++++++++++++++++--- .../src/stages/publish/openStackSwift.test.ts | 18 +++++++ 5 files changed, 114 insertions(+), 8 deletions(-) diff --git a/packages/techdocs-common/src/stages/publish/awsS3.test.ts b/packages/techdocs-common/src/stages/publish/awsS3.test.ts index b2954f7b2e..ada3bf1b8f 100644 --- a/packages/techdocs-common/src/stages/publish/awsS3.test.ts +++ b/packages/techdocs-common/src/stages/publish/awsS3.test.ts @@ -286,6 +286,9 @@ describe('AwsS3Publish', () => { mockFs.restore(); mockFs({ [entityRootDir]: { + html: { + 'file.html': '', + }, img: { 'with spaces.png': 'found it', }, @@ -318,5 +321,20 @@ describe('AwsS3Publish', () => { ); expect(jsResponse.text).toEqual('found it too'); }); + + it('should pass text/plain content-type for html', async () => { + const { + kind, + metadata: { namespace, name }, + } = entity; + + const response = await request(app).get( + `/${namespace}/${kind}/${name}/html/file.html`, + ); + expect(response.text).toEqual(''); + expect(response.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); + }); }); }); diff --git a/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts b/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts index 568393662b..9ddf640abf 100644 --- a/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts +++ b/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts @@ -343,6 +343,9 @@ describe('publishing with valid credentials', () => { mockFs.restore(); mockFs({ [entityRootDir]: { + html: { + 'file.html': '', + }, img: { 'with spaces.png': 'found it', }, @@ -375,5 +378,20 @@ describe('publishing with valid credentials', () => { ); expect(jsResponse.text).toEqual('found it too'); }); + + it('should pass text/plain content-type for html', async () => { + const { + kind, + metadata: { namespace, name }, + } = entity; + + const response = await request(app).get( + `/${namespace}/${kind}/${name}/html/file.html`, + ); + expect(response.text).toEqual(''); + expect(response.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); + }); }); }); diff --git a/packages/techdocs-common/src/stages/publish/googleStorage.test.ts b/packages/techdocs-common/src/stages/publish/googleStorage.test.ts index c4c032015e..23110ebcf4 100644 --- a/packages/techdocs-common/src/stages/publish/googleStorage.test.ts +++ b/packages/techdocs-common/src/stages/publish/googleStorage.test.ts @@ -285,6 +285,9 @@ describe('GoogleGCSPublish', () => { mockFs.restore(); mockFs({ [entityRootDir]: { + html: { + 'file.html': '', + }, img: { 'with spaces.png': 'found it', }, @@ -315,7 +318,21 @@ describe('GoogleGCSPublish', () => { const jsResponse = await request(app).get( `/${namespace}/${kind}/${name}/some%20folder/also%20with%20spaces.js`, ); - expect(jsResponse.text).toEqual('found it too'); + }); + + it('should pass text/plain content-type for html', async () => { + const { + kind, + metadata: { namespace, name }, + } = entity; + + const response = await request(app).get( + `/${namespace}/${kind}/${name}/html/file.html`, + ); + expect(response.text).toEqual(''); + expect(response.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); }); }); }); diff --git a/packages/techdocs-common/src/stages/publish/local.test.ts b/packages/techdocs-common/src/stages/publish/local.test.ts index cd1a17ab5f..a5dca5f057 100644 --- a/packages/techdocs-common/src/stages/publish/local.test.ts +++ b/packages/techdocs-common/src/stages/publish/local.test.ts @@ -16,8 +16,11 @@ import { getVoidLogger, PluginEndpointDiscovery, + resolvePackagePath, } from '@backstage/backend-common'; import { ConfigReader } from '@backstage/config'; +import express from 'express'; +import request from 'supertest'; import mockFs from 'mock-fs'; import * as os from 'os'; import { LocalPublish } from './local'; @@ -35,11 +38,21 @@ const createMockEntity = (annotations = {}) => { }; }; +const testDiscovery: jest.Mocked = { + getBaseUrl: jest.fn().mockResolvedValue('http://localhost:7000/api/techdocs'), + getExternalBaseUrl: jest.fn(), +}; + const logger = getVoidLogger(); const tmpDir = os.platform() === 'win32' ? 'C:\\tmp\\generatedDir' : '/tmp/generatedDir'; +const resolvedDir = resolvePackagePath( + '@backstage/plugin-techdocs-backend', + 'static/docs', +); + describe('local publisher', () => { it('should publish generated documentation dir', async () => { mockFs({ @@ -48,13 +61,6 @@ describe('local publisher', () => { }, }); - const testDiscovery: jest.Mocked = { - getBaseUrl: jest - .fn() - .mockResolvedValue('http://localhost:7000/api/techdocs'), - getExternalBaseUrl: jest.fn(), - }; - const mockConfig = new ConfigReader({}); const publisher = new LocalPublish(mockConfig, logger, testDiscovery); @@ -66,4 +72,33 @@ describe('local publisher', () => { mockFs.restore(); }); + + describe('docsRouter', () => { + const mockConfig = new ConfigReader({}); + const publisher = new LocalPublish(mockConfig, logger, testDiscovery); + let app: express.Express; + + beforeEach(() => { + app = express().use(publisher.docsRouter()); + + mockFs.restore(); + mockFs({ + [resolvedDir]: { + 'some-file.html': 'found it', + }, + }); + }); + + afterEach(() => { + mockFs.restore(); + }); + + it('should pass text/plain content-type for html', async () => { + const response = await request(app).get(`/some-file.html`); + expect(response.text).toEqual('found it'); + expect(response.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); + }); + }); }); diff --git a/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts b/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts index aeaceeaaa6..35e5586ee4 100644 --- a/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts +++ b/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts @@ -291,6 +291,9 @@ describe('OpenStackSwiftPublish', () => { mockFs.restore(); mockFs({ [entityRootDir]: { + html: { + 'file.html': '', + }, img: { 'with spaces.png': 'found it', }, @@ -323,5 +326,20 @@ describe('OpenStackSwiftPublish', () => { ); expect(jsResponse.text).toEqual('found it too'); }); + + it('should pass text/plain content-type for html', async () => { + const { + kind, + metadata: { namespace, name }, + } = entity; + + const response = await request(app).get( + `/${namespace}/${kind}/${name}/html/file.html`, + ); + expect(response.text).toEqual(''); + expect(response.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); + }); }); }); From 58ba10677a2762e0b5db168539d42518b5be5388 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Fri, 28 May 2021 14:13:30 +0200 Subject: [PATCH 3/9] Enforce plain text header for html files Signed-off-by: Eric Peterson --- .../src/stages/publish/helpers.test.ts | 2 +- .../techdocs-common/src/stages/publish/helpers.ts | 12 ++++++++++-- packages/techdocs-common/src/stages/publish/local.ts | 12 +++++++++++- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/packages/techdocs-common/src/stages/publish/helpers.test.ts b/packages/techdocs-common/src/stages/publish/helpers.test.ts index 1fae66f3d2..ae872be016 100644 --- a/packages/techdocs-common/src/stages/publish/helpers.test.ts +++ b/packages/techdocs-common/src/stages/publish/helpers.test.ts @@ -20,7 +20,7 @@ import { getFileTreeRecursively, getHeadersForFileExtension } from './helpers'; describe('getHeadersForFileExtension', () => { const correctMapOfExtensions = [ - ['.html', 'text/html; charset=utf-8'], + ['.html', 'text/plain; charset=utf-8'], ['.css', 'text/css; charset=utf-8'], ['.png', 'image/png'], ['.jpg', 'image/jpeg'], diff --git a/packages/techdocs-common/src/stages/publish/helpers.ts b/packages/techdocs-common/src/stages/publish/helpers.ts index 138ec611e0..63568699c7 100644 --- a/packages/techdocs-common/src/stages/publish/helpers.ts +++ b/packages/techdocs-common/src/stages/publish/helpers.ts @@ -28,10 +28,18 @@ export type responseHeadersType = { export const getHeadersForFileExtension = ( fileExtension: string, ): responseHeadersType => { - return { + const headerType = { 'Content-Type': mime.contentType(fileExtension) || 'text/plain; charset=utf-8', - } as responseHeadersType; + }; + + // Prevent sanitization bypass by preventing browers from directly rendering + // the contents of HTML files kept in storage. + if (headerType['Content-Type'].match(/html/)) { + headerType['Content-Type'] = 'text/plain; charset=utf-8'; + } + + return headerType; }; /** diff --git a/packages/techdocs-common/src/stages/publish/local.ts b/packages/techdocs-common/src/stages/publish/local.ts index e09473cf60..036053627b 100644 --- a/packages/techdocs-common/src/stages/publish/local.ts +++ b/packages/techdocs-common/src/stages/publish/local.ts @@ -31,6 +31,7 @@ import { ReadinessResponse, TechDocsMetadata, } from './types'; +import { getHeadersForFileExtension } from './helpers'; // TODO: Use a more persistent storage than node_modules or /tmp directory. // Make it configurable with techdocs.publisher.local.publishDirectory @@ -132,7 +133,16 @@ export class LocalPublish implements PublisherBase { } docsRouter(): express.Handler { - return express.static(staticDocsDir); + return express.static(staticDocsDir, { + // Handle content-type header the same as all other publishers. + setHeaders: (res, filePath) => { + const fileExtension = path.extname(filePath); + const { 'Content-Type': header } = getHeadersForFileExtension( + fileExtension, + ); + res.setHeader('Content-Type', header); + }, + }); } async hasDocsBeenGenerated(entity: Entity): Promise { From 104d2d44ee8c8e2c558be663d214457cdc5dbeae Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Fri, 28 May 2021 17:40:52 +0200 Subject: [PATCH 4/9] Account for SVG/XML files too. Signed-off-by: Eric Peterson --- .../src/stages/publish/awsS3.test.ts | 19 ++++++++++++----- .../stages/publish/azureBlobStorage.test.ts | 19 ++++++++++++----- .../src/stages/publish/googleStorage.test.ts | 20 +++++++++++++----- .../src/stages/publish/helpers.test.ts | 2 +- .../src/stages/publish/helpers.ts | 4 ++-- .../src/stages/publish/local.test.ts | 17 ++++++++++----- .../src/stages/publish/openStackSwift.test.ts | 21 +++++++++++++------ 7 files changed, 73 insertions(+), 29 deletions(-) diff --git a/packages/techdocs-common/src/stages/publish/awsS3.test.ts b/packages/techdocs-common/src/stages/publish/awsS3.test.ts index ada3bf1b8f..d4fb0d2dc2 100644 --- a/packages/techdocs-common/src/stages/publish/awsS3.test.ts +++ b/packages/techdocs-common/src/stages/publish/awsS3.test.ts @@ -287,10 +287,11 @@ describe('AwsS3Publish', () => { mockFs({ [entityRootDir]: { html: { - 'file.html': '', + 'unsafe.html': '', }, img: { 'with spaces.png': 'found it', + 'unsafe.svg': '', }, 'some folder': { 'also with spaces.js': 'found it too', @@ -328,11 +329,19 @@ describe('AwsS3Publish', () => { metadata: { namespace, name }, } = entity; - const response = await request(app).get( - `/${namespace}/${kind}/${name}/html/file.html`, + const htmlResponse = await request(app).get( + `/${namespace}/${kind}/${name}/html/unsafe.html`, ); - expect(response.text).toEqual(''); - expect(response.header).toMatchObject({ + expect(htmlResponse.text).toEqual(''); + expect(htmlResponse.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); + + const svgResponse = await request(app).get( + `/${namespace}/${kind}/${name}/img/unsafe.svg`, + ); + expect(svgResponse.text).toEqual(''); + expect(svgResponse.header).toMatchObject({ 'content-type': 'text/plain; charset=utf-8', }); }); diff --git a/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts b/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts index 9ddf640abf..0bf720ae31 100644 --- a/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts +++ b/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts @@ -344,10 +344,11 @@ describe('publishing with valid credentials', () => { mockFs({ [entityRootDir]: { html: { - 'file.html': '', + 'unsafe.html': '', }, img: { 'with spaces.png': 'found it', + 'unsafe.svg': '', }, 'some folder': { 'also with spaces.js': 'found it too', @@ -385,11 +386,19 @@ describe('publishing with valid credentials', () => { metadata: { namespace, name }, } = entity; - const response = await request(app).get( - `/${namespace}/${kind}/${name}/html/file.html`, + const htmlResponse = await request(app).get( + `/${namespace}/${kind}/${name}/html/unsafe.html`, ); - expect(response.text).toEqual(''); - expect(response.header).toMatchObject({ + expect(htmlResponse.text).toEqual(''); + expect(htmlResponse.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); + + const svgResponse = await request(app).get( + `/${namespace}/${kind}/${name}/img/unsafe.svg`, + ); + expect(svgResponse.text).toEqual(''); + expect(svgResponse.header).toMatchObject({ 'content-type': 'text/plain; charset=utf-8', }); }); diff --git a/packages/techdocs-common/src/stages/publish/googleStorage.test.ts b/packages/techdocs-common/src/stages/publish/googleStorage.test.ts index 23110ebcf4..4db1d83c7e 100644 --- a/packages/techdocs-common/src/stages/publish/googleStorage.test.ts +++ b/packages/techdocs-common/src/stages/publish/googleStorage.test.ts @@ -286,10 +286,11 @@ describe('GoogleGCSPublish', () => { mockFs({ [entityRootDir]: { html: { - 'file.html': '', + 'unsafe.html': '', }, img: { 'with spaces.png': 'found it', + 'unsafe.svg': '', }, 'some folder': { 'also with spaces.js': 'found it too', @@ -318,6 +319,7 @@ describe('GoogleGCSPublish', () => { const jsResponse = await request(app).get( `/${namespace}/${kind}/${name}/some%20folder/also%20with%20spaces.js`, ); + expect(jsResponse.text).toEqual('found it too'); }); it('should pass text/plain content-type for html', async () => { @@ -326,11 +328,19 @@ describe('GoogleGCSPublish', () => { metadata: { namespace, name }, } = entity; - const response = await request(app).get( - `/${namespace}/${kind}/${name}/html/file.html`, + const htmlResponse = await request(app).get( + `/${namespace}/${kind}/${name}/html/unsafe.html`, ); - expect(response.text).toEqual(''); - expect(response.header).toMatchObject({ + expect(htmlResponse.text).toEqual(''); + expect(htmlResponse.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); + + const svgResponse = await request(app).get( + `/${namespace}/${kind}/${name}/img/unsafe.svg`, + ); + expect(svgResponse.text).toEqual(''); + expect(svgResponse.header).toMatchObject({ 'content-type': 'text/plain; charset=utf-8', }); }); diff --git a/packages/techdocs-common/src/stages/publish/helpers.test.ts b/packages/techdocs-common/src/stages/publish/helpers.test.ts index ae872be016..d5af98e7e1 100644 --- a/packages/techdocs-common/src/stages/publish/helpers.test.ts +++ b/packages/techdocs-common/src/stages/publish/helpers.test.ts @@ -25,7 +25,7 @@ describe('getHeadersForFileExtension', () => { ['.png', 'image/png'], ['.jpg', 'image/jpeg'], ['.jpeg', 'image/jpeg'], - ['.svg', 'image/svg+xml'], + ['.svg', 'text/plain; charset=utf-8'], ['.json', 'application/json; charset=utf-8'], ['.this-in-not-an-extension', 'text/plain; charset=utf-8'], ]; diff --git a/packages/techdocs-common/src/stages/publish/helpers.ts b/packages/techdocs-common/src/stages/publish/helpers.ts index 63568699c7..3ca424055f 100644 --- a/packages/techdocs-common/src/stages/publish/helpers.ts +++ b/packages/techdocs-common/src/stages/publish/helpers.ts @@ -34,8 +34,8 @@ export const getHeadersForFileExtension = ( }; // Prevent sanitization bypass by preventing browers from directly rendering - // the contents of HTML files kept in storage. - if (headerType['Content-Type'].match(/html/)) { + // the contents of untrusted content. + if (headerType['Content-Type'].match(/html|xml/)) { headerType['Content-Type'] = 'text/plain; charset=utf-8'; } diff --git a/packages/techdocs-common/src/stages/publish/local.test.ts b/packages/techdocs-common/src/stages/publish/local.test.ts index a5dca5f057..114401f1db 100644 --- a/packages/techdocs-common/src/stages/publish/local.test.ts +++ b/packages/techdocs-common/src/stages/publish/local.test.ts @@ -84,7 +84,8 @@ describe('local publisher', () => { mockFs.restore(); mockFs({ [resolvedDir]: { - 'some-file.html': 'found it', + 'unsafe.html': '', + 'unsafe.svg': '', }, }); }); @@ -93,10 +94,16 @@ describe('local publisher', () => { mockFs.restore(); }); - it('should pass text/plain content-type for html', async () => { - const response = await request(app).get(`/some-file.html`); - expect(response.text).toEqual('found it'); - expect(response.header).toMatchObject({ + it('should pass text/plain content-type for unsafe types', async () => { + const htmlResponse = await request(app).get(`/unsafe.html`); + expect(htmlResponse.text).toEqual(''); + expect(htmlResponse.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); + + const svgResponse = await request(app).get(`/unsafe.svg`); + expect(svgResponse.text).toEqual(''); + expect(svgResponse.header).toMatchObject({ 'content-type': 'text/plain; charset=utf-8', }); }); diff --git a/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts b/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts index 35e5586ee4..dfac7fb0f7 100644 --- a/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts +++ b/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts @@ -292,9 +292,10 @@ describe('OpenStackSwiftPublish', () => { mockFs({ [entityRootDir]: { html: { - 'file.html': '', + 'unsafe.html': '', }, img: { + 'unsafe.svg': '', 'with spaces.png': 'found it', }, 'some folder': { @@ -327,17 +328,25 @@ describe('OpenStackSwiftPublish', () => { expect(jsResponse.text).toEqual('found it too'); }); - it('should pass text/plain content-type for html', async () => { + it('should pass text/plain content-type for unsafe types', async () => { const { kind, metadata: { namespace, name }, } = entity; - const response = await request(app).get( - `/${namespace}/${kind}/${name}/html/file.html`, + const htmlResponse = await request(app).get( + `/${namespace}/${kind}/${name}/html/unsafe.html`, ); - expect(response.text).toEqual(''); - expect(response.header).toMatchObject({ + expect(htmlResponse.text).toEqual(''); + expect(htmlResponse.header).toMatchObject({ + 'content-type': 'text/plain; charset=utf-8', + }); + + const svgResponse = await request(app).get( + `/${namespace}/${kind}/${name}/img/unsafe.svg`, + ); + expect(svgResponse.text).toEqual(''); + expect(svgResponse.header).toMatchObject({ 'content-type': 'text/plain; charset=utf-8', }); }); From 1b24ae1c7fc7611213bb9d7a0fbbd66726835752 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Fri, 28 May 2021 17:45:16 +0200 Subject: [PATCH 5/9] Account for displaying SVGs in the frontend. Signed-off-by: Eric Peterson --- .../reader/transformers/addBaseUrl.test.ts | 41 ++++++++++++++++++- .../src/reader/transformers/addBaseUrl.ts | 23 +++++++++-- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts b/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts index afed2eff73..2b17d04144 100644 --- a/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts +++ b/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts @@ -47,8 +47,17 @@ const mockEntityId = { namespace: '', name: '', }; - describe('addBaseUrl', () => { + const originalFetch = global.fetch; + + beforeEach(() => { + global.fetch = jest.fn(); + }); + + afterEach(() => { + global.fetch = originalFetch; + }); + it('contains relative paths', () => { createTestShadowDom(fixture, { preTransformers: [ @@ -86,4 +95,34 @@ describe('addBaseUrl', () => { '', ); }); + + it('transforms svg img src to data uri', async () => { + const svgContent = ''; + const expectedSrc = `data:image/svg+xml;base64,${Buffer.from( + svgContent, + ).toString('base64')}`; + + (global.fetch as jest.Mock).mockReturnValue({ + text: jest.fn().mockResolvedValue(svgContent), + }); + + const root = createTestShadowDom('', { + preTransformers: [ + addBaseUrl({ + techdocsStorageApi, + entityId: mockEntityId, + path: '', + }), + ], + postTransformers: [], + }); + + await new Promise(done => { + process.nextTick(() => { + const actualSrc = root.getElementById('x')?.getAttribute('src'); + expect(expectedSrc).toEqual(actualSrc); + done(); + }); + }); + }); }); diff --git a/plugins/techdocs/src/reader/transformers/addBaseUrl.ts b/plugins/techdocs/src/reader/transformers/addBaseUrl.ts index 9bb5418bc2..98db162b46 100644 --- a/plugins/techdocs/src/reader/transformers/addBaseUrl.ts +++ b/plugins/techdocs/src/reader/transformers/addBaseUrl.ts @@ -38,10 +38,27 @@ export const addBaseUrl = ({ .forEach(async (elem: T) => { const elemAttribute = elem.getAttribute(attributeName); if (!elemAttribute) return; - elem.setAttribute( - attributeName, - await techdocsStorageApi.getBaseUrl(elemAttribute, entityId, path), + + // Special handling for SVG images. + const newValue = await techdocsStorageApi.getBaseUrl( + elemAttribute, + entityId, + path, ); + if (attributeName === 'src' && elemAttribute.endsWith('.svg')) { + try { + const svg = await fetch(newValue); + const svgContent = await svg.text(); + elem.setAttribute( + attributeName, + `data:image/svg+xml;base64,${btoa(svgContent)}`, + ); + } catch (e) { + elem.setAttribute('alt', `Error: ${elemAttribute}`); + } + } else { + elem.setAttribute(attributeName, newValue); + } }); }; From 348c46896fed3a0b92bd376ad5825323637e402a Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Fri, 28 May 2021 17:46:08 +0200 Subject: [PATCH 6/9] Disallow object tags Signed-off-by: Eric Peterson --- plugins/techdocs/src/reader/transformers/sanitizeDOM/tags.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/techdocs/src/reader/transformers/sanitizeDOM/tags.ts b/plugins/techdocs/src/reader/transformers/sanitizeDOM/tags.ts index 094c82b5ff..9bb641c09d 100644 --- a/plugins/techdocs/src/reader/transformers/sanitizeDOM/tags.ts +++ b/plugins/techdocs/src/reader/transformers/sanitizeDOM/tags.ts @@ -153,7 +153,6 @@ export const svg = [ 'mask', 'metadata', 'mpath', - 'object', 'path', 'pattern', 'polygon', From 33f6e986858b8620a30b415638b911c589a008ae Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Mon, 31 May 2021 17:51:33 +0200 Subject: [PATCH 7/9] More explicit tests, and tidier helper. Signed-off-by: Eric Peterson --- .../src/stages/publish/helpers.test.ts | 6 ++++ .../src/stages/publish/helpers.ts | 29 ++++++++++++------- .../src/stages/publish/local.ts | 8 ++--- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/packages/techdocs-common/src/stages/publish/helpers.test.ts b/packages/techdocs-common/src/stages/publish/helpers.test.ts index d5af98e7e1..7eb591bdb2 100644 --- a/packages/techdocs-common/src/stages/publish/helpers.test.ts +++ b/packages/techdocs-common/src/stages/publish/helpers.test.ts @@ -21,11 +21,17 @@ import { getFileTreeRecursively, getHeadersForFileExtension } from './helpers'; describe('getHeadersForFileExtension', () => { const correctMapOfExtensions = [ ['.html', 'text/plain; charset=utf-8'], + ['.htm', 'text/plain; charset=utf-8'], + ['.HTML', 'text/plain; charset=utf-8'], + ['.dhtml', 'text/plain; charset=utf-8'], + ['.xhtml', 'text/plain; charset=utf-8'], + ['.xml', 'text/plain; charset=utf-8'], ['.css', 'text/css; charset=utf-8'], ['.png', 'image/png'], ['.jpg', 'image/jpeg'], ['.jpeg', 'image/jpeg'], ['.svg', 'text/plain; charset=utf-8'], + ['.SVG', 'text/plain; charset=utf-8'], ['.json', 'application/json; charset=utf-8'], ['.this-in-not-an-extension', 'text/plain; charset=utf-8'], ]; diff --git a/packages/techdocs-common/src/stages/publish/helpers.ts b/packages/techdocs-common/src/stages/publish/helpers.ts index 3ca424055f..22da949aec 100644 --- a/packages/techdocs-common/src/stages/publish/helpers.ts +++ b/packages/techdocs-common/src/stages/publish/helpers.ts @@ -16,6 +16,22 @@ import mime from 'mime-types'; import recursiveReadDir from 'recursive-readdir'; +/** + * Helper to get the expected content-type for a given file extension. Also + * takes XSS mitigation into account. + */ +const getContentTypeForExtension = (ext: string): string => { + const defaultContentType = 'text/plain; charset=utf-8'; + + // Prevent sanitization bypass by preventing browsers from directly rendering + // the contents of untrusted files. + if (ext.match(/htm|xml|svg/i)) { + return defaultContentType; + } + + return mime.contentType(ext) || defaultContentType; +}; + export type responseHeadersType = { 'Content-Type': string; }; @@ -28,18 +44,9 @@ export type responseHeadersType = { export const getHeadersForFileExtension = ( fileExtension: string, ): responseHeadersType => { - const headerType = { - 'Content-Type': - mime.contentType(fileExtension) || 'text/plain; charset=utf-8', + return { + 'Content-Type': getContentTypeForExtension(fileExtension), }; - - // Prevent sanitization bypass by preventing browers from directly rendering - // the contents of untrusted content. - if (headerType['Content-Type'].match(/html|xml/)) { - headerType['Content-Type'] = 'text/plain; charset=utf-8'; - } - - return headerType; }; /** diff --git a/packages/techdocs-common/src/stages/publish/local.ts b/packages/techdocs-common/src/stages/publish/local.ts index 036053627b..bf49c5b926 100644 --- a/packages/techdocs-common/src/stages/publish/local.ts +++ b/packages/techdocs-common/src/stages/publish/local.ts @@ -137,10 +137,10 @@ export class LocalPublish implements PublisherBase { // Handle content-type header the same as all other publishers. setHeaders: (res, filePath) => { const fileExtension = path.extname(filePath); - const { 'Content-Type': header } = getHeadersForFileExtension( - fileExtension, - ); - res.setHeader('Content-Type', header); + const headers = getHeadersForFileExtension(fileExtension); + for (const [header, value] of Object.entries(headers)) { + res.setHeader(header, value); + } }, }); } From aad98c544e59369901fe9e0a85f6357644dceb5c Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Mon, 31 May 2021 18:01:38 +0200 Subject: [PATCH 8/9] Initial changeset. Signed-off-by: Eric Peterson --- .changeset/techdocs-fresh-and-clean.md | 6 ++++++ .github/styles/vocab.txt | 1 + 2 files changed, 7 insertions(+) create mode 100644 .changeset/techdocs-fresh-and-clean.md diff --git a/.changeset/techdocs-fresh-and-clean.md b/.changeset/techdocs-fresh-and-clean.md new file mode 100644 index 0000000000..52d9988c36 --- /dev/null +++ b/.changeset/techdocs-fresh-and-clean.md @@ -0,0 +1,6 @@ +--- +'@backstage/techdocs-common': patch +'@backstage/plugin-techdocs': patch +--- + +Fixes multiple XSS and sanitization bypass vulnerabilities in TechDocs. For details, see [ link to security advisories here ] ... diff --git a/.github/styles/vocab.txt b/.github/styles/vocab.txt index b8f028a2ae..d91b840292 100644 --- a/.github/styles/vocab.txt +++ b/.github/styles/vocab.txt @@ -210,6 +210,7 @@ rst rsync ruleset sam +sanitization scaffolded scaffolder Scaffolder From 2d1bc6e6d4ec85bb9d351809ca1ba9ab5fabbbd0 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Wed, 2 Jun 2021 19:01:16 +0200 Subject: [PATCH 9/9] Remove reference to advisories. We'll add manually to docs after they are published. Signed-off-by: Eric Peterson --- .changeset/techdocs-fresh-and-clean.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/techdocs-fresh-and-clean.md b/.changeset/techdocs-fresh-and-clean.md index 52d9988c36..615294c3c4 100644 --- a/.changeset/techdocs-fresh-and-clean.md +++ b/.changeset/techdocs-fresh-and-clean.md @@ -3,4 +3,4 @@ '@backstage/plugin-techdocs': patch --- -Fixes multiple XSS and sanitization bypass vulnerabilities in TechDocs. For details, see [ link to security advisories here ] ... +Fixes multiple XSS and sanitization bypass vulnerabilities in TechDocs.