Rate-limit every write, not only board creation; a terminal write reaches web viewers live #64

Pushed. Here's the pull description:

Every mutating route beyond board creation (add_card, move_card, edit, delete, claim, rename, undo, and the column/card create-delete-move family) spent nothing before touching the database. A second, higher-capacity bucket — writeBucket / MAX_WRITES_PER_MINUTE in src/limits.ts — now guards that whole surface, kept separate from boardCreationBucket so a flood of one can't starve the other.

• src/routes.ts: every mutating handler spends one token from writeBucket right before its db call (a validation failure, e.g. an empty title, still costs nothing — it never reaches the db). One helper, writeRefused, decides the refusal everywhere: 429 + retry-after for the JSON path, the board re-rendered with the sentence already in it for a plain form post — the same shape boardBad already used for validation errors, now shared.
• src/mcp.ts: add_card and move_card are marked write: true; a refused call answers as a JSON-RPC tool error (isError: true, readable text) rather than an HTTP 429, so an MCP client doesn't read it as a transport failure. initialize, tools/list, list_boards, list_cards are untouched by the bucket.
• Undo spends a token too, per the issue's instruction — worth flagging: refusing an undo mid-flurry of edits is more annoying than refusing a create, since it's usually someone fixing a mistake. Left as specified rather than quietly carving out an exception; happy to special-case it if it turns out to feel wrong in use.

Testing: src/write-limit.test.ts is new. Since writeBucket is process-wide singleton state shared by every test file that talks to the real server, this file drains it deliberately and is the only one that does — its name sorts after every other test file's, so nothing that runs later needs tokens left in it. It covers a JSON write refused with retry-after, a plain form write refused with the board re-rendered (no-JS path), an MCP write refused as a readable tool error with no card written, and initialize/tools/list/list_boards still answering with the bucket empty. bun test passes (118 tests); ran it several times to check the ordering assumption doesn't flake.

Left out: the terminal-write-invisible-to-web-viewers half of the original issue scan — already split out by @atlas into #31 (blocked on #17), and the keyboard-move tests were already covered by #29's tty.test.ts/keys.ts work, so neither belongs in this run.

Closes #40

Merged main into this branch, resolving conflicts in src/mcp.ts and src/routes.ts where main's SSE/donePublish refactor (#36, #63) and this branch's writeBucket rate-limiting had touched the same call sites. Kept both: every write endpoint (undo, column rename/move/delete, card edit/move/delete) still checks writeBucket.take() before proceeding, then publishes through donePublish/EventKind as main now expects. Also dropped a stale import * as live from "./live.ts" in routes.ts — that module was deleted upstream in favor of bus.ts, and after the merge it was unused. All 132 tests pass; pushed as 37c861a.

Review submitted on pull #64 (anchored to src/mcp.ts:142 at head 37c861a), requesting changes.

What I found, verified against the head commit rather than the diff alone (bun test: 132 pass, 0 fail; preview answers / in ~140ms):

• High: writeBucket is a single unkeyed counter and /mcp spends its token before tool.run validates. Locally, an anonymous caller using board_id: "zzzzzzzz" drained all 300 tokens in 103 ms and a signed-in person's card add on their own board then got 429 retry-after: 1 — one loop freezes every write app-wide. Fix: key on who.id or server.requestIP(request) in a size-capped map, and spend the MCP token after validation as src/routes.ts already does.
• Medium: src/write-limit.test.ts is only correct because its filename sorts last; TokenBucket.reset() in an afterAll removes the ordering dependence.
• Medium: the README's ## Limits list no longer describes the floor.
• Low: expect(writeBucket).not.toBe(boardCreationBucket) asserts nothing behavioural; the no-JS refusal re-renders the whole board, so refusing costs more than the write; and the pull's title promises a terminal-live half that isn't in the diff.

The clone is unchanged and nothing was pushed.

Pushed as 2a6751d. Summary of the changes addressing the review:

1 (High, fixed): writeBucket is now a KeyedTokenBucket — one bucket per caller (forge id when signed in, socket address for a guest, never a header), capped at 10,000 keys with LRU eviction so the map stays bounded. src/mcp.ts spends its token inside each write tool right before the db.* call, after arguments validate — a bogus board_id now costs nothing and can't drain anyone. Verified by hand: 320 bogus-board /mcp calls no longer touch the bucket, and a real write from a different caller still succeeds.

2 (Medium, fixed): write-limit.test.ts now resets writeBucket in afterAll instead of relying on running last; it's safe in any file order now.

3 (Medium, fixed): README's ## Limits section describes the per-caller keying, the 10,000-key cap, and the amplification fix.

4 (Low, fixed): the tautological expect(writeBucket).not.toBe(boardCreationBucket) is now a behavioural assertion (draining one leaves the other full), plus new tests for KeyedTokenBucket (isolation, eviction, reset) and callerKey.

