check for invalid rate limit inputs

2 files changed, 155 insertions, 3 deletions

diff --git a/packages/backend/src/server/api/SkRateLimiterService.ts b/packages/backend/src/server/api/SkRateLimiterService.ts
index 7726edfb31..b3c09d01c2 100644
--- a/packages/backend/src/server/api/SkRateLimiterService.ts
+++ b/packages/backend/src/server/api/SkRateLimiterService.ts
@@ -131,6 +131,10 @@ export class SkRateLimiterService {
 			};
 		}
 
+		if (factor <= 0) {
+			throw new Error(`Rate limit factor is zero or negative: ${factor}`);
+		}
+
 		if (isLegacyRateLimit(limit)) {
 			return await this.limitLegacy(limit, actor, factor);
 		} else {
@@ -149,7 +153,7 @@ export class SkRateLimiterService {
 		}
 
 		// Convert the "max" limit into a leaky bucket with 1 drip / second rate.
-		if (limit.max && limit.duration) {
+		if (limit.max != null && limit.duration != null) {
 			promises.push(
 				this.limitBucket({
 					type: 'bucket',
@@ -172,6 +176,9 @@ export class SkRateLimiterService {
 	}
 
 	private async limitMin(limit: LegacyRateLimit & { minInterval: number }, actor: string, factor: number): Promise<LimitInfo | null> {
+		if (limit.minInterval === 0) return null;
+		if (limit.minInterval < 0) throw new Error(`Invalid rate limit ${limit.key}: minInterval is negative (${limit.minInterval})`);
+
 		const counter = await this.getLimitCounter(limit, actor, 'min');
 		const minInterval = Math.max(Math.ceil(limit.minInterval / factor), 0);
 
@@ -205,10 +212,14 @@ export class SkRateLimiterService {
 	}
 
 	private async limitBucket(limit: RateLimit, actor: string, factor: number): Promise<LimitInfo> {
+		if (limit.size < 1) throw new Error(`Invalid rate limit ${limit.key}: size is less than 1 (${limit.size})`);
+		if (limit.dripRate != null && limit.dripRate < 1) throw new Error(`Invalid rate limit ${limit.key}: dripRate is less than 1 (${limit.dripRate})`);
+		if (limit.dripSize != null && limit.dripSize < 1) throw new Error(`Invalid rate limit ${limit.key}: dripSize is less than 1 (${limit.dripSize})`);
+
 		const counter = await this.getLimitCounter(limit, actor, 'bucket');
 		const bucketSize = Math.max(Math.ceil(limit.size * factor), 1);
-		const dripRate = (limit.dripRate ?? 1000);
-		const dripSize = (limit.dripSize ?? 1);
+		const dripRate = Math.ceil(limit.dripRate ?? 1000);
+		const dripSize = Math.ceil(limit.dripSize ?? 1);
 
 		// Update drips
 		if (counter.c > 0) {
diff --git a/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts b/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts
index 7e0c01f849..2297c2bc03 100644
--- a/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts
+++ b/packages/backend/test/unit/server/api/SkRateLimiterServiceTests.ts
@@ -298,6 +298,90 @@ describe(SkRateLimiterService, () => {
 				expect(counter?.c).toBe(1);
 				expect(counter?.t).toBe(0);
 			});
+
+			it('should throw if factor is zero', async () => {
+				const promise = serviceUnderTest().limit(limit, actor, 0);
+
+				await expect(promise).rejects.toThrow(/factor is zero or negative/);
+			});
+
+			it('should throw if factor is negative', async () => {
+				const promise = serviceUnderTest().limit(limit, actor, -1);
+
+				await expect(promise).rejects.toThrow(/factor is zero or negative/);
+			});
+
+			it('should throw if size is zero', async () => {
+				limit.size = 0;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/size is less than 1/);
+			});
+
+			it('should throw if size is negative', async () => {
+				limit.size = -1;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/size is less than 1/);
+			});
+
+			it('should throw if size is fraction', async () => {
+				limit.size = 0.5;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/size is less than 1/);
+			});
+
+			it('should throw if dripRate is zero', async () => {
+				limit.dripRate = 0;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/dripRate is less than 1/);
+			});
+
+			it('should throw if dripRate is negative', async () => {
+				limit.dripRate = -1;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/dripRate is less than 1/);
+			});
+
+			it('should throw if dripRate is fraction', async () => {
+				limit.dripRate = 0.5;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/dripRate is less than 1/);
+			});
+
+			it('should throw if dripSize is zero', async () => {
+				limit.dripSize = 0;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/dripSize is less than 1/);
+			});
+
+			it('should throw if dripSize is negative', async () => {
+				limit.dripSize = -1;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/dripSize is less than 1/);
+			});
+
+			it('should throw if dripSize is fraction', async () => {
+				limit.dripSize = 0.5;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/dripSize is less than 1/);
+			});
 		});
 
 		describe('with min interval', () => {
@@ -451,6 +535,35 @@ describe(SkRateLimiterService, () => {
 				expect(minCounter?.c).toBe(1);
 				expect(minCounter?.t).toBe(0);
 			});
+
+			it('should throw if factor is zero', async () => {
+				const promise = serviceUnderTest().limit(limit, actor, 0);
+
+				await expect(promise).rejects.toThrow(/factor is zero or negative/);
+			});
+
+			it('should throw if factor is negative', async () => {
+				const promise = serviceUnderTest().limit(limit, actor, -1);
+
+				await expect(promise).rejects.toThrow(/factor is zero or negative/);
+			});
+
+			it('should skip if minInterval is zero', async () => {
+				limit.minInterval = 0;
+
+				const info = await serviceUnderTest().limit(limit, actor);
+
+				expect(info.blocked).toBeFalsy();
+				expect(info.remaining).toBe(Number.MAX_SAFE_INTEGER);
+			});
+
+			it('should throw if minInterval is negative', async () => {
+				limit.minInterval = -1;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/minInterval is negative/);
+			});
 		});
 
 		describe('with legacy limit', () => {
@@ -578,6 +691,34 @@ describe(SkRateLimiterService, () => {
 				expect(i1.blocked).toBeTruthy();
 				expect(i2.blocked).toBeFalsy();
 			});
+
+			it('should throw if factor is zero', async () => {
+				const promise = serviceUnderTest().limit(limit, actor, 0);
+
+				await expect(promise).rejects.toThrow(/factor is zero or negative/);
+			});
+
+			it('should throw if factor is negative', async () => {
+				const promise = serviceUnderTest().limit(limit, actor, -1);
+
+				await expect(promise).rejects.toThrow(/factor is zero or negative/);
+			});
+
+			it('should throw if max is zero', async () => {
+				limit.max = 0;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/size is less than 1/);
+			});
+
+			it('should throw if max is negative', async () => {
+				limit.max = -1;
+
+				const promise = serviceUnderTest().limit(limit, actor);
+
+				await expect(promise).rejects.toThrow(/size is less than 1/);
+			});
 		});
 
 		describe('with legacy limit and min interval', () => {