Merge pull request #15774 from github/repo-sync

repo sync

Author: Octomerger

middleware/index.js

@@ -36,6 +36,7 @@ import archivedEnterpriseVersionsAssets from './archived-enterprise-versions-ass
 import events from './events.js'
 import search from './search.js'
 import healthz from './healthz.js'
+import remoteIP from './remote-ip.js'
 import archivedEnterpriseVersions from './archived-enterprise-versions.js'
 import robots from './robots.js'
 import earlyAccessLinks from './contextualizers/early-access-links.js'
@@ -182,6 +183,10 @@ 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'))
@@ -237,6 +242,7 @@ export default function (app) {
   app.use('/events', asyncMiddleware(instrument(events, './events')))
   app.use('/search', asyncMiddleware(instrument(search, './search')))
   app.use('/healthz', asyncMiddleware(instrument(healthz, './healthz')))
+  app.get('/_ip', asyncMiddleware(instrument(remoteIP, './remoteIP')))
 
   // Check for a dropped connection before proceeding (again)
   app.use(haltOnDroppedConnection)

middleware/rate-limit.js

@@ -1,39 +1,27 @@
 import rateLimit from 'express-rate-limit'
-import RedisStore from 'rate-limit-redis'
-import createRedisClient from '../lib/redis/create-client.js'
+import statsd from '../lib/statsd.js'
 
-const isProduction = process.env.NODE_ENV === 'production'
-const { REDIS_URL } = process.env
-const rateLimitDatabaseNumber = 0
 const EXPIRES_IN_AS_SECONDS = 60
 
-// The reason the options object is created outside like this is for the
-// necessity of avoiding setting a key called `store` even if it's set
-// to `undefined`.
-// More context here: https://github.com/nfriedly/express-rate-limit/issues/289
-const options = {
-  // 1 minute (or practically unlimited outside of production)
-  windowMs: isProduction ? EXPIRES_IN_AS_SECONDS * 1000 : 1, // Non-Redis configuration in `ms`. Used as a fallback when Redis is not working or active.
+export default rateLimit({
+  // 1 minute
+  windowMs: EXPIRES_IN_AS_SECONDS * 1000,
   // limit each IP to X requests per windowMs
-  max: 250,
-  // Don't rate limit requests for 200s and redirects
-  // Or anything with a status code less than 400
-  skipSuccessfulRequests: true,
-}
+  // We currently have about 25 instances in production. That's routed
+  // in Azure to spread the requests to each healthy instance.
+  // So, the true rate limit, per `windowMs`, is this number multiplied
+  // by the current number of instances.
+  // We have see DDoS attempts against prod that hits the `/` endpoint
+  // (and not following the redirect to `/en`) at roughly 200k per minute.
+  max: 100,
 
-// When available, use Redis; if not, defaults to an in-memory store
-if (REDIS_URL) {
-  options.store = new RedisStore({
-    client: createRedisClient({
-      url: REDIS_URL,
-      db: rateLimitDatabaseNumber,
-      name: 'rate-limit',
-    }),
-    // 1 minute (or practically unlimited outside of production)
-    expiry: isProduction ? EXPIRES_IN_AS_SECONDS : 1, // Redis configuration in `s`
-    // If Redis is not connected, let the request succeed as failover
-    passIfNotConnected: true,
-  })
-}
-
-export default rateLimit(options)
+  handler: (request, response, next, options) => {
+    const tags = [`url:${request.url}`, `ip:${request.ip}`]
+    statsd.increment('rate_limit', 1, tags)
+    // NOTE! At the time of writing, the actual rate limiting is disabled!
+    // At least we can start recording how often this happens in Datadog.
+    // The following line is commented out and replaced with `next()`
+    // response.status(options.statusCode).send(options.message)
+    next()
+  },
+})

middleware/remote-ip.js

@@ -0,0 +1,12 @@
+import { cacheControlFactory } from './cache-control.js'
+
+const noCacheControl = cacheControlFactory(0)
+
+export default async function remoteIp(req, res, next) {
+  noCacheControl(res)
+  res.send(
+    `ip=${req.ip}\t` +
+      `x-forwarded-for=${req.headers['x-forwarded-for']}\t` +
+      `fastly-client-ip=${req.headers['fastly-client-ip']}`
+  )
+}

package-lock.json

@@ -60,12 +60,10 @@
     "node-fetch": "^3.2.0",
     "parse5": "^6.0.1",
     "port-used": "^2.0.8",
-    "rate-limit-redis": "^2.1.0",
     "react": "^17.0.2",
     "react-dom": "^17.0.2",
     "react-markdown": "^8.0.0",
     "react-syntax-highlighter": "^15.4.5",
-    "redis": "^3.1.2",
     "rehype-autolink-headings": "^6.1.1",
     "rehype-highlight": "^5.0.2",
     "rehype-raw": "^6.1.1",

package.json

@@ -0,0 +1,25 @@
+import { get } from '../helpers/supertest.js'
+import { expect, jest } from '@jest/globals'
+
+describe('remote ip debugging', () => {
+  jest.setTimeout(5 * 60 * 1000)
+
+  test('basics', async () => {
+    const res = await get('/_ip')
+    expect(res.statusCode).toBe(200)
+    const kv = res.text.trim().split(/\s/g)
+    expect(kv[0].startsWith('ip=')).toBeTruthy()
+    expect(kv[1].startsWith('x-forwarded-for=')).toBeTruthy()
+    expect(kv[2].startsWith('fastly-client-ip=')).toBeTruthy()
+  })
+
+  test('carrying the x-forwarded-for header', async () => {
+    const res = await get('/_ip', {
+      headers: {
+        'X-Forwarded-For': '123.123.0.1',
+      },
+    })
+    expect(res.statusCode).toBe(200)
+    expect(res.text.includes('x-forwarded-for=123.123.0.1')).toBeTruthy()
+  })
+})