sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

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

Open wereHamster opened this issue 2 years ago • 7 comments

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.

wereHamster avatar Mar 15 '22 22:03 wereHamster

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.

lobsterkatie avatar Mar 17 '22 06:03 lobsterkatie

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

wereHamster avatar Mar 17 '22 07:03 wereHamster

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.

lobsterkatie avatar Mar 18 '22 02:03 lobsterkatie

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);
  });
};

wereHamster avatar Mar 18 '22 04:03 wereHamster

Nice, thanks for sharing, @wereHamster!

lobsterkatie avatar Mar 25 '22 06:03 lobsterkatie

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?

seanparmelee avatar Jul 14 '22 13:07 seanparmelee

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.

petertflem avatar Aug 29 '22 10:08 petertflem

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

https://github.com/getsentry/sentry-javascript/pull/6121

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

smeubank avatar Nov 30 '22 14:11 smeubank

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,
    },
  })],
});

lobsterkatie avatar Nov 30 '22 19:11 lobsterkatie