From 7b2dd3fab1b03966ca3ca1e13d0ce75b17c4bae8 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sun, 6 Sep 2026 22:47:37 +0200 Subject: [PATCH] fix(security): stop a gallery viewer's own image fetches spending the anonymous budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The general per-IP limiter was inert until b0f33c17 registered it ahead of the routers (budget 100, raised to 300 by 19e125d8), and 839bf4e4 then stopped gallery tokens from earning the authenticated skip. Since that release every guest has been paying for the gallery's thumbnails out of 300 requests per 15 minutes per IP. A 546-photo grid runs out mid-scroll: the remaining tiles come back 429, which the frontend turned into blank tiles with no error and no retry, and the next refresh finds the photo list limited too. Reproduced in iOS Safari against a seeded 546-photo Grid gallery: loading in bursts, then nothing, no console output, no recovery — the exact shape of the large-gallery report, whose plateaus were 125, 235, 300 and 308 tiles. A token that verifies and names the gallery in the path is now exempt on the image routes only: thumbnail, preview, hero and photo, GET only. The photo list, downloads, feedback and every write stay on the budget, a token for one gallery buys nothing on another, and the exemption rides on skip_authenticated so an operator who turns the skip off counts guests too. The 839bf4e4 concern — a free token as an unlimited budget on every /api route — stays closed; what this hands back is the bandwidth of routes a 300-request budget never bounded anyway. Relates to issue 1287 --- .../galleryImageRateLimitSkip.test.js | 83 +++++++++++++++++++ backend/src/services/rateLimitService.js | 49 ++++++++++- 2 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 backend/__tests__/middleware/galleryImageRateLimitSkip.test.js diff --git a/backend/__tests__/middleware/galleryImageRateLimitSkip.test.js b/backend/__tests__/middleware/galleryImageRateLimitSkip.test.js new file mode 100644 index 00000000..c7bab86c --- /dev/null +++ b/backend/__tests__/middleware/galleryImageRateLimitSkip.test.js @@ -0,0 +1,83 @@ +/** + * A verified gallery viewer's own image fetches do not spend the anonymous + * per-IP budget (#1287). + * + * Since gallery tokens stopped earning the authenticated skip, a guest on a + * 546-photo grid ran out of the default 300 requests per 15 minutes + * mid-scroll; the rest of the tiles came back 429 and rendered blank, and the + * next refresh found the photo list limited too. Reproduced in iOS Safari + * against a seeded gallery: loading in bursts, then nothing, no error + * anywhere. The image routes are exempt for a token that verifies and names + * the gallery in the path; everything else stays on the budget. + */ +const jwt = require('jsonwebtoken'); + +process.env.JWT_SECRET = 'gallery-image-skip-secret'; + +jest.mock('../../src/database/db', () => ({ db: jest.fn(), withRetry: (fn) => fn() })); +jest.mock('../../src/utils/logger', () => ({ error: jest.fn(), warn: jest.fn(), info: jest.fn(), debug: jest.fn() })); + +const { isOwnGalleryImageRequest, shouldSkipRateLimit } = require('../../src/services/rateLimitService'); + +const iat = Math.floor(Date.now() / 1000) - 10; +const galleryToken = (eventSlug) => jwt.sign({ type: 'gallery', eventId: 1, eventSlug, iat }, process.env.JWT_SECRET, { issuer: 'picpeak-auth' }); +const adminToken = () => jwt.sign({ type: 'admin', id: 1, iat }, process.env.JWT_SECRET, { issuer: 'picpeak-auth' }); +const req = (path, token, method = 'GET') => ({ + path, method, cookies: {}, headers: token ? { authorization: `Bearer ${token}` } : {}, +}); +const config = { enabled: true, skipAuthenticated: true, publicEndpointsOnly: false }; + +describe('a gallery viewer fetching its own images', () => { + it.each(['thumbnail', 'preview', 'hero', 'photo'])('skips the budget on /%s', (route) => { + const r = req(`/api/gallery/wedding-2026/${route}/42`, galleryToken('wedding-2026')); + expect(isOwnGalleryImageRequest(r)).toBe(true); + expect(shouldSkipRateLimit(r, config)).toBe(true); + }); + + it('also reads the per-slug gallery cookie, as the browser sends it', () => { + const r = { path: '/api/gallery/wedding-2026/thumbnail/42', method: 'GET', headers: {}, + cookies: { 'gallery_token_wedding-2026': galleryToken('wedding-2026') } }; + expect(isOwnGalleryImageRequest(r)).toBe(true); + }); +}); + +describe('everything else stays on the budget', () => { + it('the photo list, downloads and feedback', () => { + const token = galleryToken('wedding-2026'); + for (const path of ['/api/gallery/wedding-2026/photos', '/api/gallery/wedding-2026/download/42', + '/api/gallery/wedding-2026/download-all', '/api/gallery/wedding-2026/info', '/api/gallery/wedding-2026/feedback/42']) { + expect(isOwnGalleryImageRequest(req(path, token))).toBe(false); + expect(shouldSkipRateLimit(req(path, token), config)).toBe(false); + } + }); + + it('a token minted for a different gallery', () => { + const r = req('/api/gallery/wedding-2026/thumbnail/42', galleryToken('other-gallery')); + expect(isOwnGalleryImageRequest(r)).toBe(false); + expect(shouldSkipRateLimit(r, config)).toBe(false); + }); + + it('no token, a garbage token, a token under another secret, a token without a slug', () => { + const path = '/api/gallery/wedding-2026/thumbnail/42'; + const foreign = jwt.sign({ type: 'gallery', eventSlug: 'wedding-2026', iat }, 'someone-elses-secret'); + const slugless = jwt.sign({ type: 'gallery', eventId: 1, iat }, process.env.JWT_SECRET); + for (const token of [undefined, 'not-a-jwt', foreign, slugless]) { + expect(isOwnGalleryImageRequest(req(path, token))).toBe(false); + } + }); + + it('a non-GET on an image path', () => { + expect(isOwnGalleryImageRequest(req('/api/gallery/wedding-2026/photo/42', galleryToken('wedding-2026'), 'DELETE'))).toBe(false); + }); + + it('an admin token still skips everywhere, and is not what this checks', () => { + const r = req('/api/gallery/wedding-2026/thumbnail/42', adminToken()); + expect(isOwnGalleryImageRequest(r)).toBe(false); + expect(shouldSkipRateLimit(r, config)).toBe(true); + }); + + it('the operator switch skip_authenticated=false counts guests too', () => { + const r = req('/api/gallery/wedding-2026/thumbnail/42', galleryToken('wedding-2026')); + expect(shouldSkipRateLimit(r, { ...config, skipAuthenticated: false })).toBe(false); + }); +}); diff --git a/backend/src/services/rateLimitService.js b/backend/src/services/rateLimitService.js index 4a3def69..e00472bd 100644 --- a/backend/src/services/rateLimitService.js +++ b/backend/src/services/rateLimitService.js @@ -118,6 +118,8 @@ function isAuthenticated(req) { // Only an admin session earns the skip. A gallery token is minted for // free on password-less galleries and slideshow links, so treating it as // "authenticated" handed anyone an unlimited budget on every /api route. + // The one thing a gallery token does buy is its own gallery's images — + // see isOwnGalleryImageRequest below. if (decoded.type !== 'admin') { return false; } @@ -130,6 +132,46 @@ function isAuthenticated(req) { } } +// The routes a gallery viewer fetches once per tile: thumbnail, preview, +// hero, photo. Downloads, the photo list, feedback and everything else stay +// on the budget. +const GALLERY_IMAGE_RE = /^\/api\/gallery\/([^/]+)\/(thumbnail|preview|hero|photo)\/[^/]+$/i; + +/** + * A verified gallery viewer fetching that gallery's own images (#1287). + * + * Since gallery tokens stopped earning the authenticated skip, every guest + * has been spending the anonymous per-IP budget — 300 requests per 15 + * minutes by default — on the gallery's thumbnails. A 546-photo grid runs + * out of budget mid-scroll: the remaining tiles come back 429, which the + * frontend rendered as blank tiles with no error, and the next refresh + * finds the photo list rate-limited too. A per-IP limit that one legitimate + * viewer exhausts on one gallery protects nothing. + * + * So a token that verifies AND names the gallery in the path is exempt on + * the image routes only. The token proves the viewer passed whatever gate + * the gallery has (the gate itself is still limited), the slug check stops a + * token for one gallery buying images from another, and GET-only keeps every + * write on the budget. The bandwidth these routes cost was never something a + * 300-request budget bounded — a viewer can refetch a cached thumbnail 300 + * times too — so nothing is given up here that the budget actually held. + */ +function isOwnGalleryImageRequest(req) { + if (req.method && req.method !== 'GET') return false; + const match = (req.path || '').match(GALLERY_IMAGE_RE); + if (!match) return false; + const slug = match[1]; + try { + const token = getGalleryTokenFromRequest(req, slug); + if (!token) return false; + const decoded = jwt.verify(token, process.env.JWT_SECRET); + return Boolean(decoded) && typeof decoded === 'object' + && decoded.type === 'gallery' && decoded.eventSlug === slug; + } catch (error) { + return false; + } +} + /** * Determine if rate limiting should be applied to this request */ @@ -145,8 +187,10 @@ function shouldSkipRateLimit(req, config) { return false; } - // Check if we should skip authenticated requests - if (config.skipAuthenticated && isAuthenticated(req)) { + // Check if we should skip authenticated requests. A gallery viewer's own + // image fetches ride on the same switch: an operator who turns the skip + // off gets every request counted, guests included. + if (config.skipAuthenticated && (isAuthenticated(req) || isOwnGalleryImageRequest(req))) { return true; } @@ -292,5 +336,6 @@ module.exports = { createRateLimiter, createAuthRateLimiter, isAuthenticated, + isOwnGalleryImageRequest, shouldSkipRateLimit };