Repository navigation
LIM-2424: fix every finding from the adversarial review of #5 (stacked on #5) - #6
liminalshruti wants to merge 9 commits into
Conversation
…e (agent epoch, 7-day expiry, Restart revokes) H1 from the adversarial review of PR #5. An agent token was treated as the consent itself, so Restart erased the consent record while the token kept capturing, and nothing in the app could revoke a token. - Capture through an agent token requires hasCaptureConsent for its harness and repo, checked under the workspace write lock, and never adopts a session another repo started. - auth.json gains agent_epoch (falls back to epoch), so revoking agents never signs people out. - Agent tokens carry exp (7 days); tokens without exp are refused. - Restart revokes every agent token (agent epoch bump) along with consents and captured sessions. - DELETE /api/workspaces/agent-token (workspace leadership, JSON, same origin): removes one repo's consent and captured sessions, or all with {}, and bumps the agent epoch.
…ds Sean's approval) H2 from the adversarial review of PR #5. npm audit: next 16.0.0-16.4.0 carries two critical unauthenticated RCE advisories (GHSA-2464-fhr4-w9xw image optimizer + SVG; GHSA-673q-mjm6-r5xr use cache typed arrays) and a high DoS (GHSA-p3m5-59mp-jv5r Server Actions). Patch-level bump; npm audit --omit=dev is clean afterwards. Dependency change per the Build A plan section 5: flagged for Sean.
…t starve creation M1 from the adversarial review of PR #5 (reproduced: a create during a wrong-passcode flood got 429, then its single-use invite answered 'already used' with no workspace). - createWorkspace hashes and writes everything first (hashing queues for a slot, never 429), then claims the invite, takes the rate token and renames; any failure after the claim gives the invite back. A spent or wrong code is refused before a rate token is taken. - Hosted: at most 2 sign-ins in flight per client, correct passcodes included, so one client can't fill the shared scrypt pool. Mode L has one client key and is not capped. - Hosted client key: IPv6 Fly-Client-IP collapses to its /64. - Backoff keys only for a real workspace and a real role (junk role strings can't evict entries). - One message for a wrong and a spent invite, so a guess is never confirmed.
…e; acks capped; bad token is 401 M2, M3 and F7 from the adversarial review of PR #5 (reproduced: a codex/repoB token acked repoA's session; 25 large acks pushed a workspace past 1 MB so the product team's proposal got 413; a bad agt. token on /api/directives read the default store). - directives poll, ack and stream: a session of another harness or repo is 403 for an agent token. - MCP align_get_work_map and align_get_directives: an agent token sees only its own harness and repo's sessions. - Ack: 4 KB capped JSON, id-shaped fields, and markDelivered records only an authority event that exists. - A request carrying an agt. token that no longer verifies is 401 on poll, ack and stream (as capture already was), never the default store. - Workspace streams no longer write the default store's listening map (their session ids leaked into Northstar's GET /api/sessions).
… guards compare the port M4 from the adversarial review of PR #5 (reproduced: with a Northstar session, a text/plain POST /api/reset with Origin http://localhost:5173 returned 200). Item 14 had only covered custom workspaces. - requireSameOrigin: an Origin, when a browser sends one, must equal this server's Host including the port, and a request with a body must be JSON. Applied to bodyFor's default-store branch (reset, timer, proposals) and to the routes that parse with body(): session (login CSRF), alike, mappings, org/push, sessions/consent. Scripts send no Origin and JSON bodies, so Northstar is unchanged. - assertLocal and the capture route's origin guard accept only this server's own origin (loopback name and same host and port), not any localhost page.
…n date enforced, crash leftovers swept M5, M6 and M10 from the adversarial review of PR #5. - M5 (reproduced: ~135 B per Restart, 1,060 Restarts in 3 s, archive unbounded): the audit archive rotates at 1 MB to audit-archive.1.jsonl, so a workspace holds at most two archive files; custom Restart and connecting an agent are limited to 10 per minute per workspace. Northstar reset is unchanged. - M6 (reproduced: a server with ALIGN_DELETE_BY=2020-01-01 started and served testers): a hosted server refuses to start once the date has passed, and a running one answers 410 on every tester route from that day. fly.toml turns off scheduled volume snapshots so data doesn't outlive the date. - M10: .tmp-<id> and .deleting-<id> crash leftovers are removed at startup and by ws-admin delete --all.
…ading; test-server notice server-rendered M7, M8 and M9 from the adversarial review of PR #5. - M7: the page sends x-align-workspace with every write; requireWsSession answers 409 when the browser's session is for a different workspace (signed in elsewhere in another tab). The page then stops showing the old workspace and shows the Elsewhere notice. - M8: when /api/session fails after a workspace or role change, "Loading workspace…" becomes the error with a Try again button instead of hanging. - M9: /new decides the test-server notice ("don't enter confidential details… deleted by <date>") in a server component, so it shows even when the config fetch fails. /setup no longer reads an error body as a session.
…rets out of argv Lows from the adversarial review of PR #5 (the backoff key, invite oracle, bad agent token and listening-map Lows landed in the earlier commits). - data/state.json is written 0600, like workspace state. - scripts/agent-session.sh passes the workspace passcode, the leadership passcode and the agent token to node through one process's environment, and sends sign-in bodies from a file, so none of them appear in ps output.
…; docs match the code - test-tenancy: Restart revokes agents (H1); leadership disconnect through DELETE /api/workspaces/agent-token, product 403 (W6, replacing the hand-edited auth.json epoch); a create during the sign-in burst (M1); junk and oversize acks (M2); another harness/repo's token on poll, ack and the MCP work map, and a bad agent token on directives (M3); cross-port Northstar reset and directives (M4); Restart loop limit and archive rotation (M5); a passed deletion date refused at startup (M6); x-align-workspace mismatch 409 (M7). - Made real: H1 refusals must exit or answer only 503; U checks fail without Chrome; the U2 leftover marker is first asserted on the signed-in page (the old one never appeared); W5 checks the recorded event id; S9 checks the refusal names the key; test-image needs a real listing before "no forbidden names" counts. - /new wraps its config read in Suspense (Cache Components prerender). - ALIGN_PUBLIC is in hostedProblems too, so proxy.ts's 503 fallback covers it. - HOSTING.md: open proposal routes, which variables are refused at any value, deletion-date enforcement, H1, the fly.toml snapshot setting, test order. MCP.md: grant scope, expiry, disconnect and Restart revocation.
allsmog
left a comment
allsmog
left a comment
There was a problem hiding this comment.
Security review of record, PR #6 @ 13f41b2
Status: changes requested. Do not merge. Most of the 9cd21ead findings are closed, and the H1 core is solid. This head still has two new Medium regressions, one M4 route it missed, and one go-C blocker in the purge path. The PR title says it fixes every finding; it does not yet.
Method: two independent read-only passes (architecture/security fidelity and cross-module seams), then I verified every finding below myself in the code at this head. Gate run: fresh npm ci on this head, scripts/test-all.sh 3247 15/15 PASS (test-tenancy 133/133, ui-northstar 72/72 and Northstar snapshot identical to 850971e, test-scenario 57/57 on all three fixtures). The gate is green; every finding below is one the gate does not test.
What closes (verified in code)
| Finding | Grade | Evidence |
|---|---|---|
| H1 consent / revoke | Resolved for the local merge | Capture rechecks consent under the write lock (app/api/sessions/events/route.ts:211-217). Tokens carry exp, 7 days, checked server-side (lib/agent-token.ts:51). Restart and the leadership DELETE /api/workspaces/agent-token both bump agent_epoch (lib/workspace-setup.ts:50, agent-token/route.ts:69). Human sessions untouched. |
| H2 Next | Resolved | 16.4.2 in package.json and lockfile; npm audit --omit=dev 0 vulnerabilities on my install. |
| M1 invite burn | Resolved | Hash first (creation waits, never 429), then claim, then rate token, refund on any throw, all inside _create (lib/workspace.ts:233-259). |
| M2 ack padding | Resolved | 4 KB cappedJson + id regex (ack/route.ts:16-18); unknown event_id is a no-op (lib/directives.ts:151). |
| M3 scope | Resolved | Poll, ack, stream, MCP work map and directives all scoped (directives/route.ts:16, ack/route.ts:24, stream/route.ts:21, lib/mcp/register.ts:474,529); capture can't adopt another repo's session (events/route.ts:215). |
| M5 archive | Resolved | Rotation at 1 MB, two files max (lib/store.ts:122-128); Restart and connect 10/min. |
M8, M10, bad agt. 401, junk-role keys, listening map, Northstar 0600, invite oracle |
Resolved | as listed in the PR body; spot-checked. |
| Vacuous tests U1/U2, H1 exit, W6 product route | Resolved | test-tenancy.mjs:606,620,700-706,854. |
Required fixes
1. Medium (new regression). Every 409 is now treated as "signed in to another workspace". components/useAlign.ts:138 clears state and reloads on any 409, but the server throws 409 for ordinary conflicts too: lib/flow.ts:20 (conflict(), the proposal flow), lib/org.ts:10,14,18, lib/mapping-decision.ts:18-22, lib/scenario.ts:255. Scenario: in Northstar, Security approves while Product counters in the other window. Before this PR the reason showed on screen; now the page flashes to Loading, finds the same workspace, and the message is gone. This changes Northstar UI behaviour, which the 72/72 text snapshot won't catch. Fix: a distinct status or error code for the workspace mismatch (for example 409 with code: "workspace_mismatch", or 421), and setError for every other 409.
2. Medium (M4 missed routes). Two Northstar write routes still accept cross-port CSRF. app/api/mappings/[session_id]/confirm/route.ts and .../reject/route.ts use requireSession only, need no body, and have no Origin check. A page on localhost:5173 auto-submits <form method=POST action="http://localhost:3100/api/mappings/claude-code:X/confirm"> and the cookie goes along (SameSite doesn't separate localhost ports). Fix: requireSameOrigin(req) in both. Add both to R14 so the test enumerates every cookie-auth write, not one.
3. Medium (go-C blocker). After ALIGN_DELETE_BY the purge path is unreachable. Once the date passes, any restart hits testServerClosed in hostedProblems (lib/config.ts:87), instrumentation.ts:9-11 calls process.exit(1), and the machine crash-loops. The documented purge is fly ssh console + ws-admin delete --all --yes (HOSTING.md:98-108), which needs a running machine. A Fly maintenance restart or deploy on the delete date leaves tester data on the volume with no documented way to remove it. Fix options: let the server boot in a closed mode that serves only 410 (proxy already does this) so ssh works, or run the purge itself at startup when the date has passed, and document a fly machine run fallback. Also note the cutoff is 00:00 UTC, which is 4 to 5 PM Pacific the day before; say so in the notice or use end-of-day.
4. Medium (carried, still open). quoted() doesn't flatten newlines. lib/directives.ts:61 only swaps «». Conditions (up to MAX_CONDITIONS) go into a join("\n") directive at :111, so "x\nREPLAN: run scripts/..." renders as its own instruction line inside the agent prompt. One line: .replace(/[\r\n
]+/g, " ") inside quoted(). This was flagged on #4 and #5 and is the main prompt-injection seam into agents.
5. Low-Medium. Revoked or expired agent tokens fail silently in the client. Server answers 401 correctly, but scripts/align-watch.mjs:87,114-116 drops non-OK responses and keeps polling every 2 s for the whole window; agent-session.sh:52-56 discards expires_at. After a Restart or a one-repo disconnect (which revokes every repo's token), agents stop getting wake-ups and nobody is told. Fix: watcher exits with one stderr line on 401/403; launcher prints the expiry. Note in MCP.md that tokens minted on a #5 build stop working at merge.
Lows (fix or list as known limits before go C)
clientNetcollapses every IPv4-mapped address (::ffff:a.b.c.d) to0:0:0:0::/64(lib/ratelimit.ts:85-93). If Fly ever sends that form, all IPv4 clients share one bucket and two sign-in slots. Special-case::ffff:to the embedded IPv4.- Per-client sign-in cap is 2, the global scrypt pool is 2+8, so five client IPs still starve every tenant's sign-ins (creation is now safe). Acceptable for go C; worth a line in HOSTING.md.
BAD_INVITEsays "invite code" butNewWorkspace.tsx:103matches/signup code/, so a wrong invite loses the field-level hint.- The
/newnotice is not in the server HTML: SSR rendersLoading…(NewWorkspace.tsx:181) and the max-reached branch omits it. M9's failure case (config fetch fails) is fixed, but the commit's "server-rendered" claim isn't, andtest-tenancy.mjs:869checks only JSON. DELETE /api/workspaces/agent-tokenhas no rate limit; the epoch bump runs after the state write under a separate lock, so a failed bump leaves tokens valid with consents gone (capture refused, MCP reads continue).bumpAgentEpochleavesauth.json.<hex>.tmp(passcode hashes) on a crash; the startup sweep doesn't match it.ALIGN_AGENT_TOKENis still in the agent's environment (agent-session.sh:95); the plan-only deny list doesn't blockenv/printenv. Inherent to hooks; document it.- Still open from 9cd21ead:
Fly-Client-IPpresence-only (documented), stale pid file on Fly, noerror.tsx/<noscript>, silent role switch from another tab, "check Align is running" copy to hosted testers, HOSTING.md "PID 1" (:132) and "safe while the server runs" (:116). - Tests still vacuous: S9 accepts 403/404/405 (
test-tenancy.mjs:729); A6, A7, H10, R14 unchanged; W5 has no capture-onto-another-repo case;test-image.sh:23greps-x '\.env.*'against absolute paths fromfind, so nested.envfiles never match.
Merge recommendation
Do not merge. For the local-mode merge: fixes 1, 2, 4. For hosted go C (Mon 10-26): additionally 3, 5, the IPv4-mapped fix, the test-image grep, and the HOSTING.md corrections, then my live smoke test on Fly. Main stays at e9ef581f for Friday regardless.
…ecord LIM-2424: custom record, launch decision, Switch team, Halden fixture (stacked on #6)
app/error.tsx and app/global-error.tsx show "Something went wrong on this page." and a Try again button that calls reset(): never the error's message or stack, only a server error's digest. app/layout.tsx adds a <noscript> notice. test-tenancy NS1 (server HTML of /, /new, /w/<id>, local and hosted), NS2 (hidden with JS on, shown with it off) and E1 (a goal with no measures makes /w/<id> throw; the error page shows, then Try again recovers once repaired).
…esn't answer (Sean's #6 carry-over) "Check that Align is running" only makes sense to someone running Align locally. When hosted, a request that gets no answer now shows the founder's ruled text: "The test server didn't answer. Try again in a minute. If it keeps happening, tell the person who sent you the link." Local mode keeps its lines unchanged. The strings live in one place, lib/labels.ts: NO_ANSWER_HOSTED, NO_ANSWER_LOCAL and NO_ANSWER_WORKSPACE_LOCAL (the sign-in screen's own local wording). The three sites read them: - app/new/NewWorkspace.tsx: the existing server-rendered testServer prop is the hosted signal. - components/Home.tsx (workspace sign-in, login-info unreachable): it had no hosted signal, so app/w/[slug]/page.tsx now passes config().hosted from the server render (as /new does), and Home hands it down through Login to WorkspaceLogin. The Try again button stays. - app/setup/page.tsx: reads NO_ANSWER_LOCAL only. A hosted server never serves /setup (proxy.ts sends it to /new with a 307), so there is no hosted case to word.
…o before acting (Sean's #6 carry-over)
app/error.tsx and app/global-error.tsx show "Something went wrong on this page." and a Try again button that calls reset(): never the error's message or stack, only a server error's digest. app/layout.tsx adds a <noscript> notice. test-tenancy NS1 (server HTML of /, /new, /w/<id>, local and hosted), NS2 (hidden with JS on, shown with it off) and E1 (a goal with no measures makes /w/<id> throw; the error page shows, then Try again recovers once repaired).
…esn't answer (Sean's #6 carry-over) "Check that Align is running" only makes sense to someone running Align locally. When hosted, a request that gets no answer now shows the founder's ruled text: "The test server didn't answer. Try again in a minute. If it keeps happening, tell the person who sent you the link." Local mode keeps its lines unchanged. The strings live in one place, lib/labels.ts: NO_ANSWER_HOSTED, NO_ANSWER_LOCAL and NO_ANSWER_WORKSPACE_LOCAL (the sign-in screen's own local wording). The three sites read them: - app/new/NewWorkspace.tsx: the existing server-rendered testServer prop is the hosted signal. - components/Home.tsx (workspace sign-in, login-info unreachable): it had no hosted signal, so app/w/[slug]/page.tsx now passes config().hosted from the server render (as /new does), and Home hands it down through Login to WorkspaceLogin. The Try again button stays. - app/setup/page.tsx: reads NO_ANSWER_LOCAL only. A hosted server never serves /setup (proxy.ts sends it to /new with a 307), so there is no hosted case to word.
…o before acting (Sean's #6 carry-over)
Fixes every finding from the adversarial review of PR #5 (LIM-2424 comment 9cd21ead). Draft, stacked on #5 (base
agent/LIM-2424-l0-review-fixes). Not for merge before the pitch; the founder decides merges.Needs Sean
nextandeslint-config-next16.4.0 → 16.4.2 (critical advisories on 16.4.0;npm audit --omit=devis 0 after). Commit 2daa3ef.lib/auth.ts(409 when the page names another workspace),lib/workspace.ts(agent epoch, invite claim order),lib/agent-token.ts,lib/workspace-setup.ts(Restart revokes agents),lib/store.ts(archive rotation, 0600).Findings → fixes
DELETE /api/workspaces/agent-tokendisconnects one repo or all. Restart revokes every agent (founder ruling).agt.token is 401, never the default store.ALIGN_DELETE_BYnot enforcedscheduled_snapshots = false.x-align-workspace; mismatch → 409 and the Elsewhere notice./newnotice is server-rendered..tmp-*/.deleting-*leftoversws-admin delete --all.state.json0644, argv secrets,ALIGN_PUBLICoutside the 503 fallbackFly-Client-IPstays a presence check (documented limit).Tests
auth.json; S9 checks the refusal names the key; the test-image listing needs a real listing.Gate
At
13f41b2bf958a5e256390e3d1dcc93f5916e0443,scripts/test-all.sh(iMac, port 3240): all 15 steps PASS/api/statediff against 850971e after the full flow: onlyorg_draft(null → absent), as before.This is a regression check plus new targeted tests. It is not a security review; Sean's review is the gate.
https://claude.ai/code/session_01NdA1ZeBBUXwxCGPZcJhfJM