From c9a49717d7e2b1df60bfbd2f320c410306df32b8 Mon Sep 17 00:00:00 2001 From: Darrell van Swinderen Date: Tue, 22 Sep 2026 23:23:00 +0200 Subject: [PATCH] fix(security): the publisher code scanning alerts Closes #25. **Rate limits, keyed on the visitor rather than the proxy.** The embed page and the API both read the disk on every request, and nothing capped either. `middlewares/rateLimits.ts` on `express-rate-limit`: - The public embed page: 300 a minute per visitor. - The API: **only failures count**, 60 a minute, and the limit runs before the token check so a wrong guess is one. A batch publish of short clips over a LAN is several good requests a second, and a ceiling on those would eventually refuse the owner. - `/media` is not limited: a video that seeks is a burst of Range requests, and the edge answers most of them. The key is `CF-Connecting-IP`, then the first `X-Forwarded-For` hop, then the socket. This runs behind a reverse proxy, so `req.ip` is the proxy for every request and a limit keyed on it would be one bucket for the whole internet. Forging either header buys a fresh bucket, which is no worse than no limit, and never gets anybody in. **The bearer header is read by hand.** `/^Bearer\s+(.+)$/` let `\s+` and `.+` fight over the same spaces, quadratic on the one header anybody on the internet can send. A test holds the new parse under 100 ms on 200,000 spaces. **Two log lines no longer use the filename as a format string.** A `%s` in an uploaded name would have been read as a directive. Gates: `tests/unit/publisher`, 12 new cases. `publisher-views-check.mjs` gained three checks (the 61st wrong guess is a 429, another visitor still gets in, 80 good requests in a row are all answered), 20 in all, exit 0. `discord-webhook-check.mjs` exit 0. `npm run check`: 689 tests. Co-Authored-By: Claude Opus 5.5 (1M context) --- CLAUDE.md | 8 +++ publisher/package-lock.json | 29 +++++++++ publisher/package.json | 1 + publisher/src/actions/UnpublishClipAction.ts | 6 +- publisher/src/index.ts | 3 +- publisher/src/middlewares/rateLimits.ts | 67 ++++++++++++++++++++ publisher/src/middlewares/requireToken.ts | 11 +++- publisher/src/routes/index.ts | 5 +- scripts/publisher-views-check.mjs | 31 +++++++++ tests/unit/publisher/publishToken.spec.ts | 28 ++++++++ tests/unit/publisher/rateLimits.spec.ts | 34 ++++++++++ 11 files changed, 217 insertions(+), 6 deletions(-) create mode 100644 publisher/src/middlewares/rateLimits.ts create mode 100644 tests/unit/publisher/rateLimits.spec.ts diff --git a/CLAUDE.md b/CLAUDE.md index 5e6708a9..cc2b7606 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -890,6 +890,14 @@ trim of a published clip unpublishes and publishes again within seconds and woul two messages per trim. The webhook URL is a secret in the same class as `PUBLISH_TOKEN` and is never logged; `scripts/discord-webhook-check.mjs` greps the log for it. +**Rate limits key on a header, never on `req.ip`.** The publisher runs behind a reverse proxy and +often Cloudflare, so `req.ip` is the proxy for every request, and a limit keyed on it is one bucket +for the whole internet. `middlewares/rateLimits.ts` reads `CF-Connecting-IP`, then the first +`X-Forwarded-For` hop. The embed page allows 300 a minute; the API counts **only failures**, 60 a +minute, because a batch publish over a LAN is several successful requests a second and a ceiling on +those would eventually refuse the owner. `/media` is not limited: a seeking video is a burst of +Range requests. + `cachePrewarm.ts` asks for a clip's own public URLs once after publishing, so the first viewer, who is usually whoever just pressed Publish, does not pay for the miss. It waits for the purge in front of it, it drains the body because an edge that has not finished receiving an object does not store diff --git a/publisher/package-lock.json b/publisher/package-lock.json index 8249d2be..89dab1be 100644 --- a/publisher/package-lock.json +++ b/publisher/package-lock.json @@ -11,6 +11,7 @@ "cors": "^2.8.6", "dotenv": "^17.4.2", "express": "^5.2.1", + "express-rate-limit": "^8.7.0", "multer": "^2.4.0" }, "devDependencies": { @@ -1362,6 +1363,25 @@ "url": "https://opencollective.com/express" } }, + "node_modules/express-rate-limit": { + "version": "8.7.0", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.7.0.tgz", + "integrity": "sha512-hOwV7WOxXfjRpAM1DSJWZDXx3GhplwD8IfwuwvogD8i1Qnkgosw/H45s4ZnFAUHDAhPjlY9hLBvJhKmGMyY26g==", + "license": "MIT", + "dependencies": { + "debug": "^4.4.3", + "ip-address": "^10.2.0" + }, + "engines": { + "node": ">= 16" + }, + "funding": { + "url": "https://github.com/sponsors/express-rate-limit" + }, + "peerDependencies": { + "express": ">= 4.11" + } + }, "node_modules/express/node_modules/media-typer": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/media-typer/-/media-typer-1.1.1.tgz", @@ -1609,6 +1629,15 @@ "integrity": "sha512-k/vGaX4/Yla3WzyMCvTQOXYeIHvqOKtnqBduzTHpzpQZzAskKMhZ2K+EnBiSM9zGSoIFeMpXKxa4dYeZIQqewQ==", "license": "ISC" }, + "node_modules/ip-address": { + "version": "10.7.2", + "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.7.2.tgz", + "integrity": "sha512-7H/2gFSIitxc0hG3nOI1glS8QLo/EHBFFLk8vEUjXY/xu0AdL8jZ9U1IzO2PUm0d2D/ofQcAifb0g6OBkt8U7w==", + "license": "MIT", + "engines": { + "node": ">= 12" + } + }, "node_modules/ipaddr.js": { "version": "1.9.1", "resolved": "https://registry.npmjs.org/ipaddr.js/-/ipaddr.js-1.9.1.tgz", diff --git a/publisher/package.json b/publisher/package.json index fa20ffbf..f830dd99 100644 --- a/publisher/package.json +++ b/publisher/package.json @@ -12,6 +12,7 @@ "cors": "^2.8.6", "dotenv": "^17.4.2", "express": "^5.2.1", + "express-rate-limit": "^8.7.0", "multer": "^2.4.0" }, "devDependencies": { diff --git a/publisher/src/actions/UnpublishClipAction.ts b/publisher/src/actions/UnpublishClipAction.ts index 26b7f0ba..1bf8fd9a 100644 --- a/publisher/src/actions/UnpublishClipAction.ts +++ b/publisher/src/actions/UnpublishClipAction.ts @@ -39,7 +39,9 @@ export class UnpublishClipAction extends BaseAction { +app.get('/:filename', pageLimit, (req: express.Request<{ filename: string }>, res) => { /* * `path.basename`, because a route parameter is not a filename. * diff --git a/publisher/src/middlewares/rateLimits.ts b/publisher/src/middlewares/rateLimits.ts new file mode 100644 index 00000000..737c8be9 --- /dev/null +++ b/publisher/src/middlewares/rateLimits.ts @@ -0,0 +1,67 @@ +import type { Request } from 'express'; +import { ipKeyGenerator, rateLimit } from 'express-rate-limit'; + +/** + * How often one visitor may ask this server to read its own disk. + * + * The embed page and the API both touch the filesystem on every request, the + * page for a sidecar and the API for a directory listing or an upload, so + * without a ceiling one script in a loop is enough to keep the disk busy for + * everybody else. `/media` is deliberately not limited: a video that seeks is + * a burst of Range requests, and the edge answers most of them anyway. + * + * **Who "one visitor" is has to be read from a header.** This runs behind a + * reverse proxy, often behind Cloudflare as well, so `req.ip` is the proxy for + * every request and a limit keyed on it would be one bucket for the whole + * internet: the first busy evening would lock every viewer out at once. + * `CF-Connecting-IP` is Cloudflare's own, then the first `X-Forwarded-For` + * hop, then the socket. + * + * Both headers can be forged by anyone who can reach the origin directly, and + * that is accepted: forging one buys a fresh bucket, which is the same as no + * limit, which is where this started. It never lets anybody in; the token does + * that. + */ +export function visitorKey(req: Pick): string { + const cloudflare = req.header('cf-connecting-ip')?.trim(); + const forwarded = req.header('x-forwarded-for')?.split(',')[0]?.trim(); + const ip = cloudflare || forwarded || req.ip || 'unknown'; + // Groups an IPv6 address by its /56, since one household holds a whole range. + return ipKeyGenerator(ip); +} + +const shared = { + windowMs: 60_000, + standardHeaders: 'draft-8', + legacyHeaders: false, + keyGenerator: visitorKey, + // The key is read from the forwarding headers on purpose, above; the + // library's own check assumes `trust proxy` and would warn on every request. + validate: { xForwardedForHeader: false }, +} as const; + +/** + * The public embed page. Five a second, sustained for a minute, is far past + * anybody pressing reload, and a Discord unfurl is one request. + */ +export const pageLimit = rateLimit({ + ...shared, + limit: 300, + message: { status: 429, code: 'TOO_MANY_REQUESTS', message: 'Too many requests, slow down.' }, +}); + +/** + * The API, which the desktop drives, and where only failures count. + * + * A batch publish of short clips over a LAN is an upload, a poster and a + * metadata write per clip, several a second, so any ceiling on every request + * would eventually refuse the owner. What needs bounding is somebody without + * the token, and every one of their requests is a 401: sixty wrong answers a + * minute is a long way past a mistyped token and a long way short of guessing. + */ +export const apiLimit = rateLimit({ + ...shared, + limit: 60, + skipSuccessfulRequests: true, + message: { status: 429, code: 'TOO_MANY_REQUESTS', message: 'Too many requests, slow down.' }, +}); diff --git a/publisher/src/middlewares/requireToken.ts b/publisher/src/middlewares/requireToken.ts index 8eb95df0..93a2ebc3 100644 --- a/publisher/src/middlewares/requireToken.ts +++ b/publisher/src/middlewares/requireToken.ts @@ -22,8 +22,15 @@ function digest(value: string): Buffer { function presented(header: string | undefined, fallback: string | undefined): string | null { if (header) { - const bearer = /^Bearer\s+(.+)$/i.exec(header.trim()); - if (bearer) return bearer[1].trim(); + // Read by hand rather than with `/^Bearer\s+(.+)$/`, where `\s+` and `.+` + // can both take the same spaces and a long run of them is quadratic. This + // header is the one part of a request anybody on the internet can send. + const value = header.trim(); + const scheme = value.slice(0, 6); + if (scheme.toLowerCase() === 'bearer' && /\s/.test(value.charAt(6))) { + const token = value.slice(7).trim(); + if (token) return token; + } } return fallback?.trim() || null; } diff --git a/publisher/src/routes/index.ts b/publisher/src/routes/index.ts index baf229c7..674767d4 100644 --- a/publisher/src/routes/index.ts +++ b/publisher/src/routes/index.ts @@ -1,6 +1,7 @@ import express from 'express'; import { publishRouter } from './publish.js'; import { requireToken } from '../middlewares/requireToken.js'; +import { apiLimit } from '../middlewares/rateLimits.js'; export const apiRouter = express.Router(); @@ -8,7 +9,9 @@ export const apiRouter = express.Router(); * Everything under `/api/publish` writes to the disk, so everything under it * needs the token. Reading stays open: `/media/...` and the embed page are the * whole point of the thing. + * + * The limit runs before the token, so a wrong guess counts against it too. */ -apiRouter.use('/publish', requireToken, publishRouter); +apiRouter.use('/publish', apiLimit, requireToken, publishRouter); diff --git a/scripts/publisher-views-check.mjs b/scripts/publisher-views-check.mjs index bc506e77..64e95b21 100644 --- a/scripts/publisher-views-check.mjs +++ b/scripts/publisher-views-check.mjs @@ -326,6 +326,37 @@ ok('and carry on from where they were', seen.clips[0].views === 3, String(seen.c // dashboard would reset itself on every restart and never let a suggestion through. ok('and counting still began when it began', seen.countingSince === firstStarted, String(seen.countingSince)); +/* + * A ceiling on guessing. The API allows 60 failures a minute per visitor, and a + * wrong token is one, since the limit runs before the token check. A + * different visitor, by Cloudflare's header, still gets in: a limit keyed on + * the proxy would be one bucket for the whole internet. + */ +console.log('\nrate limits'); +let lastGuess = 0; +for (let i = 0; i < 61; i++) { + const response = await fetch(`${base}/api/publish/stats`, { + headers: { Authorization: 'Bearer wrong', 'CF-Connecting-IP': '203.0.113.9' }, + }); + lastGuess = response.status; + await response.body?.cancel(); +} +ok('the 61st wrong guess in a minute is a 429', lastGuess === 429, String(lastGuess)); +const otherVisitor = await fetch(`${base}/api/publish/stats`, { + headers: { Authorization: `Bearer ${TOKEN}`, 'CF-Connecting-IP': '203.0.113.10' }, +}); +ok('and somebody else is not locked out by it', otherVisitor.status === 200, String(otherVisitor.status)); +// The owner's own traffic never counts, or a batch publish would lock them out. +let lastOwn = 0; +for (let i = 0; i < 80; i++) { + const response = await fetch(`${base}/api/publish/stats`, { + headers: { Authorization: `Bearer ${TOKEN}`, 'CF-Connecting-IP': '203.0.113.10' }, + }); + lastOwn = response.status; + await response.body?.cancel(); +} +ok('and eighty good requests in a row are all answered', lastOwn === 200, String(lastOwn)); + await stop(server); if (failures.length) { diff --git a/tests/unit/publisher/publishToken.spec.ts b/tests/unit/publisher/publishToken.spec.ts index cdef0c1b..3e5d0786 100644 --- a/tests/unit/publisher/publishToken.spec.ts +++ b/tests/unit/publisher/publishToken.spec.ts @@ -152,3 +152,31 @@ describe('what an upload gets once the token is set', () => { expect(fakeExchange().sent.status).toBe(401); }); }); + +describe('reading the bearer header by hand', () => { + beforeEach(() => { + process.env.PUBLISH_TOKEN = 'the-real-token'; + }); + + it('takes any case and any run of whitespace after the scheme', () => { + expect(fakeExchange({ authorization: 'bearer the-real-token' }).next).toHaveBeenCalledOnce(); + expect(fakeExchange({ authorization: 'Bearer\tthe-real-token' }).next).toHaveBeenCalledOnce(); + expect(fakeExchange({ authorization: ' Bearer the-real-token ' }).next).toHaveBeenCalledOnce(); + }); + + it('does not read a scheme glued to the token', () => { + expect(fakeExchange({ authorization: 'Bearerthe-real-token' }).sent.status).toBe(401); + }); + + it('falls back to the other header when the bearer is empty', () => { + const { next } = fakeExchange({ authorization: 'Bearer ', 'x-publish-token': 'the-real-token' }); + expect(next).toHaveBeenCalledOnce(); + }); + + it('is linear on the header that made the regex quadratic', () => { + const hostile = `Bearer${' '.repeat(100_000)}x${' '.repeat(100_000)}`; + const started = performance.now(); + expect(fakeExchange({ authorization: hostile }).sent.status).toBe(401); + expect(performance.now() - started).toBeLessThan(100); + }); +}); diff --git a/tests/unit/publisher/rateLimits.spec.ts b/tests/unit/publisher/rateLimits.spec.ts new file mode 100644 index 00000000..e840b6da --- /dev/null +++ b/tests/unit/publisher/rateLimits.spec.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from 'vitest'; +import { visitorKey } from '../../../publisher/src/middlewares/rateLimits.js'; + +/** + * Who one visitor is, behind a proxy. + * + * `req.ip` is the proxy for every request, so a limit keyed on it is one bucket + * for the whole internet. These hold the order the headers are read in. + */ +function request(headers: Record, ip = '172.17.0.1') { + return { ip, header: (name: string) => headers[name.toLowerCase()] } as never; +} + +describe('visitorKey', () => { + it("prefers Cloudflare's own header", () => { + expect(visitorKey(request({ 'cf-connecting-ip': '203.0.113.7', 'x-forwarded-for': '198.51.100.1' }))).toBe( + '203.0.113.7', + ); + }); + + it('then the first forwarded hop, not the proxy', () => { + expect(visitorKey(request({ 'x-forwarded-for': '198.51.100.1, 172.17.0.1' }))).toBe('198.51.100.1'); + }); + + it('then the socket', () => { + expect(visitorKey(request({}))).toBe('172.17.0.1'); + }); + + it('groups an IPv6 household into one bucket', () => { + const a = visitorKey(request({ 'cf-connecting-ip': '2001:db8:1:2::1' })); + const b = visitorKey(request({ 'cf-connecting-ip': '2001:db8:1:2::ffff' })); + expect(a).toBe(b); + }); +});