req.user from Sequelize causes server to hang

[idris]

When an error is picked up by the connect errorHandler middleware, if that request contains a user object from Sequelize, the server hangs for a long time.

I assume this happens because Raven is trying to serialize the User object, and that gets into all kinds of weird things, including the Postgres driver itself.

Side note: This also causes #210 and #212, because the Postgres driver has a bug where if you enumerate all properties, it will try to require pg-native, even if it's not in your dependencies.

Proposed Solution

There are a few ways to resolve this. I prefer the first.

Request sanitizing function

Allow a function to be passed into the middleware that accepts a request and returns a sanitized request. My implementation would look like this:

(req) => {
  const { user, ...sanitizedRequest } = req; // Remove user from req
  sanitizedRequest.user = { id: user.id }; // Add userId for context
  return sanitizedRequest;
}

Recognize Sequelize models in req.user

Since req.user is popularly used with Sequelize, in parseRequest we could recognize that it's a sequelize model and call req.user.get({ plain: true }) to make it a plain JS object.

[LewisJEllis]

Awesome report/breakdown @idris - thanks! This brings a lot of clarity around that pg-native issue. I'm definitely on board with fixing this in one way or another; Sequelize is way too popular for us to not work with. For now, the closest thing to a workaround is rolling your own slightly-modified-but-mostly-just-duplicate of the error handler middleware to call parsers.parseRequest(req, { user: req.user.get({ plain: true })); instead of parsers.parseRequest(req);, and that's pretty rough.

In general I think it's better to avoid ecosystem-member-interop-specific code paths like what you describe in option 2, so I would be inclined to agree with you on the request sanitizing function as the flexible solution (even though option 2 does make a simpler UX in this case). I think that function would fit better as a configurable option on the Raven instance rather than an option to the middleware, though, and I still need to figure out how user context data should fit in vs user data off the req obj, which one should take precedence, should they merge or just either-or, etc, but that isn't really the core issue and I think those details will fall in to place.

The other point of consideration is that with upcoming breadcrumb support, we're going to have to have a fair amount of ecosystem-player-specific code for automatically capturing queries, logs, etc from various popular libraries. I'm still getting my thoughts in line on that, but it makes me wonder if maybe library-interop-specific code isn't such a bad thing.

If you need a fix for something right away, I think the self-rolled-error-handler I described should work as a stopgap, but I'll figure this out and get the nicer fix into a raven-node beta release soon.

[idris]

Re: workaround, I just wrote a super simple wrapper around your middleware to avoid copy/pasting code:

  app.use((err, req, res, next) => {
    const { user, ...sanitizedRequest } = req;
    if (user) {
      sanitizedRequest.user = { id: user.id };
    }

    return raven.middleware.express.errorHandler(SENTRY_DSN_SERVER)(err, sanitizedRequest, res, next);
  });

sanitizedRequest.user = user.get({ plain: true }); would work as well, but the userId is usually all I want for context anyway

[jpadilla]

I'd really love for the errorHandler not handle req.user automatically. In some cases req.user might have "sensitive" fields that you don't really want ending up in Sentry.

[adamlc]

I think probably the best solution would be to add some sort of callback that gets called by the errorHandler middleware that we could use to include / exclude user context. That would probably be the most straight forward I'd say! Something like this:

app.use(Raven.errorHandler({
    userContext: (req, res) => {
        return {
            user_id: req.user.id
        };
    }
}));

Looking at the code it should be easy enough to add in 👍

[benvinegar]

Maybe instead of just taking user, we (attempt to) grab a bunch of predefined fields. I'd prefer this worked as "magic" as possible without configuration, but obviously stalling the server is bad.

[benvinegar]

@MaxBittker – I can tell from the +1s that this is probably an important issue. My suggested easy fix is that, instead of just copying the user object wholesale by default, we only copy a handful of "safe" properties: username, email, id, and perhaps the fields documented in passport's API.

[LewisJEllis]

Addressed in #274; will publish a new version to npm tomorrow.

[seromenho]

The server doesn't hang anymore, but it seems it's not working as expected,
It's not parsing any fields from req.user if req.user is a sequelize instance.
I'll try to look at this again to confirm and/or propose a solution.
What I'm using for now:

app.use(function onError(err, req, res, next) {
  req.user = req.user.toJSON();
  next(err);
});
app.use(Raven.errorHandler());

[LewisJEllis]

@seromenho as of #274 we provide a parseUser option which you can use like this in your setup:

Raven.config({
  parseUser: function (req) {
    return req.user.toJSON();
  }
}).install();

[seromenho]

@LewisJEllis Oh yeah that!. Thank you.