feat(core): Add and use dataCollection.graphQL#22221
Conversation
s1gr1d
requested review from
Lms24,
andreiborza,
chargome,
isaacs,
logaretm,
mydea and
nicohrubec
and removed request for
a team
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| graphQL?: { | ||
| document?: boolean; | ||
| variables?: boolean; | ||
| }; |
There was a problem hiding this comment.
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.
size-limit report 📦
|
| graphQL?: { | ||
| document?: boolean; | ||
| variables?: boolean; | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // 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 }, |
There was a problem hiding this comment.
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.
2 participants
Sets the default collection behavior to
trueas it has been collected already.