diff --git a/middleware/index.js b/middleware/index.js index 56e172358e37..bbfe4626afd6 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -75,15 +75,47 @@ const asyncMiddleware = (fn) => (req, res, next) => { Promise.resolve(fn(req, res, next)).catch(next) } -// The IP address that Fastly regards as the true client making the request w/ fallback to req.ip -morgan.token('client-ip', (req) => req.headers['fastly-client-ip'] || req.ip) -const productionLogFormat = `:client-ip - ":method :url" :status - :response-time ms` +// By default, `:remote-addr` is described as following in the morgon docs: +// +// The remote address of the request. This will use req.ip, otherwise +// the standard req.connection.remoteAddress value (socket address). +// +// But in production, by default, `req.ip` is the IP of the Azure machine +// which is something like "104.156.87.177:28244" which is *not* the +// end user. BUT! Because we configure `app.set('trust proxy', true)` +// *before* morgain is enabled, it will use the first entry from +// the `x-forwarded-for` header which is looking like this: +// "75.40.90.27, 157.52.111.52, 104.156.87.177:5786" which is +// "{USER'S IP}, {FASTLY'S POP IP}, {AZURE'S IP}". +// Incidentally, that first IP in the comma separated list is the +// same as the value of `req.headers['fastly-client-ip']` but +// Fastly will put that into the X-Forwarded-IP. +// By leaning in to X-Forwarded-IP (*and* the use +// `app.set('trust proxy', true)`) we can express ourselves here +// without having to use vendor specific headers. +const productionLogFormat = `:remote-addr - ":method :url" :status - :response-time ms` export default function (app) { // *** Request connection management *** if (!isTest) app.use(timeout) app.use(abort) + // Don't use the proxy's IP, use the requester's for rate limiting or + // logging. + // See https://expressjs.com/en/guide/behind-proxies.html + // Essentially, setting this means it believe that the IP is the + // first of the `X-Forwarded-For` header values. + // If it was 0 (or false), the value would be that + // of `req.socket.remoteAddress`. + // Now, the `req.ip` becomes the first entry from x-forwarded-for + // and falls back on `req.socket.remoteAddress` in all other cases. + // Their documentation says: + // + // If true, the client's IP address is understood as the + // left-most entry in the X-Forwarded-For header. + // + app.set('trust proxy', true) + // *** Request logging *** // Enabled in development and azure deployed environments // Not enabled in Heroku because the Heroku router + papertrail already logs the request information @@ -181,13 +213,6 @@ export default function (app) { } // *** Early exits *** - // Don't use the proxy's IP, use the requester's for rate limiting - // See https://expressjs.com/en/guide/behind-proxies.html - // Essentially, setting this means it believe that the IP is the - // first of the `X-Forwarded-For` header values. - // If it was 0 (or false), the value would be that - // of `req.socket.remoteAddress`. - app.set('trust proxy', 1) app.use(rateLimit) app.use(instrument(handleInvalidPaths, './handle-invalid-paths')) app.use(asyncMiddleware(instrument(handleNextDataPath, './handle-next-data-path'))) diff --git a/middleware/rate-limit.js b/middleware/rate-limit.js index e4691e6bba27..709eb57d6bbb 100644 --- a/middleware/rate-limit.js +++ b/middleware/rate-limit.js @@ -21,8 +21,7 @@ export default rateLimit({ legacyHeaders: false, handler: (request, response, next, options) => { - const ip = request.headers['x-forwarded-for'] || request.ip - const tags = [`url:${request.url}`, `ip:${ip}`] + const tags = [`url:${request.url}`, `ip:${request.ip}`] statsd.increment('middleware.rate_limit', 1, tags) // This is temporary until we fully understand fully that the // rate limiter really is working in production. diff --git a/tests/routing/remote-ip.js b/tests/routing/remote-ip.js index 88e35424ad72..2a2878e3f0cb 100644 --- a/tests/routing/remote-ip.js +++ b/tests/routing/remote-ip.js @@ -24,4 +24,17 @@ describe('remote ip debugging', () => { const kv = JSON.parse(res.text) expect(kv['x-forwarded-for']).toBe('123.123.0.1') }) + + test('req.ip becomes the first value from x-forwarded-for', async () => { + const xForwardedFor = '100.0.0.1, 100.0.0.2, 100.0.0.3' + const res = await get('/_ip', { + headers: { + 'X-Forwarded-For': xForwardedFor, + }, + }) + expect(res.statusCode).toBe(200) + const kv = JSON.parse(res.text) + expect(kv.ip).toBe('100.0.0.1') + expect(kv['x-forwarded-for']).toBe(xForwardedFor) + }) })