Skip to content
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/astro/test/server/middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ describe('sentryMiddleware', () => {
httpHeaders: { request: true, response: true },
httpBodies: [],
queryParams: true,
graphQL: { document: true, variables: true },
genAI: { inputs: false, outputs: false },
stackFrameVariables: true,
frameContextLines: 5,
Expand Down
11 changes: 6 additions & 5 deletions packages/browser/src/integrations/graphqlClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from '@sentry/core/browser';
import type { FetchHint, XhrHint } from '@sentry/browser-utils';
import { getBodyString, getFetchRequestArgBody, SENTRY_XHR_DATA_KEY } from '@sentry/browser-utils';
import { GRAPHQL_DOCUMENT } from '@sentry/conventions/attributes';

interface GraphQLClientOptions {
endpoints: Array<string | RegExp>;
Expand Down Expand Up @@ -88,9 +89,9 @@ function _updateSpanWithGraphQLData(client: Client, options: GraphQLClientOption
const operationInfo = _getGraphQLOperation(graphqlBody);
span.updateName(`${httpMethod} ${httpUrl} (${operationInfo})`);

// Handle standard requests - always capture the query document
if (isStandardRequest(graphqlBody)) {
span.setAttribute('graphql.document', graphqlBody.query);
// 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);
}

// Handle persisted operations - capture hash for debugging
Expand Down Expand Up @@ -126,8 +127,8 @@ function _updateBreadcrumbWithGraphQLData(client: Client, options: GraphQLClient

data['graphql.operation'] = operationInfo;

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

if (isPersistedRequest(graphqlBody)) {
Expand Down
26 changes: 25 additions & 1 deletion packages/browser/test/integrations/graphqlClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,14 +313,18 @@ describe('GraphqlClient', () => {
});

describe('beforeOutgoingRequestSpan handler', () => {
function setupHandler(endpoints: Array<string | RegExp>): (span: SentrySpan, hint: FetchHint | XhrHint) => void {
function setupHandler(
endpoints: Array<string | RegExp>,
graphQLDocument = true,
): (span: SentrySpan, hint: FetchHint | XhrHint) => void {
let capturedListener: ((span: SentrySpan, hint: FetchHint | XhrHint) => void) | undefined;
const mockClient = {
on: (eventName: string, cb: (span: SentrySpan, hint: FetchHint | XhrHint) => void) => {
if (eventName === 'beforeOutgoingRequestSpan') {
capturedListener = cb;
}
},
getDataCollectionOptions: () => ({ graphQL: { document: graphQLDocument, variables: true } }),
} as unknown as Client;

const integration = graphqlClientIntegration({ endpoints });
Expand Down Expand Up @@ -420,5 +424,25 @@ describe('GraphqlClient', () => {
expect(json.description).toBe('custom span');
expect(json.data['graphql.document']).toBeUndefined();
});

test('omits graphql.document when dataCollection.graphQL.document is false', () => {
const handler = setupHandler([/\/graphql$/], false);
const span = new SentrySpan({
name: 'POST http://localhost:4000/graphql',
op: 'http.client',
attributes: {
'http.method': 'POST',
'http.url': 'http://localhost:4000/graphql',
url: 'http://localhost:4000/graphql',
},
});

handler(span, makeFetchHint('http://localhost:4000/graphql', requestBody));

const json = spanToJSON(span);
// The span is still renamed with the operation, but the document is not attached.
expect(json.description).toBe('POST http://localhost:4000/graphql (query GetHello)');
expect(json.data['graphql.document']).toBeUndefined();
});
});
});
22 changes: 21 additions & 1 deletion packages/core/src/types/datacollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,26 @@ export interface DataCollection {
queryParams?: CollectBehavior;

/**
* Controls generative AI input/output recording.
* Allow collection of GraphQL operation data when using the SDK's GraphQL integrations
*
* These options do not add GraphQL instrumentation on their own. The corresponding integration must still be enabled
* for any data to be captured.
*
* - `document`: attach the GraphQL document (query/mutation source text).
* - `variables`: attach the variables passed to GraphQL operations.
* @default { document: true, variables: true }
*/
graphQL?: {
document?: boolean;
variables?: boolean;
};
Comment on lines +63 to +66

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 graphQL.variables configuration option is defined but never used, making the feature to control GraphQL variable collection non-functional.
Severity: HIGH

Suggested Fix

Implement the logic to read the getDataCollectionOptions().graphQL.variables setting. Based on its value, conditionally collect or suppress the variables field from GraphQL requests. This logic should be added where GraphQL bodies are processed, such as in packages/browser/src/integrations/graphqlClient.ts and packages/server-utils/src/graphql/utils.ts.

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/types/datacollection.ts#L59-L62

Potential issue: The `graphQL.variables` configuration option is defined and documented,
but its value is never read in the production codebase. No code checks
`getDataCollectionOptions().graphQL.variables` before collecting GraphQL variables.
Since no variable collection code exists, the setting has no effect. Users who set
`graphQL: { variables: false }` believing they are suppressing variable collection are
not actually having variables collected (as the feature is missing), and users who set
`variables: true` believing variables will be captured find they are not. The API
contract is unfulfilled as the implementation is missing.

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

Comment on lines +63 to +66

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a blocker but something I was discussing with @ericapisani, this creates the expectation that the SDK is going to apply these options to all GraphQL requests which is not accurate.

It would only apply if the user is using the client or the server GraphQL integrations. GraphQL requests outside of the integrations coverage won't be covered by these options which I find weird. I know the gen AI spans is already a precedent, so I don't have strong opinions here.

An alternative I considered is adding the dataCollection option to the integrations themselves rather than a top-level option.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The core idea of the top-level option is to have only one place where all of this is defined in an on/off switch manner (like: this is allowed to be sent, but you also need the integration to be enabled). This makes it easier to see it all at once. According to the spec, it's also possible to add it on integration level, which would overwrite the option set on the root-level.

Maybe we should make this more clear in the JSDoc?

@logaretm logaretm Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's fine, we already have a precedent for it with gen AI stuff so not feeling too strongly about it but it is something that I wanted to bring up is all.

Maybe adding a line or two in the JS doc to explain what "it won't do" will help here.


/**
* Allow generative AI input/output recording when using the SDK's AI integrations.
*
* These options do not add AI instrumentation on their own. The corresponding integration must still be enabled for
* any data to be captured. Integration-level options, when set, take precedence over these
* values.
* @default { inputs: true, outputs: true }
*/
genAI?: {
Expand All @@ -77,5 +96,6 @@ export interface DataCollection {
*/
export type ResolvedDataCollection = Required<DataCollection> & {
httpHeaders: Required<NonNullable<DataCollection['httpHeaders']>>;
graphQL: Required<NonNullable<DataCollection['graphQL']>>;
genAI: Required<NonNullable<DataCollection['genAI']>>;
};
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export function defaultPiiToCollectionOptions(sendDefaultPii?: boolean): Resolve
httpHeaders: { request: true, response: true },
httpBodies: ['incomingRequest', 'outgoingRequest', 'incomingResponse', 'outgoingResponse'],
queryParams: true,
graphQL: { document: true, variables: true },
genAI: { inputs: true, outputs: true },
stackFrameVariables: true,
frameContextLines: 7, // default should be 5, but ContextLines integration uses 7
Expand All @@ -26,6 +27,9 @@ export function defaultPiiToCollectionOptions(sendDefaultPii?: boolean): Resolve
httpHeaders: { request: { deny: PII_HEADER_SNIPPETS }, response: { deny: PII_HEADER_SNIPPETS } },
httpBodies: [],
queryParams: { deny: PII_HEADER_SNIPPETS },
// The GraphQL document has literal values redacted at collection time, so it was historically
// always attached regardless of `sendDefaultPii`; keep it on to preserve that behavior.
graphQL: { document: true, variables: true },
Comment on lines +30 to +32

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: When sendDefaultPii is false, graphQL.variables is incorrectly set to true. This contradicts the goal of minimizing PII collection and sets up a potential future PII leak.
Severity: HIGH

Suggested Fix

Change the default setting for graphQL.variables to false when sendDefaultPii is false in defaultPiiToCollectionOptions.ts. This aligns the behavior with other PII-sensitive settings and prevents a future potential PII leak. The line should be changed to graphQL: { document: true, variables: false }.

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/defaultPiiToCollectionOptions.ts#L30-L32

Potential issue: The default data collection options set `graphQL.variables` to `true`
even when `sendDefaultPii` is `false`. The purpose of `sendDefaultPii: false` is to
prevent the collection of Personally Identifiable Information (PII). GraphQL variables
can contain sensitive user data, and unlike `graphQL.document`, they are not redacted.
While no code currently consumes the `graphQL.variables` setting, this configuration is
a design error. It creates a high risk that future code implementing GraphQL variable
collection will inadvertently capture and send PII for users who have opted out, as the
default setting will be assumed to be safe.

genAI: { inputs: false, outputs: false },
stackFrameVariables: true,
frameContextLines: 7, // default should be 5, but ContextLines integration uses 7
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const DEFAULTS: ResolvedDataCollection = {
httpHeaders: { request: true, response: true },
httpBodies: ['incomingRequest', 'outgoingRequest', 'incomingResponse', 'outgoingResponse'],
queryParams: true,
graphQL: { document: true, variables: true },
genAI: { inputs: true, outputs: true },
stackFrameVariables: true,
frameContextLines: 5,
Expand Down Expand Up @@ -43,6 +44,10 @@ export function resolveDataCollectionOptions(options: {
},
httpBodies: dc.httpBodies ?? base.httpBodies,
queryParams: dc.queryParams ?? base.queryParams,
graphQL: {
document: dc.graphQL?.document ?? base.graphQL.document,
variables: dc.graphQL?.variables ?? base.graphQL.variables,
},
genAI: {
inputs: dc.genAI?.inputs ?? base.genAI.inputs,
outputs: dc.genAI?.outputs ?? base.genAI.outputs,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export function createMockClient(userInfo = true, genAI?: { inputs: boolean; out
httpHeaders: { request: true, response: true },
httpBodies: [],
queryParams: true,
graphQL: { document: true, variables: true },
genAI: genAIOptions,
stackFrameVariables: true,
frameContextLines: 5,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ describe('defaultPiiToCollectionOptions', () => {
httpHeaders: { request: true, response: true },
httpBodies: ['incomingRequest', 'outgoingRequest', 'incomingResponse', 'outgoingResponse'],
queryParams: true,
graphQL: { document: true, variables: true },
genAI: { inputs: true, outputs: true },
stackFrameVariables: true,
frameContextLines: 7,
Expand All @@ -25,6 +26,7 @@ describe('defaultPiiToCollectionOptions', () => {
},
httpBodies: [],
queryParams: { deny: ['forwarded', '-ip', 'remote-', 'via', '-user'] },
graphQL: { document: true, variables: true },
genAI: { inputs: false, outputs: false },
stackFrameVariables: true,
frameContextLines: 7,
Expand All @@ -41,6 +43,7 @@ describe('defaultPiiToCollectionOptions', () => {
},
httpBodies: [],
queryParams: { deny: ['forwarded', '-ip', 'remote-', 'via', '-user'] },
graphQL: { document: true, variables: true },
genAI: { inputs: false, outputs: false },
stackFrameVariables: true,
frameContextLines: 7,
Expand All @@ -57,6 +60,7 @@ describe('defaultPiiToCollectionOptions', () => {
},
httpBodies: [],
queryParams: { deny: ['forwarded', '-ip', 'remote-', 'via', '-user'] },
graphQL: { document: true, variables: true },
genAI: { inputs: false, outputs: false },
stackFrameVariables: true,
frameContextLines: 7,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ describe('resolveDataCollectionOptions', () => {
httpHeaders: { request: true, response: true },
httpBodies: ['incomingRequest', 'outgoingRequest', 'incomingResponse', 'outgoingResponse'],
queryParams: true,
graphQL: { document: true, variables: true },
genAI: { inputs: true, outputs: true },
stackFrameVariables: true,
frameContextLines: 5,
Expand All @@ -21,6 +22,8 @@ describe('resolveDataCollectionOptions', () => {
expect(result.userInfo).toBe(false);
expect(result.httpBodies).toEqual([]);
expect(result.genAI).toEqual({ inputs: false, outputs: false });
// GraphQL documents are redacted at collection time, so they stay on to preserve legacy behavior.
expect(result.graphQL).toEqual({ document: true, variables: true });
expect(result.stackFrameVariables).toBe(true);
expect(result.frameContextLines).toBe(7);
});
Expand All @@ -45,6 +48,7 @@ describe('resolveDataCollectionOptions', () => {
expect(result.httpHeaders).toEqual({ request: true, response: true });
expect(result.httpBodies).toEqual(['incomingRequest', 'outgoingRequest', 'incomingResponse', 'outgoingResponse']);
expect(result.queryParams).toBe(true);
expect(result.graphQL).toEqual({ document: true, variables: true });
expect(result.genAI).toEqual({ inputs: true, outputs: true });
});

Expand All @@ -54,6 +58,7 @@ describe('resolveDataCollectionOptions', () => {
expect(result.userInfo).toBe(false);
expect(result.httpBodies).toEqual([]);
expect(result.genAI).toEqual({ inputs: false, outputs: false });
expect(result.graphQL).toEqual({ document: true, variables: true });
});
});

Expand Down Expand Up @@ -87,6 +92,7 @@ describe('resolveDataCollectionOptions', () => {
expect(result.cookies).toBe(true);
expect(result.httpHeaders).toEqual({ request: true, response: true });
expect(result.queryParams).toBe(true);
expect(result.graphQL).toEqual({ document: true, variables: true });
expect(result.genAI).toEqual({ inputs: true, outputs: true });
expect(result.stackFrameVariables).toBe(true);
expect(result.frameContextLines).toBe(5);
Expand Down Expand Up @@ -114,6 +120,17 @@ describe('resolveDataCollectionOptions', () => {
expect(result.genAI.outputs).toBe(true);
});

it('merges nested graphQL partially', () => {
const result = resolveDataCollectionOptions({
dataCollection: {
graphQL: { document: false },
},
});

expect(result.graphQL.document).toBe(false);
expect(result.graphQL.variables).toBe(true);
});

it('supports allow/deny list for cookies', () => {
const result = resolveDataCollectionOptions({
dataCollection: {
Expand All @@ -139,14 +156,17 @@ describe('resolveDataCollectionOptions', () => {
it('always returns all fields', () => {
const result = resolveDataCollectionOptions({});

expect(Object.keys(result)).toHaveLength(8);
expect(Object.keys(result)).toHaveLength(9);
expect(result).toHaveProperty('userInfo');
expect(result).toHaveProperty('cookies');
expect(result).toHaveProperty('httpHeaders');
expect(result).toHaveProperty('httpHeaders.request');
expect(result).toHaveProperty('httpHeaders.response');
expect(result).toHaveProperty('httpBodies');
expect(result).toHaveProperty('queryParams');
expect(result).toHaveProperty('graphQL');
expect(result).toHaveProperty('graphQL.document');
expect(result).toHaveProperty('graphQL.variables');
expect(result).toHaveProperty('genAI');
expect(result).toHaveProperty('genAI.inputs');
expect(result).toHaveProperty('genAI.outputs');
Expand Down
1 change: 1 addition & 0 deletions packages/core/test/lib/utils/request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,7 @@ describe('request utils', () => {
httpHeaders: { request: true, response: true },
httpBodies: [],
queryParams: true,
graphQL: { document: true, variables: true },
genAI: { inputs: true, outputs: true },
stackFrameVariables: true,
frameContextLines: 5,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import type {
Token,
} from './graphql-types';
import type { Span, SpanAttributes } from '@sentry/core';
import { isObjectLike, SPAN_STATUS_ERROR, startInactiveSpan, withActiveSpan } from '@sentry/core';
import { getClient, isObjectLike, SPAN_STATUS_ERROR, startInactiveSpan, withActiveSpan } from '@sentry/core';
import { AllowedOperationTypes, SpanNames, TokenKind } from './enum';
import { AttributeNames } from './enums/AttributeNames';
import { OTEL_GRAPHQL_DATA_SYMBOL, OTEL_PATCHED_SYMBOL } from './symbols';
Expand All @@ -38,8 +38,10 @@ export const isPromise = (value: any): value is Promise<unknown> => {
};

export function addSpanSource(span: Span, loc?: Location, start?: number, end?: number): void {
const source = getSourceFromLocation(loc, start, end);
span.setAttribute(AttributeNames.SOURCE, source);
if (getClient()?.getDataCollectionOptions().graphQL.document === true) {
const source = getSourceFromLocation(loc, start, end);
span.setAttribute(AttributeNames.SOURCE, source);
}
}

function createFieldIfNotExists(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import * as core from '@sentry/core';
import { addSpanSource } from '../../../../src/integrations/tracing/graphql/vendored/utils';
import type { Location, Token } from '@sentry/server-utils/orchestrion';

vi.spyOn(core, 'getClient');

function mockClient(graphQLDocument: boolean): void {
vi.mocked(core.getClient).mockReturnValue({
getDataCollectionOptions: () => ({ graphQL: { document: graphQLDocument, variables: true } }),
} as any);
}

function makeLocation(): Location {
const token: Token = {
kind: 'Name',
start: 0,
end: 5,
line: 1,
column: 1,
value: 'hello',
prev: null,
next: null,
};
return { start: 0, end: 5, startToken: token };
}

describe('addSpanSource dataCollection gate', () => {
beforeEach(() => {
vi.clearAllMocks();
});

it('sets graphql.source attribute when graphQL.document is true', () => {
mockClient(true);
const span = { setAttribute: vi.fn() } as any;
addSpanSource(span, makeLocation());
expect(span.setAttribute).toHaveBeenCalledWith('graphql.source', expect.any(String));
});

it('does not set attribute when graphQL.document is false', () => {
mockClient(false);
const span = { setAttribute: vi.fn() } as any;
addSpanSource(span, makeLocation());
expect(span.setAttribute).not.toHaveBeenCalled();
});

it('does not set attribute when getClient() is undefined', () => {
vi.mocked(core.getClient).mockReturnValue(undefined);
const span = { setAttribute: vi.fn() } as any;
addSpanSource(span, makeLocation());
expect(span.setAttribute).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ describe('wrapMiddlewareHandlerWithSentry', () => {
httpHeaders: { request: true, response: true },
httpBodies: [],
queryParams: true,
graphQL: { document: true, variables: true },
genAI: { inputs: false, outputs: false },
stackFrameVariables: true,
frameContextLines: 5,
Expand Down
1 change: 1 addition & 0 deletions packages/remix/test/server/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ function createMockClient(httpBodies: string[] = []): Client {
httpHeaders: { request: true, response: true },
httpBodies,
queryParams: true,
graphQL: { document: true, variables: true },
genAI: { inputs: true, outputs: true },
stackFrameVariables: true,
frameContextLines: 5,
Expand Down
Loading
Loading