diff --git a/AUDIT.md b/AUDIT.md index d4f044a..18bb60f 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -22,18 +22,21 @@ document. The substrate (Layer 1) is solid. Layers 2–4 were originally shaped-but-not- wired; most of the blocking defects below are now closed, verified by reading the current code (not just by trusting this file) and by targeted -tests. Two things are not yet proven: +tests. Three things are not yet proven: -1. **A real end-to-end unattended run.** The first live attempt (2026-07-28) - surfaced **B12** (fixed and confirmed live, see below), and re-verifying - that fix surfaced **B13** (open) — `agent.start` can return success while - silently never starting an agent, under back-to-back leases. One task - (`wD:p1`) did make it all the way to a real attached `claude` session, so - the lease path is provably reachable, but B13 means it's not yet reliable - enough to call proven — occupancy/rotation/handoff/completion still - haven't been exercised end-to-end against a session that's guaranteed to - actually exist. -2. **Cross-machine (federation) correctness.** Deliberately deferred — see +1. **A real end-to-end unattended run.** The live attempts of 2026-07-28 + surfaced B12 through B17. All are now closed in code (B17 on the worker + path only), but **B13 through B17 were each found live and none has been + re-verified live** — occupancy/rotation/handoff/completion still haven't + been exercised end-to-end against a session guaranteed to exist. Treat + "closed" here as "the code no longer contains the defect," not as + operational proof. +2. **The operator surface was unauthenticated in the live deployment.** + Fixed in code (**B18**: mandatory Web token, cookie sessions for the + browser), but the fix is a breaking config change — the service will not + start until `.orchestra-config/orchestra.env` sets + `ORCHESTRA_WEB_TOKEN`. See B18 and the command-channel section. +3. **Cross-machine (federation) correctness.** Deliberately deferred — see "The federation fork" below. `go build ./...`, `go vet ./...`, and `go test ./...` all pass. @@ -71,15 +74,116 @@ worktree. | Spec layer | State | |---|---| | L1 substrate (§3, §4) | Built and correct in the main path. | -| L2 harness (§5) | Occupancy, rotation, and completion all wired and reachable; B12's readiness race is fixed and confirmed live, but B13 (`agent.start` silently no-op'ing under back-to-back leases) still blocks reliable live verification of what's downstream. | +| L2 harness (§5) | Occupancy, rotation, and completion all wired and reachable. B12 fixed and confirmed live; B13–B16 fixed in code but not re-verified live. Unique per-task agent names (B16) and ambiguity-safe prompting (B14) are the substantive changes. | | L3 continuity (§6) | Handoff schema, pickup validation, scratch branches, TASK.md are all wired into the live path (Phase 4 complete). | -| L4 surfaces (§7) | Brief/standup/delivery real; quota has a post-hoc producer only (no live push feed yet); authz bypass closed. | +| L4 surfaces (§7) | Brief/standup/delivery real; quota has a post-hoc producer only (no live push feed yet); B8's authz bypass closed, but the new web UI reintroduces an equivalent one (B18). | --- -## Blocking defect currently open +## Blocking defects -### B14 — `agent.prompt` can duplicate a task prompt after an ambiguous wait timeout (found live 2026-07-28) — open +B14, B15 and B16 are now closed by code and covered by tests; none has been +re-verified against a live cross-machine run. B17 is closed on the worker +path only and remains open for Design A. B18 (the unauthenticated web +surface) is closed in code but **requires an env change before the service +will start** — see B18 and "What's next". + +Nothing is currently blocking in the sense B12/B13 were. The open defects +B19–B21 and S12–S13, all found while implementing B18, are correctness and +hygiene gaps in the new command channel rather than things that stop a task +from running. The real remaining risk is evidential: B13 through B17 were +each found live and none has been re-verified live. + +### B18 — the web UI surface is unauthenticated (found by audit 2026-07-28, uncommitted working tree) — closed (code fix; requires an env change before restart) + +`cmd/orchestra/main.go:205` mounts the browser read/write surface with no +handler-local credential check: + +```go +mux.Handle("/v1/ui/", ui.Server{Store: s, Workers: workers, Coordinator: coordinator, ...}.Handler()) +``` + +Unlike the worker/harness endpoints on the same mux (`main.go:342, 372, +467, 821, 840`), it performs no check of its own and stamps its events +`Surface: string(authz.Web)` (`internal/ui/ui.go:222, 337, 368`). + +**It is not, however, ungated in principle.** `authz.HTTP` wraps the entire +mux (`main.go:1167`) and defaults a request with no `X-Orchestra-Surface` +header to `Web`, so `/v1/ui/` is covered by `ORCHESTRA_WEB_TOKEN`. The +defect is that this gate is opt-in — `authz.HTTP`'s own comment says +"authentication is optional for local development," and `expected != ""` is +the only thing standing between the LAN and the surface. + +**The live deployment has opted out.** `.orchestra-config/orchestra.env` +sets no `ORCHESTRA_*_TOKEN` at all, and the server binds `":"+port` +(all interfaces, 9145). So on the deployed homesrv instance this surface is +in fact reachable unauthenticated from the LAN. + +**Consequence.** Unauthenticated reachable actions include `TaskCreated`, +and per-task `release`, `block`, `complete` and `handoff` +(`ui.go:345-361`). Most seriously, `grant_approval` / `deny_approval` +(`ui.go:301`) terminate in `pane.send_text` against a live pane — so an +unauthenticated caller can inject keystrokes into a running agent session on +workpc. B14 was filed because Orchestra could *accidentally* advance a +permission dialog; this surface allows it deliberately, from the network. + +**Why simply setting `ORCHESTRA_WEB_TOKEN` does not fix it.** +`mux.Handle("/", webui.Handler())` serves the SPA from the same listener, +behind the same middleware. A browser cannot attach an `Authorization` +header to a top-level document load, so setting the token returns 401 for +`index.html` and the web UI stops loading entirely. Bearer-token auth and a +browser-served SPA are incompatible as currently wired; that is likely why +no token is set. + +**Required fix (design decision, not a one-liner).** The browser needs a +credential it can actually present on a document load — a login endpoint +that exchanges the Web token for an `HttpOnly`, `SameSite=Strict` session +cookie, with `authz.HTTP` accepting either that cookie or a bearer token for +the `Web` surface. Alongside it: make the token mandatory rather than +opt-in whenever the UI is mounted (refuse to start, rather than silently +serving an open control plane), and bind to loopback by default so exposure +is a deliberate configuration act. Approval commands in particular must not +be reachable without it. + +**Fix (2026-07-28):** + +1. **The token is now mandatory** (`cmd/orchestra/main.go`): startup calls + `log.Fatal` if `ORCHESTRA_WEB_TOKEN` is empty. Serving the control plane + openly is no longer something a missing env var can cause silently. +2. **`authz.Sessions`** (`internal/authz/authz.go`) issues 32 random bytes as + a session value and stores only its SHA-256, so a leaked snapshot of the + map yields nothing usable. Sessions expire (12h default) and are swept on + each issue. +3. **`POST /v1/ui/session`** verifies the Web token in constant time and sets + the value as an `HttpOnly`, `SameSite=Strict`, `Secure` cookie. `Secure` + is dropped only if `ORCHESTRA_UI_INSECURE_COOKIE` is set, which is + required to reach the UI over plain HTTP. +4. **`authz.HTTPWithSessions`** accepts that cookie *in place of* the bearer + token, and only for the `Web` surface; the bearer comparison is also now + constant-time. Two paths are exempt from the gate, both necessarily: the + login endpoint (which performs its own check — gating it would make login + unreachable) and non-`/v1/` GET/HEAD, i.e. the SPA shell and its static + assets, which are not secrets. Every `/v1/` control path stays gated. + +`TestWebSessionCookieGatesControlPathsOnly` asserts the whole shape: an +uncredentialed control path is 401, the shell and login endpoint are +reachable, a valid cookie authorizes, a forged one does not, and a cookie +presented on the TUI surface is rejected. `TestSessionExpires` covers TTL. + +**Operational precondition — this is a breaking config change.** +`.orchestra-config/orchestra.env` currently sets no tokens, so +`orchestra.service` will refuse to start until `ORCHESTRA_WEB_TOKEN` is +added (plus `ORCHESTRA_UI_INSECURE_COOKIE=1` if the UI is served over plain +HTTP). Setting a Web token also newly gates the other `/v1/` surfaces that +default to `Web` — any existing unauthenticated client of this instance will +begin getting 401s and needs the token too. Do not restart the service +without making that env change first. + +**Still open:** the listener binds all interfaces (`":"+port`); loopback-by- +default was considered and deliberately not taken, so exposure remains a +network-level concern. + +### B14 — `agent.prompt` can duplicate a task prompt after an ambiguous wait timeout (found live 2026-07-28) — closed (code fix; not re-verified live) Live Claude E2E task `06FTF8CPH3K3DPN6XQA7G3WRQ8` (pane `wM:p1`) received the same initial `Begin Orchestra task ...` prompt twice. Claude completed a @@ -127,7 +231,29 @@ adds one. In addition, never issue `agent.prompt` while `agent_status` is as requiring explicit operator approval. Do not run further unattended OpenCode E2E tasks until both controls are verified live. -### B15 — a coordinator-side block cannot reconcile a later live completion (found live 2026-07-28) — open +**Fix (2026-07-28), in `Client.Prompt` (`internal/herdr/herdr.go:280`):** + +1. The RPC deadline is now derived from the requested wait, not the client + default: when the caller's context has no deadline within `wait + 5s`, + `Prompt` installs one (`herdr.go:307-314`). A 60s `wait.until=idle` no + longer expires against a 10s transport deadline. +2. Retry is split by error class. A JSON-RPC protocol error is herdr's + explicit rejection *before* it acted and stays retryable for the bounded + boot window; a transport failure after the write is ambiguous and is + never resent (`herdr.go:315-325`). Covered by + `TestPromptDoesNotRetryAmbiguousDelivery`, which closes the connection + mid-`agent.prompt` and asserts exactly one `agent.prompt` is issued. +3. The authorization-bypass half is addressed by inspecting the pane before + prompting: `Prompt` refuses a pane whose `agent_status` is `blocked` + (`herdr.go:285`) and refuses any pane whose transcript matches + `permissionPrompt` (`herdr.go:292`). Approval is now an explicit, + separately authorized operation — see the command-channel section below. + +Not re-verified live. The original live reproduction was cross-machine, and +the "do not run further unattended OpenCode E2E tasks until both controls +are verified live" instruction above still stands. + +### B15 — a coordinator-side block cannot reconcile a later live completion (found live 2026-07-28) — closed (code fix; operator-mediated, not re-verified live) The same OpenCode E2E task `06FTF9Z8RP5FM4F9M2SKAQNFZ4` was marked `TaskBlocked` when the coordinator's `agent.prompt` RPC timed out, despite @@ -150,7 +276,26 @@ blocker and evidence without pretending it never happened. A live pane must not be left outside the coordinator's session map simply because prompt delivery timed out. -### B16 — hardcoded harness name makes each harness globally single-instance (found live 2026-07-28) — open +**Fix (2026-07-28), in `Coordinator.Start` +(`internal/orchestrator/orchestrator.go:911-922`):** when the lease call +fails but a pane was actually created, the session is completed with +`HerdrID`, `TaskFileSHA` and `ConventionsHash` and persisted via +`rememberSession` *before* `block()` records `TaskBlocked`. `Reconcile` +(`orchestrator.go:421`) explicitly retains mappings for `StateBlocked` as +well as `StateLeased`, so a restart does not orphan them either. The live +pane therefore stays observable through `Capture` and the monitor. + +**Scope limit — reconciliation is operator-mediated, not automatic.** +Nothing promotes a blocked task to completed on its own; the router still +never retries a blocked task. What exists now is the evidence and the +surface: the retained session plus the `complete` / `release` / `handoff` +actions on the UI task endpoint (`internal/ui/ui.go:345-361`), and the +pre-existing versioned `TaskCorrected` workflow (S8) for recording the +correction without pretending the block never happened. That satisfies the +"must not be left outside the session map" requirement; it does not +implement autonomous reconciliation, which remains unbuilt by choice. + +### B16 — hardcoded harness name makes each harness globally single-instance (found live 2026-07-28) — closed (code fix; contract test only partially satisfied) While starting the real OpenCode healthcheck task `06FTFDW22833F1CCB8K43Z8808`, Orchestra created worktree/pane `wP:p1` but @@ -172,7 +317,26 @@ digits, `-`, `_`, maximum 32 characters) and persist it in `Session` for subsequent lifecycle operations. Add a contract test that starts two same- harness sessions in distinct panes and verifies both attach. -### B17 — opaque harnesses must not author canonical handoff anchors (found live 2026-07-28) — open +**Fix (2026-07-28):** `agentName(harness, taskID)` +(`internal/herdr/herdr.go:452`) derives a bounded, validated name — a +per-harness prefix (`oc`/`cl`/`cx`), the lowercased task ID with everything +outside `[a-z0-9_-]` collapsed, capped at 32 characters. Over-long IDs keep +a leading stem plus an 8-hex-character SHA-256 suffix, because plain +truncation made distinct long task IDs collide in herdr's machine-global +namespace. `StartAgent` passes it as `name` while `kind` stays the +configured harness (`herdr.go:393`), persists it as `Session.AgentName` +(`herdr.go:145, 445`), and binds it for prompt routing (`herdr.go:446`, +`adapter.go:144`). Pane-scoped operations deliberately keep using +`PaneID` — `AgentName` is only for agent-targeted calls (`adapter.go:531`). + +**Requirement not fully met:** the requested contract test does not exist. +What is covered is name derivation — `TestAgentNameIsBoundedAndValid` and +`TestAgentNameLongIDsDoNotCollide` — plus `TestPromptDoesNotRetryAmbiguous +Delivery` asserting the prompt target is the unique name. Nothing yet starts +two same-harness sessions in distinct panes and verifies both attach, so the +`agent_name_taken` failure mode itself is untested end-to-end. + +### B17 — opaque harnesses must not author canonical handoff anchors (found live 2026-07-28) — closed on the worker path; open for Design A The real OpenCode healthcheck run showed that prompting an opaque agent to write the full `continuity.Handoff` schema is the wrong ownership boundary. @@ -228,6 +392,22 @@ attached Claude at 08:00:43Z. The renewed attempt then hit B14's prompt timeout and became blocked. This is expected retry behavior, but illustrates why lifecycle retries must be visible to operators. +**Status (2026-07-28): satisfied on the worker path, unchanged for Design A.** +`cmd/orchestra-worker` now implements the required ownership boundary. The +harness is asked once for a bounded semantic report; `releaseReady` +(`cmd/orchestra-worker/main.go:180`) then runs `CLIAdapter.Release` and +`herdr.HeadSHA` **on the machine that hosts the worktree**, so `HEAD`, +branch, dirty paths and SHA-256 values are collected locally and the +canonical artifact is validated before publication. The handoff-provenance +correction documented at the end of this file supplies the parsing and +validation half. + +The homesrv coordinator's Design A path is unchanged and still cannot +satisfy this: it does not own the workpc checkout, so any Git-derived fact +it seals is derived from the wrong filesystem. B17 is therefore only closed +for tasks that actually run through a worker. The standing rule holds — +homesrv must not rotate or clean up a non-local worktree. + ### B13 — `agent.start` silently no-ops under back-to-back leases (found live 2026-07-28) — closed (code fix; not yet re-verified live) Discovered while live-verifying the B12 fix below. B12 itself is confirmed @@ -380,6 +560,185 @@ the specific shell/agent-readiness race B12 was originally filed for, which --- +## Web UI and the capture / approval command channel (uncommitted, 2026-07-28) + +Roughly 520 lines of Go across eight tracked files, plus the untracked +`internal/ui`, `internal/webui` and `web/` trees, were added after the last +commit and were previously undocumented here. Recording the design so the +next session does not have to re-derive it. + +**Capture/command model (`internal/federation/federation.go`).** Two new +worker-scoped maps on `Registry`. `PutCapture` stores the latest pane text +per (worker, task) and bumps a monotonic `Revision` only when the text or +pane actually changes. `Queue` accepts only `grant_approval` / +`deny_approval`, and requires a non-zero `CaptureRevision` — a command is +bound to the exact capture an operator was looking at. `CompleteCommand` +refuses to resolve a command twice. + +**Worker side (`cmd/orchestra-worker/main.go`).** `publishCaptures` pushes +recent pane text for every held session; `runCommands` polls +`/v1/federation/commands` and, for each command, re-reads the pane, +re-publishes it, and refuses with `stale` if the resulting revision differs +from the one the command was issued against. `approvalResponse` decides the +keystroke: it sends `y`/`n` only for a visible `[y/n]`/`(y/n)` prompt, and +sends bare Enter only for OpenCode's fully-labelled `Allow once / Allow +always / Reject … enter confirm` selector — where Enter is a bounded +one-time grant. It refuses to *deny* through that selector, because doing so +would require unobservable navigation. Unknown dialog layouts are rejected +outright rather than guessed at. Both branches are covered by +`TestApprovalResponseOpenCodeAllowOnce` and +`TestWorkerApprovalCommandIsRevisionBoundAndAcknowledged`. + +This is the right shape for B14's authorization half: approval becomes an +explicit, revision-bound, separately-issued operation instead of a side +effect of prompting, and it executes on the worker that owns the pane rather +than over a coordinator-driven remote socket. + +**Coordinator side.** `Coordinator.RespondApproval` and +`herdr.ApprovalResponder` (`internal/herdr/adapter.go:602`) are the +local-herdr equivalent, applying the same re-read-before-send rule via an +`expectedCapture` comparison and the same y/n-only refusal. +`Coordinator.RequestHandoff` exposes the handoff request as an operator +action without releasing the pane. + +**Known weaknesses of this channel.** The defects found in it are filed +individually below — **B19** (federated approvals emit no event), **B20** +(the local capture revision is a timestamp, not a change counter) and +**B21** (`Registry.commands` never prunes). Two more, not separately filed: + +- **Captures and commands are in-memory only.** `Registry.captures` and + `Registry.commands` have no persistence, so a coordinator restart drops + pending approvals silently and resets revision counters — which, with a + reset counter, could let a stale command match a new capture. This is the + other half of B21: the lifecycle forgets across restarts and never forgets + within one. +- **`RespondApproval` (coordinator path) has no test**, unlike its worker + counterpart — and per B20 it is the path whose revision is meaningless, + so its text-comparison guard is the only thing actually binding the + decision to what the operator saw. That guard is untested. +- **`web/` is a full Vite/React app whose build output is embedded** via + `internal/webui`'s `go:embed assets/*`. The embedded assets are checked in + as build output; there is no documented step tying `web/` sources to a + rebuild of `internal/webui/assets`, so the two can silently diverge. + +## Found while implementing B18 (2026-07-28) — not yet fixed + +These were noticed while wiring the session gate. None is fixed; all were +verified against the code rather than inferred. + +### B19 — federated approvals leave no audit trail + +`Server.action`'s `grant_approval` / `deny_approval` branch +(`internal/ui/ui.go:301-341`) has two exits that are **not** symmetric. The +local-coordinator path calls `RespondApproval` and then appends +`ApprovalGranted` / `ApprovalDenied` to the store. The federated path calls +`s.Workers.Queue(...)`, writes the command to the response, and **returns +before appending any event**. + +So the single most safety-critical operation in the system — injecting a +keystroke that advances a permission dialog on another machine — is +invisible in the event log precisely when it crosses a machine boundary. +There is no record of who approved what, or that an approval happened at +all. §7.1's authorization model assumes the event log is the record. + +Fixing this needs a decision the local path did not have to make: the +command is *queued*, not executed, so the honest event is a request at queue +time plus a resolution when the worker acknowledges (`CompleteCommand` +already carries `acknowledged` / `rejected` / `stale`). Emitting +`ApprovalGranted` at queue time would claim an outcome that has not +happened yet. + +### B20 — the local capture revision is a timestamp, not a change counter + +`Server.capture` (`ui.go:80`) fabricates `Revision: +uint64(time.Now().UnixNano())` for the coordinator path. The federation +path's revision is a real per-change counter (`Registry.PutCapture` bumps it +only when pane text or pane ID actually changes), and the worker's staleness +check depends on that meaning. + +Two consequences: + +1. The local revision changes on every read, so it conveys nothing about + whether the pane changed. The local path is still safe, but only because + `RespondApproval` re-reads and compares capture *text* — the revision it + reports to the browser is decorative. +2. If a task has both a live coordinator session and a published worker + capture, `capture()` prefers the coordinator (`ui.go:77`) and returns a + timestamp revision, which `action()` then passes to `Queue` as + `CaptureRevision`. That value can never equal the worker's counter, so + the worker resolves every such command `stale` and the approval silently + never happens. The precedence between the two capture sources needs to be + explicit rather than incidental. + +### B21 — `Registry.commands` is append-only + +`r.commands[worker]` is only ever appended to (`federation.go:119`); +`CompleteCommand` flips a status in place and nothing is ever deleted. +Resolved commands accumulate for the process lifetime, and `Commands()` +rescans the entire history on every worker poll. Combined with the +in-memory-only storage already noted, the lifecycle is wrong at both ends: +it forgets across restarts and never forgets within one. + +### S12 — `ORCHESTRA_NTFY_TOKEN` serves two unrelated purposes + +The same env var is the ntfy *server* credential (`main.go:1153`, +`delivery.Ntfy{Token: ...}`) and the authz *surface* credential for +`authz.Ntfy` (`main.go:1165`). These are unrelated secrets with different +trust boundaries: one is handed to a third-party notification server, the +other authenticates callers to Orchestra. Setting the former silently makes +it a valid inbound credential. They need separate variables. + +### S13 — dead `auth()` middleware in `cmd/orchestra/main.go` + +`func auth(next http.Handler)` (`main.go:1212`) is defined and never +referenced; Go does not flag unused functions, so `go vet` stays quiet. It +reimplements the notify-only rule that `authz.HTTP` already enforces. It is +harmless today and a trap tomorrow — a second, divergent copy of the +authorization policy sitting next to the real one. Delete it. + +### Positive note — B8 is more strongly closed than it was + +`authz.HTTP` downgrades a caller-declared `system` surface to `Web` before +the token comparison. Previously that landed on a surface whose token was +unset in production, i.e. no gate at all. With `ORCHESTRA_WEB_TOKEN` now +mandatory, the downgrade lands on a genuinely gated surface, so the B8 +bypass is closed by construction rather than by the header check alone. + +--- + +## What's next + +In dependency order, not importance order: + +1. **Apply the B18 env change** before any restart — + `.orchestra-config/orchestra.env` needs `ORCHESTRA_WEB_TOKEN` (plus + `ORCHESTRA_UI_INSECURE_COOKIE=1` for plain HTTP), and every existing + unauthenticated `/v1/` client of this instance needs the token too. The + service will not start otherwise. Nothing else can be tested live until + this is done. +2. **S13, then S12** — deleting dead policy code and splitting the conflated + ntfy token are both small, local, and reduce the chance of the next + auth change being made against the wrong copy. +3. **B19** — decide the queued-approval event model and give federated + approvals an audit trail. This is the largest correctness gap in the new + command channel. +4. **B20 and B21** — make the capture revision mean one thing, make the + coordinator/worker precedence explicit, and give commands a retention + policy. B20 in particular can silently defeat approvals. +5. **Diagnose the flaky router test** (see open gaps). A nondeterministic + assignment test is exactly the failure shape this codebase has hidden + real defects behind before; do not paper over it with a retry. +6. **Then, and only then, the live re-verification** that B13–B17 all still + lack. Each was found live and closed on paper; the code fixes are + unproven against a real cross-machine run, and that remains the single + biggest gap between this document and reality. + +Deliberately *not* next: building further on Design A's cross-machine calls, +and closing B17 for the coordinator path. Both wait on the federation-fork +decision below. + +--- + ## Closed defects Each entry is the flattened final state — design + what's verified — not the @@ -863,10 +1222,31 @@ implemented when nothing can reach it. ## Known open gaps (as of 2026-07-28) -- **B13** (above) — `agent.start` can silently no-op under back-to-back - leases, with no error surfaced; blocks reliable live verification of - everything downstream (occupancy, rotation, handoff, completion) against a - real task, even though B12's readiness race is now fixed. +Numbered defects are not repeated here — B19, B20, B21, S12 and S13 are in +"Found while implementing B18" above, and the ordering across all of them is +in "What's next". This list is the unnumbered residue: conditions that are +known, accepted, or not actionable as a single fix. + +- **B18's env change is not yet applied.** The code now refuses to start + without `ORCHESTRA_WEB_TOKEN`, but `.orchestra-config/orchestra.env` does + not set one — `orchestra.service` will fail to start until it does. +- **The listener still binds all interfaces**; loopback-by-default was + considered and not taken (B18). +- **`TestAssignsByAffinityCapabilityAndConcurrency` is flaky** (`no task + leased`, roughly 1 run in 10, reproduced with `-count=10`). Predates these + changes — confirmed by stashing them — and is unrelated to authz. A + nondeterministic router assignment test is exactly the kind of thing this + codebase has been bitten by before; it should be diagnosed, not retried. +- **B14/B15/B16 are code-fixed but not re-verified live**, and B17 is closed + only for tasks that run through a worker. Each was found live, so a code + fix plus unit tests is weaker evidence than the failure that produced it. + In particular B16 still lacks the requested two-pane same-harness attach + contract test, so its actual `agent_name_taken` failure mode is untested. +- **B15's reconciliation is operator-mediated only.** Blocked tasks keep + their session and are correctable through the UI, but nothing promotes + them automatically and the router still never retries a blocked task. +- **`web/` build output and `internal/webui/assets` can silently diverge**; + no documented rebuild step ties them together. - **Cross-machine lease correctness proven at the primitive level only** — needs an actual homesrv/workpc pair over the real mesh; this repo cannot exercise that by itself. diff --git a/cmd/orchestra/main.go b/cmd/orchestra/main.go index 783c3d2..7b867a0 100644 --- a/cmd/orchestra/main.go +++ b/cmd/orchestra/main.go @@ -2,6 +2,7 @@ package main import ( "context" + "crypto/subtle" "encoding/json" "errors" "io" @@ -20,6 +21,8 @@ import ( "orchestra/internal/registry" "orchestra/internal/router" "orchestra/internal/store" + "orchestra/internal/ui" + "orchestra/internal/webui" "os" "path/filepath" "strconv" @@ -198,6 +201,58 @@ func main() { } } mux := http.NewServeMux() + // B18: the UI is a full control plane — it can create tasks, release or + // complete them, and inject approval keystrokes into live panes. Refuse + // to serve it unauthenticated rather than silently exposing that on + // whatever interface the listener binds to. + webToken := os.Getenv("ORCHESTRA_WEB_TOKEN") + if webToken == "" { + log.Fatal("ORCHESTRA_WEB_TOKEN must be set: it gates the web UI's task, lifecycle and approval controls") + } + sessions := &authz.Sessions{} + // A browser cannot put a Bearer token on a document load, so it trades + // the token once for an HttpOnly cookie. Same credential, presentable + // form; no new authority is created here. + mux.HandleFunc("/v1/ui/session", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + var body struct { + Token string `json:"token"` + } + _ = json.NewDecoder(http.MaxBytesReader(w, r.Body, 4096)).Decode(&body) + supplied := body.Token + if supplied == "" { + supplied = strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + } + if subtle.ConstantTimeCompare([]byte(supplied), []byte(webToken)) != 1 { + http.Error(w, "invalid token", http.StatusUnauthorized) + return + } + v, err := sessions.Issue() + if err != nil { + http.Error(w, "session unavailable", http.StatusInternalServerError) + return + } + http.SetCookie(w, &http.Cookie{ + Name: authz.SessionCookie, Value: v, Path: "/", + HttpOnly: true, SameSite: http.SameSiteStrictMode, + Secure: os.Getenv("ORCHESTRA_UI_INSECURE_COOKIE") == "", + MaxAge: int((12 * time.Hour).Seconds()), + }) + w.WriteHeader(http.StatusNoContent) + }) + // Browser-specific endpoints intentionally present a joined read model; + // raw lifecycle endpoints below remain stable for workers and harnesses. + mux.Handle("/v1/ui/", ui.Server{Store: s, Workers: workers, Coordinator: coordinator, Route: func(e domain.Event) error { + if rt == nil { + return nil + } + _, err := rt.HandleEvent(e) + return err + }}.Handler()) + mux.Handle("/", webui.Handler()) workers.OnOffline = func(w federation.Worker) { for _, t := range s.Tasks() { if t.State == domain.StateLeased && t.Lease != nil && t.Lease.HarnessID == w.ID { @@ -831,6 +886,68 @@ func main() { } return wid, nil } + mux.HandleFunc("/v1/federation/commands", func(w http.ResponseWriter, r *http.Request) { + wid, err := workerAuth(r) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return + } + if r.Method != http.MethodGet { + http.Error(w, "method not allowed", 405) + return + } + out, err := workers.Commands(wid) + if err != nil { + http.Error(w, err.Error(), 404) + return + } + json.NewEncoder(w).Encode(out) + }) + mux.HandleFunc("/v1/federation/commands/", func(w http.ResponseWriter, r *http.Request) { + wid, err := workerAuth(r) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return + } + if r.Method != http.MethodPost { + http.Error(w, "method not allowed", 405) + return + } + var body struct { + Status string `json:"status"` + Message string `json:"message"` + } + if json.NewDecoder(r.Body).Decode(&body) != nil || (body.Status != "acknowledged" && body.Status != "stale" && body.Status != "rejected") { + http.Error(w, "invalid command result", 400) + return + } + commandID := strings.TrimPrefix(r.URL.Path, "/v1/federation/commands/") + command, ok := workers.Command(wid, commandID) + if !ok { + http.Error(w, "command not found", 404) + return + } + if err := workers.CompleteCommand(wid, commandID, body.Status, body.Message); err != nil { + http.Error(w, err.Error(), 409) + return + } + // Audit lifecycle evidence only after the worker reports the herdr + // input was acknowledged; a queued browser click is never an approval. + if body.Status == "acknowledged" { + if t, ok := s.Task(command.TaskID); ok { + typ := "ApprovalGranted" + if command.Kind == "deny_approval" { + typ = "ApprovalDenied" + } + payload, _ := json.Marshal(map[string]any{"subject_ref": command.ID, "pane_id": command.PaneID, "capture_revision": command.CaptureRevision}) + e := domain.Event{ID: id(), Type: typ, TaskID: command.TaskID, Version: t.Version + 1, Payload: payload, Surface: string(authz.System)} + if err := s.Append(e); err != nil { + log.Printf("record approval %s: %v", command.ID, err) + } + } + } + w.WriteHeader(http.StatusNoContent) + }) mux.HandleFunc("/v1/federation/events", func(w http.ResponseWriter, r *http.Request) { wid, err := workerAuth(r) if err != nil { @@ -873,7 +990,7 @@ func main() { w.WriteHeader(http.StatusNoContent) }) mux.HandleFunc("/v1/federation/workers/", func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost || (!strings.HasSuffix(r.URL.Path, "/heartbeat") && !strings.HasSuffix(r.URL.Path, "/handoff") && !strings.HasSuffix(r.URL.Path, "/complete")) { + if r.Method != http.MethodPost || (!strings.HasSuffix(r.URL.Path, "/heartbeat") && !strings.HasSuffix(r.URL.Path, "/handoff") && !strings.HasSuffix(r.URL.Path, "/complete") && !strings.HasSuffix(r.URL.Path, "/captures")) { http.Error(w, "not found", 404) return } @@ -882,10 +999,15 @@ func main() { http.Error(w, "not found", 404) return } - if _, err := workerAuth(r); err != nil { + wid, err := workerAuth(r) + if err != nil { http.Error(w, err.Error(), 401) return } + if wid != parts[3] { + http.Error(w, "worker identity mismatch", http.StatusForbidden) + return + } if strings.HasSuffix(r.URL.Path, "/heartbeat") { if err := workers.Heartbeat(parts[3]); err != nil { http.Error(w, err.Error(), 404) @@ -894,6 +1016,24 @@ func main() { w.WriteHeader(http.StatusNoContent) return } + if strings.HasSuffix(r.URL.Path, "/captures") { + var c federation.Capture + if json.NewDecoder(r.Body).Decode(&c) != nil { + http.Error(w, "invalid capture", 400) + return + } + if t, ok := s.Task(c.TaskID); !ok || t.Lease == nil || t.Lease.HarnessID != parts[3] { + http.Error(w, "lease not owned", 409) + return + } + out, err := workers.PutCapture(parts[3], c) + if err != nil { + http.Error(w, err.Error(), 400) + return + } + json.NewEncoder(w).Encode(out) + return + } var b struct { TaskID string `json:"task_id"` TTLSeconds int `json:"ttl_seconds"` @@ -1067,7 +1207,7 @@ func main() { authz.MCP: os.Getenv("ORCHESTRA_MCP_TOKEN"), authz.Maven: os.Getenv("ORCHESTRA_MAVEN_TOKEN"), authz.Telegram: os.Getenv("ORCHESTRA_TELEGRAM_TOKEN"), authz.Ntfy: os.Getenv("ORCHESTRA_NTFY_TOKEN"), } - log.Fatal(http.ListenAndServe(":"+port, authz.HTTP(tokens, mux))) + log.Fatal(http.ListenAndServe(":"+port, authz.HTTPWithSessions(tokens, sessions, mux))) } func auth(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/authz/authz.go b/internal/authz/authz.go index a3729f5..bc86b1d 100644 --- a/internal/authz/authz.go +++ b/internal/authz/authz.go @@ -3,9 +3,15 @@ package authz import ( + "crypto/rand" + "crypto/sha256" + "crypto/subtle" + "encoding/hex" "fmt" "net/http" "strings" + "sync" + "time" ) type Surface string @@ -66,10 +72,86 @@ func AuthorizeEvent(s Surface, typ string) error { return nil } +// SessionCookie carries a browser's proof of the Web-surface token. A +// top-level document load cannot set an Authorization header, so a +// bearer-only gate forces operators to run the UI with no token at all +// (AUDIT.md B18). The cookie is the browser-presentable equivalent; it is +// never a second credential, only a receipt for the same token. +const SessionCookie = "orchestra_session" + +// SessionPath is the one Web-surface endpoint exempt from the token gate, +// because it *is* the token check: it verifies the Web token itself and +// exchanges it for a cookie. Gating it would make login unreachable. +const SessionPath = "/v1/ui/session" + +// Sessions issues and validates those receipts. Values are random and stored +// hashed, so a leaked snapshot of this map does not yield a usable cookie. +type Sessions struct { + mu sync.Mutex + TTL time.Duration + ids map[string]time.Time +} + +func (s *Sessions) ttl() time.Duration { + if s.TTL > 0 { + return s.TTL + } + return 12 * time.Hour +} + +// Issue mints a session value. The caller must have already verified the +// Web-surface token; Issue does not check credentials itself. +func (s *Sessions) Issue() (string, error) { + b := make([]byte, 32) + if _, err := rand.Read(b); err != nil { + return "", err + } + v := hex.EncodeToString(b) + sum := sha256.Sum256([]byte(v)) + s.mu.Lock() + defer s.mu.Unlock() + if s.ids == nil { + s.ids = map[string]time.Time{} + } + now := time.Now() + for k, exp := range s.ids { + if now.After(exp) { + delete(s.ids, k) + } + } + s.ids[hex.EncodeToString(sum[:])] = now.Add(s.ttl()) + return v, nil +} + +func (s *Sessions) Valid(v string) bool { + if v == "" { + return false + } + sum := sha256.Sum256([]byte(v)) + s.mu.Lock() + defer s.mu.Unlock() + exp, ok := s.ids[hex.EncodeToString(sum[:])] + if !ok { + return false + } + if time.Now().After(exp) { + delete(s.ids, hex.EncodeToString(sum[:])) + return false + } + return true +} + // HTTP enforces the same policy at the bus boundary. Authentication is // optional for local development; when a token is supplied, control surfaces // must present it as a Bearer token. func HTTP(tokens map[Surface]string, next http.Handler) http.Handler { + return HTTPWithSessions(tokens, nil, next) +} + +// HTTPWithSessions additionally accepts a valid session cookie in place of a +// Bearer token, but only for the Web surface — every non-browser surface +// still has to present the token directly. +func HTTPWithSessions(tokens map[Surface]string, sessions *Sessions, next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { s := ParseSurface(r.Header.Get("X-Orchestra-Surface")) if s == "" { @@ -85,9 +167,26 @@ func HTTP(tokens map[Surface]string, next http.Handler) http.Handler { if s == System { s = Web } - if expected := tokens[s]; expected != "" && r.Header.Get("Authorization") != "Bearer "+expected { - http.Error(w, "unauthorized surface", http.StatusUnauthorized) - return + if expected := tokens[s]; expected != "" && subtle.ConstantTimeCompare([]byte(r.Header.Get("Authorization")), []byte("Bearer "+expected)) != 1 { + ok := false + if s == Web && sessions != nil { + if c, err := r.Cookie(SessionCookie); err == nil { + ok = sessions.Valid(c.Value) + } + // The login endpoint authenticates itself, and the SPA shell + // must load before a browser can present anything. Static + // assets are not secrets; every /v1/ control path stays gated. + if r.URL.Path == SessionPath { + ok = true + } + if !strings.HasPrefix(r.URL.Path, "/v1/") && (r.Method == http.MethodGet || r.Method == http.MethodHead) { + ok = true + } + } + if !ok { + http.Error(w, "unauthorized surface", http.StatusUnauthorized) + return + } } if (s == Telegram || s == Ntfy) && r.Method != http.MethodGet && r.Method != http.MethodHead { http.Error(w, "notify-only surface", http.StatusForbidden) diff --git a/internal/authz/authz_test.go b/internal/authz/authz_test.go index 9de6fe5..6511d06 100644 --- a/internal/authz/authz_test.go +++ b/internal/authz/authz_test.go @@ -4,6 +4,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" ) func TestSurfaceCapabilities(t *testing.T) { @@ -46,3 +47,66 @@ func TestSystemSurfaceDowngradedByHTTPMiddleware(t *testing.T) { t.Fatalf("unexpected status %d", rec.Code) } } + +// B18: the web UI is a full control plane. A session cookie must be an +// alternative *presentation* of the Web token, never a widening of it. +func TestWebSessionCookieGatesControlPathsOnly(t *testing.T) { + tokens := map[Surface]string{Web: "secret"} + sessions := &Sessions{} + h := HTTPWithSessions(tokens, sessions, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + do := func(method, path string, c *http.Cookie, surface string) int { + req := httptest.NewRequest(method, path, nil) + if c != nil { + req.AddCookie(c) + } + if surface != "" { + req.Header.Set("X-Orchestra-Surface", surface) + } + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec.Code + } + if got := do(http.MethodPost, "/v1/ui/tasks", nil, ""); got != http.StatusUnauthorized { + t.Fatalf("uncredentialed control path = %d, want 401", got) + } + // The SPA shell and the login endpoint must stay reachable, or no + // browser could ever obtain a cookie in the first place. + if got := do(http.MethodGet, "/index.html", nil, ""); got != http.StatusOK { + t.Fatalf("SPA shell = %d, want 200", got) + } + if got := do(http.MethodPost, SessionPath, nil, ""); got != http.StatusOK { + t.Fatalf("login endpoint = %d, want 200", got) + } + v, err := sessions.Issue() + if err != nil { + t.Fatal(err) + } + if got := do(http.MethodPost, "/v1/ui/tasks", &http.Cookie{Name: SessionCookie, Value: v}, ""); got != http.StatusOK { + t.Fatalf("session-cookie control path = %d, want 200", got) + } + if got := do(http.MethodPost, "/v1/ui/tasks", &http.Cookie{Name: SessionCookie, Value: "forged"}, ""); got != http.StatusUnauthorized { + t.Fatalf("forged cookie = %d, want 401", got) + } + // A cookie must not authenticate a non-browser surface. + tokens[TUI] = "tui-secret" + if got := do(http.MethodPost, "/v1/tasks", &http.Cookie{Name: SessionCookie, Value: v}, "tui"); got != http.StatusUnauthorized { + t.Fatalf("cookie on TUI surface = %d, want 401", got) + } +} + +func TestSessionExpires(t *testing.T) { + s := &Sessions{TTL: time.Millisecond} + v, err := s.Issue() + if err != nil { + t.Fatal(err) + } + time.Sleep(5 * time.Millisecond) + if s.Valid(v) { + t.Fatal("expired session accepted") + } + if s.Valid("") { + t.Fatal("empty session accepted") + } +}