Merge pull request #15821 from github/repo-sync

repo sync

Author: Octomerger

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')))

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.

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