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

withSentry in nextjs can not be configured to scrub cookies (sensitive data) #4723

Closed
wereHamster opened this issue Mar 15, 2022 · 9 comments
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK Type: Improvement

Comments

@wereHamster
Copy link

wereHamster commented Mar 15, 2022

Problem Statement

All I want is to scrub certain (or all) cookies from the events sent to Sentry. Basic data scrubbing of sensitive fields.

The beforeSend hook is never called. Don't know why, but I saw in the code that the beforeSend hook is not called when the event type is transaction, and all events I see go through the sentry code are transactions.

I'm using withSentry from the @sentry/nextjs package, which internally calls parseRequest that's responsible for extracting the relevant sensitive data from the request. The parseRequest function accepts (optional) options, that AFAICS can be used to limit what keys are extracted from the request (defaults include cookies). The withSentry function however does not allow passing any options to parseRequest.

Solution Brainstorm

Allow options to be passed to withSentry to allow it to override what keys are extracted by extractRequestData. The requestHandler function can be configured in such a way, for example.

Or provide a hook that's called on /all/ events before they are sent to Sentry, not just some.

@lobsterkatie
Copy link
Member

lobsterkatie commented Mar 17, 2022

Hi, @wereHamster.

Thanks for bringing this up. We'll chat as a team to see how we best want to handle this. In the meantime, as a workaround, you can add an event processor, and it'll work just like beforeSend, except that it will run on both error and transaction events:

Sentry.addGlobalEventProcessor(event => {
  // make any changes to event data that you'd like here
  return event;
});

The one drawback is that in order to make sure that the processor runs after parseRequest, you'll need to call this inside your route handler.

@wereHamster
Copy link
Author

Do I really have to do it for each request? Can't I call addGlobalEventProcessor just after Sentry.init() ?

@lobsterkatie
Copy link
Member

lobsterkatie commented Mar 18, 2022

Do I really have to do it for each request? Can't I call addGlobalEventProcessor just after Sentry.init() ?

Yes, and no, unfortunately you can't. I wouldn't be calling it a workaround in that case, LOL!

The reason is that if you do that, withSentry will come along and add the parseRequest processor after the one you added at startup. (Each request gets its own instance of the processor, with the request held in a closure.) So yours will run, but the problematic data won't have been added yet, so it won't do you any good. Fortunately (for the sake of this workaround, at least), withSentry adds the processor and then calls your handler. So if your handler then adds the processor, everything will happen in the right order.

In the long run, my team agrees that it's reasonable to add the options from the original parseRequest into withSentry, and I've added it to our backlog.

@wereHamster
Copy link
Author

Couldn't get it to work with the global event processor, but this appears to work (uses the same means to add the event processor as the code in withSentry, getCurrentHub().currentScope().addEventProcessor()):

/**
 * A wrapper around Sentry.withSentry() that removes sensitive data (cookies)
 * from the event.
 *
 * Update this function once there is a nicer way to do that.
 * https://github.com/getsentry/sentry-javascript/issues/4723.
 */
export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler => {
  function sanitizeEvent(event: Sentry.Event) {
    if (event.request) {
      delete event.request.cookies;
      delete event.request.headers;
    }

    return event;
  }

  return Sentry.withSentry((req, res) => {
    Sentry.getCurrentHub().getScope()?.addEventProcessor(sanitizeEvent);
    return origHandler(req, res);
  });
};

@lobsterkatie
Copy link
Member

Nice, thanks for sharing, @wereHamster!

@seanparmelee
Copy link

Hi @lobsterkatie, I'm running into this issue as well except that I'm not using withSentry. Instead, I'm only using the automatic instrumentation via initializing sentry in a sentry.server.config.js file (and injected into my NextJS application via withSentryConfig). I've added a beforeSend function to the config object being passed into Sentry.init as instructed in the docs, but this function is never called because (as mentioned above) beforeSend is skipped for transactions https://github.com/getsentry/sentry-javascript/blob/6.19.7/packages/core/src/baseclient.ts#L567-L569.

Would you like me to log a separate issue for this since it's not specific to using withSentry? or maybe this issue title should be renamed?

@petertflem
Copy link

I also tried to scrub some data with beforeSend, and nothing happened. I guess it is the same issue, I might try to wrap ´withSentry` as well.

@smeubank
Copy link
Member

there is now beforeSendTransaction which should be able to resolve this issue where beforeSend cannot be used for transaction events. Hopefully this makes things much simpler.

available with latest versions since 7.18.0

#6121

I will close this issue, please reopen if you need anything else!

@lobsterkatie
Copy link
Member

lobsterkatie commented Nov 30, 2022

Actually, you can prevent the data from being attached in the first place using the RequestData integration. (You're making me realize that though I added it to the node docs, I also need to add it to the browser docs for platforms which are frontend-backend combos (like nextjs). I'll do that (update: done). In the meantime, see https://docs.sentry.io/platforms/node/configuration/integrations/default-integrations/#requestdata (and the Modifying System Integrations section below it).)

TL;DR, you're going to want to do something like this in your sentry.server.config.js:

import { Integrations } from '@sentry/nextjs';
const { RequestData } = Integrations

Sentry.init({
  integrations: [ new RequestData({
    include: {
      cookies: false,
    },
  })],
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK Type: Improvement
Projects
None yet
Development

No branches or pull requests

5 participants