Skip to content

Commit b82b19e

Browse files
chargomeclaude
chargome
and
claude
committed
test(core): Cover breadcrumb query filtering and tighten span assertions
Breadcrumb query filtering had no test coverage, so re-leaking a token would not have failed CI. Adds cases for the default denylist, off mode, allowList and extra deny terms on both outgoing request breadcrumb paths. The node fetch path needs its own file because the existing test module mocks `getClient` without `getDataCollectionOptions`. Also fixes two span tests that claimed more than they asserted: one checks the span name is untouched but never looked at it, and the other claimed to cover attributes set after the span starts while passing them in at creation. The latter now registers a `processSpan` subscriber, mirroring how `requestDataIntegration` sets `url.full`. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
1 parent 43aab06 commit b82b19e

3 files changed

Lines changed: 143 additions & 12 deletions

File tree

‎packages/core/test/lib/integrations/http/add-outgoing-request-breadcrumb.test.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
22
import * as breadcrumbsModule from '../../../../src/breadcrumbs';
3+
import { withScope } from '../../../../src/currentScopes';
34
import { addOutgoingRequestBreadcrumb } from '../../../../src/integrations/http/add-outgoing-request-breadcrumb';
45
import type { HttpClientRequest, HttpIncomingMessage } from '../../../../src/integrations/http/types';
6+
import type { CollectBehavior } from '../../../../src/types/datacollection';
7+
import { getDefaultTestClientOptions, TestClient } from '../../../mocks/client';
58

69
function makeMockRequest(overrides: Partial<Record<string, unknown>> = {}): HttpClientRequest {
710
return {
@@ -164,4 +167,44 @@ describe('addOutgoingRequestBreadcrumb', () => {
164167
const callArg = vi.mocked(breadcrumbsModule.addBreadcrumb).mock.calls[0]![0];
165168
expect(callArg.data?.['http.method']).toBe('GET');
166169
});
170+
171+
// Breadcrumbs never reach the span pipeline, so this is the only place `urlQueryParams` is applied to them.
172+
describe('dataCollection.urlQueryParams', () => {
173+
function breadcrumbQuery(path: string, urlQueryParams?: CollectBehavior): unknown {
174+
const client = new TestClient(
175+
getDefaultTestClientOptions({
176+
dsn: 'https://[email protected]/1',
177+
...(urlQueryParams !== undefined ? { dataCollection: { urlQueryParams } } : {}),
178+
}),
179+
);
180+
181+
return withScope(scope => {
182+
scope.setClient(client);
183+
addOutgoingRequestBreadcrumb(makeMockRequest({ path }), makeMockResponse());
184+
185+
const callArg = vi.mocked(breadcrumbsModule.addBreadcrumb).mock.calls.at(-1)![0];
186+
return callArg.data?.['url.query'];
187+
});
188+
}
189+
190+
it('filters sensitive params and preserves encoding by default', () => {
191+
expect(breadcrumbQuery('/api/test?token=abc123&q=a%20b%26c&page=5')).toBe('token=[Filtered]&q=a%20b%26c&page=5');
192+
});
193+
194+
it('omits the query entirely when collection is off', () => {
195+
expect(breadcrumbQuery('/api/test?token=abc123&page=5', false)).toBeUndefined();
196+
});
197+
198+
it('honors allowList mode', () => {
199+
expect(breadcrumbQuery('/api/test?page=1&ref=x&sort=name', { allow: ['page', 'sort'] })).toBe(
200+
'page=1&ref=[Filtered]&sort=name',
201+
);
202+
});
203+
204+
it('honors extra deny terms', () => {
205+
expect(breadcrumbQuery('/api/test?page=1&utm_source=email', { deny: ['utm'] })).toBe(
206+
'page=1&utm_source=[Filtered]',
207+
);
208+
});
209+
});
167210
});

‎packages/core/test/lib/tracing/spans/captureSpan.test.ts‎

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -796,10 +796,12 @@ describe('applyScopeToSegmentSpan integration', () => {
796796
});
797797

