fix(security): the publisher code scanning alerts

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

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": {

publisher/src/actions/UnpublishClipAction.ts

@@ -39,7 +39,9 @@ export class UnpublishClipAction extends BaseAction<UnpublishClipInput, Unpublis
       await fs.unlink(thumbPath);
     } catch (err: any) {
       if (err?.code !== 'ENOENT') {
-        console.warn(`Failed to delete thumbnail ${thumbPath}:`, err.message);
+        // The path carries the uploaded filename, so it goes in as an argument:
+        // as the first one, a `%s` in a filename would be read as a directive.
+        console.warn('Failed to delete thumbnail %s: %s', thumbPath, err.message);
       }
     }
 
@@ -49,7 +51,7 @@ export class UnpublishClipAction extends BaseAction<UnpublishClipInput, Unpublis
       await fs.unlink(metaPath);
     } catch (err: any) {
       if (err?.code !== 'ENOENT') {
-        console.warn(`Failed to delete metadata ${metaPath}:`, err.message);
+        console.warn('Failed to delete metadata %s: %s', metaPath, err.message);
       }
     }
 

publisher/src/index.ts

@@ -6,6 +6,7 @@ import fs from 'node:fs';
 import { apiRouter } from './routes/index.js';
 import { errorHandler } from './middlewares/errorHandler.js';
 import { reportTokenState } from './middlewares/requireToken.js';
+import { pageLimit } from './middlewares/rateLimits.js';
 import { posterPathFor, posterUrlFor } from './utils/posterPath.js';
 import { goodBitLabel, parseGoodBits, type PublishedGoodBit } from './utils/goodBits.js';
 import { flushViewCounts, loadViewCounts, recordView } from './services/viewCounter.js';
@@ -131,7 +132,7 @@ app.use('/media', express.static(UPLOAD_DIR, {
 
 
 // Video embed page with Open Graph meta tags for Discord/social media
-app.get('/:filename', (req, res) => {
+app.get('/:filename', pageLimit, (req: express.Request<{ filename: string }>, res) => {
   /*
    * `path.basename`, because a route parameter is not a filename.
    *

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<Request, 'header' | 'ip'>): 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.' },
+});

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;
 }

publisher/src/routes/index.ts

@@ -1,14 +1,17 @@
 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();
 
 /**
  * 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);
 
 

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) {

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);
+  });
+});

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<string, string>, 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);
+  });
+});