diff --git a/.changeset/eleven-llamas-heal.md b/.changeset/eleven-llamas-heal.md new file mode 100644 index 0000000000..aeb433c1bc --- /dev/null +++ b/.changeset/eleven-llamas-heal.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +Temporarily revert the rate limiting diff --git a/packages/backend-app-api/config.d.ts b/packages/backend-app-api/config.d.ts index cbf65849be..137615a152 100644 --- a/packages/backend-app-api/config.d.ts +++ b/packages/backend-app-api/config.d.ts @@ -14,8 +14,6 @@ * limitations under the License. */ -import { HumanDuration } from '@backstage/types'; - export interface Config { backend?: { auth?: { @@ -35,23 +33,6 @@ export interface Config { */ dangerouslyDisableDefaultAuthPolicy?: boolean; }; - /** @visibility frontend */ - rateLimit?: - | false - | { - /** - * Limit each IP to max requests per window, defaults to 60 requests. - */ - max?: number; - /** - * The duration for which the rate limit is enforced, defaults to 1 minute. - */ - window?: HumanDuration; - /** - * Disable the rate limit verification. - */ - disable?: true; - }; }; /** Discovery options. */ diff --git a/packages/backend-app-api/package.json b/packages/backend-app-api/package.json index 57b4032d6f..a5337e56ec 100644 --- a/packages/backend-app-api/package.json +++ b/packages/backend-app-api/package.json @@ -64,7 +64,6 @@ "cors": "^2.8.5", "express": "^4.17.1", "express-promise-router": "^4.1.0", - "express-rate-limit": "^7.2.0", "fs-extra": "^11.2.0", "helmet": "^6.0.0", "jose": "^5.0.0", diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/createCredentialsBarrier.test.ts b/packages/backend-app-api/src/services/implementations/httpRouter/createCredentialsBarrier.test.ts index 5beeaa3a8e..a5246c91b2 100644 --- a/packages/backend-app-api/src/services/implementations/httpRouter/createCredentialsBarrier.test.ts +++ b/packages/backend-app-api/src/services/implementations/httpRouter/createCredentialsBarrier.test.ts @@ -21,21 +21,18 @@ import request from 'supertest'; import { createCredentialsBarrier } from './createCredentialsBarrier'; import { mockCredentials, mockServices } from '@backstage/backend-test-utils'; import { MiddlewareFactory } from '../../../http'; -import { CacheService } from '@backstage/backend-plugin-api'; -import { JsonObject } from '@backstage/types'; const errorMiddleware = MiddlewareFactory.create({ config: mockServices.rootConfig(), logger: mockServices.rootLogger(), }).error(); -function setup(options?: { cache?: CacheService; config?: JsonObject }) { +function setup() { const barrier = createCredentialsBarrier({ httpAuth: mockServices.httpAuth({ defaultCredentials: mockCredentials.none(), }), - config: mockServices.rootConfig({ data: options?.config }), - cache: options?.cache ?? mockServices.cache.mock(), + config: mockServices.rootConfig(), }); const app = express(); @@ -176,188 +173,3 @@ describe('createCredentialsBarrier', () => { await request(app).get('/other').send().expect(200); }); }); - -it('do not limit authenticated requests', async () => { - const now = Date.now(); - jest.useFakeTimers({ now }); - const cacheMock = mockServices.cache.mock(); - const twoMinutesInMilliseconds = 2 * 60 * 1000; - const resetTime = now + twoMinutesInMilliseconds; - cacheMock.get.mockResolvedValue({ totalHits: 2, resetTime }); - - const max = 2; - const configMock = { - backend: { - rateLimit: { - unauthorized: { - max, // 2 requests per window - window: { minutes: 2 }, // rate limit window expiration time, - }, - }, - }, - }; - const { app, barrier } = setup({ - cache: cacheMock, - config: configMock, - }); - - // exceed the rate limit for authenticated requests - for (let i = 0; i < max + 1; i += 1) { - await request(app) - .get('/') - .set('authorization', mockCredentials.user.header()) - .send() - .expect(200); - } - - barrier.addAuthPolicy({ allow: 'user-cookie', path: '/' }); - - for (let i = 0; i < max + 1; i += 1) { - await request(app) - .get('/') - .set('cookie', mockCredentials.limitedUser.cookie()) - .send() - .expect(200); - } - - jest.useRealTimers(); -}); - -it('limit the number of unauthenticated requests', async () => { - const now = Date.now(); - jest.useFakeTimers({ now }); - const cacheMock = mockServices.cache.mock(); - const twoMinutesInMilliseconds = 2 * 60 * 1000; - const resetTime = now + twoMinutesInMilliseconds; - cacheMock.get - .mockResolvedValueOnce(undefined) // Request 1 - .mockResolvedValueOnce({ totalHits: 1, resetTime }) // Request 2 - .mockResolvedValueOnce({ totalHits: 2, resetTime }) // Request 3 - .mockResolvedValueOnce(undefined); // Request 4 - - const configMock = { - backend: { - rateLimit: { - unauthorized: { - max: 2, // 2 requests per window - window: { minutes: 2 }, // rate limit window expiration time, - }, - }, - }, - }; - const { app, barrier } = setup({ - cache: cacheMock, - config: configMock, - }); - - barrier.addAuthPolicy({ allow: 'unauthenticated', path: '/public' }); - - const randomIp = '48.105.15.17'; - // Enable trust proxy to get the real IP from the X-Forwarded-For header - app.enable('trust proxy'); - await request(app) - .get('/public') - .set('X-Forwarded-For', randomIp) - .send() - .expect(200); - await request(app) - .get('/public') - .set('X-Forwarded-For', randomIp) - .send() - .expect(200); - await request(app) - .get('/public') - .set('X-Forwarded-For', randomIp) - .send() - .expect(429); - await request(app) - .get('/public') - .set('X-Forwarded-For', randomIp) - .send() - .expect(200); - - expect(cacheMock.get).toHaveBeenCalledTimes(4); - expect(cacheMock.set).toHaveBeenNthCalledWith( - 1, - `unauthorized_rate_limit_${randomIp}`, // unauthorized_rate_limit_ is the default prefix for the rate limit store - { resetTime, totalHits: 1 }, - { ttl: twoMinutesInMilliseconds }, - ); - expect(cacheMock.set).toHaveBeenNthCalledWith( - 2, - `unauthorized_rate_limit_${randomIp}`, - { resetTime, totalHits: 2 }, - { ttl: twoMinutesInMilliseconds }, - ); - expect(cacheMock.set).toHaveBeenNthCalledWith( - 3, - `unauthorized_rate_limit_${randomIp}`, - { resetTime, totalHits: 3 }, - { ttl: twoMinutesInMilliseconds }, - ); - expect(cacheMock.set).toHaveBeenNthCalledWith( - 4, - `unauthorized_rate_limit_${randomIp}`, - { resetTime, totalHits: 1 }, - { ttl: twoMinutesInMilliseconds }, - ); - - jest.useRealTimers(); -}); - -it('skip limiting requests when the rate limit is disabled', async () => { - const now = Date.now(); - jest.useFakeTimers({ now }); - const max = 2; - const cacheMock = mockServices.cache.mock(); - const twoMinutesInMilliseconds = 2 * 60 * 1000; - const resetTime = now + twoMinutesInMilliseconds; - - // Simulate that the totalHits exceeds the configured max value - cacheMock.get.mockResolvedValue({ totalHits: max + 1, resetTime }); - - const { app: app1, barrier: barrier1 } = setup({ - cache: cacheMock, - config: { - backend: { - rateLimit: { - unauthorized: { - max, - window: { minutes: 2 }, - disabled: true, - }, - }, - }, - }, - }); - - barrier1.addAuthPolicy({ allow: 'unauthenticated', path: '/public' }); - - for (let i = 0; i < max + 1; i += 1) { - await request(app1) - .get('/public') - .set('authorization', mockCredentials.user.header()) - .send() - .expect(200); - } - - // Simulate that the totalHits exceeds the default max value - cacheMock.get.mockResolvedValue({ totalHits: 61, resetTime }); - - const { app: app2, barrier: barrier2 } = setup({ - cache: cacheMock, - config: { - backend: { - rateLimit: { - unauthorized: false, - }, - }, - }, - }); - - barrier2.addAuthPolicy({ allow: 'unauthenticated', path: '/public' }); - - await request(app2).get('/public').send().expect(200); - - jest.useRealTimers(); -}); diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/createCredentialsBarrier.ts b/packages/backend-app-api/src/services/implementations/httpRouter/createCredentialsBarrier.ts index b47b926576..a69fa5a804 100644 --- a/packages/backend-app-api/src/services/implementations/httpRouter/createCredentialsBarrier.ts +++ b/packages/backend-app-api/src/services/implementations/httpRouter/createCredentialsBarrier.ts @@ -15,17 +15,12 @@ */ import { - CacheService, HttpAuthService, HttpRouterServiceAuthPolicy, RootConfigService, } from '@backstage/backend-plugin-api'; -import { durationToMilliseconds } from '@backstage/types'; import { RequestHandler } from 'express'; import { pathToRegexp } from 'path-to-regexp'; -import { rateLimit } from 'express-rate-limit'; -import { readDurationFromConfig } from '@backstage/config'; -import { RateLimitStore } from './rateLimitStore'; export function createPathPolicyPredicate(policyPath: string) { if (policyPath === '/' || policyPath === '*') { @@ -44,12 +39,11 @@ export function createPathPolicyPredicate(policyPath: string) { export function createCredentialsBarrier(options: { httpAuth: HttpAuthService; config: RootConfigService; - cache: CacheService; }): { middleware: RequestHandler; addAuthPolicy: (policy: HttpRouterServiceAuthPolicy) => void; } { - const { httpAuth, config, cache } = options; + const { httpAuth, config } = options; const disableDefaultAuthPolicy = config.getOptionalBoolean( 'backend.auth.dangerouslyDisableDefaultAuthPolicy', @@ -65,49 +59,13 @@ export function createCredentialsBarrier(options: { const unauthenticatedPredicates = new Array<(path: string) => boolean>(); const cookiePredicates = new Array<(path: string) => boolean>(); - const rateLimitConfig = config.getOptional('backend.rateLimit.unauthorized'); - - const disabled = - rateLimitConfig === false || - (typeof rateLimitConfig === 'object' && - config?.getOptionalBoolean('backend.rateLimit.unauthorized.disabled') === - true); - - const duration = - typeof rateLimitConfig === 'object' && - config?.has('backend.rateLimit.unauthorized.window') - ? readDurationFromConfig( - config.getConfig('backend.rateLimit.unauthorized.window'), - ) - : undefined; - - const windowMs = duration ? durationToMilliseconds(duration) : 1 * 60 * 1000; - - const max = - typeof rateLimitConfig === 'object' && - config?.has('backend.rateLimit.unauthorized.max') - ? config.getNumber('backend.rateLimit.unauthorized.max') - : 60; - - // Default rate limit is 60 requests per 1 minute - const limiter = rateLimit({ - windowMs, - limit: max, - standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers - legacyHeaders: false, // Disable the `X-RateLimit-*` headers, - store: RateLimitStore.fromOptions({ cache }), - skip() { - return disabled; - }, - }); - - const middleware: RequestHandler = (req, res, next) => { + const middleware: RequestHandler = (req, _, next) => { const allowsUnauthenticated = unauthenticatedPredicates.some(predicate => predicate(req.path), ); if (allowsUnauthenticated) { - limiter(req, res, next); + next(); return; } diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.ts b/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.ts index 50f9e5c97a..e72d091b96 100644 --- a/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.ts @@ -39,32 +39,20 @@ export const httpRouterServiceFactory = createServiceFactory( (options?: HttpRouterFactoryOptions) => ({ service: coreServices.httpRouter, deps: { - cache: coreServices.cache, plugin: coreServices.pluginMetadata, config: coreServices.rootConfig, lifecycle: coreServices.lifecycle, rootHttpRouter: coreServices.rootHttpRouter, httpAuth: coreServices.httpAuth, }, - async factory({ - httpAuth, - config, - cache, - plugin, - rootHttpRouter, - lifecycle, - }) { + async factory({ httpAuth, config, plugin, rootHttpRouter, lifecycle }) { const getPath = options?.getPath ?? (id => `/api/${id}`); const path = getPath(plugin.getId()); const router = PromiseRouter(); rootHttpRouter.use(path, router); - const credentialsBarrier = createCredentialsBarrier({ - httpAuth, - config, - cache, - }); + const credentialsBarrier = createCredentialsBarrier({ httpAuth, config }); router.use(createLifecycleMiddleware({ lifecycle })); router.use(credentialsBarrier.middleware); diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/rateLimitStore.test.ts b/packages/backend-app-api/src/services/implementations/httpRouter/rateLimitStore.test.ts deleted file mode 100644 index 22ab7c816d..0000000000 --- a/packages/backend-app-api/src/services/implementations/httpRouter/rateLimitStore.test.ts +++ /dev/null @@ -1,187 +0,0 @@ -/* - * Copyright 2024 The Backstage Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { ServiceMock, mockServices } from '@backstage/backend-test-utils'; -import { RateLimitStore } from './rateLimitStore'; -import { CacheService } from '@backstage/backend-plugin-api'; - -describe('RateLimitStore', () => { - let cacheServiceMock: ServiceMock; - let rateLimitStore: RateLimitStore; - - beforeEach(() => { - jest.useFakeTimers({ now: Date.parse('2024-03-15T08:39:11.869Z') }); - jest.clearAllMocks(); - cacheServiceMock = mockServices.cache.mock(); - rateLimitStore = RateLimitStore.fromOptions({ - prefix: 'rl_', - cache: cacheServiceMock, - }); - }); - - afterEach(() => { - jest.useRealTimers(); - }); - - it('should initialize with default options', () => { - expect(rateLimitStore.windowMs).toBe(1 * 60 * 1000); - }); - - it('should initialize with custom options', () => { - rateLimitStore.init({ windowMs: 10 * 60 * 1000 }); - expect(rateLimitStore.windowMs).toBe(10 * 60 * 1000); - }); - - it('should get rate limit info for existing key', async () => { - const key = 'existingKey'; - const existingValue = { - totalHits: 5, - resetTime: new Date(), - }; - - cacheServiceMock.get.mockResolvedValue({ - totalHits: existingValue.totalHits, - resetTime: existingValue.resetTime.getTime(), - }); - - const returnedValue = await rateLimitStore.get(key); - - expect(returnedValue).toEqual(existingValue); - expect(cacheServiceMock.get).toHaveBeenCalledWith( - rateLimitStore.prefixKey(key), - ); - }); - - it('should get default rate limit info for non-existing key', async () => { - const key = 'nonExistingKey'; - const defaultValue = { - totalHits: 0, - resetTime: new Date(Date.now() + rateLimitStore.windowMs), - }; - - cacheServiceMock.get.mockResolvedValue(undefined); - - const returnedValue = await rateLimitStore.get(key); - - expect(returnedValue).toEqual(defaultValue); - expect(cacheServiceMock.get).toHaveBeenCalledWith( - rateLimitStore.prefixKey(key), - ); - }); - - it('should increment rate limit for existing key', async () => { - const key = 'exisitingKey'; - const existingValue = { - totalHits: 5, - resetTime: new Date(), - }; - - cacheServiceMock.get.mockResolvedValue({ - totalHits: existingValue.totalHits, - resetTime: existingValue.resetTime.getTime(), - }); - - const incrementedValue = await rateLimitStore.increment(key); - - expect(incrementedValue.totalHits).toBe(existingValue.totalHits + 1); - expect(incrementedValue.resetTime).toEqual(existingValue.resetTime); - expect(cacheServiceMock.set).toHaveBeenCalledWith( - rateLimitStore.prefixKey(key), - { - totalHits: existingValue.totalHits + 1, - resetTime: existingValue.resetTime.getTime(), - }, - { ttl: rateLimitStore.windowMs }, - ); - }); - - it('should increment rate limit for non-existing key', async () => { - const key = 'nonExistingKey'; - - cacheServiceMock.get.mockResolvedValue(undefined); - - const incrementedValue = await rateLimitStore.increment(key); - - expect(incrementedValue.totalHits).toBe(1); - expect(incrementedValue.resetTime).toBeInstanceOf(Date); - expect(cacheServiceMock.set).toHaveBeenCalledWith( - rateLimitStore.prefixKey(key), - { - totalHits: incrementedValue.totalHits, - resetTime: incrementedValue.resetTime?.getTime(), - }, - { ttl: rateLimitStore.windowMs }, - ); - }); - - it('should decrement rate limit for existing key', async () => { - const key = 'exisitingKey'; - const existingValue = { - totalHits: 5, - resetTime: new Date(), - }; - - cacheServiceMock.get.mockResolvedValue({ - totalHits: existingValue.totalHits, - resetTime: existingValue.resetTime.getTime(), - }); - - await rateLimitStore.decrement(key); - - expect(cacheServiceMock.set).toHaveBeenCalledWith( - rateLimitStore.prefixKey(key), - { - totalHits: existingValue.totalHits - 1, - resetTime: existingValue.resetTime.getTime(), - }, - { ttl: rateLimitStore.windowMs }, - ); - }); - - it('should not decrement rate limit below zero', async () => { - const key = 'testKey'; - const existingValue = { - totalHits: 0, - resetTime: new Date(), - }; - - cacheServiceMock.get.mockResolvedValue({ - totalHits: existingValue.totalHits, - resetTime: existingValue.resetTime.getTime(), - }); - - await rateLimitStore.decrement(key); - - expect(cacheServiceMock.set).toHaveBeenCalledWith( - rateLimitStore.prefixKey(key), - { - totalHits: 0, - resetTime: existingValue.resetTime.getTime(), - }, - { ttl: rateLimitStore.windowMs }, - ); - }); - - it('should reset rate limit for existing key', async () => { - const key = 'exitingKey'; - - await rateLimitStore.resetKey(key); - - expect(cacheServiceMock.delete).toHaveBeenCalledWith( - rateLimitStore.prefixKey(key), - ); - }); -}); diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/rateLimitStore.ts b/packages/backend-app-api/src/services/implementations/httpRouter/rateLimitStore.ts deleted file mode 100644 index 6286522a2a..0000000000 --- a/packages/backend-app-api/src/services/implementations/httpRouter/rateLimitStore.ts +++ /dev/null @@ -1,119 +0,0 @@ -/* - * Copyright 2024 The Backstage Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { CacheService } from '@backstage/backend-plugin-api'; -import type { - Store, - Options, - IncrementResponse, - ClientRateLimitInfo, -} from 'express-rate-limit'; - -type CacheStoreOptions = { - /** - * Optional field to differentiate hit countswhen multiple rate-limits are in use - */ - prefix?: string; - /** - * The cache service to use for storing the hit counts. - */ - cache: CacheService; -}; - -type CacheStoreValue = { - totalHits: number; - resetTime: number; -}; - -/** - * A `Store` that stores the hit count for each client. - */ -export class RateLimitStore implements Store { - /** - * The duration of time before which all hit counts are reset (in milliseconds). - * default: 60 requests per minute - */ - windowMs: number = 1 * 60 * 1000; - prefix: string; - #cache: CacheService; - - private constructor(options: CacheStoreOptions) { - this.prefix = options.prefix ?? 'unauthorized_rate_limit_'; - this.#cache = options.cache; - } - - static fromOptions(options: CacheStoreOptions): RateLimitStore { - return new RateLimitStore(options); - } - - #getDefaultValue() { - return { totalHits: 0, resetTime: Date.now() + this.windowMs }; - } - - init(options: Partial): void { - if (options.windowMs) { - this.windowMs = options.windowMs; - } - } - - prefixKey(key: string): string { - return `${this.prefix}${key}`; - } - - async get(key: string): Promise { - const value = - (await this.#cache.get(this.prefixKey(key))) ?? - this.#getDefaultValue(); - return { - totalHits: value.totalHits, - resetTime: new Date(value.resetTime), - }; - } - - async increment(key: string): Promise { - const value = - (await this.#cache.get(this.prefixKey(key))) ?? - this.#getDefaultValue(); - const totalHits = value.totalHits + 1; - const resetTime = value.resetTime; - await this.#cache.set( - this.prefixKey(key), - { totalHits, resetTime }, - { ttl: this.windowMs }, - ); - return { - totalHits, - resetTime: new Date(resetTime), - }; - } - - async decrement(key: string): Promise { - const value = - (await this.#cache.get(this.prefixKey(key))) ?? - this.#getDefaultValue(); - const totalHits = value.totalHits > 0 ? value.totalHits - 1 : 0; - const resetTime = value.resetTime; - await this.#cache.set( - this.prefixKey(key), - { totalHits, resetTime }, - { ttl: this.windowMs }, - ); - } - - async resetKey(key: string): Promise { - await this.#cache.delete(this.prefixKey(key)); - } -} diff --git a/yarn.lock b/yarn.lock index 99797b5466..6211690757 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3247,7 +3247,6 @@ __metadata: cors: ^2.8.5 express: ^4.17.1 express-promise-router: ^4.1.0 - express-rate-limit: ^7.2.0 fs-extra: ^11.2.0 helmet: ^6.0.0 http-errors: ^2.0.0 @@ -27608,15 +27607,6 @@ __metadata: languageName: node linkType: hard -"express-rate-limit@npm:^7.2.0": - version: 7.2.0 - resolution: "express-rate-limit@npm:7.2.0" - peerDependencies: - express: 4 || 5 || ^5.0.0-beta.1 - checksum: 6adc3e06d430e91cf8ef8da23107466ffd9193b5680b03bc12702894cab0adcbb980634602b839b3a1444620f2379707098af3a4dcc38c909e89214abaf6c1bf - languageName: node - linkType: hard - "express-session@npm:^1.17.1, express-session@npm:^1.17.3": version: 1.18.0 resolution: "express-session@npm:1.18.0"