Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(utils): Fail silently if the provided Dsn is invalid #8121

Merged
merged 5 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/core/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ export function getReportDialogEndpoint(
},
): string {
const dsn = makeDsn(dsnLike);
if (!dsn) {
Copy link
Member Author

@Lms24 Lms24 May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just return an empty script tag instead of throwing, causing the dialog not to be fetched (which is fine IMO)

return '';
}

const endpoint = `${getBaseApiEndpoint(dsn)}embed/error-page/`;

let encodedOptions = `dsn=${dsnToString(dsn)}`;
Expand Down
8 changes: 6 additions & 2 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,16 +112,20 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
*/
protected constructor(options: O) {
this._options = options;

if (options.dsn) {
this._dsn = makeDsn(options.dsn);
} else {
__DEBUG_BUILD__ && logger.warn('No DSN provided, client will not do anything.');
}

if (this._dsn) {
const url = getEnvelopeEndpointWithUrlEncodedAuth(this._dsn, options);
this._transport = options.transport({
recordDroppedEvent: this.recordDroppedEvent.bind(this),
...options.transportOptions,
url,
});
} else {
__DEBUG_BUILD__ && logger.warn('No DSN provided, client will not do anything.');
}
}

Expand Down
12 changes: 9 additions & 3 deletions packages/core/src/transports/multiplexed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,13 @@ export function makeMultiplexedTransport<TO extends BaseTransportOptions>(
const fallbackTransport = createTransport(options);
const otherTransports: Record<string, Transport> = {};

function getTransport(dsn: string): Transport {
function getTransport(dsn: string): Transport | undefined {
if (!otherTransports[dsn]) {
const url = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(dsn));
const validatedDsn = dsnFromString(dsn);
if (!validatedDsn) {
return undefined;
}
const url = getEnvelopeEndpointWithUrlEncodedAuth(validatedDsn);
otherTransports[dsn] = createTransport({ ...options, url });
}

Expand All @@ -66,7 +70,9 @@ export function makeMultiplexedTransport<TO extends BaseTransportOptions>(
return eventFromEnvelope(envelope, eventTypes);
}

const transports = matcher({ envelope, getEvent }).map(dsn => getTransport(dsn));
const transports = matcher({ envelope, getEvent })
.map(dsn => getTransport(dsn))
.filter((t): t is Transport => !!t);

// If we have no transports to send to, use the fallback transport
if (transports.length === 0) {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/lib/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const dsnPublic = 'https://abc@sentry.io:1234/subpath/123';
const tunnel = 'https://hello.com/world';
const _metadata = { sdk: { name: 'sentry.javascript.browser', version: '12.31.12' } } as ClientOptions['_metadata'];

const dsnPublicComponents = makeDsn(dsnPublic);
const dsnPublicComponents = makeDsn(dsnPublic)!;

describe('API', () => {
describe('getEnvelopeEndpointWithUrlEncodedAuth', () => {
Expand Down
12 changes: 6 additions & 6 deletions packages/core/test/lib/base.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,19 @@ describe('BaseClient', () => {
});

test('allows missing Dsn', () => {
expect.assertions(1);

const options = getDefaultTestClientOptions();
const client = new TestClient(options);

expect(client.getDsn()).toBeUndefined();
expect(client.getTransport()).toBeUndefined();
});

test('throws with invalid Dsn', () => {
expect.assertions(1);

test('handles being passed an invalid Dsn', () => {
const options = getDefaultTestClientOptions({ dsn: 'abc' });
expect(() => new TestClient(options)).toThrow(SentryError);
const client = new TestClient(options);

expect(client.getDsn()).toBeUndefined();
expect(client.getTransport()).toBeUndefined();
});
});

Expand Down
18 changes: 16 additions & 2 deletions packages/core/test/lib/transports/multiplexed.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ import { TextEncoder } from 'util';
import { createTransport, getEnvelopeEndpointWithUrlEncodedAuth, makeMultiplexedTransport } from '../../../src';

const DSN1 = 'https://1234@5678.ingest.sentry.io/4321';
const DSN1_URL = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(DSN1));
const DSN1_URL = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(DSN1)!);

const DSN2 = 'https://5678@1234.ingest.sentry.io/8765';
const DSN2_URL = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(DSN2));
const DSN2_URL = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(DSN2)!);

const ERROR_EVENT = { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' };
const ERROR_ENVELOPE = createEnvelope<EventEnvelope>({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, [
Expand Down Expand Up @@ -83,6 +83,20 @@ describe('makeMultiplexedTransport', () => {
await transport.send(ERROR_ENVELOPE);
});

it('Falls back to options DSN when a matched DSN is invalid', async () => {
expect.assertions(1);

const makeTransport = makeMultiplexedTransport(
createTestTransport(url => {
expect(url).toBe(DSN1_URL);
}),
() => ['invalidDsn'],
);

const transport = makeTransport({ url: DSN1_URL, ...transportOptions });
await transport.send(ERROR_ENVELOPE);
});

it('DSN can be overridden via match callback', async () => {
expect.assertions(1);

Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/lib/transports/offline.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ const REPLAY_EVENT: ReplayEvent = {
replay_type: 'buffer',
};

const DSN = dsnFromString('https://public@dsn.ingest.sentry.io/1337');
const DSN = dsnFromString('https://public@dsn.ingest.sentry.io/1337')!;

const DATA = 'nothing';

Expand Down
3 changes: 3 additions & 0 deletions packages/nextjs/src/client/tunnelRoute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ export function applyTunnelRouteOption(options: BrowserOptions): void {
const tunnelRouteOption = globalWithInjectedValues.__sentryRewritesTunnelPath__;
if (tunnelRouteOption && options.dsn) {
const dsnComponents = dsnFromString(options.dsn);
if (!dsnComponents) {
return;
}
const sentrySaasDsnMatch = dsnComponents.host.match(/^o(\d+)\.ingest\.sentry\.io$/);
if (sentrySaasDsnMatch) {
const orgId = sentrySaasDsnMatch[1];
Expand Down
19 changes: 15 additions & 4 deletions packages/nextjs/test/utils/tunnelRoute.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ beforeEach(() => {
});

describe('applyTunnelRouteOption()', () => {
it('should correctly apply `tunnelRoute` option when conditions are met', () => {
it('Correctly applies `tunnelRoute` option when conditions are met', () => {
globalWithInjectedValues.__sentryRewritesTunnelPath__ = '/my-error-monitoring-route';
const options: any = {
dsn: 'https://11111111111111111111111111111111@o2222222.ingest.sentry.io/3333333',
Expand All @@ -22,7 +22,7 @@ describe('applyTunnelRouteOption()', () => {
expect(options.tunnel).toBe('/my-error-monitoring-route?o=2222222&p=3333333');
});

it('should not apply `tunnelRoute` when DSN is missing', () => {
it("Doesn't apply `tunnelRoute` when DSN is missing", () => {
globalWithInjectedValues.__sentryRewritesTunnelPath__ = '/my-error-monitoring-route';
const options: any = {
// no dsn
Expand All @@ -33,7 +33,18 @@ describe('applyTunnelRouteOption()', () => {
expect(options.tunnel).toBeUndefined();
});

it("should not apply `tunnelRoute` option when `tunnelRoute` option wasn't injected", () => {
it("Doesn't apply `tunnelRoute` when DSN is invalid", () => {
globalWithInjectedValues.__sentryRewritesTunnelPath__ = '/my-error-monitoring-route';
const options: any = {
dsn: 'invalidDsn',
} as BrowserOptions;

applyTunnelRouteOption(options);

expect(options.tunnel).toBeUndefined();
});

it("Doesn't apply `tunnelRoute` option when `tunnelRoute` option wasn't injected", () => {
const options: any = {
dsn: 'https://11111111111111111111111111111111@o2222222.ingest.sentry.io/3333333',
} as BrowserOptions;
Expand All @@ -43,7 +54,7 @@ describe('applyTunnelRouteOption()', () => {
expect(options.tunnel).toBeUndefined();
});

it('should not apply `tunnelRoute` option when DSN is not a SaaS DSN', () => {
it("Doesn't `tunnelRoute` option when DSN is not a SaaS DSN", () => {
globalWithInjectedValues.__sentryRewritesTunnelPath__ = '/my-error-monitoring-route';
const options: any = {
dsn: 'https://11111111111111111111111111111111@example.com/3333333',
Expand Down
4 changes: 3 additions & 1 deletion packages/node/test/transports/http.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ const defaultOptions = {
textEncoder,
};

// empty function to keep test output clean
const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});

describe('makeNewHttpTransport()', () => {
afterEach(() => {
jest.clearAllMocks();
Expand Down Expand Up @@ -403,7 +406,6 @@ describe('makeNewHttpTransport()', () => {
});

it('should warn if an invalid url is passed', async () => {
const consoleWarnSpy = jest.spyOn(console, 'warn');
const transport = makeNodeTransport({ ...defaultOptions, url: 'invalid url' });
await transport.send(EVENT_ENVELOPE);
expect(consoleWarnSpy).toHaveBeenCalledWith(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('Unit | util | createReplayEnvelope', () => {
projectId: '123',
protocol: 'https',
publicKey: 'abc',
});
})!;

it('creates an envelope for a given Replay event', () => {
const envelope = createReplayEnvelope(replayEvent, payloadWithSequence, dsn);
Expand Down
45 changes: 31 additions & 14 deletions packages/utils/src/dsn.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { DsnComponents, DsnLike, DsnProtocol } from '@sentry/types';

import { SentryError } from './error';
import { logger } from './logger';

/** Regular expression used to parse a Dsn. */
const DSN_REGEX = /^(?:(\w+):)\/\/(?:(\w+)(?::(\w+)?)?@)([\w.-]+)(?::(\d+))?\/(.+)/;
Expand Down Expand Up @@ -30,13 +30,16 @@ export function dsnToString(dsn: DsnComponents, withPassword: boolean = false):
* Parses a Dsn from a given string.
*
* @param str A Dsn as string
* @returns Dsn as DsnComponents
* @returns Dsn as DsnComponents or undefined if @param str is not a valid DSN string
*/
export function dsnFromString(str: string): DsnComponents {
export function dsnFromString(str: string): DsnComponents | undefined {
const match = DSN_REGEX.exec(str);

if (!match) {
throw new SentryError(`Invalid Sentry Dsn: ${str}`);
// This should be logged to the console
// eslint-disable-next-line no-console
console.error(`Invalid Sentry Dsn: ${str}`);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to log this as a console error so that this error is easier to spot. More fine-grained validation is still only logged if debug is set

return undefined;
}

const [protocol, publicKey, pass = '', host, port = '', lastPath] = match.slice(1);
Expand Down Expand Up @@ -71,38 +74,52 @@ function dsnFromComponents(components: DsnComponents): DsnComponents {
};
}

function validateDsn(dsn: DsnComponents): boolean | void {
function validateDsn(dsn: DsnComponents): boolean {
if (!__DEBUG_BUILD__) {
return;
return true;
}

const { port, projectId, protocol } = dsn;

const requiredComponents: ReadonlyArray<keyof DsnComponents> = ['protocol', 'publicKey', 'host', 'projectId'];
requiredComponents.forEach(component => {
const hasMissingRequiredComponent = requiredComponents.find(component => {
if (!dsn[component]) {
throw new SentryError(`Invalid Sentry Dsn: ${component} missing`);
logger.error(`Invalid Sentry Dsn: ${component} missing`);
return true;
}
return false;
});

if (hasMissingRequiredComponent) {
return false;
}

if (!projectId.match(/^\d+$/)) {
throw new SentryError(`Invalid Sentry Dsn: Invalid projectId ${projectId}`);
logger.error(`Invalid Sentry Dsn: Invalid projectId ${projectId}`);
return false;
}

if (!isValidProtocol(protocol)) {
throw new SentryError(`Invalid Sentry Dsn: Invalid protocol ${protocol}`);
logger.error(`Invalid Sentry Dsn: Invalid protocol ${protocol}`);
return false;
}

if (port && isNaN(parseInt(port, 10))) {
throw new SentryError(`Invalid Sentry Dsn: Invalid port ${port}`);
logger.error(`Invalid Sentry Dsn: Invalid port ${port}`);
return false;
}

return true;
}

/** The Sentry Dsn, identifying a Sentry instance and project. */
export function makeDsn(from: DsnLike): DsnComponents {
/**
* Creates a valid Sentry Dsn object, identifying a Sentry instance and project.
* @returns a valid DsnComponents object or `undefined` if @param from is an invalid DSN source
*/
export function makeDsn(from: DsnLike): DsnComponents | undefined {
const components = typeof from === 'string' ? dsnFromString(from) : dsnFromComponents(from);
validateDsn(components);
if (!components || !validateDsn(components)) {
return undefined;
}
return components;
}