ref(core): Deprecate sendDefaultPii in favor or dataCollection#21277
Conversation
s1gr1d
force-pushed
the
sig/deprecate-sendDefaultPii
branch
from
f7c312e to
1d8958c
Compare
s1gr1d
requested review from
a team,
JPeer264,
chargome,
logaretm,
mydea and
nicohrubec
and removed request for
a team and
logaretm
|
@BugBot review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 707e738. Configure here.
| genAI: { inputs: false, outputs: false }, | ||
| httpHeaders: { deny: ['forwarded', '-ip', 'remote-', 'via', '-user'] }, | ||
| cookies: { deny: ['forwarded', '-ip', 'remote-', 'via', '-user'] }, | ||
| queryParams: { deny: ['forwarded', '-ip', 'remote-', 'via', '-user'] }, |
There was a problem hiding this comment.
super-l: Should we use different deny examples for each entry?
For cookies we could use csrf or token, and for queryParams there would be state for OAuth requests.
There was a problem hiding this comment.
csrf and token are sensitive and will be filtered always: https://github.com/getsentry/sentry-javascript/blob/develop/packages/core/src/utils/data-collection/filtering-snippets.ts|
We can also include state in there.
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Linting fails, but once this is through it is a LGTM
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Nice! Let's wait until we docs in place for this before merging 👍
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| consoleIntegration(), | ||
| // TODO(v11): integration can be included - but integration should not add IP address etc | ||
| ...(options.sendDefaultPii ? [requestDataIntegration()] : []), | ||
| requestDataIntegration(), | ||
| ]; |
There was a problem hiding this comment.
Bug: The requestDataIntegration() is now always included, causing unintended collection of request headers, cookies, and query parameters for users who haven't opted into PII collection.
Severity: HIGH
Suggested Fix
Reinstate a guard around the requestDataIntegration() inclusion. The integration should only be added if the user has explicitly opted into data collection, for example by checking sendDefaultPii or a more modern data collection option. This will prevent the default collection of sensitive request data for users who have not consented.
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/vercel-edge/src/sdk.ts#L59-L61
Potential issue: The removal of the `sendDefaultPii` guard around
`requestDataIntegration()` in the `vercel-edge` SDK causes it to be unconditionally
included. By default, this integration collects and sends request headers, cookies, and
query parameters, even when `sendDefaultPii` is not explicitly enabled. This is a change
in behavior for existing users who previously did not have this data collected, leading
to an unintended privacy regression where potentially sensitive information is sent to
Sentry without explicit opt-in.
Did we get this right? 👍 / 👎 to inform future reviews.
3 participants
Deprecates
sendDefaultPii.Some
sendDefaultPiioccurences are still there as we still test against the deprecated behavior. This will be removed in v11.Closes #20937