From 59dcdf24f38ad810f92ebdcf1777e301b6924132 Mon Sep 17 00:00:00 2001 From: Pierre-Yves B Date: Thu, 20 May 2021 08:02:44 +0100 Subject: [PATCH] Remove rate limiting functionality (#6513) --- config/custom-environment-variables.yml | 2 - config/default.yml | 2 - config/development.yml | 2 - config/test.yml | 2 - core/server/monitor.js | 57 ----------------------- core/server/prometheus-metrics.js | 10 ----- core/server/rate-limit.js | 60 ------------------------- core/server/server.js | 11 +---- 8 files changed, 1 insertion(+), 145 deletions(-) delete mode 100644 core/server/monitor.js delete mode 100644 core/server/rate-limit.js diff --git a/config/custom-environment-variables.yml b/config/custom-environment-variables.yml index a5d3747e55..18de1c353c 100644 --- a/config/custom-environment-variables.yml +++ b/config/custom-environment-variables.yml @@ -57,8 +57,6 @@ public: cacheHeaders: defaultCacheLengthSeconds: 'BADGE_MAX_AGE_SECONDS' - rateLimit: 'RATE_LIMIT' - fetchLimit: 'FETCH_LIMIT' requestTimeoutSeconds: 'REQUEST_TIMEOUT_SECONDS' diff --git a/config/default.yml b/config/default.yml index 26c2537abd..32d1e09c98 100644 --- a/config/default.yml +++ b/config/default.yml @@ -27,8 +27,6 @@ public: cacheHeaders: defaultCacheLengthSeconds: 120 - rateLimit: true - handleInternalErrors: true fetchLimit: '10MB' diff --git a/config/development.yml b/config/development.yml index f800291861..e4b3598e60 100644 --- a/config/development.yml +++ b/config/development.yml @@ -5,6 +5,4 @@ public: cors: allowedOrigin: ['http://localhost:3000'] - rateLimit: false - handleInternalErrors: false diff --git a/config/test.yml b/config/test.yml index 2c5bfca98f..0c53ed5ba8 100644 --- a/config/test.yml +++ b/config/test.yml @@ -3,8 +3,6 @@ public: address: 'localhost' port: 1111 - rateLimit: false - redirectUrl: 'http://frontend.example.test' rasterUrl: 'http://raster.example.test' diff --git a/core/server/monitor.js b/core/server/monitor.js deleted file mode 100644 index 0ae2cac793..0000000000 --- a/core/server/monitor.js +++ /dev/null @@ -1,57 +0,0 @@ -'use strict' - -const RateLimit = require('./rate-limit') - -function setRoutes({ rateLimit }, { server, metricInstance }) { - const ipRateLimit = new RateLimit({ - // Exclude IPs for GitHub Camo, determined experimentally by running e.g. - // `curl --insecure -u ":shields-secret" https://s0.shields-server.com/sys/rate-limit` - safelist: /^(?:192\.30\.252\.\d+)|(?:140\.82\.115\.\d+)$/, - }) - const badgeTypeRateLimit = new RateLimit({ maxHitsPerPeriod: 6000 }) - const refererRateLimit = new RateLimit({ - maxHitsPerPeriod: 600, - safelist: /^https?:\/\/shields\.io\/$/, - }) - - server.handle((req, res, next) => { - if (rateLimit) { - const ip = - (req.headers['x-forwarded-for'] || '').split(', ')[0] || - req.socket.remoteAddress - const badgeType = req.url.split(/[/-]/).slice(0, 3).join('') - const referer = req.headers.referer - - if (ipRateLimit.isBanned(ip, req, res)) { - metricInstance.noteRateLimitExceeded('ip') - return - } - if (badgeTypeRateLimit.isBanned(badgeType, req, res)) { - metricInstance.noteRateLimitExceeded('badge_type') - return - } - if (refererRateLimit.isBanned(referer, req, res)) { - metricInstance.noteRateLimitExceeded('referrer') - return - } - } - - next() - }) - - server.get('/sys/rate-limit', (req, res) => { - res.json({ - ip: ipRateLimit.toJSON(), - badgeType: badgeTypeRateLimit.toJSON(), - referer: refererRateLimit.toJSON(), - }) - }) - - return function () { - ipRateLimit.stop() - badgeTypeRateLimit.stop() - refererRateLimit.stop() - } -} - -module.exports = { setRoutes } diff --git a/core/server/prometheus-metrics.js b/core/server/prometheus-metrics.js index ae693a95c7..a19c916d92 100644 --- a/core/server/prometheus-metrics.js +++ b/core/server/prometheus-metrics.js @@ -25,12 +25,6 @@ module.exports = class PrometheusMetrics { ], registers: [this.register], }), - rateLimitExceeded: new prometheus.Counter({ - name: 'rate_limit_exceeded_total', - help: 'Count of rate limit exceeded by type', - labelNames: ['rate_limit_type'], - registers: [this.register], - }), serviceResponseSize: new prometheus.Histogram({ name: 'service_response_bytes', help: 'Service response size in bytes', @@ -82,10 +76,6 @@ module.exports = class PrometheusMetrics { return this.counters.responseTime.observe(responseTime) } - noteRateLimitExceeded(rateLimitType) { - return this.counters.rateLimitExceeded.labels(rateLimitType).inc() - } - createServiceResponseSizeHistogram({ category, serviceFamily, name }) { const service = decamelize(name) return this.counters.serviceResponseSize.labels( diff --git a/core/server/rate-limit.js b/core/server/rate-limit.js deleted file mode 100644 index c520d8aa64..0000000000 --- a/core/server/rate-limit.js +++ /dev/null @@ -1,60 +0,0 @@ -'use strict' - -// A rate limit ensures that a request parameter gets flagged if it goes -// above a limit. -module.exports = class RateLimit { - constructor(options = {}) { - // this.hits: Map from request parameters to the number of hits. - this.hits = new Map() - this.period = options.period || 200 // 3 min ⅓, in seconds - this.maxHitsPerPeriod = options.maxHitsPerPeriod || 1000 - this.banned = new Set() - this.bannedUrls = new Set() - this.safelist = options.safelist || /(?!)/ // Matches nothing by default. - this.interval = setInterval(this.resetHits.bind(this), this.period * 1000) - } - - stop() { - clearInterval(this.interval) - this.interval = undefined - } - - resetHits() { - this.hits.clear() - this.banned.clear() - this.bannedUrls.clear() - } - - isBanned(reqParam, req, res) { - const hitsInCurrentPeriod = this.hits.get(reqParam) || 0 - if ( - reqParam != null && - !this.safelist.test(reqParam) && - hitsInCurrentPeriod > this.maxHitsPerPeriod - ) { - this.banned.add(reqParam) - } - - if (this.banned.has(reqParam)) { - res.statusCode = 429 - res.setHeader('Retry-After', String(this.period)) - res.end( - `Exceeded limit ${this.maxHitsPerPeriod} requests ` + - `per ${this.period} seconds` - ) - this.bannedUrls.add(req.url) - return true - } - - this.hits.set(reqParam, hitsInCurrentPeriod + 1) - return false - } - - toJSON() { - return { - banned: [...this.banned], - hits: [...this.hits], - urls: [...this.bannedUrls], - } - } -} diff --git a/core/server/server.js b/core/server/server.js index f63c5bb852..36e9d57d73 100644 --- a/core/server/server.js +++ b/core/server/server.js @@ -20,7 +20,6 @@ const { clearRegularUpdateCache } = require('../legacy/regular-update') const { rasterRedirectUrl } = require('../badge-urls/make-badge-url') const { nonNegativeInteger } = require('../../services/validators') const log = require('./log') -const sysMonitor = require('./monitor') const PrometheusMetrics = require('./prometheus-metrics') const InfluxMetrics = require('./influx-metrics') @@ -139,7 +138,6 @@ const publicConfigSchema = Joi.object({ trace: Joi.boolean().required(), }).required(), cacheHeaders: { defaultCacheLengthSeconds: nonNegativeInteger }, - rateLimit: Joi.boolean().required(), handleInternalErrors: Joi.boolean().required(), fetchLimit: Joi.string().regex(/^[0-9]+(b|kb|mb|gb|tb)$/i), requestTimeoutSeconds: nonNegativeInteger, @@ -431,7 +429,6 @@ class Server { bind: { port, address: hostname }, ssl: { isSecure: secure, cert, key }, cors: { allowedOrigin }, - rateLimit, requireCloudflare, } = this.config.public @@ -451,13 +448,7 @@ class Server { this.requireCloudflare() } - const { metricInstance } = this - this.cleanupMonitor = sysMonitor.setRoutes( - { rateLimit }, - { server: camp, metricInstance } - ) - - const { githubConstellation } = this + const { githubConstellation, metricInstance } = this await githubConstellation.initialize(camp) if (metricInstance) { if (this.config.public.metrics.prometheus.endpointEnabled) {