-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add cookies as an optional property in the request handler #2167
Add cookies as an optional property in the request handler #2167
Conversation
Thanks for the PR! As sentry-javascript/packages/node/src/handlers.ts Lines 118 to 147 in a952720
|
- Revert cookie configuration option. - Add optional array to request argument - Filter any keys from the request interface that don't match the array
@@ -112,6 +112,17 @@ function extractRequestData(req: { [key: string]: any }): { [key: string]: strin | |||
url: absoluteUrl, | |||
}; | |||
|
|||
const attributes = Array.isArray(keys) ? keys : []; | |||
|
|||
if (attributes.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using slightly different logic here than the extractUserData
function, as you are already running some custom logic to set/manipulate keys on the request interface.
In this PR we build the request interface as before then remove any of the properties not specified in the optional array (if it exists).
This behaviour is slightly different to the extractUserData
fn as it allows users to request any of the properties from the request interface rather than any properties of the req
argument itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's a bit too ambiguous we could do something like below which would allow users to extract any property on the req
object:
- Set the requested properties
- Run the rest of the current logic to overwrite the values
- Merge the requested properties with the default values
- Delete any keys that don't match the requested properties from the array.
function extractRequestData(...) {
// 1.
const request: { [key: string]: string } = {};
const attributes = Array.isArray(keys) ? keys : [];
attributes.forEach(key => {
if ({}.hasOwnProperty.call(req, key)) {
request[key] = (req as { [key: string]: string })[key];
}
});
----
// 2.
// set values
const headers = ...
const method = ...
----
// 3.
// request interface
const requestInterface: {
[key: string]: any;
} = {
...request
cookies,
data,
headers,
method,
query_string: query,
url: absoluteUrl,
};
----
// 4.
// Delete any of the default keys not specified in the array
if (attributes.length) {
Object.keys(requestInterface).forEach(key => {
/** Remove any of the unspecified keys in the options from the request interface */
if (!attributes.includes(key)) {
delete requestInterface[key];
}
});
}
return requestInterface
}
}, | ||
}; | ||
|
||
describe('parseRequest.user properties', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought i'd add a bit of test coverage for some of this logic for both the user
and request
properties
Everything looks great (and thanks for the tests) except one thing, which is We can make use of your second idea, with a slight twist by using function extractRequestData(req, keys) {
const request = {};
(Array.isArray(keys) ? keys : DEFAULT_KEYS).forEach(key => {
switch (key) {
case "headers":
request.headers = req.headers || req.header || {};
break;
case "protocol":
request.protocol =
req.protocol === "https" || req.secure || (req.socket || {}).encrypted
? "https"
: "http";
break;
case "host":
request.host = req.hostname || req.host || headers.host || "<no host>";
break;
default:
if ({}.hasOwnProperty.call(req, key)) {
request[key] = req[key];
}
}
});
return request;
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above.
Reworked it just a bit and merged manually to make it into 5.6.0 release. Your work and tests has been preserved :) Thanks for the PR and all the changes! https://github.com/getsentry/sentry-javascript/commits/master |
Legend, thanks for that 👍 |
Corresponding docs PR: getsentry/sentry-docs#1145 |
We are using the
requestHandler
in our express application but we don't want cookies to be added to all of our errors as some cookies contain personal data.We have used the settings in the Sentry UI to set
cookies
as a sensitive field and we also have abeforeSend
block that deletes cookies from the error event.Neither of these have prevented the cookies being added to our errors.
This PR allows users to configure if they want cookies to be added to the request data. It feels like a reasonable configuration option to provide given the likelihood of cookies containing some personal data.
yarn lint
) & (yarn test
).