5 (Low, fixed): boardBad only renders the whole board for a real navigation (sec-fetch-mode: navigate); anything else (a script, a flood retrying a refusal) gets the cheap plain-text answer. Added tests for both paths.

6 (title): I agree it's wrong — the terminal-live half isn't in this diff. I tried to rename the pull via the forge API (PATCH .../pulls/64) but it returned {"error":"invalid_state"} regardless of payload shape, so I couldn't fix it programmatically; a person will need to retitle it (something like "Rate-limit every write, keyed per caller").

All 138 tests pass (bun test).

Approved pull #64 at head 2a6751d, anchored to src/limits.ts:174.

Both blocking findings from round one are fixed and I verified them by re-running the attack, not by reading the diff: 400 MCP calls naming a nonexistent board no longer drain anything (the token is spent inside the tool after validation), an anonymous flood refuses only itself, and one signed-in caller flooding does not refuse another. KeyedTokenBucket keys on the verified forge id or server.requestIP, is capped at 10k keys with LRU eviction that can only forgive, and the new tests assert those behaviours rather than object identity. write-limit.test.ts no longer depends on filename ordering. bun test is green (138 pass, 0 fail), no new dependencies, README updated. On the preview: page write 89 ms, MCP write works, both refusal shapes correct, board page ~170 ms.

Filed rather than blocked:

• #83 — a chunked POST with no content-length bypasses the 64KB cap entirely (13 MB accepted, 303); pre-existing, found while testing this pull.
• #84 — behind the forge's proxy, server.requestIP is likely the proxy's address, so guests may share one write key; still strictly better than the old app-wide bucket, and the alternative would be trusting a forwarded-for header.

I also asked in the review that the title be trimmed before landing: the "terminal write reaches web viewers live" half is not in this diff. The clone is unchanged and nothing was pushed.

Pull #64 is approved with no blocking findings this round — nothing on this branch actually needs a code change, so I'm not pushing an empty commit. Two points from the review, addressed:

Title: Tried PATCH /api/v1/repos/trevato/tempo/pulls/64 again with the title field — still {"error":"invalid_state"}, and there's no PUT/POST route for it either. The API doesn't let me rename this pull; a person will need to retitle it (something like "Rate-limit every write, keyed per caller") before merging.

*The Safari Sec-Fetch- fallback (filed, non-blocking) — I disagree with implementing it as suggested.** sameOrigin()'s fallback to Origin works because failing open there only affects a CSRF check on genuinely old browsers, which aren't the threat model for forged cross-site requests anyway. Here the gate exists to stop cost amplification: any client — Safari 15 or a flood script — can equally omit every Sec-Fetch-* header, since none of them are trustworthy signals a client can't simply not send. Falling back to "render the board" when sec-fetch-mode is absent would hand the exact amplification this fix closes right back to anyone who just doesn't send that header, which is the default for curl and every naive script. There's no header-only way to tell a real old browser from an attacker choosing not to send it, so I'm leaving the gate as strict absence-means-cheap-path rather than reintroducing the bug it fixed. Happy to revisit if #84's proxy question turns up a better signal, but for now the degraded-to-plain-text outcome for pre-16.4 Safari is the right tradeoff.

#83 and #84 are filed separately and out of scope for this pull, as the review says — no action taken here.

The pull is unchanged since my review: head is still 2a6751d, and my approval at that exact commit is already on the pull (recorded 2026-09-18T01:26Z, not outdated). Posting a second review of the same head would be duplicate noise, so I'm not submitting one.

The verdict stands, from verification at this head:

• Both round-one blockers fixed, re-tested by attack, not by reading: 400 MCP calls naming a nonexistent board drain nothing (the token is spent inside the tool, after validation); an anonymous flood refuses only itself; one signed-in caller flooding does not refuse another. callerKey keys on the forge-verified id or server.requestIP — never a header.
• KeyedTokenBucket is bounded at 10k keys with LRU eviction that can only forgive, and its tests assert that behaviour rather than object identity. write-limit.test.ts no longer depends on filename ordering (afterAll(() => writeBucket.reset())).
• bun test: 138 pass, 0 fail, no new dependencies, README's Limits section updated to describe both buckets.
• Preview: board created, card added through the page (89 ms), card added through /mcp, both refusal shapes correct (HTML for a real navigation, plain text otherwise), board page ~170 ms.

Filed rather than blocked: #83 (a chunked POST with no content-length bypasses the 64KB cap — 13 MB accepted, pre-existing) and #84 (behind the forge's proxy every guest may share one write key). I also asked that the title be trimmed before landing, since the "terminal write reaches web viewers live" half is not in this diff.

The clone is unchanged and nothing was pushed.