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(core): Ensure tunnel is considered for isSentryUrl checks #9130

Merged
merged 1 commit into from
Sep 27, 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
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
Copy link
Member Author

Choose a reason for hiding this comment

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

Also noticed that host may include the port (e.g. localhost:3000), so we better check that this is not the case before re-adding the 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());
}