From 9a3ca0fd10c3623abdf493bcb97a164624fa803a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Sun, 28 Jun 2026 11:52:10 -0600 Subject: [PATCH 1/2] fix(rateLimit): exempt all relation DELETE routes from IP rate limiter - Extend skip logic to cover all 10 relation/association DELETE endpoints - Add RELATION_DELETE_PATTERNS array for maintainability - Add authenticated user test variant (mirrors real call path) - Add negative assertion test for paths extending beyond relation routes - Fix comment accuracy (tokenAuth = authentication, controller = ownership) Closes #1707 --- config/rateLimit/rateLimiter.js | 43 ++++++++++- test/integration/2_utils/rateLimiter.test.js | 81 ++++++++++++++++---- 2 files changed, 107 insertions(+), 17 deletions(-) diff --git a/config/rateLimit/rateLimiter.js b/config/rateLimit/rateLimiter.js index 10d4444ff..b17935b95 100644 --- a/config/rateLimit/rateLimiter.js +++ b/config/rateLimit/rateLimiter.js @@ -89,6 +89,39 @@ const deleteMax = (req) => { return USER_DELETE_MAX; }; +/** + * Patterns matching relation/association DELETE endpoints. + * These routes only remove a link between two entities — no data is destroyed. + * They are exempt from the restrictive DELETE rate limit. + * + * Covered routes: + * DELETE /api/v1/entrances/:entranceId/cavers/:caverId + * DELETE /api/v1/caves/:caveId/organizations/:organizationId + * DELETE /api/v1/cavers/:caverId/organizations/:organizationId + * DELETE /api/v1/cavers/:caverId/groups/:groupId + * DELETE /api/v1/countries/:id/organizations/:organizationId + * DELETE /api/v1/countries/:countryId/regions/:regionId/organizations/:organizationId + * DELETE /api/v1/massifs/:id/organizations/:organizationId + * DELETE /api/v1/entrances/:entranceId/documents/:documentId + * DELETE /api/v1/caves/:caveId/documents/:documentId + * DELETE /api/v1/massifs/:massifId/documents/:documentId + */ +const RELATION_DELETE_PATTERNS = [ + /^\/api\/v1\/entrances\/\d+\/cavers\/\d+$/, + /^\/api\/v1\/caves\/\d+\/organizations\/\d+$/, + /^\/api\/v1\/cavers\/\d+\/organizations\/\d+$/, + /^\/api\/v1\/cavers\/\d+\/groups\/\d+$/, + /^\/api\/v1\/countries\/\d+\/organizations\/\d+$/, + /^\/api\/v1\/countries\/\d+\/regions\/\d+\/organizations\/\d+$/, + /^\/api\/v1\/massifs\/\d+\/organizations\/\d+$/, + /^\/api\/v1\/entrances\/\d+\/documents\/\d+$/, + /^\/api\/v1\/caves\/\d+\/documents\/\d+$/, + /^\/api\/v1\/massifs\/\d+\/documents\/\d+$/, +]; + +const isRelationDelete = (path) => + RELATION_DELETE_PATTERNS.some((pattern) => pattern.test(path)); + module.exports = { generalRateLimit: rateLimit({ windowMs: RATE_LIMIT_WINDOW_MS, @@ -134,10 +167,12 @@ module.exports = { return true; } - // Removing an entrance from a user's exploration list is a low-risk - // self-service action (tokenAuth already ensures ownership). Skip the - // restrictive DELETE rate limit so users can freely manage their list. - if (/^\/api\/v1\/entrances\/\d+\/cavers\/\d+$/.test(req.path)) { + // Relation DELETE routes only remove an association row — no entity is + // destroyed. tokenAuth ensures the caller is authenticated, and the + // controller enforces ownership/authorization. Skip the restrictive + // DELETE rate limit so users can freely manage associations. + // The general rate limiter still applies to these routes. + if (isRelationDelete(req.path)) { return true; } diff --git a/test/integration/2_utils/rateLimiter.test.js b/test/integration/2_utils/rateLimiter.test.js index f225bc1ad..58ea6da6f 100644 --- a/test/integration/2_utils/rateLimiter.test.js +++ b/test/integration/2_utils/rateLimiter.test.js @@ -292,20 +292,75 @@ describe('Rate Limiter', () => { } }); - it('should skip rate limiting for explored entrance DELETE route', async () => { - const rateLimiter = freshRateLimiter(); - const app = express(); - app.use(rateLimiter.deleteRateLimit); - app.delete( - '/api/v1/entrances/:entranceId/cavers/:caverId', - (req, res) => res.status(200).send('ok') - ); + describe('relation DELETE exemptions', () => { + const RELATION_PATHS = [ + '/api/v1/entrances/42/cavers/7', + '/api/v1/caves/1/organizations/3', + '/api/v1/cavers/5/organizations/2', + '/api/v1/cavers/5/groups/1', + '/api/v1/countries/10/organizations/4', + '/api/v1/countries/10/regions/2/organizations/4', + '/api/v1/massifs/8/organizations/3', + '/api/v1/entrances/42/documents/99', + '/api/v1/caves/1/documents/50', + '/api/v1/massifs/8/documents/12', + ]; + + it('should skip rate limiting for all relation DELETE routes (unauthenticated)', async () => { + const rateLimiter = freshRateLimiter(); + const app = express(); + app.use(rateLimiter.deleteRateLimit); + RELATION_PATHS.forEach((p) => { + app.delete(p, (req, res) => res.status(200).send('ok')); + }); + + const agent = supertest.agent(app); + for (const p of RELATION_PATHS) { + for (let i = 0; i < TEST_USER_DELETE_LIMIT + 3; i += 1) { + await agent.delete(p).expect(200); + } + } + }); - const agent = supertest.agent(app); - // Send more than the user delete limit — all should pass - for (let i = 0; i < TEST_USER_DELETE_LIMIT + 5; i += 1) { - await agent.delete('/api/v1/entrances/42/cavers/7').expect(200); - } + it('should skip rate limiting for relation DELETE routes (authenticated user)', async () => { + const rateLimiter = freshRateLimiter(); + const app = express(); + app.use((req, res, next) => { + req.token = { groups: [], nickname: 'User', id: 10 }; + next(); + }); + app.use(rateLimiter.deleteRateLimit); + RELATION_PATHS.forEach((p) => { + app.delete(p, (req, res) => res.status(200).send('ok')); + }); + + const agent = supertest.agent(app); + for (const p of RELATION_PATHS) { + for (let i = 0; i < TEST_USER_DELETE_LIMIT + 3; i += 1) { + await agent.delete(p).expect(200); + } + } + }); + + it('should still rate limit paths that extend beyond a relation route', async () => { + const rateLimiter = freshRateLimiter(); + const app = express(); + app.use(rateLimiter.deleteRateLimit); + app.delete('/api/v1/entrances/42/cavers/7/extra', (req, res) => + res.status(200).send('ok') + ); + + const agent = supertest.agent(app); + const responses = []; + for (let i = 0; i < TEST_USER_DELETE_LIMIT + 5; i += 1) { + const res = await agent.delete( + '/api/v1/entrances/42/cavers/7/extra' + ); + responses.push(res.status); + } + + should(responses.filter((s) => s === 429).length).be.above(0); + }); }); }); }); From 8a021f0b8d8ab847fda3a50e18bad9962e33406a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Sun, 28 Jun 2026 12:05:39 -0600 Subject: [PATCH 2/2] fix(rateLimit): address review feedback on PR #1710 - Add regression test for destructive DELETE route still being rate-limited - Add MAINTENANCE comment warning about pattern/route drift --- config/rateLimit/rateLimiter.js | 4 ++++ test/integration/2_utils/rateLimiter.test.js | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/config/rateLimit/rateLimiter.js b/config/rateLimit/rateLimiter.js index b17935b95..d8089d8f4 100644 --- a/config/rateLimit/rateLimiter.js +++ b/config/rateLimit/rateLimiter.js @@ -94,6 +94,10 @@ const deleteMax = (req) => { * These routes only remove a link between two entities — no data is destroyed. * They are exempt from the restrictive DELETE rate limit. * + * MAINTENANCE: When adding a new relation DELETE route in config/routes.js, + * add a corresponding pattern here so the new route is not subject to the + * restrictive 1-per-hour DELETE rate limit. + * * Covered routes: * DELETE /api/v1/entrances/:entranceId/cavers/:caverId * DELETE /api/v1/caves/:caveId/organizations/:organizationId diff --git a/test/integration/2_utils/rateLimiter.test.js b/test/integration/2_utils/rateLimiter.test.js index 58ea6da6f..f9e446e38 100644 --- a/test/integration/2_utils/rateLimiter.test.js +++ b/test/integration/2_utils/rateLimiter.test.js @@ -342,6 +342,24 @@ describe('Rate Limiter', () => { } }); + it('should still rate limit destructive DELETE routes (entity deletion)', async () => { + const rateLimiter = freshRateLimiter(); + const app = express(); + app.use(rateLimiter.deleteRateLimit); + app.delete('/api/v1/entrances/42', (req, res) => + res.status(200).send('ok') + ); + + const agent = supertest.agent(app); + const responses = []; + for (let i = 0; i < TEST_USER_DELETE_LIMIT + 5; i += 1) { + const res = await agent.delete('/api/v1/entrances/42'); + responses.push(res.status); + } + + should(responses.filter((s) => s === 429).length).be.above(0); + }); + it('should still rate limit paths that extend beyond a relation route', async () => { const rateLimiter = freshRateLimiter(); const app = express();