Merge pull request from GHSA-7hv8-3fr9-j2hv

catalog xss fixes

.changeset/big-bags-glow.md

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

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

.changeset/purple-panthers-know.md

@@ -0,0 +1,5 @@
+---
+'@backstage/catalog-model': patch
+---
+
+Add additional validation for location references.

.changeset/red-pugs-tap.md

@@ -0,0 +1,5 @@
+---
+'@backstage/core-components': patch
+---
+
+Updated Link URL validation to be more strict.

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",
+    );
   });
 });
 

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}`;
 }
 

packages/core-components/src/components/Link/Link.test.tsx

@@ -173,4 +173,24 @@ describe('<Link />', () => {
       });
     });
   });
+
+  it('throws an error when attempting to link to script code', () => {
+    expect(() =>
+      // eslint-disable-next-line no-script-url
+      render(wrapInTestApp(<Link to="javascript:alert('hello')">Script</Link>)),
+    ).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"`,
+    );
+  });
 });

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<typeof window.open>
+  ) {
+    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<MaterialLinkProps, 'to'> &
   Omit<RouterLinkProps, 'to'> & {
     to: string;
@@ -144,6 +170,12 @@ export const Link = React.forwardRef<any, LinkProps>(
     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<any, MouseEvent>) => {
       onClick?.(event);
       if (!noTrack) {

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