Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .size-limit.js
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ module.exports = [
ignore: [...builtinModules, ...nodePrefixedBuiltinModules],
gzip: false,
brotli: false,
limit: '502 KiB',
limit: '503 KiB',
disablePlugins: ['@size-limit/webpack'],
webpack: false,
modifyEsbuildConfig: function (config) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ it('Captures form-urlencoded request body', async ({ signal }) => {
headers: expect.any(Object),
method: 'POST',
url: expect.stringContaining('/post-form'),
data: 'username=test&password=secret',
data: 'username=test&password=[Filtered]',
},
},
// Raw URL span (source `url`), so the TwP DSC omits the span name.
Expand Down
13 changes: 11 additions & 2 deletions packages/browser/src/integrations/graphqlClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ interface GraphQLOperation {

const INTEGRATION_NAME = 'GraphQLClient' as const;

// Matches the Int, Float, String, and BlockString literals in a document, the same set the
// server-side GraphQL integration redacts from the parsed AST. Names, enums, and booleans stay.
const GRAPHQL_LITERAL_RE = /"""[\s\S]*?"""|"(?:[^"\\\n]|\\.)*"|-?\b\d+(?:\.\d+)?(?:[eE][+-]?\d+)?\b/g;

/** Replaces every literal value in a raw GraphQL document, since literals can carry user data. */
export function _redactGraphqlDocument(document: string): string {
return document.replace(GRAPHQL_LITERAL_RE, match => (match.startsWith('"') ? '"*"' : '*'));
}

