IP address lost when using express behind a proxy and 'trust proxy' is enabled

[rwky]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

When app.enable('trust proxy') is enabled req.ip is undefined here https://github.com/getsentry/raven-node/blob/master/lib/parsers.js#L154 this results in req.connection.remoteAddress being used.

What is the expected behavior?

req.ip shouldn't be undefined and used.

I've created a test case:

1. In an empty folder run npm install express raven

2. Create index.js with the following:

var http = require('http')
var express = require('express')
var app = express()
app.enable('trust proxy')

var raven = require('raven')

var dsn = '';
raven.config(dsn, {
  autoBreadcrumbs: true
}).install();

app.use(raven.requestHandler());

app.get('/', function(req, res) {
    res.send('hello world ' + req.ip)
})

app.get('/err', function(req, res) { 
    throw new Error('test')
})

app.use(raven.errorHandler());
http.createServer(app).listen(8888);

3. Set up a proxy to forward to the node app, I used nginx with the following config:

 location / {
                proxy_pass http://172.17.0.1:8888;
                proxy_set_header  X-Forwarded-For $remote_addr;
               proxy_set_header  X-Real-IP       $remote_addr;
        }

4. Visit / via the proxy and you'll see hello world and the IP printed

5. Visit /err via the proxy, it'll throw an error, check this in sentry it won't show the IP. I checked that the line mentioned above is resulting in req.ip undefined.

I've not had time to debug why req.ip is undefined but since it's not undefined when accessed outside raven I'm assuming it's a bug in raven.

[goorockey]

+1

I found Object.prototype.hasOwnProperty.call(req, 'ip')) return false, which leads ip field does not assigned in extend and req.ip remains undefined in Raven:

client.js

      var req = extend({}, this._globalContext.req, domainContext.req, kwargs.req);
      if (Object.keys(req).length > 0) {
        var parseUser = Object.keys(kwargs.user).length === 0 ? this.parseUser : false;
        extend(kwargs, parsers.parseRequest(req, parseUser));
        delete kwargs.req;
      }

utils.js

module.exports.extend = Object.assign || function (target) {
  for (var i = 1; i < arguments.length; i++) {
    var source = arguments[i];
    for (var key in source) {
      if (Object.prototype.hasOwnProperty.call(source, key)) {
        target[key] = source[key];
      }
    }
  }
  return target;
};

[rwky]

As a work around I'm using mergeContext to merge { user: { ip: req.ip }}

I'll see if I can make a PR at some point to fix it properly though if someone else wants to do it that'd be great.

[wbyoung]

Though I'm not sure this is really a bug in Raven, I came across this issue discussing X-Forwarded-For, and thought the little bit of code I posted over on this issue could be helpful:

/* @flow */

import Raven from 'raven';
import type {
  $Request as Request,
  $Response as Response,
  NextFunction,
  Middleware,
} from 'express';

export default function sentryContextMiddleware(): Middleware {
  return (req: Request, res: Response, next: NextFunction) => {
    const remoteIp =
      (req.headers['x-forwarded-for'] || '').split(',')[0] ||
      (req.connection && (req.connection: any).remoteAddress);

    const user: { [string]: mixed } = {
      ip_address: remoteIp, // eslint-disable-line camelcase
    };

    Raven.mergeContext({ user });

    next();
  };
}

[kamilogorek]

Addressed in #379 Let's keep the conversation in the PR.
ip is indeed non-enumerable when using with proxy feature.