Skip to content

ref: Allow to extract user IP address without req.user present #2467

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.

Sign up for GitHub

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 1 commit into from
Mar 4, 2020
Merged

ref: Allow to extract user IP address without req.user present #2467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
63 changes: 37 additions & 26 deletions packages/node/src/handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,42 +174,28 @@ const DEFAULT_USER_KEYS = ['id', 'username', 'email'];

/** JSDoc */
function extractUserData(
req: {
ip?: string;
connection?: {
remoteAddress?: string;
};
user?: {
[key: string]: any;
};
user: {
[key: string]: any;
},
keys: boolean | string[],
): { [key: string]: any } {
const user: { [key: string]: any } = {};
const extractedUser: { [key: string]: any } = {};
const attributes = Array.isArray(keys) ? keys : DEFAULT_USER_KEYS;

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

// client ip:
// node: req.connection.remoteAddress
// express, koa: req.ip
const ip = req.ip || (req.connection && req.connection.remoteAddress);

if (ip) {
user.ip_address = ip;
}

return user;
return extractedUser;
}

/**
* Options deciding what parts of the request to use when enhancing an event
*/
interface ParseRequestOptions {
ip?: boolean;
request?: boolean | string[];
serverName?: boolean;
transaction?: boolean | TransactionTypes;
Expand All @@ -229,11 +215,19 @@ export function parseRequest(
event: Event,
req: {
[key: string]: any;
user?: {
[key: string]: any;
};
ip?: string;
connection?: {
remoteAddress?: string;
};
},
options?: ParseRequestOptions,
): Event {
// tslint:disable-next-line:no-parameter-reassignment
options = {
ip: false,
request: true,
serverName: true,
transaction: true,
Expand All @@ -260,11 +254,28 @@ export function parseRequest(
event.server_name = global.process.env.SENTRY_NAME || os.hostname();
}

if (options.user && req.user) {
event.user = {
...event.user,
...extractUserData(req, options.user),
};
if (options.user) {
const extractedUser = req.user ? extractUserData(req.user, options.user) : {};

if (Object.keys(extractedUser)) {
event.user = {
...event.user,
...extractedUser,
};
Comment on lines +261 to +264
Copy link
Member

Choose a reason for hiding this comment

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

By doing them in this order, is it possible we'd be overwriting user data added using withScope with the extracted data?

Copy link
Contributor Author

@kamilogorek kamilogorek Mar 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but this was the previous behavior and I didn't want to change it here.

Copy link
Member

Choose a reason for hiding this comment

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

I ask because it just came up as a bug on the Python side (see getsentry/sentry-python#637). Even if we don't do it here (if we want to consider it a breaking change), might be worth thinking about as a way to prevent future user confusion/frustration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Let's get this in first, so it allows for extraction at all. It won't do any more harm in this form anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Member

Choose a reason for hiding this comment

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

Created an issue so we don't forget :-)

#2472

}
}

// client ip:
// node: req.connection.remoteAddress
// express, koa: req.ip
if (options.ip) {
const ip = req.ip || (req.connection && req.connection.remoteAddress);
if (ip) {
event.user = {
...event.user,
ip_address: ip,
};
}
}

if (options.transaction && !event.transaction) {
Expand Down
80 changes: 47 additions & 33 deletions packages/node/test/handlers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,52 +22,66 @@ describe('parseRequest', () => {
const DEFAULT_USER_KEYS = ['id', 'username', 'email'];
const CUSTOM_USER_KEYS = ['custom_property'];

test('parseRequest.user only contains the default properties from the user', done => {
const fakeEvent: Event = {};
const parsedRequest: Event = parseRequest(fakeEvent, mockReq);
const userKeys = Object.keys(parsedRequest.user);

expect(userKeys).toEqual(DEFAULT_USER_KEYS);
expect(userKeys).not.toEqual(expect.arrayContaining(CUSTOM_USER_KEYS));
done();
test('parseRequest.user only contains the default properties from the user', () => {
const parsedRequest: Event = parseRequest({}, mockReq, {
user: DEFAULT_USER_KEYS,
});
expect(Object.keys(parsedRequest.user as any[])).toEqual(DEFAULT_USER_KEYS);
});

test('parseRequest.user only contains the custom properties specified in the options.user array', done => {
const options = {
test('parseRequest.user only contains the custom properties specified in the options.user array', () => {
const parsedRequest: Event = parseRequest({}, mockReq, {
user: CUSTOM_USER_KEYS,
};
const fakeEvent: Event = {};
const parsedRequest: Event = parseRequest(fakeEvent, mockReq, options);
const userKeys = Object.keys(parsedRequest.user);
});
expect(Object.keys(parsedRequest.user as any[])).toEqual(CUSTOM_USER_KEYS);
});
});

expect(userKeys).toEqual(CUSTOM_USER_KEYS);
expect(userKeys).not.toEqual(expect.arrayContaining(DEFAULT_USER_KEYS));
done();
describe('parseRequest.ip property', () => {
test('can be extracted from req.ip', () => {
const parsedRequest: Event = parseRequest(
{},
{
...mockReq,
ip: '123',
},
{
ip: true,
},
);
expect(parsedRequest.user!.ip_address).toEqual('123');
});

test('can extract from req.connection.remoteAddress', () => {
const parsedRequest: Event = parseRequest(
{},
{
...mockReq,
connection: {
remoteAddress: '321',
},
},
{
ip: true,
},
);
expect(parsedRequest.user!.ip_address).toEqual('321');
});
});

describe('parseRequest.request properties', () => {
test('parseRequest.request only contains the default set of properties from the request', done => {
test('parseRequest.request only contains the default set of properties from the request', () => {
const DEFAULT_REQUEST_PROPERTIES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url'];
const fakeEvent: Event = {};
const parsedRequest: Event = parseRequest(fakeEvent, mockReq, {});
expect(Object.keys(parsedRequest.request)).toEqual(DEFAULT_REQUEST_PROPERTIES);
done();
const parsedRequest: Event = parseRequest({}, mockReq, {});
expect(Object.keys(parsedRequest.request as any[])).toEqual(DEFAULT_REQUEST_PROPERTIES);
});

test('parseRequest.request only contains the specified properties in the options.request array', done => {
const EXCLUDED_PROPERTIES = ['cookies', 'method'];
test('parseRequest.request only contains the specified properties in the options.request array', () => {
const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url'];
const options = {
const parsedRequest: Event = parseRequest({}, mockReq, {
request: INCLUDED_PROPERTIES,
};
const fakeEvent: Event = {};
const parsedRequest: Event = parseRequest(fakeEvent, mockReq, options);
const requestKeys = Object.keys(parsedRequest.request);

expect(requestKeys).toEqual(INCLUDED_PROPERTIES);
expect(requestKeys).not.toEqual(expect.arrayContaining(EXCLUDED_PROPERTIES));
done();
});
expect(Object.keys(parsedRequest.request as any[])).toEqual(['data', 'headers', 'query_string', 'url']);
});
});
});