diff --git a/.changeset/techdocs-fresh-and-clean.md b/.changeset/techdocs-fresh-and-clean.md new file mode 100644 index 0000000000..615294c3c4 --- /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. 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 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/awsS3.test.ts b/packages/techdocs-common/src/stages/publish/awsS3.test.ts index b2954f7b2e..d4fb0d2dc2 100644 --- a/packages/techdocs-common/src/stages/publish/awsS3.test.ts +++ b/packages/techdocs-common/src/stages/publish/awsS3.test.ts @@ -286,8 +286,12 @@ describe('AwsS3Publish', () => { mockFs.restore(); mockFs({ [entityRootDir]: { + html: { + 'unsafe.html': '', + }, img: { 'with spaces.png': 'found it', + 'unsafe.svg': '', }, 'some folder': { 'also with spaces.js': 'found it too', @@ -318,5 +322,28 @@ 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 htmlResponse = await request(app).get( + `/${namespace}/${kind}/${name}/html/unsafe.html`, + ); + 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 568393662b..0bf720ae31 100644 --- a/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts +++ b/packages/techdocs-common/src/stages/publish/azureBlobStorage.test.ts @@ -343,8 +343,12 @@ describe('publishing with valid credentials', () => { mockFs.restore(); mockFs({ [entityRootDir]: { + html: { + 'unsafe.html': '', + }, img: { 'with spaces.png': 'found it', + 'unsafe.svg': '', }, 'some folder': { 'also with spaces.js': 'found it too', @@ -375,5 +379,28 @@ 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 htmlResponse = await request(app).get( + `/${namespace}/${kind}/${name}/html/unsafe.html`, + ); + 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 f29e1f10be..4db1d83c7e 100644 --- a/packages/techdocs-common/src/stages/publish/googleStorage.test.ts +++ b/packages/techdocs-common/src/stages/publish/googleStorage.test.ts @@ -285,8 +285,12 @@ describe('GoogleGCSPublish', () => { mockFs.restore(); mockFs({ [entityRootDir]: { + html: { + 'unsafe.html': '', + }, img: { 'with spaces.png': 'found it', + 'unsafe.svg': '', }, 'some folder': { 'also with spaces.js': 'found it too', @@ -309,11 +313,36 @@ 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`, ); expect(jsResponse.text).toEqual('found it too'); }); + + it('should pass text/plain content-type for html', async () => { + const { + kind, + metadata: { namespace, name }, + } = entity; + + const htmlResponse = await request(app).get( + `/${namespace}/${kind}/${name}/html/unsafe.html`, + ); + 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 1fae66f3d2..7eb591bdb2 100644 --- a/packages/techdocs-common/src/stages/publish/helpers.test.ts +++ b/packages/techdocs-common/src/stages/publish/helpers.test.ts @@ -20,12 +20,18 @@ import { getFileTreeRecursively, getHeadersForFileExtension } from './helpers'; describe('getHeadersForFileExtension', () => { const correctMapOfExtensions = [ - ['.html', 'text/html; charset=utf-8'], + ['.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', 'image/svg+xml'], + ['.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 138ec611e0..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; }; @@ -29,9 +45,8 @@ export const getHeadersForFileExtension = ( fileExtension: string, ): responseHeadersType => { return { - 'Content-Type': - mime.contentType(fileExtension) || 'text/plain; charset=utf-8', - } as responseHeadersType; + 'Content-Type': getContentTypeForExtension(fileExtension), + }; }; /** diff --git a/packages/techdocs-common/src/stages/publish/local.test.ts b/packages/techdocs-common/src/stages/publish/local.test.ts index cd1a17ab5f..114401f1db 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,40 @@ 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]: { + 'unsafe.html': '', + 'unsafe.svg': '', + }, + }); + }); + + afterEach(() => { + mockFs.restore(); + }); + + 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/local.ts b/packages/techdocs-common/src/stages/publish/local.ts index e09473cf60..bf49c5b926 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 headers = getHeadersForFileExtension(fileExtension); + for (const [header, value] of Object.entries(headers)) { + res.setHeader(header, value); + } + }, + }); } async hasDocsBeenGenerated(entity: Entity): Promise { diff --git a/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts b/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts index aeaceeaaa6..dfac7fb0f7 100644 --- a/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts +++ b/packages/techdocs-common/src/stages/publish/openStackSwift.test.ts @@ -291,7 +291,11 @@ describe('OpenStackSwiftPublish', () => { mockFs.restore(); mockFs({ [entityRootDir]: { + html: { + 'unsafe.html': '', + }, img: { + 'unsafe.svg': '', 'with spaces.png': 'found it', }, 'some folder': { @@ -323,5 +327,28 @@ describe('OpenStackSwiftPublish', () => { ); expect(jsResponse.text).toEqual('found it too'); }); + + it('should pass text/plain content-type for unsafe types', async () => { + const { + kind, + metadata: { namespace, name }, + } = entity; + + const htmlResponse = await request(app).get( + `/${namespace}/${kind}/${name}/html/unsafe.html`, + ); + 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/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); + } }); }; 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',