Navigation Menu

Skip to content
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

fix(node): Remove Cookie header from event if cookies should not be sent to Sentry #5898

Merged
merged 1 commit into from Oct 6, 2022

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Oct 6, 2022

When using the Sentry.requestMiddleware in Node (Express), users have the option to define which properties of the incoming requests should be added to the event that is sent to Sentry. Among other options, they can set

  • headers to add the request's http headers to event.request.headers
  • cookies to add the request's cookies to event.request.cookies

However, there is a problem if cookies is not specified but headers is: The Cookie http header of the incoming request is still added to event.request.headers as it is treated like a normal http header. Therefore, cookie data (potentially containing sensitive data) is added to the event although users expect it to not be added.

This PR fixes this behaviour by removing the Cookie http header from the collected headers, if cookies is not specified in the requestHandler options but headers is. Additionally, it adds two tests to check this new behaviour.

The PR applies to both, the deprecated and new options structure.

fixes #5458

@Lms24 Lms24 marked this pull request as ready for review October 6, 2022 10:25
@Lms24 Lms24 changed the title fix(node): Remove Cookie header from event.request.headers if cookies should not be sent to Sentry fix(node): Remove Cookie header from event if cookies should not be sent to Sentry Oct 6, 2022
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Good catch!

@Lms24 Lms24 merged commit 458374a into master Oct 6, 2022
@Lms24 Lms24 deleted the lms-fix-node-requestHandler-cookies branch October 6, 2022 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookies are being sent in an error-report by default in Express app
2 participants