Skip to content

Commit

Permalink
fix(core): Ensure tunnel is considered for isSentryUrl checks (#9130
Browse files Browse the repository at this point in the history
)

Also streamline these and use the same util in all places.
  • Loading branch information
mydea committed Sep 27, 2023
1 parent 95e1801 commit e10748b
Show file tree
Hide file tree
Showing 13 changed files with 134 additions and 79 deletions.
1 change: 1 addition & 0 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export { FunctionToString, InboundFilters } from './integrations';
export { prepareEvent } from './utils/prepareEvent';
export { createCheckInEnvelope } from './checkin';
export { hasTracingEnabled } from './utils/hasTracingEnabled';
export { isSentryRequestUrl } from './utils/isSentryRequestUrl';
export { DEFAULT_ENVIRONMENT } from './constants';
export { ModuleMetadata } from './integrations/metadata';
import * as Integrations from './integrations';
Expand Down
29 changes: 29 additions & 0 deletions packages/core/src/utils/isSentryRequestUrl.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import type { DsnComponents, Hub } from '@sentry/types';

/**
* Checks whether given url points to Sentry server
* @param url url to verify
*/
export function isSentryRequestUrl(url: string, hub: Hub): boolean {
const client = hub.getClient();
const dsn = client && client.getDsn();
const tunnel = client && client.getOptions().tunnel;

return checkDsn(url, dsn) || checkTunnel(url, tunnel);
}

function checkTunnel(url: string, tunnel: string | undefined): boolean {
if (!tunnel) {
return false;
}

return removeTrailingSlash(url) === removeTrailingSlash(tunnel);
}

function checkDsn(url: string, dsn: DsnComponents | undefined): boolean {
return dsn ? url.includes(dsn.host) : false;
}

function removeTrailingSlash(str: string): string {
return str[str.length - 1] === '/' ? str.slice(0, -1) : str;
}
26 changes: 26 additions & 0 deletions packages/core/test/lib/utils/isSentryRequestUrl.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import type { Hub } from '@sentry/types';

import { isSentryRequestUrl } from '../../../src';