798798
describe('dataCollection.urlQueryParams', () => {
799+
const SPAN_NAME_PATH = '/api/users';
800+
799801
function captureUrlSpan(
800802
attributes: Record<string, string>,
801803
dataCollection?: { urlQueryParams?: boolean | { allow: string[] } | { deny: string[] } },
802-
): Record<string, { value: unknown }> | undefined {
804+
) {
803805
const client = new TestClient(
804806
getDefaultTestClientOptions({
805807
dsn: 'https://[email protected]/1',
@@ -810,16 +812,22 @@ describe('applyScopeToSegmentSpan integration', () => {
810812

811813
const span = withScope(scope => {
812814
scope.setClient(client);
813-
const span = startInactiveSpan({ name: 'my-span', attributes });
815+
const span = startInactiveSpan({ name: `GET ${SPAN_NAME_PATH}`, attributes });
814816
span.end();
815817
return span;
816818
});
817819

818-
return captureSpan(span, client).attributes as Record<string, { value: unknown }> | undefined;
820+
return captureSpan(span, client);
821+
}
822+
823+
function captureUrlSpanAttributes(
824+
...args: Parameters<typeof captureUrlSpan>
825+
): Record<string, { value: unknown }> | undefined {
826+
return captureUrlSpan(...args).attributes as Record<string, { value: unknown }> | undefined;
819827
}
820828

821829
it('filters sensitive query params in `url.full` and `url.query` by default', () => {
822-
const attributes = captureUrlSpan({
830+
const attributes = captureUrlSpanAttributes({
823831
'url.full': 'https://example.com/api/users?token=abc123&q=a%20b%26c&page=5',
824832
'url.query': 'token=abc123&q=a%20b%26c&page=5',
825833
'url.path': '/api/users',
@@ -830,16 +838,19 @@ describe('applyScopeToSegmentSpan integration', () => {
830838
});
831839

832840
it('leaves `url.path` and the span name untouched', () => {
833-
const attributes = captureUrlSpan({
841+
const serialized = captureUrlSpan({
834842
'url.full': 'https://example.com/api/users?token=abc123',
835843
'url.path': '/api/users',
836844
});
845+
const attributes = serialized.attributes as Record<string, { value: unknown }> | undefined;
837846

838847
expect(attributes?.['url.path']?.value).toBe('/api/users');
848+
expect(serialized.name).toBe(`GET ${SPAN_NAME_PATH}`);
849+
expect(attributes?.['sentry.segment.name']?.value).toBe(`GET ${SPAN_NAME_PATH}`);
839850
});
840851

841852
it('removes query data entirely when collection is off', () => {
842-
const attributes = captureUrlSpan(
853+
const attributes = captureUrlSpanAttributes(
843854
{
844855
'url.full': 'https://example.com/api/users?token=abc123&page=5',
845856
'url.query': 'token=abc123&page=5',
@@ -852,7 +863,7 @@ describe('applyScopeToSegmentSpan integration', () => {
852863
});
853864

854865
it('honors allowList mode', () => {
855-
const attributes = captureUrlSpan(
866+
const attributes = captureUrlSpanAttributes(
856867
{ 'url.query': 'page=1&ref=x&sort=name' },
857868
{ urlQueryParams: { allow: ['page', 'sort'] } },
858869
);
@@ -861,22 +872,38 @@ describe('applyScopeToSegmentSpan integration', () => {
861872
});
862873

863874
it('honors extra deny terms', () => {
864-
const attributes = captureUrlSpan(
875+
const attributes = captureUrlSpanAttributes(
865876
{ 'url.query': 'page=1&utm_source=email' },
866877
{ urlQueryParams: { deny: ['utm'] } },
867878
);
868879

869880
expect(attributes?.['url.query']?.value).toBe('page=1&utm_source=[Filtered]');
870881
});
871882

872-
it('filters attributes set by instrumentation that runs after the span starts', () => {
873-
const attributes = captureUrlSpan({ 'url.full': 'https://example.com/s?session=abc&ok=1' });
883+
it('filters URL attributes set by a `processSpan` subscriber after the span ended', () => {
884+
const client = new TestClient(
885+
getDefaultTestClientOptions({ dsn: 'https://[email protected]/1', tracesSampleRate: 1 }),
886+
);
887+
888+
// Mirrors `requestDataIntegration`, which sets `url.full` during `processSegmentSpan`
889+
client.on('processSpan', spanJSON => {
890+
safeSetSpanJSONAttributes(spanJSON, { 'url.full': 'https://example.com/s?session=abc&ok=1' });
891+
});
892+
893+
const span = withScope(scope => {
894+
scope.setClient(client);
895+
const span = startInactiveSpan({ name: 'my-span' });
896+
span.end();
897+
return span;
898+
});
899+
900+
const attributes = captureSpan(span, client).attributes as Record<string, { value: unknown }> | undefined;
874901

875902
expect(attributes?.['url.full']?.value).toBe('https://example.com/s?session=[Filtered]&ok=1');
876903
});
877904

878905
it('filters URL attributes stored as attribute objects', () => {
879-
const attributes = captureUrlSpan({
906+
const attributes = captureUrlSpanAttributes({
880907
'url.full': { value: 'https://example.com/s?token=abc&ok=1', type: 'string' },
881908
'url.query': { value: 'token=abc&ok=1', type: 'string' },
882909
} as unknown as Record<string, string>);
@@ -886,7 +913,7 @@ describe('applyScopeToSegmentSpan integration', () => {
886913
});
887914

888915
it('removes an attribute-object `url.query` when collection is off', () => {
889-
const attributes = captureUrlSpan(
916+
const attributes = captureUrlSpanAttributes(
890917
{ 'url.query': { value: 'token=abc', type: 'string' } } as unknown as Record<string, string>,
891918
{ urlQueryParams: false },
892919
);

‎packages/node/test/utils/outgoingFetchRequestBreadcrumb.test.ts‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import type { CollectBehavior } from '@sentry/core';
2+
import { addBreadcrumb, getCurrentScope, withScope } from '@sentry/core';
3+
import { beforeEach, describe, expect, it, vi } from 'vitest';
4+
import type { UndiciRequest, UndiciResponse } from '../../src/integrations/node-fetch/types';
5+
import { addFetchRequestBreadcrumb } from '../../src/utils/outgoingFetchRequest';
6+
import { NodeClient } from '../../src';
7+
import { getDefaultNodeClientOptions } from '../helpers/getDefaultNodeClientOptions';
8+
9+
vi.mock('@sentry/core', async () => {
10+
const actual = (await vi.importActual('@sentry/core')) as Record<string, unknown>;
11+
return { ...actual, addBreadcrumb: vi.fn() };
12+
});
13+
14+
function makeRequest(path: string): UndiciRequest {
15+
return { method: 'GET', origin: 'https://example.com', path, headers: {} } as unknown as UndiciRequest;
16+
}
17+
18+
const RESPONSE = { statusCode: 200 } as unknown as UndiciResponse;
19+
20+
/**
21+
* Breadcrumbs never reach the span pipeline, so `addFetchRequestBreadcrumb` is the only place
22+
* `dataCollection.urlQueryParams` is applied to outgoing fetch breadcrumbs.
23+
*/
24+
describe('addFetchRequestBreadcrumb', () => {
25+
beforeEach(() => {
26+
vi.clearAllMocks();
27+
});
28+
29+
function breadcrumbQuery(path: string, urlQueryParams?: CollectBehavior): unknown {
30+
const client = new NodeClient(
31+
getDefaultNodeClientOptions(urlQueryParams !== undefined ? { dataCollection: { urlQueryParams } } : {}),
32+
);
33+
34+
return withScope(scope => {
35+
scope.setClient(client);
36+
getCurrentScope().setClient(client);
37+
addFetchRequestBreadcrumb(makeRequest(path), RESPONSE);
38+
39+
const callArg = vi.mocked(addBreadcrumb).mock.calls.at(-1)![0];
40+
return callArg.data?.['url.query'];
41+
});
42+
}
43+
44+
it('filters sensitive params and preserves encoding by default', () => {
45+
expect(breadcrumbQuery('/api?token=abc123&q=a%20b%26c&page=5')).toBe('token=[Filtered]&q=a%20b%26c&page=5');
46+
});
47+
48+
it('omits the query entirely when collection is off', () => {
49+
expect(breadcrumbQuery('/api?token=abc123&page=5', false)).toBeUndefined();
50+
});
51+
52+
it('honors allowList mode', () => {
53+
expect(breadcrumbQuery('/api?page=1&ref=x&sort=name', { allow: ['page', 'sort'] })).toBe(
54+
'page=1&ref=[Filtered]&sort=name',
55+
);
56+
});
57+
58+
it('honors extra deny terms', () => {
59+
expect(breadcrumbQuery('/api?page=1&utm_source=email', { deny: ['utm'] })).toBe('page=1&utm_source=[Filtered]');
60+
});
61+
});

0 commit comments

Comments
 (0)