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
15 changes: 13 additions & 2 deletions packages/browser/src/integrations/graphqlClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ 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.
// The block-string branch consumes escaped `\"""` as a unit, so the lazy match cannot end on an
// escaped delimiter and leak the remainder of the block.
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 +114,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 +151,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
102 changes: 101 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,44 @@
* @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: """a \\""" b""") { id } }')).toBe(
'mutation { post(body: "*") { id } }',
);
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 +400,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 +563,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
83 changes: 83 additions & 0 deletions packages/core/src/utils/data-collection/filterHttpBody.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
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`. Keys are limited to the
* characters `application/x-www-form-urlencoded` encoding produces and raw whitespace disqualifies
* (encoded forms write spaces as `+` or `%20`), so XML, multipart, prose, and URLs never count as
* a pseudo-form that the filter would then rewrite.
*/
const FORM_SEGMENT_RE = /^(?:[\w%.*+-]+(?:=[^&\s]*)?)?$/;

/**
* 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 thread
logaretm marked this conversation as resolved.
}
Comment thread
sentry[bot] marked this conversation as resolved.

/**
* Scrubs the values of known-sensitive keys in an HTTP body the SDK collected itself, before it
Comment on lines +23 to +25

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 filterQueryParams function incorrectly adds =[Filtered] to sensitive keys that originally have no value, altering the request's structure and semantics.
Severity: LOW

Suggested Fix

Modify the logic in filterQueryParams to check if the original pair contains an = character. If a sensitive key is found and the original pair was valueless (no =), it should be replaced with just the encoded key or a value that preserves its valueless nature, not ${encodedKey}=${FILTERED_VALUE}.

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#L23-L25

Potential issue: The `filterQueryParams` function unconditionally appends
`=${FILTERED_VALUE}` to any key identified as sensitive. This logic does not account for
valueless keys, such as boolean flags in a form body (e.g.,
`sensitive_flag&other_param=value`). When `sensitive_flag` is filtered, it is
incorrectly transformed into `sensitive_flag=[Filtered]`, which changes the semantics of
the request. This contradicts the stated goal of preserving the request's structure
during data collection and could lead to incorrect server-side processing for APIs that
rely on valueless parameters.

* 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),
Comment thread
logaretm marked this conversation as resolved.
]),
);
}
19 changes: 15 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,22 +161,32 @@ 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);
Comment thread
logaretm marked this conversation as resolved.

// Using TextEncoder to get byte length for UTF-8 strings
const encoder = new TextEncoder();
const bytes = encoder.encode(body);
const bodyByteLength = bytes.length;

// Requests without a content-length header bypass the early size check, so the hard cap is
// enforced again after reading — both paths skip oversized bodies alike.
if (bodyByteLength > MAX_BODY_BYTE_LENGTH) {
DEBUG_BUILD && debug.log('Skipping body capture: body too large', bodyByteLength);
return;
}

let truncatedBody: string;
if (bodyByteLength > maxBodySize) {
const decoder = new TextDecoder();
Expand Down Expand Up @@ -229,7 +240,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