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

Cookies are being sent in an error-report by default in Express app #5458

Closed
3 tasks done
Tracked by #5510
ozerovs opened this issue Jul 25, 2022 · 5 comments · Fixed by #5898
Closed
3 tasks done
Tracked by #5510

Cookies are being sent in an error-report by default in Express app #5458

ozerovs opened this issue Jul 25, 2022 · 5 comments · Fixed by #5898

Comments

@ozerovs
Copy link

ozerovs commented Jul 25, 2022

Is there an existing issue for this?

How do you use Sentry?

Self-hosted/on-premise

Which package are you using?

@sentry/node

SDK Version

7.7.0

Framework Version

7.7.0

Link to Sentry event

No response

Steps to Reproduce

Using @sentry/node, Sentry.Handlers.requestHandler does not clean up cookie sent in the headers, from express-based app by default.

We are using a React-app with express-server dealing with SSR.
In case of SSR-errors, @sentry/node is being used for handling errors.
According to documentation, passing this options object: { request: ['headers', 'method', 'query_string', 'url'] } in Sentry.Handlers.requestHandler, will NOT include cookies into the payload with an error-event sent to Sentry.
In our case it does and Sentry parses it as if we would actually pass the 'cookies' key into the options.request array.

Here is code to reproduce this in a basic express-app:

const Sentry = require('@sentry/node');
const app = require('express')();
const port = 3000;

Sentry.init({
  dsn: DSN_KEY,
  autoSessionTracking: false,
  release: RELEASE,
  beforeSend(event) {
    console.log(event); // Will have cookie prop in the headers. 
  },
});

app.use(Sentry.Handlers.requestHandler({
  request: ['headers', 'method', 'query_string', 'url'],
}));

app.get('/', (req, res) => {
  throw new Error('pep, an error');
  res.send('Hello World!');
});

app.use(Sentry.Handlers.errorHandler({
  shouldHandleError() {
    return true;
  },
}));

app.listen(port, () => {
  console.log(`Example app listening on port ${port}`);
});

Expected Result

Not passing 'cookies' key in the options.request array will clean up cookies from headers.

Actual Result

While not passing cookies key in the options.request we still get cookies in the reports, being sent to Sentry.

@Lms24
Copy link
Member

Lms24 commented Jul 27, 2022

Hi @ozerovs, thanks for writing in! We had a quick look at this issue and there might be something wrong on our end. We still need to look into it in greater detail. Backlogging this for now - thanks for reporting!

@thijs
Copy link

thijs commented Oct 6, 2022

So has this issue had any progress made on it? I have the same problem and I now have PII in my sentry errors, which I really do not want.

@Lms24
Copy link
Member

Lms24 commented Oct 6, 2022

Hi @thijs and apologies for the delay! We're looking into it right now.

@Lms24
Copy link
Member

Lms24 commented Oct 6, 2022

Hi @thijs and @ozerovs,

I looked into this (thanks for the reproduction instruction) and found that the Sentry.requestHandler middleware is indeed adding the Cookie Http header to event.request.headers, even if cookies is explicitly not set in the middleware's options. I just opened a PR (#5898) to fix this.

Can you confirm that cookies were only wrongfully present in event.request.headers but not in event.request.cookies?

Lms24 added a commit that referenced this issue Oct 6, 2022
…hould not be sent to Sentry (#5898)

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 patch 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.

fixes #5458
@Lms24
Copy link
Member

Lms24 commented Oct 6, 2022

This was auto-closed by GH after merging the PR. Let me know if this PR doesn't entirely solve your issue (see my comment above) then I'll reopen this issue. We'll release this in the next patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants