feat: support per-user rate limiting on REST endpoints

.changeset/shaky-hotels-wash.md

@@ -0,0 +1,5 @@
+---
+'@rocket.chat/meteor': minor
+---
+
+Adds support for rate limiting REST endpoints per user rather than per IP address, through the new `per` option of `rateLimiterOptions`, and applies it to `chat.sendMessage` — users connecting from a shared address no longer compete for a single message allowance.

apps/meteor/.mocharc.api.js

@@ -9,7 +9,7 @@ module.exports = /** @satisfies {import('mocha').MochaOptions} */ ({
 	timeout: 10000,
 	bail: false,
 	retries: 0,
-	file: 'tests/end-to-end/teardown.ts',
+	file: ['tests/end-to-end/setup.ts', 'tests/end-to-end/teardown.ts'],
 	reporter: 'tests/end-to-end/reporter.ts',
 	spec: ['tests/end-to-end/api/*.ts', 'tests/end-to-end/api/helpers/**/*', 'tests/end-to-end/api/methods/**/*', 'tests/end-to-end/apps/*'],
 });

apps/meteor/definition/externals/meteor/rate-limit.d.ts

@@ -2,8 +2,15 @@ declare module 'meteor/rate-limit' {
 	type RateLimiterOptionsToCheck = {
 		IPAddr: string;
 		route: string;
+		userId?: string;
 	};
 
+	type RateLimiterMatcher = (input: string) => unknown;
+
+	type RateLimiterRule = {
+		route: string;
+	} & ({ IPAddr: RateLimiterMatcher } | { userId: RateLimiterMatcher });
+
 	type RateLimiterCheckResult = {
 		allowed: boolean;
 		timeToReset: number;
@@ -15,11 +22,6 @@ declare module 'meteor/rate-limit' {
 
 		public increment(input: RateLimiterOptionsToCheck);
 
-		public addRule(
-			rule: { IPAddr: (input: any) => any; route: string },
-			numRequestsAllowed: number,
-			intervalTime: number,
-			callback?: () => void,
-		): void;
+		public addRule(rule: RateLimiterRule, numRequestsAllowed: number, intervalTime: number, callback?: () => void): string;
 	}
 }

apps/meteor/server/api/ApiClass.ts

@@ -38,9 +38,11 @@ import type {
 	GenericRouteExecutionContext,
 	TooManyRequestsResult,
 	SuccessStatusCodes,
+	RateLimiterOptions,
 } from './definition';
 import { getUserInfo } from './lib/getUserInfo';
 import { parseJsonQuery } from './lib/parseJsonQuery';
+import { buildRateLimiterInput, buildRateLimiterRule } from './rateLimiterKey';
 import type { APIActionContext } from './router';
 import { RocketChatAPIRouter } from './router';
 import { isObject } from '../../lib/utils/isObject';
@@ -127,10 +129,7 @@ interface IAPIDefaultFieldsToExclude {
 	inviteToken: number;
 }
 
-export type RateLimiterOptions = {
-	numRequestsAllowed?: number;
-	intervalTimeInMS?: number;
-};
+export type { RateLimiterOptions } from './definition';
 
 export const defaultRateLimiterOptions: RateLimiterOptions = {
 	numRequestsAllowed: settings.get<number>('API_Enable_Rate_Limiter_Limit_Calls_Default'),
@@ -260,7 +259,6 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
 		return (
 			(typeof rateLimiterOptions === 'object' || rateLimiterOptions === undefined) &&
 			Boolean(this.version) &&
-			!process.env.TEST_MODE &&
 			Boolean(defaultRateLimiterOptions.numRequestsAllowed && defaultRateLimiterOptions.intervalTimeInMS)
 		);
 	}
@@ -419,7 +417,8 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
 		return (
 			rateLimiterDictionary.hasOwnProperty(route) &&
 			settings.get<boolean>('API_Enable_Rate_Limiter') === true &&
-			(process.env.NODE_ENV !== 'development' || settings.get<boolean>('API_Enable_Rate_Limiter_Dev') === true) &&
+			((process.env.NODE_ENV !== 'development' && !process.env.TEST_MODE) ||
+				settings.get<boolean>('API_Enable_Rate_Limiter_Dev') === true) &&
 			!(userId && (await hasPermissionAsync(userId, 'api-bypass-rate-limit')))
 		);
 	}
@@ -434,8 +433,10 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
 			return;
 		}
 
-		rateLimiterDictionary[objectForRateLimitMatch.route].rateLimiter.increment(objectForRateLimitMatch);
-		const attemptResult = await rateLimiterDictionary[objectForRateLimitMatch.route].rateLimiter.check(objectForRateLimitMatch);
+		const input = buildRateLimiterInput({ ...objectForRateLimitMatch, userId });
+
+		rateLimiterDictionary[objectForRateLimitMatch.route].rateLimiter.increment(input);
+		const attemptResult = await rateLimiterDictionary[objectForRateLimitMatch.route].rateLimiter.check(input);
 		const timeToResetAttempsInSeconds = Math.ceil(attemptResult.timeToReset / 1000);
 		response.headers.set(
 			'X-RateLimit-Limit',
@@ -530,12 +531,8 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
 					rateLimiter: new RateLimiter(),
 					options: rateLimiterOptions,
 				};
-				const rateLimitRule = {
-					IPAddr: (input: any) => input,
-					route,
-				};
 				rateLimiterDictionary[route].rateLimiter.addRule(
-					rateLimitRule,
+					buildRateLimiterRule(route, rateLimiterOptions.per),
 					rateLimiterOptions.numRequestsAllowed as number,
 					rateLimiterOptions.intervalTimeInMS as number,
 				);

apps/meteor/server/api/api.ts

@@ -5,6 +5,7 @@ import type express from 'express';
 import { WebApp } from 'meteor/webapp';
 
 import { APIClass } from './ApiClass';
+import type { RateLimiterOptions } from './definition';
 import { type APIActionHandler, RocketChatAPIRouter } from './router';
 import { metrics } from '../lib/metrics';
 import { settings } from '../settings';
@@ -21,10 +22,7 @@ export type Prettify<T> = {
 	[K in keyof T]: T[K];
 } & unknown;
 
