catalog-backend: add additional validation in stitching step

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
Co-authored-by: Fredrik Adelöw <freben@gmail.com>

.changeset/big-bags-glow.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend': patch
+---
+
+Add additional validation as security precations for output entities.

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<string, Error>) => void;
-  #promise: Promise<Record<string, Error>>;
+class WaitingProgressTracker implements ProgressTrackerWithErrorReports {
+  #resolve: (errors: Record<string, Error[]>) => void;
+  #promise: Promise<Record<string, Error[]>>;
   #counts = new Map<string, number>();
-  #errors = new Map<string, Error>();
+  #errors = new Map<string, Error[]>();
   #inFlight = new Array<Promise<void>>();
 
   constructor(private readonly entityRefs?: Set<string>) {
-    let resolve: (errors: Record<string, Error>) => void;
-    this.#promise = new Promise<Record<string, Error>>(_resolve => {
+    let resolve: (errors: Record<string, Error[]>) => void;
+    this.#promise = new Promise<Record<string, Error[]>>(_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<Record<string, Error>> {
+  reportError(unprocessedEntity: Entity, errors: Error[]): void {
+    this.#errors.set(stringifyEntityRef(unprocessedEntity), errors);
+  }
+
+  async wait(): Promise<Record<string, Error[]>> {
     return this.#promise;
   }
 
@@ -191,10 +205,6 @@ class TestHarness {
       location: LocationSpec,
       emit: CatalogProcessorEmit,
     ): Promise<Entity>;
-    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",
+        ),
+      ],
+    });
+  });
 });

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