describe('isSentryRequestUrl', () => {
it.each([
['', 'sentry-dsn.com', '', false],
['http://sentry-dsn.com/my-url', 'sentry-dsn.com', '', true],
['http://sentry-dsn.com', 'sentry-dsn.com', '', true],
['http://tunnel:4200', 'sentry-dsn.com', 'http://tunnel:4200', true],
['http://tunnel:4200', 'sentry-dsn.com', 'http://tunnel:4200/', true],
['http://tunnel:4200/', 'sentry-dsn.com', 'http://tunnel:4200', true],
['http://tunnel:4200/a', 'sentry-dsn.com', 'http://tunnel:4200', false],
])('works with url=%s, dsn=%s, tunnel=%s', (url: string, dsn: string, tunnel: string, expected: boolean) => {
const hub = {
getClient: () => {
return {
getOptions: () => ({ tunnel }),
getDsn: () => ({ host: dsn }),
};
},
} as unknown as Hub;

expect(isSentryRequestUrl(url, hub)).toBe(expected);
});
});
1 change: 1 addition & 0 deletions packages/integrations/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
}
},
"dependencies": {
"@sentry/core": "7.72.0",
"@sentry/types": "7.72.0",
"@sentry/utils": "7.72.0",
"localforage": "^1.8.1",
Expand Down
23 changes: 6 additions & 17 deletions packages/integrations/src/httpclient.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { getCurrentHub, isSentryRequestUrl } from '@sentry/core';
import type {
Event as SentryEvent,
EventProcessor,
Expand Down Expand Up @@ -345,30 +346,18 @@ export class HttpClient implements Integration {
);
}

/**
* Checks whether given url points to Sentry server
*
* @param url url to verify
*/
private _isSentryRequest(url: string): boolean {
const client = this._getCurrentHub && this._getCurrentHub().getClient();

if (!client) {
return false;
}

const dsn = client.getDsn();
return dsn ? url.includes(dsn.host) : false;
}

/**
* Checks whether to capture given response as an event
*
* @param status response status code
* @param url response url
*/
private _shouldCaptureResponse(status: number, url: string): boolean {
return this._isInGivenStatusRanges(status) && this._isInGivenRequestTargets(url) && !this._isSentryRequest(url);
return (
this._isInGivenStatusRanges(status) &&
this._isInGivenRequestTargets(url) &&
!isSentryRequestUrl(url, getCurrentHub())
);
}

/**
Expand Down
15 changes: 4 additions & 11 deletions packages/node-experimental/src/integrations/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ import { SpanKind } from '@opentelemetry/api';
import { registerInstrumentations } from '@opentelemetry/instrumentation';
import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import { hasTracingEnabled, Transaction } from '@sentry/core';
import { hasTracingEnabled, isSentryRequestUrl, Transaction } from '@sentry/core';
import { getCurrentHub } from '@sentry/node';
import { _INTERNAL_getSentrySpan } from '@sentry/opentelemetry-node';
import type { EventProcessor, Hub, Integration } from '@sentry/types';
import type { ClientRequest, IncomingMessage, ServerResponse } from 'http';

import type { NodeExperimentalClient, OtelSpan } from '../types';
import { getRequestSpanData } from '../utils/getRequestSpanData';
import { getRequestUrl } from '../utils/getRequestUrl';

interface TracingOptions {
/**
Expand Down Expand Up @@ -93,8 +94,8 @@ export class Http implements Integration {
instrumentations: [
new HttpInstrumentation({
ignoreOutgoingRequestHook: request => {
const host = request.host || request.hostname;
return isSentryHost(host);
const url = getRequestUrl(request);
return url ? isSentryRequestUrl(url, getCurrentHub()) : false;
},

ignoreIncomingRequestHook: request => {
Expand Down Expand Up @@ -224,11 +225,3 @@ function getHttpUrl(attributes: Attributes): string | undefined {
const url = attributes[SemanticAttributes.HTTP_URL];
return typeof url === 'string' ? url : undefined;
}

/**
* Checks whether given host points to Sentry server
*/
function isSentryHost(host: string | undefined): boolean {
const dsn = getCurrentHub().getClient()?.getDsn();
return dsn && host ? host.includes(dsn.host) : false;
}
15 changes: 15 additions & 0 deletions packages/node-experimental/src/utils/getRequestUrl.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import type { RequestOptions } from 'http';

/** Build a full URL from request options. */
export function getRequestUrl(requestOptions: RequestOptions): string {
const protocol = requestOptions.protocol || '';
const hostname = requestOptions.hostname || requestOptions.host || '';
// Don't log standard :80 (http) and :443 (https) ports to reduce the noise
// Also don't add port if the hostname already includes a port
const port =
!requestOptions.port || requestOptions.port === 80 || requestOptions.port === 443 || /^(.*):(\d+)$/.test(hostname)
? ''
: `:${requestOptions.port}`;
const path = requestOptions.path ? requestOptions.path : '/';
return `${protocol}//${hostname}${port}${path}`;
}
20 changes: 20 additions & 0 deletions packages/node-experimental/test/utils/getRequestUrl.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import type { RequestOptions } from 'http';

import { getRequestUrl } from '../../src/utils/getRequestUrl';

describe('getRequestUrl', () => {
it.each([
[{ protocol: 'http:', hostname: 'localhost', port: 80 }, 'http://localhost/'],
[{ protocol: 'http:', hostname: 'localhost', host: 'localhost:80', port: 80 }, 'http://localhost/'],
[{ protocol: 'http:', hostname: 'localhost', port: 3000 }, 'http://localhost:3000/'],
[{ protocol: 'http:', host: 'localhost:3000', port: 3000 }, 'http://localhost:3000/'],
[{ protocol: 'https:', hostname: 'localhost', port: 443 }, 'https://localhost/'],
[{ protocol: 'https:', hostname: 'localhost', port: 443, path: '/my-path' }, 'https://localhost/my-path'],
[
{ protocol: 'https:', hostname: 'www.example.com', port: 443, path: '/my-path' },
'https://www.example.com/my-path',
],
])('works with %s', (input: RequestOptions, expected: string | undefined) => {
expect(getRequestUrl(input)).toBe(expected);
});
});
6 changes: 3 additions & 3 deletions packages/node/src/integrations/http.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Hub } from '@sentry/core';
import { getCurrentHub, getDynamicSamplingContextFromClient } from '@sentry/core';
import { getCurrentHub, getDynamicSamplingContextFromClient, isSentryRequestUrl } from '@sentry/core';
import type {
DynamicSamplingContext,
EventProcessor,
Expand All @@ -21,7 +21,7 @@ import { LRUMap } from 'lru_map';
import type { NodeClient } from '../client';
import { NODE_VERSION } from '../nodeVersion';
import type { RequestMethod, RequestMethodArgs, RequestOptions } from './utils/http';
import { cleanSpanDescription, extractRawUrl, extractUrl, isSentryRequest, normalizeRequestArgs } from './utils/http';
import { cleanSpanDescription, extractRawUrl, extractUrl, normalizeRequestArgs } from './utils/http';

interface TracingOptions {
/**
Expand Down Expand Up @@ -238,7 +238,7 @@ function _createWrappedRequestMethodFactory(
const requestUrl = extractUrl(requestOptions);

// we don't want to record requests to Sentry as either breadcrumbs or spans, so just use the original method
if (isSentryRequest(requestUrl)) {
if (isSentryRequestUrl(requestUrl, getCurrentHub())) {
return originalRequestMethod.apply(httpModule, requestArgs);
}

Expand Down
9 changes: 4 additions & 5 deletions packages/node/src/integrations/undici/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getCurrentHub, getDynamicSamplingContextFromClient } from '@sentry/core';
import { getCurrentHub, getDynamicSamplingContextFromClient, isSentryRequestUrl } from '@sentry/core';
import type { EventProcessor, Integration, Span } from '@sentry/types';
import {
dynamicRequire,
Expand All @@ -12,7 +12,6 @@ import { LRUMap } from 'lru_map';

import type { NodeClient } from '../../client';
import { NODE_VERSION } from '../../nodeVersion';
import { isSentryRequest } from '../utils/http';
import type {
DiagnosticsChannel,
RequestCreateMessage,
Expand Down Expand Up @@ -138,7 +137,7 @@ export class Undici implements Integration {

const stringUrl = request.origin ? request.origin.toString() + request.path : request.path;

if (isSentryRequest(stringUrl) || request.__sentry_span__ !== undefined) {
if (isSentryRequestUrl(stringUrl, hub) || request.__sentry_span__ !== undefined) {
return;
}

Expand Down Expand Up @@ -198,7 +197,7 @@ export class Undici implements Integration {

const stringUrl = request.origin ? request.origin.toString() + request.path : request.path;

if (isSentryRequest(stringUrl)) {
if (isSentryRequestUrl(stringUrl, hub)) {
return;
}

Expand Down Expand Up @@ -238,7 +237,7 @@ export class Undici implements Integration {

const stringUrl = request.origin ? request.origin.toString() + request.path : request.path;

if (isSentryRequest(stringUrl)) {
if (isSentryRequestUrl(stringUrl, hub)) {
return;
}

Expand Down
42 changes: 21 additions & 21 deletions packages/node/src/integrations/utils/http.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,9 @@
import { getCurrentHub } from '@sentry/core';
import type * as http from 'http';
import type * as https from 'https';
import { URL } from 'url';

import { NODE_VERSION } from '../../nodeVersion';

/**
* Checks whether given url points to Sentry server
* @param url url to verify
*/
export function isSentryRequest(url: string): boolean {
const dsn = getCurrentHub().getClient()?.getDsn();
return dsn ? url.includes(dsn.host) : false;
}

/**
* Assembles a URL that's passed to the users to filter on.
* It can include raw (potentially PII containing) data, which we'll allow users to access to filter
Expand All @@ -24,11 +14,7 @@ export function isSentryRequest(url: string): boolean {
*/
// TODO (v8): This function should include auth, query and fragment (it's breaking, so we need to wait for v8)
export function extractRawUrl(requestOptions: RequestOptions): string {
const protocol = requestOptions.protocol || '';
const hostname = requestOptions.hostname || requestOptions.host || '';
// Don't log standard :80 (http) and :443 (https) ports to reduce the noise
const port =
!requestOptions.port || requestOptions.port === 80 || requestOptions.port === 443 ? '' : `:${requestOptions.port}`;
const { protocol, hostname, port } = parseRequestOptions(requestOptions);
const path = requestOptions.path ? requestOptions.path : '/';
return `${protocol}//${hostname}${port}${path}`;
}
Expand All @@ -40,13 +26,10 @@ export function extractRawUrl(requestOptions: RequestOptions): string {
* @returns Fully-formed URL
*/
export function extractUrl(requestOptions: RequestOptions): string {
const protocol = requestOptions.protocol || '';
const hostname = requestOptions.hostname || requestOptions.host || '';
// Don't log standard :80 (http) and :443 (https) ports to reduce the noise
const port =
!requestOptions.port || requestOptions.port === 80 || requestOptions.port === 443 ? '' : `:${requestOptions.port}`;
// do not include search or hash in span descriptions, per https://develop.sentry.dev/sdk/data-handling/#structuring-data
const { protocol, hostname, port } = parseRequestOptions(requestOptions);

const path = requestOptions.pathname || '/';

// always filter authority, see https://develop.sentry.dev/sdk/data-handling/#structuring-data
const authority = requestOptions.auth ? redactAuthority(requestOptions.auth) : '';

Expand Down Expand Up @@ -221,3 +204,20 @@ export function normalizeRequestArgs(
return [requestOptions];
}
}

function parseRequestOptions(requestOptions: RequestOptions): {
protocol: string;
hostname: string;
port: string;
} {
const protocol = requestOptions.protocol || '';
const hostname = requestOptions.hostname || requestOptions.host || '';
// Don't log standard :80 (http) and :443 (https) ports to reduce the noise
// Also don't add port if the hostname already includes a port
const port =
!requestOptions.port || requestOptions.port === 80 || requestOptions.port === 443 || /^(.*):(\d+)$/.test(hostname)
? ''
: `:${requestOptions.port}`;

return { protocol, hostname, port };
}
13 changes: 2 additions & 11 deletions packages/opentelemetry-node/src/utils/isSentryRequest.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import { getCurrentHub } from '@sentry/core';
import { getCurrentHub, isSentryRequestUrl } from '@sentry/core';

/**
*
Expand All @@ -16,14 +16,5 @@ export function isSentryRequestSpan(otelSpan: OtelSpan): boolean {
return false;
}

return isSentryRequestUrl(httpUrl.toString());
}

/**
* Checks whether given url points to Sentry server
* @param url url to verify
*/
function isSentryRequestUrl(url: string): boolean {
const dsn = getCurrentHub().getClient()?.getDsn();
return dsn ? url.includes(dsn.host) : false;
return isSentryRequestUrl(httpUrl.toString(), getCurrentHub());
}
13 changes: 2 additions & 11 deletions packages/replay/src/util/shouldFilterRequest.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getCurrentHub } from '@sentry/core';
import { getCurrentHub, isSentryRequestUrl } from '@sentry/core';

import type { ReplayContainer } from '../types';

Expand All @@ -12,14 +12,5 @@ export function shouldFilterRequest(replay: ReplayContainer, url: string): boole
return false;
}

return _isSentryRequest(url);
}

/**
* Checks wether a given URL belongs to the configured Sentry DSN.
*/
function _isSentryRequest(url: string): boolean {
const client = getCurrentHub().getClient();
const dsn = client && client.getDsn();
return dsn ? url.includes(dsn.host) : false;
return isSentryRequestUrl(url, getCurrentHub());
}

0 comments on commit e10748b

Please sign in to comment.