fix(pii): Add more conditionals around sending data #16143
Conversation
packages/core/src/fetch.ts
Outdated
| @@ -205,6 +205,7 @@ function endSpan(span: Span, handlerData: HandlerDataFetch): void { | |||
| if (handlerData.response) { | |||
| setHttpStatus(span, handlerData.response.status); | |||
|
|
|||
| // fixme: only send if `sendDefaultPii`? | |||
There was a problem hiding this comment.
I think content length is something we could send
There was a problem hiding this comment.
you mean w/o sendDefaultPii === true? agreed!
| @@ -45,6 +45,7 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = | |||
|
|
|||
| const includeWithDefaultPiiApplied: RequestDataIncludeOptions = { | |||
| ...include, | |||
| // todo: also exclude cookies and headers if sendDefaultPii is false | |||
There was a problem hiding this comment.
This is actually something we should do - the question is if we do this in a minor or major?
There was a problem hiding this comment.
In the Http Client integration for browser, we only include the cookie data if this is explicitly enabled:
sentry-javascript/packages/browser/src/integrations/httpclient.ts
Lines 140 to 146 in 63476fa
There was a problem hiding this comment.
Agreed, we should do it but my gut feeling is to do it in a major (see review comment)
| @@ -17,6 +17,7 @@ export function serializeFormData(formData: FormData): string { | |||
|
|
|||
| /** Get the string representation of a body. */ | |||
| export function getBodyString(body: unknown, _logger: Logger = logger): [string | undefined, NetworkMetaWarning?] { | |||
| // fixme: only add body string if `sendDefaultPii` is enabled? | |||
There was a problem hiding this comment.
For this, we'll just need to add maxRequestBodySize
There was a problem hiding this comment.
I think this is only used in conjunction with Replay and the feature being enabled but I might be missing a case
| @@ -232,7 +232,7 @@ function wrapRequestHandler<T extends RouteHandler = RouteHandler>( | |||
| if (response?.status) { | |||
| setHttpStatus(span, response.status); | |||
| isolationScope.setContext('response', { | |||
| headers: response.headers.toJSON(), | |||
| headers: response.headers.toJSON(), // fixme: headers are passed 1:1 | |||
There was a problem hiding this comment.
For all the header-related data, we need to decide on whether we want to keep sending them all, filter them by excluding e.g. authorization headers or not send them all when no PII should be sent.
There was a problem hiding this comment.
I think the data you identified should all be gated by the option (see comments). But to me it seems like we can only make this changes when we ship the next major, just so that users are more aware of the changes and they might need to set this flag.
Therefore, my overall suggestion would be to replace fixme with TODO(v10), merge this and ensure we track the change for v10.
If folks have other opinions that's fine, too. But let's make sure we avoid surprising users with data we no longer send :)
packages/core/src/fetch.ts
Outdated
| @@ -205,6 +205,7 @@ function endSpan(span: Span, handlerData: HandlerDataFetch): void { | |||
| if (handlerData.response) { | |||
| setHttpStatus(span, handlerData.response.status); | |||
|
|
|||
| // fixme: only send if `sendDefaultPii`? | |||
There was a problem hiding this comment.
you mean w/o sendDefaultPii === true? agreed!
packages/vercel-edge/src/sdk.ts
Outdated
| // fixme: integration can be included - but integration should not add IP address etc | ||
| ...(options.sendDefaultPii ? [requestDataIntegration()] : []), |
There was a problem hiding this comment.
yeah I agree that we should handle this within the integration instead of the top-level decision here.
Based on the docs work for the issue getsentry/sentry-docs#13407, I went through the repository and looked for places where a conditional check for
sendDefaultPiimight be missing.