From c5709a88bcbb5646cab980fa35cb2a8ccd0f5e4e Mon Sep 17 00:00:00 2001 From: Alexey Yarmosh Date: Fri, 27 Sep 2024 18:03:16 +0200 Subject: [PATCH] feat: remove separate get rate-limit for authenticated users --- config/default.cjs | 7 ++-- config/test.cjs | 5 ++- public/v1/spec.yaml | 3 -- src/lib/rate-limiter/rate-limiter-get.ts | 41 ++++-------------------- src/measurement/route/get-measurement.ts | 4 +-- test/tests/integration/ratelimit.test.ts | 16 ++++----- 6 files changed, 20 insertions(+), 56 deletions(-) diff --git a/config/default.cjs b/config/default.cjs index cedb0803..ae285995 100644 --- a/config/default.cjs +++ b/config/default.cjs @@ -72,10 +72,9 @@ module.exports = { authenticatedLimit: 250, reset: 3600, }, - get: { - anonymousLimit: 5, - authenticatedLimit: 5, - reset: 2, + getPerMeasurement: { + limit: 5, + reset: 20, }, }, limits: { diff --git a/config/test.cjs b/config/test.cjs index 20a8fb7c..070bf678 100644 --- a/config/test.cjs +++ b/config/test.cjs @@ -31,9 +31,8 @@ module.exports = { measurement: { maxInProgressTests: 2, rateLimit: { - get: { - anonymousLimit: 1000, - authenticatedLimit: 1000, + getPerMeasurement: { + limit: 1000, }, }, }, diff --git a/public/v1/spec.yaml b/public/v1/spec.yaml index 404114db..84e35132 100644 --- a/public/v1/spec.yaml +++ b/public/v1/spec.yaml @@ -109,9 +109,6 @@ paths: $ref: 'components/responses.yaml#/components/responses/measurement429' tags: - Measurements - security: - - {} - - bearerAuth: [] /v1/probes: get: summary: List probes currently online diff --git a/src/lib/rate-limiter/rate-limiter-get.ts b/src/lib/rate-limiter/rate-limiter-get.ts index 2c57fc1a..9eb7495c 100644 --- a/src/lib/rate-limiter/rate-limiter-get.ts +++ b/src/lib/rate-limiter/rate-limiter-get.ts @@ -8,49 +8,22 @@ import type { Next } from 'koa'; const redisClient = getPersistentRedisClient(); -export const anonymousRateLimiter = new RateLimiterRedis({ +export const rateLimiter = new RateLimiterRedis({ storeClient: redisClient, - keyPrefix: 'rate:get:anon', - points: config.get('measurement.rateLimit.get.anonymousLimit'), - duration: config.get('measurement.rateLimit.get.reset'), + keyPrefix: 'rate:get', + points: config.get('measurement.rateLimit.getPerMeasurement.limit'), + duration: config.get('measurement.rateLimit.getPerMeasurement.reset'), blockDuration: 5, }); -export const authenticatedRateLimiter = new RateLimiterRedis({ - storeClient: redisClient, - keyPrefix: 'rate:get:auth', - points: config.get('measurement.rateLimit.get.authenticatedLimit'), - duration: config.get('measurement.rateLimit.get.reset'), - blockDuration: 5, -}); - -const getRateLimiter = (ctx: ExtendedContext, extraId?: string): { - type: 'user'| 'ip', - id: string, - rateLimiter: RateLimiterRedis -} => { - if (ctx.state.user?.id) { - return { - type: 'user', - id: extraId ? `${ctx.state.user.id}:${extraId}` : ctx.state.user.id, - rateLimiter: authenticatedRateLimiter, - }; - } - - const ip = requestIp.getClientIp(ctx.req) ?? ''; - return { - type: 'ip', - id: extraId ? `${ip}:${extraId}` : ip, - rateLimiter: anonymousRateLimiter, - }; -}; - export const getMeasurementRateLimit = async (ctx: ExtendedContext, next: Next) => { if (ctx['isAdmin']) { return next(); } - const { rateLimiter, id } = getRateLimiter(ctx, ctx.params['id']); + const ip = requestIp.getClientIp(ctx.req) ?? ''; + const measurementId = ctx.params['id'] ?? ''; + const id = `${ip}:${measurementId}`; try { await rateLimiter.consume(id); diff --git a/src/measurement/route/get-measurement.ts b/src/measurement/route/get-measurement.ts index 0f4e75ca..513ab3b5 100644 --- a/src/measurement/route/get-measurement.ts +++ b/src/measurement/route/get-measurement.ts @@ -1,8 +1,6 @@ import type { DefaultContext, DefaultState, ParameterizedContext } from 'koa'; import type Router from '@koa/router'; import { getMeasurementStore } from '../store.js'; -import { corsAuthHandler } from '../../lib/http/middleware/cors.js'; -import { authenticate } from '../../lib/http/middleware/authenticate.js'; import { getMeasurementRateLimit } from '../../lib/rate-limiter/rate-limiter-get.js'; import createHttpError from 'http-errors'; @@ -27,5 +25,5 @@ const handle = async (ctx: ParameterizedContext { - router.get('/measurements/:id', '/measurements/:id([a-zA-Z0-9]+)', corsAuthHandler(), authenticate(), getMeasurementRateLimit, handle); + router.get('/measurements/:id', '/measurements/:id([a-zA-Z0-9]+)', getMeasurementRateLimit, handle); }; diff --git a/test/tests/integration/ratelimit.test.ts b/test/tests/integration/ratelimit.test.ts index 7815f300..e3677660 100644 --- a/test/tests/integration/ratelimit.test.ts +++ b/test/tests/integration/ratelimit.test.ts @@ -5,7 +5,7 @@ import { expect } from 'chai'; import { getTestServer, addFakeProbe, deleteFakeProbes, waitForProbesUpdate } from '../../utils/server.js'; import nockGeoIpProviders from '../../utils/nock-geo-ip.js'; import { anonymousRateLimiter as anonymousPostRateLimiter, authenticatedRateLimiter as authenticatedPostRateLimiter } from '../../../src/lib/rate-limiter/rate-limiter-post.js'; -import { anonymousRateLimiter as anonymousGetRateLimiter, authenticatedRateLimiter as authenticatedGetRateLimiter } from '../../../src/lib/rate-limiter/rate-limiter-get.js'; +import { rateLimiter as getRateLimiter } from '../../../src/lib/rate-limiter/rate-limiter-get.js'; import { client } from '../../../src/lib/sql/client.js'; import { GP_TOKENS_TABLE } from '../../../src/lib/http/auth.js'; import { CREDITS_TABLE } from '../../../src/lib/credits.js'; @@ -49,14 +49,12 @@ describe('rate limiter', () => { afterEach(async () => { - const [ anonGetKeys, authGetKeys ] = await Promise.all([ - await redis.keys(`rate:get:anon:${clientIpv6}:*`), - await redis.keys('rate:get:auth:89da69bd-a236-4ab7-9c5d-b5f52ce09959:*'), + const [ getKeys ] = await Promise.all([ + await redis.keys(`rate:get:${clientIpv6}:*`), await anonymousPostRateLimiter.delete(clientIpv6), await authenticatedPostRateLimiter.delete('89da69bd-a236-4ab7-9c5d-b5f52ce09959'), ]); - const getKeys = [ ...anonGetKeys, ...authGetKeys ]; getKeys.length && await redis.del(getKeys); }); @@ -172,7 +170,7 @@ describe('rate limiter', () => { type: 'ping', target: 'jsdelivr.com', }).expect(202) as Response; - await anonymousGetRateLimiter.set(`${clientIpv6}:${id}`, 999, 0); + await getRateLimiter.set(`${clientIpv6}:${id}`, 999, 0); const response = await requestAgent.get(`/v1/measurements/${id}`).send().expect(200) as Response; expect(response.headers['Retry-After']).to.not.exist; @@ -220,7 +218,7 @@ describe('rate limiter', () => { type: 'ping', target: 'jsdelivr.com', }).expect(202) as Response; - await anonymousGetRateLimiter.set(`${clientIpv6}:${id}`, 1000, 0); + await getRateLimiter.set(`${clientIpv6}:${id}`, 1000, 0); const response = await requestAgent.get(`/v1/measurements/${id}`).send().expect(429) as Response; expect(response.headers['retry-after']).to.equal('5'); @@ -268,13 +266,13 @@ describe('rate limiter', () => { expect(response.headers['x-ratelimit-remaining']).to.equal('1'); }); - it('should fail and include Retry-After header (GET measurement)', async () => { + it('should fail and include Retry-After header if ip limit is applied (GET measurement)', async () => { const { body: { id } } = await requestAgent.post('/v1/measurements') .send({ type: 'ping', target: 'jsdelivr.com', }).expect(202) as Response; - await authenticatedGetRateLimiter.set(`89da69bd-a236-4ab7-9c5d-b5f52ce09959:${id}`, 1000, 0); + await getRateLimiter.set(`${clientIpv6}:${id}`, 1000, 0); const response = await requestAgent.get(`/v1/measurements/${id}`) .set('Authorization', 'Bearer qz5kdukfcr3vggv3xbujvjwvirkpkkpx') .send().expect(429) as Response;