const _graphqlClientIntegration = ((options: GraphQLClientOptions) => {
return {
name: INTEGRATION_NAME,
Expand Down Expand Up @@ -103,7 +112,7 @@ function _updateSpanWithGraphQLData(client: Client, options: GraphQLClientOption

// Handle standard requests - capture the query document when enabled via dataCollection (default true)
if (isStandardRequest(graphqlBody) && client.getDataCollectionOptions().graphQL.document === true) {
span.setAttribute(GRAPHQL_DOCUMENT, graphqlBody.query);
span.setAttribute(GRAPHQL_DOCUMENT, _redactGraphqlDocument(graphqlBody.query));
}

// Handle persisted operations - capture hash for debugging
Expand Down Expand Up @@ -140,7 +149,7 @@ function _updateBreadcrumbWithGraphQLData(client: Client, options: GraphQLClient
data['graphql.operation'] = operationInfo;

if (isStandardRequest(graphqlBody) && client.getDataCollectionOptions().graphQL.document === true) {
data[GRAPHQL_DOCUMENT] = graphqlBody.query;
data[GRAPHQL_DOCUMENT] = _redactGraphqlDocument(graphqlBody.query);
}

if (isPersistedRequest(graphqlBody)) {
Expand Down
99 changes: 98 additions & 1 deletion packages/browser/test/integrations/graphqlClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,41 @@
* @vitest-environment jsdom
*/

import type { Client } from '@sentry/core';
import type { Breadcrumb, Client } from '@sentry/core';
import { SentrySpan, spanToJSON } from '@sentry/core';
import type { FetchHint, XhrHint } from '@sentry/browser-utils';
import { SENTRY_XHR_DATA_KEY } from '@sentry/browser-utils';
import { URL_FULL } from '@sentry/conventions/attributes';
import { describe, expect, test } from 'vitest';
import {
_getGraphQLOperation,
_redactGraphqlDocument,
getGraphQLRequestPayload,
getRequestPayloadXhrOrFetch,
graphqlClientIntegration,
parseGraphQLQuery,
} from '../../src/integrations/graphqlClient';

describe('_redactGraphqlDocument', () => {
test('replaces string and numeric literal arguments', () => {
expect(_redactGraphqlDocument('query { user(email: "jane@example.com", age: 42) { name } }')).toBe(
'query { user(email: "*", age: *) { name } }',
);
});

test('replaces block string literals', () => {
expect(_redactGraphqlDocument('mutation { post(body: """secret\nlines""") { id } }')).toBe(
'mutation { post(body: "*") { id } }',
);
});

test('leaves documents without literals untouched', () => {
const document = 'query Test($id: ID!) {\n people {\n name\n }\n}';

expect(_redactGraphqlDocument(document)).toBe(document);
});
});

describe('GraphqlClient', () => {
describe('parseGraphQLQuery', () => {
const queryOne = `query Test {
Expand Down Expand Up @@ -376,6 +397,33 @@ describe('GraphqlClient', () => {
expect(json.attributes['graphql.operation.type']).toBe('query');
});

test('redacts literals in the captured document', () => {
const handler = setupHandler([/\/graphql$/]);
const span = new SentrySpan({
name: 'POST http://localhost:4000/graphql',
op: 'http.client',
attributes: {
'http.method': 'POST',
[URL_FULL]: 'http://localhost:4000/graphql',
url: 'http://localhost:4000/graphql',
},
});

handler(
span,
makeFetchHint('http://localhost:4000/graphql', {
query: 'query GetUser { user(email: "jane@example.com", age: 42) { name } }',
operationName: 'GetUser',
variables: {},
extensions: {},
}),
);

expect(spanToJSON(span).attributes['graphql.document']).toBe(
'query GetUser { user(email: "*", age: *) { name } }',
);
});

test('keeps the low-cardinality span name with span streaming enabled', () => {
const handler = setupHandler([/\/graphql$/], true, 'stream');
const span = new SentrySpan({
Expand Down Expand Up @@ -512,4 +560,53 @@ describe('GraphqlClient', () => {
expect(json.attributes['graphql.document']).toBeUndefined();
});
});

describe('beforeOutgoingRequestBreadcrumb handler', () => {
test('redacts literals in the captured document', () => {
let capturedListener: ((breadcrumb: Breadcrumb, handlerData: FetchHint | XhrHint) => void) | undefined;
const mockClient = {
on: (eventName: string, cb: (breadcrumb: Breadcrumb, handlerData: FetchHint | XhrHint) => void) => {
if (eventName === 'beforeOutgoingRequestBreadcrumb') {
capturedListener = cb;
}
},
getOptions: () => ({}),
getDataCollectionOptions: () => ({ graphQL: { document: true, variables: true } }),
} as unknown as Client;

const integration = graphqlClientIntegration({ endpoints: [/\/graphql$/] });
integration.setup?.(mockClient);

if (!capturedListener) {
throw new Error('beforeOutgoingRequestBreadcrumb listener was not registered');
}

const breadcrumb: Breadcrumb = {
category: 'fetch',
type: 'http',
data: { url: 'http://localhost:4000/graphql', method: 'POST' },
};

capturedListener(breadcrumb, {
input: [
'http://localhost:4000/graphql',
{
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
query: 'query GetUser { user(email: "jane@example.com", age: 42) { name } }',
operationName: 'GetUser',
variables: {},
extensions: {},
}),
},
],
response: new Response(null, { status: 200 }),
startTimestamp: Date.now(),
endTimestamp: Date.now() + 1,
});

expect(breadcrumb.data?.['graphql.document']).toBe('query GetUser { user(email: "*", age: *) { name } }');
});
});
});
4 changes: 2 additions & 2 deletions packages/cloudflare/test/request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ describe('withSentry', () => {
request: new Request('https://example.com', {
method: 'POST',
headers: { 'content-type': 'application/json' },
body: JSON.stringify({ key: 'value' }),
body: JSON.stringify({ colour: 'blue' }),
}),
context,
},
Expand All @@ -353,7 +353,7 @@ describe('withSentry', () => {
},
);

expect(sentryEvent.sdkProcessingMetadata?.normalizedRequest?.data).toEqual(JSON.stringify({ key: 'value' }));
expect(sentryEvent.sdkProcessingMetadata?.normalizedRequest?.data).toEqual(JSON.stringify({ colour: 'blue' }));
});

test('does not capture cookies when dataCollection.cookies is disabled', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { Scope } from '../../scope';
import { debug } from '../../utils/debug-logger';
import { DEBUG_BUILD } from '../../debug-build';
import type { HttpIncomingMessage } from './types';
import { filterCollectedHttpBodyString } from '../../utils/data-collection/filterHttpBody';
import { getMaxBodyByteLength, type MaxRequestBodySize } from '../../utils/request';

/**
Expand Down Expand Up @@ -92,7 +93,8 @@ export function patchRequestToCaptureBody(

req.on('end', () => {
try {
const body = Buffer.concat(chunks).toString('utf-8');
// The filter runs before truncation, because a truncated JSON body no longer parses.
const body = filterCollectedHttpBodyString(Buffer.concat(chunks).toString('utf-8'));
if (body) {
// Using Buffer.byteLength here, because the body may contain characters that are not 1 byte long
const bodyByteLength = Buffer.byteLength(body, 'utf-8');
Expand Down
7 changes: 5 additions & 2 deletions packages/core/src/trpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { getClient, withIsolationScope } from './currentScopes';
import { captureException } from './exports';
import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from './semanticAttributes';
import { startSpanManual } from './tracing/trace';
import { filterCollectedHttpBody } from './utils/data-collection/filterHttpBody';
import { normalize } from './utils/normalize';
import { setNormalizationDepthOverrideHint } from './utils/normalizationHints';

Expand Down Expand Up @@ -74,15 +75,17 @@ export function trpcMiddleware(options: SentryTrpcMiddlewareOptions = {}) {
? options.attachRpcInput
: dataCollection?.httpBodies.includes('incomingRequest')
) {
// Filtering runs after normalization so class instances become plain objects the
// key-value filter can walk instead of being redacted as a whole.
if (rawInput !== undefined) {
trpcContext.input = normalize(rawInput);
trpcContext.input = filterCollectedHttpBody(normalize(rawInput));
}

if (getRawInput !== undefined && typeof getRawInput === 'function') {
try {
const rawRes = await getRawInput();

trpcContext.input = normalize(rawRes);
trpcContext.input = filterCollectedHttpBody(normalize(rawRes));
} catch {
// noop
}
Expand Down
78 changes: 78 additions & 0 deletions packages/core/src/utils/data-collection/filterHttpBody.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { isPlainObject } from '../is';
import { FILTERED_VALUE } from './filtering-snippets';
import { shouldFilterDataKey } from './filterKeyValueData';
import { filterQueryParams } from './filterQueryParams';

/** One `&`-separated form segment: empty, a bare key, or `key=value`. */
const FORM_SEGMENT_RE = /^(?:[^=]+(?:=.*)?)?$/;

/**
* A form body is `&`-separated `key=value` pairs, the only non-JSON shape whose keys the denylist
* can check. Valueless keys, empty segments, and a trailing `&` are tolerated — a too-strict gate
* would let a body like `password=secret&` skip the filter and ship raw. At least one `=` is
* required so prose is never rewritten as a pseudo-form.
*/
function isFormBody(body: string): boolean {
return body.includes('=') && body.split('&').every(segment => FORM_SEGMENT_RE.test(segment));
}
Comment on lines +15 to +17

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The isFormBody() function misidentifies XML with attributes as form-encoded, causing filterQueryParams() to corrupt the XML body when sensitive attribute names are present.
Severity: HIGH

Suggested Fix

Before applying form-body filtering, check the request's Content-Type header to ensure it is actually application/x-www-form-urlencoded. Alternatively, make the regex in isFormBody() stricter to avoid matching XML structures.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/core/src/utils/data-collection/filterHttpBody.ts#L15-L17

Potential issue: The `isFormBody()` function incorrectly classifies XML bodies
containing attributes as form-encoded because it only checks for the presence of an `=`
character. This causes the XML string to be passed to `filterQueryParams()`. For an XML
body like `<config auth_token="secret">`, the function incorrectly identifies `<config
auth_token` as a parameter key. Since this "key" contains a sensitive substring
(`auth`), the entire body is incorrectly filtered and corrupted, resulting in `<config
auth_token=[Filtered]>`. This affects any XML request body with attributes whose names
contain sensitive keywords, leading to corrupted diagnostic data.

Did we get this right? 👍 / 👎 to inform future reviews.


/**
* Scrubs the values of known-sensitive keys in an HTTP body the SDK collected itself, before it
* becomes `request.data` or `http.request.body.data`.
*
* Only values the SDK can attribute to a sensitive key are replaced. Everything else passes
* through unchanged: Relay scrubs server-side anyway and cannot tell an SDK-filtered value from a
* literal one, so client-side filtering beyond known-sensitive keys only destroys data.
*/
export function filterCollectedHttpBody(body: unknown): unknown {
if (typeof body === 'string') {
return filterCollectedHttpBodyString(body);
}

return isPlainObject(body) || Array.isArray(body) ? filterBodyValue(body) : body;
}

/**
* String-only variant of {@link filterCollectedHttpBody}. Capture sites call this before they
* truncate, because a truncated JSON body no longer parses and would pass through unfiltered.
*/
export function filterCollectedHttpBodyString(body: string): string {
if (!body) {
return body;
}

try {
const json: unknown = JSON.parse(body);
if (typeof json === 'object' && json !== null) {
return JSON.stringify(filterBodyValue(json));
}
} catch {
// Not JSON. The form-encoded attempt below runs instead.
}

if (isFormBody(body)) {
// The query-param filter keeps the body's original encoding byte-for-byte.
return filterQueryParams(body, true) ?? body;
}

return body;
}

function filterBodyValue(value: unknown): unknown {
if (Array.isArray(value)) {
return value.map(filterBodyValue);
}

if (!isPlainObject(value)) {
return value;
}

// `Object.fromEntries` instead of assigning `result[key]`, so user-controlled keys like
// `__proto__` never hit a computed property write (CodeQL js/remote-property-injection).
return Object.fromEntries(
Object.entries(value).map(([key, nested]) => [
key,
shouldFilterDataKey(key, true) ? FILTERED_VALUE : filterBodyValue(nested),
]),
);
}
12 changes: 8 additions & 4 deletions packages/core/src/utils/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { CookiePair } from './cookie';
import { parseCookieHeader } from './cookie';
import { debug } from './debug-logger';
import { FILTERED_VALUE, SENSITIVE_COOKIE_NAME_SNIPPETS } from './data-collection/filtering-snippets';
import { filterCollectedHttpBody, filterCollectedHttpBodyString } from './data-collection/filterHttpBody';
import { shouldFilterDataKey } from './data-collection/filterKeyValueData';
import { safeUnref } from './timer';
import { getUrlQuery } from './url';
Expand Down Expand Up @@ -160,17 +161,20 @@ export async function captureBodyFromWinterCGRequest(
safeUnref(setTimeout(() => resolve(null), 2000));
});

const body = await Promise.race([bodyPromise, timeoutPromise]);
const rawBody = await Promise.race([bodyPromise, timeoutPromise]);

if (body === null) {
if (rawBody === null) {
DEBUG_BUILD && debug.log('Timeout reading request body');
return;
}

if (!body) {
if (!rawBody) {
return;
}

// The filter runs before truncation, because a truncated JSON body no longer parses.
const body = filterCollectedHttpBodyString(rawBody);

// Using TextEncoder to get byte length for UTF-8 strings
const encoder = new TextEncoder();
const bytes = encoder.encode(body);
Expand Down Expand Up @@ -229,7 +233,7 @@ export function httpRequestToRequestData(request: {

// This is non-standard, but may be sometimes set
// It may be overwritten later by our own body handling
const data = (request as PolymorphicRequest).body || undefined;
const data = filterCollectedHttpBody((request as PolymorphicRequest).body || undefined);

// This is non-standard, but may be set on e.g. Next.js or Express requests
const cookies = (request as PolymorphicRequest).cookies;
Expand Down
Loading
Loading