From 177e197d8d241da5f5c7a93dd1f7e0cf484dde8f Mon Sep 17 00:00:00 2001 From: Peter Bengtsson Date: Wed, 23 Feb 2022 11:46:29 -0500 Subject: [PATCH] soft rate limit all requests (#25556) * rate limit all requests * set IP in tags to statsd * expose _ip route for assuring we get the right IP * remove old unit test * nit --- middleware/index.js | 6 ++ middleware/rate-limit.js | 54 +++++------ middleware/remote-ip.js | 12 +++ package-lock.json | 145 ++---------------------------- package.json | 2 - tests/routing/remote-ip.js | 25 ++++++ tests/unit/redis/create-client.js | 33 ------- 7 files changed, 70 insertions(+), 207 deletions(-) create mode 100644 middleware/remote-ip.js create mode 100644 tests/routing/remote-ip.js delete mode 100644 tests/unit/redis/create-client.js diff --git a/middleware/index.js b/middleware/index.js index dc9e334606db..1d6326affaa6 100644 --- a/middleware/index.js +++ b/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) diff --git a/middleware/rate-limit.js b/middleware/rate-limit.js index 1ad5f9cce3f5..f2df15d655c0 100644 --- a/middleware/rate-limit.js +++ b/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() + }, +}) diff --git a/middleware/remote-ip.js b/middleware/remote-ip.js new file mode 100644 index 000000000000..3786c9c818ad --- /dev/null +++ b/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']}` + ) +} diff --git a/package-lock.json b/package-lock.json index bfd879548e86..b5f59ae51644 100644 --- a/package-lock.json +++ b/package-lock.json @@ -58,12 +58,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", @@ -6873,14 +6871,6 @@ "node": ">=8" } }, - "node_modules/clone": { - "version": "1.0.4", - "resolved": "https://registry.npmjs.org/clone/-/clone-1.0.4.tgz", - "integrity": "sha1-2jCcwmPfFZlMaIypAheco8fNfH4=", - "engines": { - "node": ">=0.8" - } - }, "node_modules/clone-deep": { "version": "0.2.4", "resolved": "https://registry.npmjs.org/clone-deep/-/clone-deep-0.2.4.tgz", @@ -7686,14 +7676,6 @@ "node": ">=0.10.0" } }, - "node_modules/defaults": { - "version": "1.0.3", - "resolved": "https://registry.npmjs.org/defaults/-/defaults-1.0.3.tgz", - "integrity": "sha1-xlYFHpgX2f8I7YgUd/P+QBnz730=", - "dependencies": { - "clone": "^1.0.2" - } - }, "node_modules/defer-to-connect": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/defer-to-connect/-/defer-to-connect-2.0.1.tgz", @@ -7722,14 +7704,6 @@ "node": ">=0.4.0" } }, - "node_modules/denque": { - "version": "1.5.1", - "resolved": "https://registry.npmjs.org/denque/-/denque-1.5.1.tgz", - "integrity": "sha512-XwE+iZ4D6ZUB7mfYRMb5wByE8L74HCn30FBN7sWnXksWc1LO1bPDl67pBR9o/kC4z/xSNAwkMYcGgqDV3BE3Hw==", - "engines": { - "node": ">=0.10" - } - }, "node_modules/depd": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/depd/-/depd-1.1.2.tgz", @@ -9820,9 +9794,9 @@ } }, "node_modules/fs-extra": { - "version": "10.0.0", - "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-10.0.0.tgz", - "integrity": "sha512-C5owb14u9eJwizKGdchcDUQeFtlSHHthBk8pbX9Vc1PFZrLombudjDnNns88aYslCyF6IY5SUw3Roz6xShcEIQ==", + "version": "10.0.1", + "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-10.0.1.tgz", + "integrity": "sha512-NbdoVMZso2Lsrn/QwLXOy6rm0ufY2zEOKCDzJR/0kBsb0E6qed0P3iYK+Ath3BfvXEeu4JhEtXLgILx5psUfag==", "optional": true, "dependencies": { "graceful-fs": "^4.2.0", @@ -17867,15 +17841,6 @@ "node": ">= 0.6" } }, - "node_modules/rate-limit-redis": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/rate-limit-redis/-/rate-limit-redis-2.1.0.tgz", - "integrity": "sha512-6SAsTCzY0v6UCIKLOLLYqR2XzFmgdtF7jWXlSPq2FrNIZk8tZ7xwBvyGW7GFMCe5I4S9lYNdrSJ9E84rz3/CpA==", - "dependencies": { - "defaults": "^1.0.3", - "redis": "^3.0.2" - } - }, "node_modules/raw-body": { "version": "2.4.2", "resolved": "https://registry.npmjs.org/raw-body/-/raw-body-2.4.2.tgz", @@ -18218,48 +18183,6 @@ "url": "https://github.com/sponsors/sindresorhus" } }, - "node_modules/redis": { - "version": "3.1.2", - "resolved": "https://registry.npmjs.org/redis/-/redis-3.1.2.tgz", - "integrity": "sha512-grn5KoZLr/qrRQVwoSkmzdbw6pwF+/rwODtrOr6vuBRiR/f3rjSTGupbF90Zpqm2oenix8Do6RV7pYEkGwlKkw==", - "dependencies": { - "denque": "^1.5.0", - "redis-commands": "^1.7.0", - "redis-errors": "^1.2.0", - "redis-parser": "^3.0.0" - }, - "engines": { - "node": ">=10" - }, - "funding": { - "type": "opencollective", - "url": "https://opencollective.com/node-redis" - } - }, - "node_modules/redis-commands": { - "version": "1.7.0", - "resolved": "https://registry.npmjs.org/redis-commands/-/redis-commands-1.7.0.tgz", - "integrity": "sha512-nJWqw3bTFy21hX/CPKHth6sfhZbdiHP6bTawSgQBlKOVRG7EZkfHbbHwQJnrE4vsQf0CMNE+3gJ4Fmm16vdVlQ==" - }, - "node_modules/redis-errors": { - "version": "1.2.0", - "resolved": "https://registry.npmjs.org/redis-errors/-/redis-errors-1.2.0.tgz", - "integrity": "sha1-62LSrbFeTq9GEMBK/hUpOEJQq60=", - "engines": { - "node": ">=4" - } - }, - "node_modules/redis-parser": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/redis-parser/-/redis-parser-3.0.0.tgz", - "integrity": "sha1-tm2CjNyv5rS4pCin3vTGvKwxyLQ=", - "dependencies": { - "redis-errors": "^1.0.0" - }, - "engines": { - "node": ">=4" - } - }, "node_modules/refractor": { "version": "3.5.0", "resolved": "https://registry.npmjs.org/refractor/-/refractor-3.5.0.tgz", @@ -28089,11 +28012,6 @@ } } }, - "clone": { - "version": "1.0.4", - "resolved": "https://registry.npmjs.org/clone/-/clone-1.0.4.tgz", - "integrity": "sha1-2jCcwmPfFZlMaIypAheco8fNfH4=" - }, "clone-deep": { "version": "0.2.4", "resolved": "https://registry.npmjs.org/clone-deep/-/clone-deep-0.2.4.tgz", @@ -28735,14 +28653,6 @@ "resolved": "https://registry.npmjs.org/deepmerge/-/deepmerge-4.2.2.tgz", "integrity": "sha512-FJ3UgI4gIl+PHZm53knsuSFpE+nESMr7M4v9QcgB7S63Kj/6WqMiFQJpBBYz1Pt+66bZpP3Q7Lye0Oo9MPKEdg==" }, - "defaults": { - "version": "1.0.3", - "resolved": "https://registry.npmjs.org/defaults/-/defaults-1.0.3.tgz", - "integrity": "sha1-xlYFHpgX2f8I7YgUd/P+QBnz730=", - "requires": { - "clone": "^1.0.2" - } - }, "defer-to-connect": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/defer-to-connect/-/defer-to-connect-2.0.1.tgz", @@ -28762,11 +28672,6 @@ "integrity": "sha1-3zrhmayt+31ECqrgsp4icrJOxhk=", "dev": true }, - "denque": { - "version": "1.5.1", - "resolved": "https://registry.npmjs.org/denque/-/denque-1.5.1.tgz", - "integrity": "sha512-XwE+iZ4D6ZUB7mfYRMb5wByE8L74HCn30FBN7sWnXksWc1LO1bPDl67pBR9o/kC4z/xSNAwkMYcGgqDV3BE3Hw==" - }, "depd": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/depd/-/depd-1.1.2.tgz", @@ -30373,9 +30278,9 @@ "optional": true }, "fs-extra": { - "version": "10.0.0", - "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-10.0.0.tgz", - "integrity": "sha512-C5owb14u9eJwizKGdchcDUQeFtlSHHthBk8pbX9Vc1PFZrLombudjDnNns88aYslCyF6IY5SUw3Roz6xShcEIQ==", + "version": "10.0.1", + "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-10.0.1.tgz", + "integrity": "sha512-NbdoVMZso2Lsrn/QwLXOy6rm0ufY2zEOKCDzJR/0kBsb0E6qed0P3iYK+Ath3BfvXEeu4JhEtXLgILx5psUfag==", "optional": true, "requires": { "graceful-fs": "^4.2.0", @@ -36417,15 +36322,6 @@ "resolved": "https://registry.npmjs.org/range-parser/-/range-parser-1.2.1.tgz", "integrity": "sha512-Hrgsx+orqoygnmhFbKaHE6c296J+HTAQXoxEF6gNupROmmGJRoyzfG3ccAveqCBrwr/2yxQ5BVd/GTl5agOwSg==" }, - "rate-limit-redis": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/rate-limit-redis/-/rate-limit-redis-2.1.0.tgz", - "integrity": "sha512-6SAsTCzY0v6UCIKLOLLYqR2XzFmgdtF7jWXlSPq2FrNIZk8tZ7xwBvyGW7GFMCe5I4S9lYNdrSJ9E84rz3/CpA==", - "requires": { - "defaults": "^1.0.3", - "redis": "^3.0.2" - } - }, "raw-body": { "version": "2.4.2", "resolved": "https://registry.npmjs.org/raw-body/-/raw-body-2.4.2.tgz", @@ -36673,35 +36569,6 @@ } } }, - "redis": { - "version": "3.1.2", - "resolved": "https://registry.npmjs.org/redis/-/redis-3.1.2.tgz", - "integrity": "sha512-grn5KoZLr/qrRQVwoSkmzdbw6pwF+/rwODtrOr6vuBRiR/f3rjSTGupbF90Zpqm2oenix8Do6RV7pYEkGwlKkw==", - "requires": { - "denque": "^1.5.0", - "redis-commands": "^1.7.0", - "redis-errors": "^1.2.0", - "redis-parser": "^3.0.0" - } - }, - "redis-commands": { - "version": "1.7.0", - "resolved": "https://registry.npmjs.org/redis-commands/-/redis-commands-1.7.0.tgz", - "integrity": "sha512-nJWqw3bTFy21hX/CPKHth6sfhZbdiHP6bTawSgQBlKOVRG7EZkfHbbHwQJnrE4vsQf0CMNE+3gJ4Fmm16vdVlQ==" - }, - "redis-errors": { - "version": "1.2.0", - "resolved": "https://registry.npmjs.org/redis-errors/-/redis-errors-1.2.0.tgz", - "integrity": "sha1-62LSrbFeTq9GEMBK/hUpOEJQq60=" - }, - "redis-parser": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/redis-parser/-/redis-parser-3.0.0.tgz", - "integrity": "sha1-tm2CjNyv5rS4pCin3vTGvKwxyLQ=", - "requires": { - "redis-errors": "^1.0.0" - } - }, "refractor": { "version": "3.5.0", "resolved": "https://registry.npmjs.org/refractor/-/refractor-3.5.0.tgz", diff --git a/package.json b/package.json index 98e8fd24adaf..88b3a63c9a06 100644 --- a/package.json +++ b/package.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", diff --git a/tests/routing/remote-ip.js b/tests/routing/remote-ip.js new file mode 100644 index 000000000000..bc7470436903 --- /dev/null +++ b/tests/routing/remote-ip.js @@ -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() + }) +}) diff --git a/tests/unit/redis/create-client.js b/tests/unit/redis/create-client.js deleted file mode 100644 index 34d3ab63f88c..000000000000 --- a/tests/unit/redis/create-client.js +++ /dev/null @@ -1,33 +0,0 @@ -import createRedisClient from '../../../lib/redis/create-client.js' - -const redisUrl = 'http://localhost:6379' - -describe('create-client', () => { - test('returns null if no URL is provided', async () => { - expect(createRedisClient({})).toBe(null) - }) - - test('throws if database number is provided but is not a number', async () => { - expect(() => createRedisClient({ url: redisUrl, db: 'dbName' })).toThrowError( - new TypeError('Redis database number must be an integer between 0 and 15 but was: "dbName"') - ) - }) - - test('throws if database number is provided but is not an integer', async () => { - expect(() => createRedisClient({ url: redisUrl, db: 1.5 })).toThrowError( - new TypeError('Redis database number must be an integer between 0 and 15 but was: 1.5') - ) - }) - - test('throws if database number is provided but is less than 0', async () => { - expect(() => createRedisClient({ url: redisUrl, db: -1 })).toThrowError( - new TypeError('Redis database number must be an integer between 0 and 15 but was: -1') - ) - }) - - test('throws if database number is provided but is greater than max allowed', async () => { - expect(() => createRedisClient({ url: redisUrl, db: 16 })).toThrowError( - new TypeError('Redis database number must be an integer between 0 and 15 but was: 16') - ) - }) -})