From 8b091f77ca776c9c7c5279c2f9fa3c41f2958dc3 Mon Sep 17 00:00:00 2001 From: Hazelnoot Date: Sun, 8 Dec 2024 09:46:49 -0500 Subject: [PATCH] check for invalid rate limit inputs --- .../src/server/api/SkRateLimiterService.ts | 17 ++- .../server/api/SkRateLimiterServiceTests.ts | 141 ++++++++++++++++++ 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 { + 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 { + 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', () => {