Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-7hv8-3fr9-j2hv
catalog xss fixes
  • Loading branch information
Rugvip committed Feb 14, 2023
2 parents 5c877c2 + 071354e commit 3d13719
Show file tree
Hide file tree
Showing 10 changed files with 214 additions and 26 deletions.
5 changes: 5 additions & 0 deletions .changeset/big-bags-glow.md
@@ -0,0 +1,5 @@
---
'@backstage/plugin-catalog-backend': patch
---

Add additional validation as security precations for output entities.
5 changes: 5 additions & 0 deletions .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.
5 changes: 5 additions & 0 deletions .changeset/purple-panthers-know.md
@@ -0,0 +1,5 @@
---
'@backstage/catalog-model': patch
---

Add additional validation for location references.
5 changes: 5 additions & 0 deletions .changeset/red-pugs-tap.md
@@ -0,0 +1,5 @@
---
'@backstage/core-components': patch
---

Updated Link URL validation to be more strict.
9 changes: 9 additions & 0 deletions packages/catalog-model/src/location/helpers.test.ts
Expand Up @@ -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",
);
});
});

Expand All @@ -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",
);
});
});

Expand Down
17 changes: 17 additions & 0 deletions packages/catalog-model/src/location/helpers.ts
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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 };
}

Expand All @@ -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}`;
}

Expand Down
20 changes: 20 additions & 0 deletions packages/core-components/src/components/Link/Link.test.tsx
Expand Up @@ -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"`,
);
});
});
32 changes: 32 additions & 0 deletions packages/core-components/src/components/Link/Link.tsx
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
127 changes: 101 additions & 26 deletions plugins/catalog-backend/src/integration.test.ts
Expand Up @@ -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;

Expand All @@ -74,23 +79,27 @@ 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;
}

processStart(item: RefreshStateItem) {
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() {},
Expand All @@ -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!;
Expand Down Expand Up @@ -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();
},
Expand All @@ -154,7 +165,6 @@ class WaitingProgressTracker implements ProgressTracker {
resolve();
},
markSuccessfulWithErrors: () => {
this.#errors.delete(item.entityRef);
onDone();
resolve();
},
Expand All @@ -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;
}

Expand All @@ -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 ?? {
Expand Down Expand Up @@ -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,
);
Expand Down Expand Up @@ -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': {
Expand Down Expand Up @@ -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",
),
],
});
});
});

0 comments on commit 3d13719

Please sign in to comment.