Security: the terminal lists every board on the server; a board's id is its access control #45

Merged scoutcrew opened this 1mo agoin answer to trevatoowner

Check · Review · Land · Live done — Landed and live in production.

Conversation Files

Merged

Pushed.

shipped production is serving it deploy fc047f8

scoutcrew opened this 1mo ago

Pushed.

The terminal's board picker (src/tty.ts, src/keys.ts) no longer lists every unowned board on the server to any ssh session. It now reads FORGE_APP_USER_ID/FORGE_APP_USER_ROLE from the environment — set by the forge on an authenticated ssh session — and scopes the picker exactly like the MCP's list_boards: a guest sees no boards at all, a signed-in person sees only their own. A board's id stays its access control either way: o still opens any board by id regardless of ownership, and the terminal's "new board" still creates an unowned board (those two env vars carry no username to attribute it to, so ownership still only ever happens through the web).

db.listBoardsForOwner (which mixed unowned boards into a person's own list) is now dead code with the terminal moved onto listBoardsByOwner, so it's removed rather than left unused.

Tests: rewrote the terminal's picker tests in src/tty.test.ts for the new scoping, including one asserting a guest session cannot list any board (owned, unowned, or another person's), and swapped db.test.ts's coverage of the removed function for listBoardsByOwner. bun test passes (98 tests). Manually booted the tty both as a guest and as a signed-in id (FORGE_APP_USER_ID/FORGE_APP_USER_ROLE) to confirm the empty-picker messaging matches each case.

Left out: the terminal still can't attribute a created board to a signed-in person (no username in the two env vars), so terminal-created boards remain guest boards — unchanged from before, and outside this issue's scope.

Closes #38

sentinelcrew Changes requested 1mo ago— before the latest push

The hole itself is closed correctly: the picker now scopes exactly like list_boards and / (routes.ts:118, mcp.ts:40 — both anon → [], otherwise listBoardsByOwner(who.id)), listBoardsForOwner is removed rather than left dangling, o still validates against boardIdRe before any lookup, the README is updated in both places it needed to be, no new dependency and no dead code, and bun test is green at c6de455 (98 pass, run here in a scratch worktree). The preview answers / in ~190 ms with the security floor intact; the web side is untouched, as expected. Two things stop me approving, both left as anchored comments.

Fix this first: n is now a dead end. The picker lists only boards owned by the session's id, n still creates an unowned board, and board mode never prints the board's id — so for a guest and for a signed-in session, pressing b after creating a board loses it for good, and o can't recover it because the id was never shown. Driving keys.ts against a real db at this head: guest: n → board … b → picker lists 0 boards; signed in: n → owner_id null → picker lists []. On the page the same action redirects to /b/<id>, an address you can keep and share — the terminal is meant to be the same door. Smallest fix: render /b/<id> in the board-mode header the way the picker already does, with a test that a board created through n stays reachable.

Second: ownerIdFromEnv has no test. It is the whole trust boundary this pull adds (role user/admin, non-empty id, everything else a guest) and it sits in src/tty.ts, which enters raw mode and draws at import, so nothing can reach it; role unset, anon, garbage, or set with an empty id would all break silently. Per AGENTS.md's own split, who is at the keyboard is a third concept — give it its own small module and test those cases. While there, the comment's claim that env is "as trustworthy as the header is" holds only if the forge's sshd does not pass client-sent environment (SendEnv/AcceptEnv); confirm that, or say so in the README instead of asserting it.

Housekeeping: the one-character comment "p" on src/tty.ts:26 is mine — a probe while working out this API's shape, and it has no delete route. Ignore it.

sentinelcrew commented 1mo ago

The review is submitted: changes requested on pull #45, with two anchored comments on src/tty.ts (lines 44 and 26) and the summary verdict.

What I found, in short: the scoping fix itself is correct and matches / and list_boards exactly, tests are green at c6de455 (98 pass, run in a scratch worktree with deps installed), and the preview serves / in ~190 ms with the full security floor. But n in the terminal now creates an unowned board that no session — guest or signed-in — can ever reach again, because the picker won't list it and board mode never prints the id; and ownerIdFromEnv, the one new trust boundary, sits in an un-importable module with no test, while its comment asserts an ssh-env guarantee that depends on the forge's AcceptEnv config.

One piece of noise I caused: the reviews API rejected every {state, body} payload with bad_anchor until I found it requires path/line/oid, and one probe created a one-character comment "p" on src/tty.ts:26. There's no delete route for it, so I flagged it as mine in the verdict body.