Skip to content

feat(core)!: Stop setting user in requestDataIntegration #14898

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

Merged
merged 2 commits into from
Jan 7, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,21 @@ describe('express user handling', () => {
cleanupChildProcesses();
});

test('picks user from request', done => {
test('ignores user from request', done => {
expect.assertions(2);

createRunner(__dirname, 'server.js')
.expect({
event: {
user: {
id: '1',
email: 'test@sentry.io',
},
exception: {
values: [
{
value: 'error_1',
},
],
},
event: event => {
expect(event.user).toBeUndefined();
expect(event.exception?.values?.[0]?.value).toBe('error_1');
},
})
.start(done)
.makeRequest('get', '/test1', { expectError: true });
});

test('setUser overwrites user from request', done => {
test('using setUser in middleware works', done => {
createRunner(__dirname, 'server.js')
.expect({
event: {
Expand Down
4 changes: 4 additions & 0 deletions docs/migration/v8-to-v9.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ In v9, an `undefined` value will be treated the same as if the value is not defi

- When `skipOpenTelemetrySetup: true` is configured, `httpIntegration({ spans: false })` will be configured by default. This means that you no longer have to specify this yourself in this scenario. With this change, no spans are emitted once `skipOpenTelemetrySetup: true` is configured, without any further configuration being needed.

- The `requestDataIntegration` will no longer automatically set the user from `request.user`. This is an express-specific, undocumented behavior, and also conflicts with our privacy-by-default strategy. Starting in v9, you'll need to manually call `Sentry.setUser()` e.g. in a middleware to set the user on Sentry events.

### `@sentry/browser`

- The `captureUserFeedback` method has been removed. Use `captureFeedback` instead and update the `comments` field to `message`.
Expand Down Expand Up @@ -128,6 +130,8 @@ Sentry.init({
});
```

- The `DEFAULT_USER_INCLUDES` constant has been removed.

### `@sentry/react`

- The `wrapUseRoutes` method has been removed. Use `wrapUseRoutesV6` or `wrapUseRoutesV7` instead depending on what version of react router you are using.
Expand Down
1 change: 0 additions & 1 deletion packages/astro/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ export {
cron,
dataloaderIntegration,
dedupeIntegration,
DEFAULT_USER_INCLUDES,
defaultStackParser,
endSession,
expressErrorHandler,
Expand Down
1 change: 0 additions & 1 deletion packages/aws-serverless/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export {
flush,
close,
getSentryRelease,
DEFAULT_USER_INCLUDES,
createGetModuleFromFilename,
anrIntegration,
disableAnrDetectionForCallback,
Expand Down
1 change: 0 additions & 1 deletion packages/bun/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ export {
flush,
close,
getSentryRelease,
DEFAULT_USER_INCLUDES,
createGetModuleFromFilename,
anrIntegration,
disableAnrDetectionForCallback,
Expand Down
46 changes: 3 additions & 43 deletions packages/core/src/integrations/requestdata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,6 @@ export type RequestDataIntegrationOptions = {
ip?: boolean;
query_string?: boolean;
url?: boolean;
user?:
| boolean
| {
id?: boolean;
username?: boolean;
email?: boolean;
};
};
};

Expand All @@ -31,11 +24,6 @@ const DEFAULT_OPTIONS = {
ip: false,
query_string: true,
url: true,
user: {
id: true,
username: true,
email: true,
},
},
transactionNamingScheme: 'methodPath' as const,
};
Expand All @@ -49,14 +37,6 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) =
include: {
...DEFAULT_OPTIONS.include,
...options.include,
user:
options.include && typeof options.include.user === 'boolean'
? options.include.user
: {
...DEFAULT_OPTIONS.include.user,
// Unclear why TS still thinks `options.include.user` could be a boolean at this point
...((options.include || {}).user as Record<string, boolean>),
},
},
};

Expand All @@ -69,16 +49,12 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) =
// that's happened, it will be easier to add this logic in without worrying about unexpected side effects.)

const { sdkProcessingMetadata = {} } = event;
const { request, normalizedRequest, ipAddress } = sdkProcessingMetadata;
const { normalizedRequest, ipAddress } = sdkProcessingMetadata;

const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options);

// If this is set, it takes precedence over the plain request object
if (normalizedRequest) {
// Some other data is not available in standard HTTP requests, but can sometimes be augmented by e.g. Express or Next.js
const user = request ? request.user : undefined;

addNormalizedRequestDataToEvent(event, normalizedRequest, { ipAddress, user }, addRequestDataOptions);
addNormalizedRequestDataToEvent(event, normalizedRequest, { ipAddress }, addRequestDataOptions);
return event;
}

Expand All @@ -99,7 +75,7 @@ function convertReqDataIntegrationOptsToAddReqDataOpts(
integrationOptions: Required<RequestDataIntegrationOptions>,
): AddRequestDataToEventOptions {
const {
include: { ip, user, ...requestOptions },
include: { ip, ...requestOptions },
} = integrationOptions;

const requestIncludeKeys: string[] = ['method'];
Expand All @@ -109,25 +85,9 @@ function convertReqDataIntegrationOptsToAddReqDataOpts(
}
}

let addReqDataUserOpt;
if (user === undefined) {
addReqDataUserOpt = true;
} else if (typeof user === 'boolean') {
addReqDataUserOpt = user;
} else {
const userIncludeKeys: string[] = [];
for (const [key, value] of Object.entries(user)) {
if (value) {
userIncludeKeys.push(key);
}
}
addReqDataUserOpt = userIncludeKeys;
}

return {
include: {
ip,
user: addReqDataUserOpt,
request: requestIncludeKeys.length !== 0 ? requestIncludeKeys : undefined,
},
};
Expand Down
1 change: 0 additions & 1 deletion packages/core/src/utils-hoist/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ export type { PromiseBuffer } from './promisebuffer';

// TODO: Remove requestdata export once equivalent integration is used everywhere
export {
DEFAULT_USER_INCLUDES,
addNormalizedRequestDataToEvent,
winterCGHeadersToDict,
winterCGRequestToRequestData,
Expand Down
38 changes: 1 addition & 37 deletions packages/core/src/utils-hoist/requestdata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,15 @@ import type { Event, PolymorphicRequest, RequestEventData, WebFetchHeaders, WebF

import { parseCookie } from './cookie';
import { DEBUG_BUILD } from './debug-build';
import { isPlainObject } from './is';
import { logger } from './logger';
import { dropUndefinedKeys } from './object';
import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress';

const DEFAULT_INCLUDES = {
ip: false,
request: true,
user: true,
};
const DEFAULT_REQUEST_INCLUDES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url'];
export const DEFAULT_USER_INCLUDES = ['id', 'username', 'email'];

/**
* Options deciding what parts of the request to use when enhancing an event
Expand All @@ -23,7 +20,6 @@ export type AddRequestDataToEventOptions = {
include?: {
ip?: boolean;
request?: boolean | Array<(typeof DEFAULT_REQUEST_INCLUDES)[number]>;
user?: boolean | Array<(typeof DEFAULT_USER_INCLUDES)[number]>;
};

/** Injected platform-specific dependencies */
Expand All @@ -39,24 +35,6 @@ export type AddRequestDataToEventOptions = {
};
};

function extractUserData(
user: {
[key: string]: unknown;
},
keys: boolean | string[],
): { [key: string]: unknown } {
const extractedUser: { [key: string]: unknown } = {};
const attributes = Array.isArray(keys) ? keys : DEFAULT_USER_INCLUDES;

attributes.forEach(key => {
if (user && key in user) {
extractedUser[key] = user[key];
}
});

return extractedUser;
}

/**
* Add already normalized request data to an event.
* This mutates the passed in event.
Expand All @@ -65,7 +43,7 @@ export function addNormalizedRequestDataToEvent(
event: Event,
req: RequestEventData,
// This is non-standard data that is not part of the regular HTTP request
additionalData: { ipAddress?: string; user?: Record<string, unknown> },
additionalData: { ipAddress?: string },
options: AddRequestDataToEventOptions,
): void {
const include = {
Expand All @@ -87,20 +65,6 @@ export function addNormalizedRequestDataToEvent(
};
}

if (include.user) {
const extractedUser =
additionalData.user && isPlainObject(additionalData.user)
? extractUserData(additionalData.user, include.user)
: {};

if (Object.keys(extractedUser).length) {
event.user = {
...extractedUser,
...event.user,
};
}
}

if (include.ip) {
const ip = (req.headers && getClientIPAddress(req.headers)) || additionalData.ipAddress;
if (ip) {
Expand Down
17 changes: 2 additions & 15 deletions packages/core/test/lib/integrations/requestdata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ describe('`RequestData` integration', () => {

describe('option conversion', () => {
it('leaves `ip` and `user` at top level of `include`', () => {
const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ include: { ip: false, user: true } });
const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ include: { ip: false } });

void requestDataEventProcessor(event, {});
expect(addNormalizedRequestDataToEventSpy).toHaveBeenCalled();
const passedOptions = addNormalizedRequestDataToEventSpy.mock.calls[0]?.[3];

expect(passedOptions?.include).toEqual(expect.objectContaining({ ip: false, user: true }));
expect(passedOptions?.include).toEqual(expect.objectContaining({ ip: false }));
});

it('moves `true` request keys into `request` include, but omits `false` ones', async () => {
Expand All @@ -80,18 +80,5 @@ describe('`RequestData` integration', () => {
expect(passedOptions?.include?.request).toEqual(expect.arrayContaining(['data']));
expect(passedOptions?.include?.request).not.toEqual(expect.arrayContaining(['cookies']));
});

it('moves `true` user keys into `user` include, but omits `false` ones', async () => {
const requestDataEventProcessor = initWithRequestDataIntegrationOptions({
include: { user: { id: true, email: false } },
});

void requestDataEventProcessor(event, {});

const passedOptions = addNormalizedRequestDataToEventSpy.mock.calls[0]?.[3];

expect(passedOptions?.include?.user).toEqual(expect.arrayContaining(['id']));
expect(passedOptions?.include?.user).not.toEqual(expect.arrayContaining(['email']));
});
});
});
1 change: 0 additions & 1 deletion packages/google-cloud-serverless/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export {
flush,
close,
getSentryRelease,
DEFAULT_USER_INCLUDES,
createGetModuleFromFilename,
anrIntegration,
disableAnrDetectionForCallback,
Expand Down
2 changes: 0 additions & 2 deletions packages/node/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ export { cron } from './cron';

export type { NodeOptions } from './types';

export { DEFAULT_USER_INCLUDES } from '@sentry/core';

export {
// This needs exporting so the NodeClient can be used without calling init
setOpenTelemetryContextAsyncContextStrategy as setNodeAsyncContextStrategy,
Expand Down
1 change: 0 additions & 1 deletion packages/remix/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ export {
createTransport,
cron,
dedupeIntegration,
DEFAULT_USER_INCLUDES,
defaultStackParser,
endSession,
expressErrorHandler,
Expand Down
1 change: 0 additions & 1 deletion packages/solidstart/src/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export {
createTransport,
cron,
dedupeIntegration,
DEFAULT_USER_INCLUDES,
defaultStackParser,
endSession,
expressErrorHandler,
Expand Down
1 change: 0 additions & 1 deletion packages/sveltekit/src/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export {
createTransport,
cron,
dedupeIntegration,
DEFAULT_USER_INCLUDES,
defaultStackParser,
endSession,
expressErrorHandler,
Expand Down
Loading