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/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