diff --git a/.changeset/big-bags-glow.md b/.changeset/big-bags-glow.md new file mode 100644 index 0000000000..eb606838fd --- /dev/null +++ b/.changeset/big-bags-glow.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Add additional validation as security precations for output entities. diff --git a/.changeset/gorgeous-ligers-burn.md b/.changeset/gorgeous-ligers-burn.md new file mode 100644 index 0000000000..f1d8f2cf5f --- /dev/null +++ b/.changeset/gorgeous-ligers-burn.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-components': patch +--- + +Added a global override for `window.open` that helps prevent security vulnerabilities. diff --git a/.changeset/purple-panthers-know.md b/.changeset/purple-panthers-know.md new file mode 100644 index 0000000000..c53c6d8890 --- /dev/null +++ b/.changeset/purple-panthers-know.md @@ -0,0 +1,5 @@ +--- +'@backstage/catalog-model': patch +--- + +Add additional validation for location references. diff --git a/.changeset/red-pugs-tap.md b/.changeset/red-pugs-tap.md new file mode 100644 index 0000000000..85da2b7990 --- /dev/null +++ b/.changeset/red-pugs-tap.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-components': patch +--- + +Updated Link URL validation to be more strict. diff --git a/packages/catalog-model/src/location/helpers.test.ts b/packages/catalog-model/src/location/helpers.test.ts index 91a2137611..28267dcf37 100644 --- a/packages/catalog-model/src/location/helpers.test.ts +++ b/packages/catalog-model/src/location/helpers.test.ts @@ -50,6 +50,9 @@ describe('parseLocationRef', () => { expect(() => parseLocationRef('https://bleh')).toThrow( "Invalid location ref 'https://bleh', please prefix it with 'url:', e.g. 'url:https://bleh'", ); + expect(() => parseLocationRef('url:javascript:alert()')).toThrow( + "Invalid location ref 'url:javascript:alert()', target is a javascript: URL", + ); }); }); @@ -70,6 +73,12 @@ describe('stringifyLocationRef', () => { expect(() => stringifyLocationRef({ type: 'hello', target: '' })).toThrow( 'Unable to stringify location ref, empty target', ); + expect(() => + // eslint-disable-next-line no-script-url + stringifyLocationRef({ type: 'url', target: 'javascript:alert()' }), + ).toThrow( + "Invalid location ref 'url:javascript:alert()', target is a javascript: URL", + ); }); }); diff --git a/packages/catalog-model/src/location/helpers.ts b/packages/catalog-model/src/location/helpers.ts index 6f9800c5a2..a2f4d84f6f 100644 --- a/packages/catalog-model/src/location/helpers.ts +++ b/packages/catalog-model/src/location/helpers.ts @@ -17,6 +17,11 @@ import { Entity, stringifyEntityRef } from '../entity'; import { ANNOTATION_LOCATION, ANNOTATION_SOURCE_LOCATION } from './annotation'; +// See https://github.com/facebook/react/blob/f0cf832e1d0c8544c36aa8b310960885a11a847c/packages/react-dom-bindings/src/shared/sanitizeURL.js +const scriptProtocolPattern = + // eslint-disable-next-line no-control-regex + /^[\u0000-\u001F ]*j[\r\n\t]*a[\r\n\t]*v[\r\n\t]*a[\r\n\t]*s[\r\n\t]*c[\r\n\t]*r[\r\n\t]*i[\r\n\t]*p[\r\n\t]*t[\r\n\t]*\:/i; + /** * Parses a string form location reference. * @@ -56,6 +61,12 @@ export function parseLocationRef(ref: string): { ); } + if (scriptProtocolPattern.test(target)) { + throw new TypeError( + `Invalid location ref '${ref}', target is a javascript: URL`, + ); + } + return { type, target }; } @@ -78,6 +89,12 @@ export function stringifyLocationRef(ref: { throw new TypeError(`Unable to stringify location ref, empty target`); } + if (scriptProtocolPattern.test(target)) { + throw new TypeError( + `Invalid location ref '${type}:${target}', target is a javascript: URL`, + ); + } + return `${type}:${target}`; } diff --git a/packages/core-components/src/components/Link/Link.test.tsx b/packages/core-components/src/components/Link/Link.test.tsx index 3538b79052..682c4bea8a 100644 --- a/packages/core-components/src/components/Link/Link.test.tsx +++ b/packages/core-components/src/components/Link/Link.test.tsx @@ -173,4 +173,24 @@ describe('', () => { }); }); }); + + it('throws an error when attempting to link to script code', () => { + expect(() => + // eslint-disable-next-line no-script-url + render(wrapInTestApp(Script)), + ).toThrowErrorMatchingInlineSnapshot( + `"Link component rejected javascript: URL as a security precaution"`, + ); + }); +}); + +describe('window.open', () => { + it('throws an error when attempting to open script code', () => { + expect(() => + // eslint-disable-next-line no-script-url + window.open("javascript:alert('hello')"), + ).toThrowErrorMatchingInlineSnapshot( + `"Rejected window.open() with a javascript: URL as a security precaution"`, + ); + }); }); diff --git a/packages/core-components/src/components/Link/Link.tsx b/packages/core-components/src/components/Link/Link.tsx index 440fcb811f..be91753ea3 100644 --- a/packages/core-components/src/components/Link/Link.tsx +++ b/packages/core-components/src/components/Link/Link.tsx @@ -55,6 +55,32 @@ const useStyles = makeStyles( export const isExternalUri = (uri: string) => /^([a-z+.-]+):/.test(uri); +// See https://github.com/facebook/react/blob/f0cf832e1d0c8544c36aa8b310960885a11a847c/packages/react-dom-bindings/src/shared/sanitizeURL.js +const scriptProtocolPattern = + // eslint-disable-next-line no-control-regex + /^[\u0000-\u001F ]*j[\r\n\t]*a[\r\n\t]*v[\r\n\t]*a[\r\n\t]*s[\r\n\t]*c[\r\n\t]*r[\r\n\t]*i[\r\n\t]*p[\r\n\t]*t[\r\n\t]*\:/i; + +// We install this globally in order to prevent javascript: URL XSS attacks via window.open +const originalWindowOpen = window.open as typeof window.open & { + __backstage?: true; +}; +if (originalWindowOpen && !originalWindowOpen.__backstage) { + const newOpen = function open( + this: Window, + ...args: Parameters + ) { + const url = String(args[0]); + if (scriptProtocolPattern.test(url)) { + throw new Error( + 'Rejected window.open() with a javascript: URL as a security precaution', + ); + } + return originalWindowOpen.apply(this, args); + }; + newOpen.__backstage = true; + window.open = newOpen; +} + export type LinkProps = Omit & Omit & { to: string; @@ -144,6 +170,12 @@ export const Link = React.forwardRef( const external = isExternalUri(to); const newWindow = external && !!/^https?:/.exec(to); + if (scriptProtocolPattern.test(to)) { + throw new Error( + 'Link component rejected javascript: URL as a security precaution', + ); + } + const handleClick = (event: React.MouseEvent) => { onClick?.(event); if (!noTrack) { diff --git a/plugins/catalog-backend/src/integration.test.ts b/plugins/catalog-backend/src/integration.test.ts index 4ba046834b..f0f916a78d 100644 --- a/plugins/catalog-backend/src/integration.test.ts +++ b/plugins/catalog-backend/src/integration.test.ts @@ -52,9 +52,14 @@ import { } from '@backstage/plugin-catalog-node'; import { RefreshStateItem } from './database/types'; import { DefaultProviderDatabase } from './database/DefaultProviderDatabase'; +import { InputError } from '@backstage/errors'; const voidLogger = getVoidLogger(); +type ProgressTrackerWithErrorReports = ProgressTracker & { + reportError(unprocessedEntity: Entity, errors: Error[]): void; +}; + class TestProvider implements EntityProvider { #connection?: EntityProviderConnection; @@ -74,10 +79,10 @@ class TestProvider implements EntityProvider { } } -class ProxyProgressTracker implements ProgressTracker { - #inner: ProgressTracker; +class ProxyProgressTracker implements ProgressTrackerWithErrorReports { + #inner: ProgressTrackerWithErrorReports; - constructor(inner: ProgressTracker) { + constructor(inner: ProgressTrackerWithErrorReports) { this.#inner = inner; } @@ -85,12 +90,16 @@ class ProxyProgressTracker implements ProgressTracker { return this.#inner.processStart(item, voidLogger); } - setTracker(tracker: ProgressTracker) { + setTracker(tracker: ProgressTrackerWithErrorReports) { this.#inner = tracker; } + + reportError(unprocessedEntity: Entity, errors: Error[]): void { + this.#inner.reportError(unprocessedEntity, errors); + } } -class NoopProgressTracker implements ProgressTracker { +class NoopProgressTracker implements ProgressTrackerWithErrorReports { static emptyTracking = { markFailed() {}, markProcessorsCompleted() {}, @@ -102,18 +111,20 @@ class NoopProgressTracker implements ProgressTracker { processStart() { return NoopProgressTracker.emptyTracking; } + + reportError() {} } -class WaitingProgressTracker implements ProgressTracker { - #resolve: (errors: Record) => void; - #promise: Promise>; +class WaitingProgressTracker implements ProgressTrackerWithErrorReports { + #resolve: (errors: Record) => void; + #promise: Promise>; #counts = new Map(); - #errors = new Map(); + #errors = new Map(); #inFlight = new Array>(); constructor(private readonly entityRefs?: Set) { - let resolve: (errors: Record) => void; - this.#promise = new Promise>(_resolve => { + let resolve: (errors: Record) => void; + this.#promise = new Promise>(_resolve => { resolve = _resolve; }); this.#resolve = resolve!; @@ -143,7 +154,7 @@ class WaitingProgressTracker implements ProgressTracker { }; return { markFailed: (error: Error) => { - this.#errors.set(item.entityRef, error); + this.#errors.set(item.entityRef, [error]); onDone(); resolve(); }, @@ -154,7 +165,6 @@ class WaitingProgressTracker implements ProgressTracker { resolve(); }, markSuccessfulWithErrors: () => { - this.#errors.delete(item.entityRef); onDone(); resolve(); }, @@ -165,7 +175,11 @@ class WaitingProgressTracker implements ProgressTracker { }; } - async wait(): Promise> { + reportError(unprocessedEntity: Entity, errors: Error[]): void { + this.#errors.set(stringifyEntityRef(unprocessedEntity), errors); + } + + async wait(): Promise> { return this.#promise; } @@ -191,10 +205,6 @@ class TestHarness { location: LocationSpec, emit: CatalogProcessorEmit, ): Promise; - onProcessingError?(event: { - unprocessedEntity: Entity; - errors: Error[]; - }): void; }) { const config = new ConfigReader( options?.config ?? { @@ -271,13 +281,7 @@ class TestHarness { () => createHash('sha1'), 50, event => { - if (options?.onProcessingError) { - options.onProcessingError(event); - } else { - throw new Error( - `Catalog processing error, ${event.errors.join(', ')}`, - ); - } + proxyProgressTracker.reportError(event.unprocessedEntity, event.errors); }, proxyProgressTracker, ); @@ -388,7 +392,13 @@ describe('Catalog Backend Integration', () => { triggerError = true; - await expect(harness.process()).resolves.toEqual({}); + await expect(harness.process()).resolves.toEqual({ + 'component:default/test': [ + new InputError( + 'Processor Object threw an error while preprocessing; caused by Error: NOPE', + ), + ], + }); await expect(harness.getOutputEntities()).resolves.toEqual({ 'component:default/test': { @@ -685,4 +695,69 @@ describe('Catalog Backend Integration', () => { .annotations!['backstage.io/orphan'], ).toBeUndefined(); }); + + it('should reject insecure URLs', async () => { + const harness = await TestHarness.create(); + + await harness.setInputEntities([ + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/managed-by-location': 'url:.', + 'backstage.io/managed-by-origin-location': 'url:.', + 'backstage.io/view-url': ' javascript:bad()', + 'backstage.io/edit-url': ' javascript:alert()', + }, + }, + }, + ]); + + await expect(harness.process()).resolves.toEqual({}); + + await expect(harness.getOutputEntities()).resolves.toEqual({ + 'component:default/test': { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: expect.objectContaining({ + name: 'test', + annotations: expect.objectContaining({ + 'backstage.io/view-url': + 'https://backstage.io/annotation-rejected-for-security-reasons', + 'backstage.io/edit-url': + 'https://backstage.io/annotation-rejected-for-security-reasons', + }), + }), + relations: [], + }, + }); + }); + + it('should reject insecure location URLs', async () => { + const harness = await TestHarness.create(); + + await harness.setInputEntities([ + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/managed-by-location': 'url:javascript:bad()', + 'backstage.io/managed-by-origin-location': 'url:javascript:alert()', + }, + }, + }, + ]); + + await expect(harness.process()).resolves.toEqual({ + 'component:default/test': [ + new TypeError( + "Invalid location ref 'url:javascript:bad()', target is a javascript: URL", + ), + ], + }); + }); }); diff --git a/plugins/catalog-backend/src/stitching/Stitcher.ts b/plugins/catalog-backend/src/stitching/Stitcher.ts index 4742c81933..bdf9f8791a 100644 --- a/plugins/catalog-backend/src/stitching/Stitcher.ts +++ b/plugins/catalog-backend/src/stitching/Stitcher.ts @@ -17,6 +17,8 @@ import { ENTITY_STATUS_CATALOG_PROCESSING_TYPE } from '@backstage/catalog-client'; import { AlphaEntity, + ANNOTATION_EDIT_URL, + ANNOTATION_VIEW_URL, EntityRelation, EntityStatusItem, } from '@backstage/catalog-model'; @@ -32,6 +34,11 @@ import { import { buildEntitySearch } from './buildEntitySearch'; import { BATCH_SIZE, generateStableHash } from './util'; +// See https://github.com/facebook/react/blob/f0cf832e1d0c8544c36aa8b310960885a11a847c/packages/react-dom-bindings/src/shared/sanitizeURL.js +const scriptProtocolPattern = + // eslint-disable-next-line no-control-regex + /^[\u0000-\u001F ]*j[\r\n\t]*a[\r\n\t]*v[\r\n\t]*a[\r\n\t]*s[\r\n\t]*c[\r\n\t]*r[\r\n\t]*i[\r\n\t]*p[\r\n\t]*t[\r\n\t]*\:/i; + /** * Performs the act of stitching - to take all of the various outputs from the * ingestion process, and stitching them together into the final entity JSON @@ -166,6 +173,14 @@ export class Stitcher { })); } } + // We opt to do this check here as we otherwise can't guarantee that it will be run after all processors + for (const annotation of [ANNOTATION_VIEW_URL, ANNOTATION_EDIT_URL]) { + const value = entity.metadata.annotations?.[annotation]; + if (typeof value === 'string' && scriptProtocolPattern.test(value)) { + entity.metadata.annotations![annotation] = + 'https://backstage.io/annotation-rejected-for-security-reasons'; + } + } // TODO: entityRef is lower case and should be uppercase in the final // result