Skip to content

fix(pii): Add more conditionals around sending data #16143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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/browser-utils/src/networkUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Member Author

Choose a reason for hiding this comment

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

For this, we'll just need to add maxRequestBodySize

Copy link
Member

Choose a reason for hiding this comment

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

I think this is only used in conjunction with Replay and the feature being enabled but I might be missing a case

try {
if (typeof body === 'string') {
return [body];
Expand Down
2 changes: 1 addition & 1 deletion packages/browser/src/integrations/httpcontext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const httpContextIntegration = defineIntegration(() => {
const { userAgent } = WINDOW.navigator || {};

const headers = {
...event.request?.headers,
...event.request?.headers, // fixme: add conditional for `sendDefaultPii` here?
...(referrer && { Referer: referrer }),
...(userAgent && { 'User-Agent': userAgent }),
};
Expand Down
4 changes: 2 additions & 2 deletions packages/bun/src/integrations/bunserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ function wrapRequestHandler<T extends RouteHandler = RouteHandler>(
normalizedRequest: {
url: request.url,
method: request.method,
headers: request.headers.toJSON(),
headers: request.headers.toJSON(), // fixme: headers are passed 1:1
query_string: parsedUrl?.search,
} satisfies RequestEventData,
});
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

status_code: response.status,
});
}
Expand Down
1 change: 1 addition & 0 deletions packages/cloudflare/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export function getDefaultIntegrations(options: CloudflareOptions): Integration[
functionToStringIntegration(),
linkedErrorsIntegration(),
fetchIntegration(),
// todo: the `include` object should be defined based on `sendDefaultPii` directly in the integration
requestDataIntegration(sendDefaultPii ? undefined : { include: { cookies: false } }),
consoleIntegration(),
];
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ function endSpan(span: Span, handlerData: HandlerDataFetch): void {
if (handlerData.response) {
setHttpStatus(span, handlerData.response.status);

// fixme: only send if `sendDefaultPii`?
Copy link
Member Author

Choose a reason for hiding this comment

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

I think content length is something we could send

Copy link
Member

Choose a reason for hiding this comment

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

you mean w/o sendDefaultPii === true? agreed!

const contentLength = handlerData.response?.headers && handlerData.response.headers.get('content-length');

if (contentLength) {
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/integrations/requestdata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) =

const includeWithDefaultPiiApplied: RequestDataIncludeOptions = {
...include,
// todo: also exclude cookies and headers if sendDefaultPii is false
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually something we should do - the question is if we do this in a minor or major?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the Http Client integration for browser, we only include the cookie data if this is explicitly enabled:

if (_shouldSendDefaultPii()) {
try {
const cookieString = xhr.getResponseHeader('Set-Cookie') || xhr.getResponseHeader('set-cookie') || undefined;
if (cookieString) {
responseCookies = _parseCookieString(cookieString);
}

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, we should do it but my gut feeling is to do it in a major (see review comment)

ip: include.ip ?? client.getOptions().sendDefaultPii,
};

Expand Down Expand Up @@ -74,6 +75,7 @@ function addNormalizedRequestDataToEvent(
additionalData: { ipAddress?: string },
include: RequestDataIncludeOptions,
): void {
// fixme: only add when `sendDefaultPii` is enabled?
event.request = {
...event.request,
...extractNormalizedRequestData(req, include),
Expand Down Expand Up @@ -121,6 +123,7 @@ function extractNormalizedRequestData(
}

if (include.cookies) {
// fixme: cookies enabled by default?
const cookies = normalizedRequest.cookies || (headers?.cookie ? parseCookie(headers.cookie) : undefined);
requestData.cookies = cookies || {};
}
Expand Down
1 change: 1 addition & 0 deletions packages/node/src/integrations/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const _nodeContextIntegration = ((options: ContextOptions = {}) => {

const updatedContext = _updateContext(await cachedContext);

// fixme: conditional with `sendDefaultPii` here?
event.contexts = {
...event.contexts,
app: { ...updatedContext.app, ...event.contexts?.app },
Expand Down
2 changes: 2 additions & 0 deletions packages/replay-internal/src/coreHandlers/util/fetchUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,11 @@ export function enrichFetchBreadcrumb(
): void {
const { input, response } = hint;

// fixme: only add this if `sendDefaultPii` is enabled?
const body = input ? getFetchRequestArgBody(input) : undefined;
const reqSize = getBodySize(body);

// fixme: only send response body size if `sendDefaultPii` is enabled?
const resSize = response ? parseContentLengthHeader(response.headers.get('content-length')) : undefined;

if (reqSize !== undefined) {
Expand Down
1 change: 1 addition & 0 deletions packages/vercel-edge/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export function getDefaultIntegrations(options: Options): Integration[] {
linkedErrorsIntegration(),
winterCGFetchIntegration(),
consoleIntegration(),
// fixme: integration can be included - but integration should not add IP address etc
...(options.sendDefaultPii ? [requestDataIntegration()] : []),
Copy link
Member

@Lms24 Lms24 Apr 29, 2025

Choose a reason for hiding this comment

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

yeah I agree that we should handle this within the integration instead of the top-level decision here.

];
}
Expand Down
Loading