-export type RateLimiterOptions = {
-	numRequestsAllowed?: number;
-	intervalTimeInMS?: number;
-};
+export type { RateLimiterOptions } from './definition';
 
 export const defaultRateLimiterOptions: RateLimiterOptions = {
 	numRequestsAllowed: settings.get<number>('API_Enable_Rate_Limiter_Limit_Calls_Default'),

apps/meteor/server/api/definition.ts

@@ -8,6 +8,14 @@ import type { ValidateFunction } from 'ajv';
 import type { ITwoFactorOptions } from '../lib/2fa/code';
 import type { DeprecationLoggerNextPlannedVersion } from '../lib/deprecationWarningLogger';
 
+export type RateLimiterSubject = 'ip' | 'user';
+
+export type RateLimiterOptions = {
+	numRequestsAllowed?: number;
+	intervalTimeInMS?: number;
+	per?: RateLimiterSubject;
+};
+
 export type SuccessStatusCodes = Exclude<Range<208>, Range<200>>;
 
 export type RedirectStatusCodes = Exclude<Range<308>, Range<300>>;
@@ -112,12 +120,7 @@ export type SharedOptions<TMethod extends string> = (
 			authRequired?: boolean;
 			userWithoutUsername?: boolean;
 			forceTwoFactorAuthenticationForNonEnterprise?: boolean;
-			rateLimiterOptions?:
-				| {
-						numRequestsAllowed?: number;
-						intervalTimeInMS?: number;
-				  }
-				| boolean;
+			rateLimiterOptions?: RateLimiterOptions | boolean;
 			queryOperations?: string[];
 			queryFields?: string[];
 	  }
@@ -132,12 +135,7 @@ export type SharedOptions<TMethod extends string> = (
 			userWithoutUsername?: boolean;
 			twoFactorRequired: true;
 			twoFactorOptions?: ITwoFactorOptions;
-			rateLimiterOptions?:
-				| {
-						numRequestsAllowed?: number;
-						intervalTimeInMS?: number;
-				  }
-				| boolean;
+			rateLimiterOptions?: RateLimiterOptions | boolean;
 
 			queryOperations?: string[];
 			queryFields?: string[];

apps/meteor/server/api/rateLimiterKey.spec.ts

@@ -0,0 +1,77 @@
+import { buildRateLimiterInput, buildRateLimiterRule } from './rateLimiterKey';
+
+const ROUTE = '/v1/chat.sendMessagepost';
+
+const bucketOf = (rule: Record<string, unknown>) => Object.keys(rule).sort();
+
+describe('buildRateLimiterRule', () => {
+	it('should bucket by address by default', () => {
+		expect(bucketOf(buildRateLimiterRule(ROUTE))).toEqual(['IPAddr', 'route']);
+	});
+
+	it("should bucket by address when per is 'ip'", () => {
+		expect(bucketOf(buildRateLimiterRule(ROUTE, 'ip'))).toEqual(['IPAddr', 'route']);
+	});
+
+	it("should bucket by user when per is 'user'", () => {
+		expect(bucketOf(buildRateLimiterRule(ROUTE, 'user'))).toEqual(['route', 'userId']);
+	});
+
+	it('should carry the route so each endpoint counts separately', () => {
+		expect(buildRateLimiterRule(ROUTE, 'user')).toMatchObject({ route: ROUTE });
+		expect(buildRateLimiterRule(ROUTE, 'ip')).toMatchObject({ route: ROUTE });
+	});
+
+	it('should match a subject by returning it, so the package treats the rule as applicable', () => {
+		const rule = buildRateLimiterRule(ROUTE, 'user') as { userId: (input: string) => unknown };
+
+		expect(rule.userId('alice')).toBe('alice');
+	});
+});
+
+describe('buildRateLimiterInput', () => {
+	it('should carry both subjects so either rule shape matches it', () => {
+		expect(buildRateLimiterInput({ route: ROUTE, IPAddr: '1.2.3.4', userId: 'alice' })).toEqual({
+			IPAddr: '1.2.3.4',
+			userId: 'alice',
+			route: ROUTE,
+		});
+	});
+
+	it('should fall back to the address as the user subject when unauthenticated', () => {
+		expect(buildRateLimiterInput({ route: ROUTE, IPAddr: '1.2.3.4' })).toEqual({
+			IPAddr: '1.2.3.4',
+			userId: 'ip:1.2.3.4',
+			route: ROUTE,
+		});
+	});
+
+	it('should never produce a falsy subject', () => {
+		const anonymous = buildRateLimiterInput({ route: ROUTE, IPAddr: '1.2.3.4', userId: '' });
+
+		expect(anonymous.userId).toBe('ip:1.2.3.4');
+		expect(anonymous.IPAddr).toBe('1.2.3.4');
+	});
+
+	it('should keep unauthenticated subjects apart per address', () => {
+		const first = buildRateLimiterInput({ route: ROUTE, IPAddr: '1.2.3.4' });
+		const second = buildRateLimiterInput({ route: ROUTE, IPAddr: '5.6.7.8' });
+
+		expect(first.userId).not.toBe(second.userId);
+	});
+
+	it('should keep the user subject stable across addresses', () => {
+		const home = buildRateLimiterInput({ route: ROUTE, IPAddr: '1.2.3.4', userId: 'alice' });
+		const office = buildRateLimiterInput({ route: ROUTE, IPAddr: '5.6.7.8', userId: 'alice' });
+
+		expect(home.userId).toBe(office.userId);
+	});
+
+	it('should keep users on a shared address apart', () => {
+		const alice = buildRateLimiterInput({ route: ROUTE, IPAddr: '1.2.3.4', userId: 'alice' });
+		const bob = buildRateLimiterInput({ route: ROUTE, IPAddr: '1.2.3.4', userId: 'bob' });
+
+		expect(alice.userId).not.toBe(bob.userId);
+		expect(alice.IPAddr).toBe(bob.IPAddr);
+	});
+});

apps/meteor/server/api/rateLimiterKey.ts

@@ -0,0 +1,20 @@
+import type { RateLimiterOptionsToCheck, RateLimiterRule } from 'meteor/rate-limit';
+
+import type { RateLimiterSubject } from './definition';
+
+export const buildRateLimiterRule = (route: string, per: RateLimiterSubject = 'ip'): RateLimiterRule =>
+	per === 'user' ? { userId: (input: string) => input, route } : { IPAddr: (input: string) => input, route };
+
+export const buildRateLimiterInput = ({
+	route,
+	IPAddr,
+	userId,
+}: {
+	route: string;
+	IPAddr: string;
+	userId?: string;
+}): RateLimiterOptionsToCheck => ({
+	IPAddr,
+	route,
+	userId: userId || `ip:${IPAddr}`,
+});

apps/meteor/server/api/v1/chat.ts

@@ -25,6 +25,7 @@ import {
 	isChatGetStarredMessagesProps,
 	isChatGetDiscussionsProps,
 	validateBadRequestErrorResponse,
+	validateTooManyRequestsErrorResponse,
 	validateUnauthorizedErrorResponse,
 } from '@rocket.chat/rest-typings';
 import { escapeRegExp } from '@rocket.chat/tools';
@@ -895,6 +896,7 @@ const chatEndpoints = API.v1
 		'chat.sendMessage',
 		{
 			authRequired: true,
+			rateLimiterOptions: { numRequestsAllowed: 5, intervalTimeInMS: 1000, per: 'user' },
 			body: isChatSendMessageProps,
 			response: {
 				200: ajv.compile<{ message: IMessage }>({
@@ -908,6 +910,7 @@ const chatEndpoints = API.v1
 				}),
 				400: validateBadRequestErrorResponse,
 				401: validateUnauthorizedErrorResponse,
+				429: validateTooManyRequestsErrorResponse,
 			},
 		},
 		async function action() {

apps/meteor/tests/end-to-end/api/rate-limiter.ts

@@ -0,0 +1,79 @@
+import type { Credentials } from '@rocket.chat/api-client';
+import type { IUser } from '@rocket.chat/core-typings';
+import { expect } from 'chai';
+import { after, before, describe, it } from 'mocha';
+
+import { api, credentials, request } from '../../data/api-data';
+import { updateSetting } from '../../data/permissions.helper';
+import type { TestUser } from '../../data/users.helper';
+import { createUser, deleteUser, login } from '../../data/users.helper';
+
+const PASSWORD = 'rate-limiter-spec';
+
+describe('[Rate Limiter]', () => {
+	let alice: TestUser<IUser>;
+	let bob: TestUser<IUser>;
+	let aliceCredentials: Credentials;
+	let bobCredentials: Credentials;
+
+	before(async function () {
+		[alice, bob] = await Promise.all([
+			createUser({ password: PASSWORD } as Partial<IUser>),
+			createUser({ password: PASSWORD } as Partial<IUser>),
+		]);
+		[aliceCredentials, bobCredentials] = await Promise.all([login(alice.username, PASSWORD), login(bob.username, PASSWORD)]);
+
+		await updateSetting('API_Enable_Rate_Limiter_Dev', true);
+
+		const probe = await request
+			.post(api('chat.sendMessage'))
+			.set(aliceCredentials)
+			.send({ message: { rid: 'GENERAL', msg: 'probe' } });
+		if (!probe.headers['x-ratelimit-limit']) {
+			this.skip();
+		}
+	});
+
+	after(async () => {
+		await updateSetting('API_Enable_Rate_Limiter_Dev', false);
+		await Promise.all([deleteUser(alice), deleteUser(bob)]);
+	});
+
+	describe('per user', () => {
+		const send = (who: Credentials, msg: string) =>
+			request
+				.post(api('chat.sendMessage'))
+				.set(who)
+				.send({ message: { rid: 'GENERAL', msg } });
+
+		it('should reject a user past the endpoint allowance', async () => {
+			const statuses = await Promise.all([...Array(12)].map((_, i) => send(aliceCredentials, `burst ${i}`).then((res) => res.status)));
+
+			expect(statuses).to.include(200);
+			expect(statuses).to.include(429);
+		});
+
+		it('should leave another user on the same address with a full allowance', async () => {
+			const res = await send(bobCredentials, 'bob');
+
+			expect(res.status).to.equal(200);
+			expect(res.headers['x-ratelimit-remaining']).to.equal('4');
+		});
+	});
+
+	describe('bypass', () => {
+		it('should not limit a user holding api-bypass-rate-limit', async () => {
+			const statuses = await Promise.all(
+				[...Array(12)].map((_, i) =>
+					request
+						.post(api('chat.sendMessage'))
+						.set(credentials)
+						.send({ message: { rid: 'GENERAL', msg: `admin burst ${i}` } })
+						.then((res) => res.status),
+				),
+			);
+
+			expect(statuses).to.deep.equal(Array(12).fill(200));
+		});
+	});
+});

apps/meteor/tests/end-to-end/setup.ts

@@ -0,0 +1,19 @@
+import type { ISetting } from '@rocket.chat/core-typings';
+import { after, before } from 'mocha';
+
+import { getCredentials } from '../data/api-data';
+import { getSettingValueById, updateSetting } from '../data/permissions.helper';
+
+let rateLimiterInDevWasEnabled: ISetting['value'];
+
+before(async () => {
+	await new Promise<void>((resolve, reject) => getCredentials((err?: Error) => (err ? reject(err) : resolve())));
+	rateLimiterInDevWasEnabled = await getSettingValueById('API_Enable_Rate_Limiter_Dev');
+	await updateSetting('API_Enable_Rate_Limiter_Dev', false);
+});
+
+after(async () => {
+	if (rateLimiterInDevWasEnabled !== undefined) {
+		await updateSetting('API_Enable_Rate_Limiter_Dev', rateLimiterInDevWasEnabled);
+	}
+});