From d6bd7a540d8ab2b3eee0bcce0bcb426514cf9226 Mon Sep 17 00:00:00 2001 From: Heikki Hellgren Date: Tue, 26 Nov 2024 14:33:30 +0200 Subject: [PATCH] fix: review findings Signed-off-by: Heikki Hellgren --- .changeset/famous-terms-rescue.md | 3 +- app-config.yaml | 6 ++ packages/backend-defaults/config.d.ts | 8 +-- packages/backend-defaults/package.json | 2 +- .../rootHttpRouter/http/MiddlewareFactory.ts | 62 +++++++++++-------- .../http/RateLimitStoreFactory.test.ts | 35 ++++++----- .../http/RateLimitStoreFactory.ts | 16 ++--- .../rootHttpRouterServiceFactory.ts | 2 +- yarn.lock | 12 ++-- 9 files changed, 83 insertions(+), 63 deletions(-) diff --git a/.changeset/famous-terms-rescue.md b/.changeset/famous-terms-rescue.md index 78630eb532..9f38986934 100644 --- a/.changeset/famous-terms-rescue.md +++ b/.changeset/famous-terms-rescue.md @@ -4,11 +4,12 @@ Added new rate limit middleware to allow rate limiting requests to the backend +If you are using the `configure` callback of the root HTTP router service and do NOT call `applyDefaults()` inside it, please see [the relevant changes](https://github.com/backstage/backstage/pull/26725/files#diff-86ad1b6a694dd250823aee39d410428dd837c9d9a04ca8c33bd1081fbe3f22af) that were made, to see if you want to apply them as well to your custom configuration. Rate limiting can be turned on by adding the following configuration to `app-config.yaml`: ```yaml backend: rateLimit: - window: 60000 + window: 6000ms incomingRequestLimit: 100 ``` diff --git a/app-config.yaml b/app-config.yaml index eaee401d09..df80c7add0 100644 --- a/app-config.yaml +++ b/app-config.yaml @@ -36,6 +36,12 @@ backend: # keys: # - secret: ${BACKEND_SECRET} + # Used for testing rate limiting locally + # rateLimit: + # windowMs: 60000 + # incomingRequestLimit: 1 + # ipAllowList: [] + auth: # TODO: once plugins have been migrated we can remove this, but right now it # is require for the backend-next to work in this repo diff --git a/packages/backend-defaults/config.d.ts b/packages/backend-defaults/config.d.ts index 49a51ec2f5..6cd1aa183a 100644 --- a/packages/backend-defaults/config.d.ts +++ b/packages/backend-defaults/config.d.ts @@ -791,10 +791,10 @@ export interface Config { }; /** - * Rate limiting options + * Rate limiting options. Defining this as `true` will enable rate limiting with default values. */ rateLimit?: - | false + | true | { store?: | { @@ -806,9 +806,9 @@ export interface Config { }; /** * Time frame in milliseconds or as human duration for which requests are checked/remembered. - * Defaults to 6000ms. + * Defaults to one minute. */ - window?: number | HumanDuration; + window?: string | HumanDuration; /** * The maximum number of connections to allow during the `window` before rate limiting the client. * Defaults to 5. diff --git a/packages/backend-defaults/package.json b/packages/backend-defaults/package.json index c10cb9f11f..b1f9513f2c 100644 --- a/packages/backend-defaults/package.json +++ b/packages/backend-defaults/package.json @@ -168,7 +168,7 @@ "cron": "^3.0.0", "express": "^4.17.1", "express-promise-router": "^4.1.0", - "express-rate-limit": "^7.4.0", + "express-rate-limit": "^7.5.0", "fs-extra": "^11.2.0", "git-url-parse": "^15.0.0", "helmet": "^6.0.0", diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/MiddlewareFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/MiddlewareFactory.ts index 853e5aa8f6..3dddc99974 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/MiddlewareFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/MiddlewareFactory.ts @@ -44,8 +44,8 @@ import { } from '@backstage/errors'; import { applyInternalErrorFilter } from './applyInternalErrorFilter'; import { rateLimit } from 'express-rate-limit'; -import { Config } from '@backstage/config'; -import { durationToMilliseconds, HumanDuration } from '@backstage/types'; +import { readDurationFromConfig } from '@backstage/config'; +import { durationToMilliseconds } from '@backstage/types'; import { RateLimitStoreFactory } from './RateLimitStoreFactory'; type LogMeta = { @@ -242,44 +242,50 @@ export class MiddlewareFactory { * @returns An Express request handler */ rateLimit(): RequestHandler { - const conf = this.#config.getOptional( - 'backend.rateLimit', - ); - if (!conf || typeof conf !== 'object') { + const enabled = this.#config.has('backend.rateLimit'); + if (!enabled) { return (_req: Request, _res: Response, next: NextFunction) => { next(); }; } - const rateLimitOptions = conf as Config; - const window = rateLimitOptions.getOptional( - 'window', - ); - let windowMs: number | undefined; - if (window !== undefined) { - if (typeof window === 'number') { - windowMs = window; - } else if (typeof window === 'object' && !Array.isArray(window)) { - windowMs = durationToMilliseconds(window); - } else { - throw new Error( - `Invalid configuration backend.rateLimit.window: ${window}, expected milliseconds number or HumanDuration object`, - ); - } + + const useDefaults = this.#config.getOptional('backend.rateLimit') === true; + const rateLimitOptions = useDefaults + ? undefined + : this.#config.getOptionalConfig('backend.rateLimit'); + + let windowMs: number = 60000; + if (rateLimitOptions && rateLimitOptions.has('window')) { + const windowDuration = readDurationFromConfig(rateLimitOptions, { + key: 'window', + }); + windowMs = durationToMilliseconds(windowDuration); } - const ipAllowList = rateLimitOptions.getOptionalStringArray( + const ipAllowList = rateLimitOptions?.getOptionalStringArray( 'ipAllowList', ) ?? ['127.0.0.1', '0:0:0:0:0:0:0:1', '::1']; return rateLimit({ windowMs, - limit: rateLimitOptions.getOptionalNumber('incomingRequestLimit'), - skipSuccessfulRequests: rateLimitOptions.getOptionalBoolean( + limit: rateLimitOptions?.getOptionalNumber('incomingRequestLimit'), + skipSuccessfulRequests: rateLimitOptions?.getOptionalBoolean( 'skipSuccessfulRequests', ), + message: { + error: { + name: 'Error', + message: `Too many requests, please try again later`, + }, + response: { + statusCode: 429, + }, + }, + statusCode: 429, skipFailedRequests: - rateLimitOptions.getOptionalBoolean('skipFailedRequests'), - passOnStoreError: rateLimitOptions.getOptionalBoolean('passOnStoreError'), + rateLimitOptions?.getOptionalBoolean('skipFailedRequests'), + passOnStoreError: + rateLimitOptions?.getOptionalBoolean('passOnStoreError'), keyGenerator(req, _res): string { if (!req.ip) { return req.socket.remoteAddress!; @@ -298,7 +304,9 @@ export class MiddlewareFactory { validate: { trustProxy: false, }, - store: new RateLimitStoreFactory(this.#config).create(), + store: useDefaults + ? undefined + : RateLimitStoreFactory.create(this.#config), }); } diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/RateLimitStoreFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/RateLimitStoreFactory.test.ts index 4fdc513c81..c1f690474e 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/RateLimitStoreFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/RateLimitStoreFactory.test.ts @@ -13,13 +13,25 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { mockServices, TestCaches } from '@backstage/backend-test-utils'; +import { mockServices } from '@backstage/backend-test-utils'; import { RateLimitStoreFactory } from './RateLimitStoreFactory'; -import { RedisStore } from 'rate-limit-redis'; + +jest.mock('@keyv/redis', () => { + const Actual = jest.requireActual('@keyv/redis'); + return { + ...Actual, + __esModule: true, + default: jest.fn(() => { + return { + getClient: jest.fn(() => ({ + sendCommand: jest.fn().mockReturnValue('mock'), + })), + }; + }), + }; +}); describe('CacheRateLimitStoreFactory', () => { - const caches = TestCaches.create(); - afterEach(jest.clearAllMocks); it('should return undefined store with auto configuration if redis is not available', () => { @@ -32,31 +44,24 @@ describe('CacheRateLimitStoreFactory', () => { }, }, }); - const factory = new RateLimitStoreFactory(config); - const store = factory.create(); + const store = RateLimitStoreFactory.create(config); expect(store).toBeUndefined(); }); it('should return redis store if configured explicitly', async () => { - if (!caches.supports('REDIS_7')) { - return; - } - const conf = await caches.init('REDIS_7'); - const config = mockServices.rootConfig({ data: { backend: { rateLimit: { store: { client: 'redis', - connection: conf.connection, + connection: 'redis://localhost:6379', }, }, }, }, }); - const factory = new RateLimitStoreFactory(config); - const store = factory.create(); - expect(store).toBeInstanceOf(RedisStore); + const store = RateLimitStoreFactory.create(config); + expect(store).not.toBeUndefined(); }); }); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/RateLimitStoreFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/RateLimitStoreFactory.ts index 85eb3c4d86..1275095aca 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/RateLimitStoreFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/RateLimitStoreFactory.ts @@ -15,7 +15,6 @@ */ import { Config } from '@backstage/config'; import type { Store } from 'express-rate-limit'; -import KeyvRedis from '@keyv/redis'; import { RedisStore } from 'rate-limit-redis'; /** @@ -24,10 +23,8 @@ import { RedisStore } from 'rate-limit-redis'; * @internal */ export class RateLimitStoreFactory { - constructor(private readonly config: Config) {} - - create(): Store | undefined { - const store = this.config.getOptionalConfig('backend.rateLimit.store'); + static create(config: Config): Store | undefined { + const store = config.getOptionalConfig('backend.rateLimit.store'); if (!store) { return undefined; } @@ -40,12 +37,15 @@ export class RateLimitStoreFactory { } } - redis(storeConfig: Config): Store { + private static redis(storeConfig: Config): Store { const connectionString = storeConfig.getString('connection'); + const KeyvRedis = require('@keyv/redis').default; const keyv = new KeyvRedis(connectionString); return new RedisStore({ - // Keyv uses ioredis under the hood - sendCommand: (...args: string[]) => keyv.redis.call(...args), + sendCommand: async (...args: string[]) => { + const client = await keyv.getClient(); + return client.sendCommand(args); + }, }); } } diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts index 7d0d778b0c..3b50515742 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts @@ -117,11 +117,11 @@ const rootHttpRouterServiceFactoryWithOptions = ( if (trustProxy !== undefined) { app.set('trust proxy', trustProxy); } - app.use(middleware.rateLimit()); app.use(middleware.helmet()); app.use(middleware.cors()); app.use(middleware.compression()); app.use(middleware.logging()); + app.use(middleware.rateLimit()); app.use(healthRouter); app.use(routes); app.use(middleware.notFound()); diff --git a/yarn.lock b/yarn.lock index 0a73d77844..aebb875a10 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3632,7 +3632,7 @@ __metadata: cron: "npm:^3.0.0" express: "npm:^4.17.1" express-promise-router: "npm:^4.1.0" - express-rate-limit: "npm:^7.4.0" + express-rate-limit: "npm:^7.5.0" fs-extra: "npm:^11.2.0" git-url-parse: "npm:^15.0.0" helmet: "npm:^6.0.0" @@ -30045,12 +30045,12 @@ __metadata: languageName: node linkType: hard -"express-rate-limit@npm:^7.4.0": - version: 7.4.0 - resolution: "express-rate-limit@npm:7.4.0" +"express-rate-limit@npm:^7.5.0": + version: 7.5.0 + resolution: "express-rate-limit@npm:7.5.0" peerDependencies: - express: 4 || 5 || ^5.0.0-beta.1 - checksum: 10/33178c652bb1472aad2022194b5cd7963bd3e74d3eaf5e49eb1491a968fdce54551cc76b097ac10d3a1646d62cec2e6f2405ccef5ef5b60152a0c4a148749a4d + express: ^4.11 || 5 || ^5.0.0-beta.1 + checksum: 10/eff34c83bf586789933a332a339b66649e2cca95c8e977d193aa8bead577d3182ac9f0e9c26f39389287539b8038890ff023f910b54ebb506a26a2ce135b92ca languageName: node linkType: hard