From 7f96069345005bcd378acd1aa421d25a3fd3bf2b Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 08:35:43 -0400 Subject: [PATCH] docs --- ...ady-p1-resolve-std-fs-follow-up-markers.md | 39 ++ ...-refactor-lock-down-server-secrets-plan.md | 483 ++++++++++++++++ ...04-22-004-refactor-worker-jwt-auth-plan.md | 518 ++++++++++++++++++ 3 files changed, 1040 insertions(+) create mode 100644 .context/compound-engineering/todos/001-ready-p1-resolve-std-fs-follow-up-markers.md create mode 100644 docs/plans/2026-04-22-003-refactor-lock-down-server-secrets-plan.md create mode 100644 docs/plans/2026-04-22-004-refactor-worker-jwt-auth-plan.md diff --git a/.context/compound-engineering/todos/001-ready-p1-resolve-std-fs-follow-up-markers.md b/.context/compound-engineering/todos/001-ready-p1-resolve-std-fs-follow-up-markers.md new file mode 100644 index 000000000..e70782b65 --- /dev/null +++ b/.context/compound-engineering/todos/001-ready-p1-resolve-std-fs-follow-up-markers.md @@ -0,0 +1,39 @@ +--- +status: ready +priority: p1 +issue_id: "001" +tags: [rust, clippy, async-io, std-fs] +dependencies: [] +--- + +## Problem Statement + +Several Rust crates still contain `FOLLOW-UP:` markers related to blocking `std::fs` or sync I/O on async paths. The requested work is to execute the implementation plan in `~/.claude/plans/we-ll-feal-with-std-fs-jaunty-feigenbaum.md` and finish the refactors or tighten the remaining sync justifications. + +## Findings + +- The repo is currently on `main`, and the user explicitly approved proceeding there. +- `docs/solutions/` is not present, so there are no repo learnings to consult for this task. +- The current code matches the plan buckets across `fabro-agent`, `fabro-devcontainer`, `fabro-llm`, and `fabro-workflow`. + +## Proposed Solutions + +- Execute the plan in bucket order, using targeted failing checks before each production change where feasible. +- Prefer async propagation for truly async paths and `spawn_blocking` only at natural async boundaries. +- Remove or narrow `#[expect(clippy::disallowed_methods)]` annotations once the production sites are fixed. + +## Recommended Action + +Implement the plan directly, verify each bucket with crate-level tests or lint checks, then run the final formatting, clippy, workspace tests, and `FOLLOW-UP` sweep. + +## Acceptance Criteria + +- All `FOLLOW-UP:` markers under `lib/crates/` are removed. +- The planned async refactors and `spawn_blocking` boundary changes are implemented. +- Formatting and workspace clippy pass. +- Relevant crate tests pass during incremental verification. + +## Work Log + +- 2026-04-19: Created execution todo, confirmed branch choice with the user, and started inspecting the planned call sites. + diff --git a/docs/plans/2026-04-22-003-refactor-lock-down-server-secrets-plan.md b/docs/plans/2026-04-22-003-refactor-lock-down-server-secrets-plan.md new file mode 100644 index 000000000..cf02593b0 --- /dev/null +++ b/docs/plans/2026-04-22-003-refactor-lock-down-server-secrets-plan.md @@ -0,0 +1,483 @@ +--- +title: "refactor: Lock down server-level secrets handling" +type: refactor +status: active +date: 2026-04-22 +deepened: 2026-04-23 +--- + +# Lock down server-level secrets handling + +## Overview + +Server-level secrets (SESSION_SECRET, FABRO_DEV_TOKEN, GitHub App credentials, JWT keypair) flow through three independent paths today: + +- `ServerSecrets::get` reads process env *live* on every call, with on-disk envfile fallback. +- `load_or_create_local_session_secret` reads env first, then envfile, then auto-generates if missing. +- `execute_foreground` mutates parent process env via `std::env::set_var` so the in-process server's live env reads see the resolved value. + +Worker subprocess scrubs `FABRO_DEV_TOKEN`; daemon spawn does not — accidental inconsistency. + +This refactor: + +- **Makes `ServerSecrets` snapshot-based.** Reads env and envfile *once* at construction, exposes the merged result. Both env and file remain valid sources (env wins on conflict — 12-factor convention). The bug was *live* env reads coupled with parent-process mutation, not env reads themselves. +- **Eliminates parent-process env mutation.** With snapshot semantics, the foreground `set_var` becomes unnecessary; the daemon `cmd.env(SESSION_SECRET)` becomes redundant. +- **CLI install and web install share a common orchestration path for `server.env` writes.** Coordinated install flows may update server-level secrets while a server is running, but they must also own restart/handoff (web install already does this via `/install/finish`). +- **Worker and render-graph subprocesses use `env_clear` + strict fail-closed allowlists.** Worker is the trust boundary — it dispatches user-supplied workflow stages via `Sandbox`. Daemon child inherits parent env unchanged (12-factor pattern works). +- **Foreground and daemon startup share one validation path.** Daemon preflight builds the same `ServerSecrets` snapshot and runs the same server-side auth/startup validation foreground does — no separate parent-CLI validator with drift risk. +- **Legacy `FABRO_JWT_PRIVATE_KEY` / `FABRO_JWT_PUBLIC_KEY` drift is removed.** SESSION_SECRET remains the sole auth master (HKDF source for cookie key + JWT signing key per `2026-04-19-003-feat-cli-auth-login-plan.md`); install stops generating those keys and actively removes them from `server.env` on subsequent runs. +- **`clippy::disallowed_methods` denies `std::env::set_var` / `remove_var`** workspace-wide *including tests*; narrow documented post-fork/pre-exec exceptions only. + +## Problem Frame + +- **Live env reads + parent-process mutation is unsound.** `std::env::set_var` in `execute_foreground` mutates global process state inside an async runtime. Rust 2024 marks this `unsafe` because it races concurrent readers. Workspace is on 2021; moving to 2024 forces the issue. The set_var is load-bearing because `ServerSecrets::get` reads env *live*, not at construction. +- **Worker subprocess env leakage.** Workers run user-supplied workflow stages (via `Sandbox`). Today they inherit the operator's full process env — any credential the operator exported leaks to user-controlled commands. +- **`SESSION_SECRET` autogeneration is the real startup bypass.** `load_or_create_local_session_secret` mints a value and writes the envfile if missing — a server can boot with a freshly-minted secret that bypassed the install flow's full setup. Same precedent already removed for `FABRO_DEV_TOKEN` (commit `7cb6c65d5`); SESSION_SECRET is the leftover. +- **`FABRO_JWT_*` is stale config drift.** The CLI auth migration (`2026-04-19-003-feat-cli-auth-login-plan.md`) consolidated cookie + JWT signing into a single HKDF chain rooted at SESSION_SECRET. The legacy `FABRO_JWT_PRIVATE_KEY` / `FABRO_JWT_PUBLIC_KEY` envfile entries no longer participate in the runtime auth model but install still generates them and diagnostics still inspects them — they obscure the actual auth model and risk operator confusion. +- **CLI install and web install have drifted into separate persistence/restart behaviors.** They write to `server.env` via parallel code paths today; the bug-surface of "what does install actually persist" is twice as large as it should be. They should reconverge on a shared orchestration with consistent contracts. + +## Requirements Trace + +- R1. `ServerSecrets` reads env and envfile *once* at boot and exposes the merged snapshot. No live env reads from `ServerSecrets::get` or its callers on the resolution path. Env wins on conflict. +- R2. `fabro server start` does not generate any server-level secret. Missing → fail fast pointing at the install flows (CLI or web) or direct env-set. +- R3. `execute_foreground` does not mutate parent process env. +- R4. `execute_daemon` does not pass server-level secrets via `cmd.env(...)` (no longer needed; daemon child reads env+file at its own boot). +- R5. Worker and render-graph subprocesses use `env_clear` + strict fail-closed allowlists. New inherited env vars require demonstrated need plus tests. Authority-bearing values re-injected explicitly. +- R6. Tests use construction-time env stubs (via a small `EnvSource` trait) or explicit child-process `Command::env`. No test mutates process-wide env. +- R7. Foreground and daemon startup use one shared validation path; required secrets mirror current auth rules (SESSION_SECRET always; FABRO_DEV_TOKEN when dev-token auth enabled; GITHUB_APP_CLIENT_SECRET when GitHub auth enabled). +- R8. CLI install and web install share orchestration for coordinated `server.env` writes and restart/handoff. +- R9. `FABRO_JWT_PRIVATE_KEY` / `FABRO_JWT_PUBLIC_KEY` are removed from install output, diagnostics, docs, and existing `server.env` files during install flows. +- R10. Behavior changes are documented in a strategy doc and enforced via workspace-wide `clippy::disallowed_methods` — including tests — with narrow documented exceptions only. + +## Scope Boundaries + +**In scope, active** — server-level secrets read via `state.server_secret(...)`, sourced from process env or `/server.env`: + +| Secret | Consumer | Sources | +|---|---|---| +| `SESSION_SECRET` | `AppState::session_key` (`server.rs:713`); HKDF source for cookie key + JWT signing key | env (12-factor) or `server.env` (install) | +| `FABRO_DEV_TOKEN` | `worker_command` (`server.rs:3825`) | env or `server.env` | +| `GITHUB_APP_PRIVATE_KEY` | `AppState::github_credentials` (`server.rs:726`) | env or `server.env` | +| `GITHUB_APP_WEBHOOK_SECRET` | `github_webhook_routes` (`server.rs:983`) | env or `server.env` | +| `GITHUB_APP_CLIENT_SECRET` | `web_auth.rs:536` | env or `server.env` | + +Both install flows (CLI `fabro install` and web install via `/install/finish`) write the envfile; neither mutates process env. Operators on 12-factor PaaS set secrets in platform env; operators on local/server installs use one of the install flows. + +**In scope, removal targets:** + +- `FABRO_JWT_PRIVATE_KEY`, `FABRO_JWT_PUBLIC_KEY` — legacy entries that no longer participate in the runtime auth model. Install stops generating them; diagnostics stops inspecting them; existing entries are actively removed from `server.env` on subsequent install flows. + +**Startup-critical subset:** SESSION_SECRET (always), FABRO_DEV_TOKEN (when dev-token auth is enabled), and GITHUB_APP_CLIENT_SECRET (when GitHub auth is enabled). The other in-scope active secrets (GITHUB_APP_PRIVATE_KEY, GITHUB_APP_WEBHOOK_SECRET) are not boot blockers — they continue to surface where they did before (conditional route mounts, per-request errors). The plan does not promote every server secret to a startup gate. + +**Out of scope:** + +- **Vault + `ProviderCredentials`** (`secrets.json`, REST-managed, runtime-mutable). Different lifecycle. +- **`vault_or_env` for run-level credentials** (`GITHUB_TOKEN`, `DAYTONA_API_KEY`). Same env-as-source pattern, different track. +- **`{{ env.FOO }}` config interpolation.** Operator-supplied templating, different feature. +- **Workflow-stage env (Sandbox).** Stages run inside `Sandbox` (local/Docker/Daytona); their env is configured by the workflow definition + Sandbox config. Anything a workflow stage needs to execute (e.g. `git push` requiring `GITHUB_TOKEN`) routes via Vault → Sandbox, not subprocess inheritance. +- **`validate_api_key` `set_var` in `provider_auth.rs`.** Vault-side smell, deferred. +- **Tailscale and `bun --watch-web` spawns.** Different surfaces. + +## Context & Research + +### Relevant code + +- `lib/crates/fabro-server/src/server_secrets.rs` — `ServerSecrets` definition; today's live `env_lookup` closure is the bug. +- `lib/crates/fabro-server/src/server.rs:697-699` — `state.server_secret(name)` wrapper. +- `lib/crates/fabro-server/src/server.rs:703,712-715` and `jwt_auth.rs:84-99` — `resolve_auth_mode_with_lookup`, currently routed through the live `env_lookup`. +- `lib/crates/fabro-server/src/server.rs:3776-3834` (`worker_command`) — worker spawn site. +- `lib/crates/fabro-server/src/server.rs:7232-7235` — `__render-graph` spawn site. +- `lib/crates/fabro-cli/src/commands/server/start.rs:229-274` — `load_or_create_local_session_secret` and `execute_foreground` `set_var` block. +- `lib/crates/fabro-cli/src/commands/server/start.rs:319-362` — `execute_daemon` spawn flow. +- `lib/crates/fabro-cli/src/commands/install.rs:1816,1856,1864` — install SESSION_SECRET generation and persistence. +- `lib/crates/fabro-config/src/envfile.rs:204-256` — `write_env_entries`, already atomic via tmp + fsync + rename. +- `Cargo.toml:111` and `clippy.toml` — existing `disallowed_methods` mechanism. + +### Institutional learnings + +- `docs/plans/2026-04-05-server-canonical-secrets-doctor-repo-plan.md` — architectural anchor: server is canonical, install provisions, request-time reads from store. +- `docs/plans/2026-04-19-003-feat-cli-auth-login-plan.md` — `SESSION_SECRET` is now HKDF master for JWT signing + cookie key. Higher stakes. +- `docs/plans/2026-04-18-001-feat-webhook-strategy-plan.md` — webhook secret already plumbed via `AppState` from the resolved store. +- Commit `7cb6c65d5` — "Remove dev-token minting from server start." SESSION_SECRET autogenerate is the leftover. + +## Key Technical Decisions + +- **`ServerSecrets` is a snapshot, not a live reader.** A small `EnvSource` trait exists *only at construction time* and immediately materializes into an owned `HashMap`. No trait object or callback survives into runtime lookup. `get(name)` returns from the merged (env ∪ file) snapshot; env wins on conflict (12-factor). Role: the *resolved-secrets API*; the source mix is the operator's choice. +- **Production uses a real process-env `EnvSource`; tests use a map-backed stub.** No live env reads anywhere on the secret resolution path. No `EnvLookup` closure surviving into runtime. +- **`resolve_auth_mode_with_lookup` migrates to read from `server_secrets`, not raw env.** Closure passed in delegates to `self.server_secrets.get(...)`. Without this, auth-mode resolution remains a live env reader. +- **Worker and render-graph allowlists are strict fail-closed.** Worker is the trust boundary (dispatches user stages via `Sandbox`); render-graph is hygiene. New inherited env vars require demonstrated need (failing test) and intentional addition. Proxy/cert env vars are not auto-inherited. + - Worker list: `PATH`, `HOME`, `TMPDIR`, `USER`, `RUST_LOG`, `RUST_BACKTRACE`, `FABRO_HOME`, `FABRO_STORAGE_ROOT`. Plus explicit `FABRO_DEV_TOKEN` re-injection when dev-token auth is enabled. + - Render-graph list: `PATH`, `HOME`, `TMPDIR`. Plus explicit `FABRO_TELEMETRY=off`. +- **Daemon child spawn inherits parent env unchanged** (modulo existing `cmd.env_remove("FABRO_JSON")` for output-format hygiene). The daemon is fabro's own server code, not a security boundary against itself. 12-factor env vars (SESSION_SECRET, AWS_*, RAILWAY_*) flow through naturally to the daemon child's snapshot. +- **Daemon preflight reuses the same shared auth/startup validation as foreground startup.** Both modes construct the same `ServerSecrets` snapshot and call the same server-side validation logic (today's `jwt_auth::resolve_auth_mode_with_lookup` already encodes "what's required for this auth mode" — that's the shared path). No separate parent-CLI validator with drift risk. Required secrets remain exactly the current startup-critical set: SESSION_SECRET (always), FABRO_DEV_TOKEN (when dev-token auth enabled), GITHUB_APP_CLIENT_SECRET (when GitHub auth enabled). GITHUB_APP_PRIVATE_KEY and GITHUB_APP_WEBHOOK_SECRET remain in-scope server secrets but are not new universal boot blockers. +- **Install coordination policy lives in shared install orchestration, not in low-level envfile helpers.** Coordinated install flows (CLI and web) may write to `server.env` while a server is running — but only when they also own restart/handoff. Web install already does this via `/install/finish`. CLI install joins the same orchestration. Manual edits to `server.env` outside the orchestration still require operator restart discipline. +- **`pub(crate) server_secrets` field tightened to `pub(super)`.** No production code legitimately bypasses the wrapper. +- **Workspace-wide clippy ban including tests.** Add `std::env::set_var`/`remove_var` to `clippy.toml` `disallowed-methods` — applies to production AND test code. Tests must use construction-time `EnvSource` stubs or explicit child-process `Command::env`. Narrow documented exceptions only (`fabro-telemetry/src/spawn.rs:73-76` post-fork pre-execvp). +- **Legacy `FABRO_JWT_*` is removed, not preserved.** Install stops generating; diagnostics stops reading; existing envfile entries are actively cleaned up on subsequent install flows. SESSION_SECRET is the sole auth master. + +## Open Questions + +### Resolved during planning + +- **`ServerSecrets` reads env AND file, snapshots, env wins.** Process env is a legitimate source (12-factor). The bug was *live* env reads + parent mutation, not env-as-source. +- **`EnvSource` exists only at construction time.** A small trait used to materialize an owned `HashMap` once; no callback or trait object survives into runtime lookup. Production uses a real process-env source; tests use a map-backed stub. +- **Daemon child inherits parent env; only worker/render-graph scrub.** Daemon is fabro's own server code and must see whatever the operator/platform set in env. Worker is the trust boundary (dispatches user stages via `Sandbox`). +- **No carve-out for AWS / cloud-platform credentials.** Daemon inherits parent env, so AWS env names reach the object-store layer naturally. +- **Active scope is 5 server secrets plus 2 legacy removal targets.** SESSION_SECRET, FABRO_DEV_TOKEN, GITHUB_APP_PRIVATE_KEY, GITHUB_APP_WEBHOOK_SECRET, GITHUB_APP_CLIENT_SECRET are active. FABRO_JWT_PRIVATE_KEY and FABRO_JWT_PUBLIC_KEY are legacy — install stops generating them; diagnostics stops reading them; existing entries are actively removed. +- **Workflow stage env is the Sandbox's job.** This plan does not address workflow `bash`/`git`/etc. stage env — that's per-workflow Sandbox config. Workflows requiring credentials route them through Vault. +- **Web install keeps its existing restart-handoff contract.** `/install/finish` still returns the restart URL and the SPA still waits for the server to come back. No new `restart_required` flag, no manual-restart message proposal, no API surface change. +- **CLI and web install share orchestration for coordinated writes while running.** Blanket "refuse-while-running" is out. The orchestration is responsible for restart/handoff; both flows call into it. +- **Test migration shape:** full migration in Unit 1. `ServerSecrets::with_env_lookup` is deleted; tests use either `ServerSecrets::load(path, env_source)` with a map-backed `EnvSource` stub, or `provision_server_secrets(env_path, &[(name,value)])` helper (file-based injection). No `#[cfg(test)]` backdoor. +- **Visibility tightening:** `pub(crate) server_secrets` at `server.rs:579` → `pub(super)` as part of Unit 1. +- **Install lock window for interactive flows:** install orchestration's job; `gather inputs → persist + restart-handoff`. Serialization window is sub-second regardless of OAuth duration. +- **Dev-loop friction:** no `--quick` flag. Contributors run `fabro install` or set secrets directly via env (`SESSION_SECRET=$(openssl rand -hex 32) cargo run -- server start` works under the env-as-source model). +- **Compliance-driven rotation:** deferred. Captured as a known limitation in the strategy doc. + +## Implementation Units + +- [ ] **Unit 1: `ServerSecrets` becomes a snapshot built from `EnvSource` (construction-time only)** + +**Goal:** `ServerSecrets` reads env and envfile *once* at construction via a small `EnvSource` trait, materializes both into owned `HashMap`s, and exposes the merged snapshot. No trait object or closure survives into runtime lookup. Auth-mode resolution stops being a live env reader. + +**Requirements:** R1, R6 + +**Dependencies:** None. + +**Files:** +- Modify: `lib/crates/fabro-server/src/server_secrets.rs` — replace `env_lookup` closure field with `env_entries: HashMap` (owned). Introduce a small `EnvSource` trait used *only* at construction: + - `pub trait EnvSource { fn snapshot(&self) -> HashMap; }` + - `pub struct ProcessEnv;` impls `snapshot` via `std::env::vars().collect()`. + - `pub struct StubEnv(pub HashMap);` impls `snapshot` by cloning. (cfg-test or cfg(any(test, feature = "test-support")) — implementer picks based on existing patterns.) + - `pub fn load(path: PathBuf, env: &dyn EnvSource) -> Result` — calls `env.snapshot()` once, stores the result, never references `env` again. + - `get(name)` returns `self.env_entries.get(name).cloned().or_else(|| self.file_entries.get(name).cloned())`. +- Modify: `lib/crates/fabro-server/src/server.rs` — `build_app_state` accepts pre-built `ServerSecrets` and `AuthMode` from the caller (no longer constructs `ServerSecrets` itself). The single production caller is `serve_command` (via `resolve_startup` from Unit 4); test helpers construct both in the same way. Tighten `pub(crate) server_secrets` field at `server.rs:579` to `pub(super)`. Migrate the `resolve_auth_mode_with_lookup` call (`server.rs:703`) to pass a closure delegating to `self.server_secrets.get(...)`. +- Modify: `lib/crates/fabro-server/src/serve.rs:512` and `install.rs:910,965,2108` — minimal API migration: `ServerSecrets::load(path, &ProcessEnv)` instead of `with_env_lookup` at any *non-startup* construction sites (e.g. install-time inspection, install_object_store_lookup test helper). The startup construction sites at `serve.rs:594-607` are NOT modified by Unit 1 — Unit 4 collapses them into a single `resolve_startup(...)` call. To keep the workspace compiling between Unit 1 and Unit 4, Unit 1 may leave a temporary `ServerSecrets::load(path, &ProcessEnv)` call at `serve.rs:594` if needed; Unit 4 deletes it. Sequencing is documented in Unit 4's Dependencies. +- Modify: `lib/crates/fabro-server/src/server.rs:7864-7885` — replace `server_secrets_resolve_process_env_before_server_env` with a snapshot test using `StubEnv` (assert env-wins-on-conflict from the snapshot). +- Modify: `lib/crates/fabro-server/tests/it/api/cli_auth_token.rs:34`, `routing.rs:21,35-36`, `tcp.rs:28,68,173-174`, `lib/crates/fabro-cli/tests/it/support/auth_harness.rs:39-86` — migrate from `env_lookup` injection (for *secret* values) to constructing the test's `ServerSecrets` with `&StubEnv(...)`, or writing to a temp `server.env` via the `provision_server_secrets(env_path, &[(name, value)])` helper. Tests that currently pass `env_lookup` only to feed secret values into `ServerSecrets` switch to the new mechanism. +- Modify: test helpers like `create_app_state_with_env_lookup` (`server.rs:2488` and similar) — once `build_app_state` (`server.rs:2631`) requires precomputed `ServerSecrets` and `AuthMode`, the helpers must produce both internally and pass them through. The `env_lookup` parameter on these helpers is *retained* (still consumed downstream by `resolve_canonical_origin` at `server.rs:708` and slack `{{ env.FOO }}` interpolation at `server.rs:2676`, both out of scope). New shape: helper accepts an additional `&dyn EnvSource` parameter (or constructs a `StubEnv`/`ProcessEnv` based on the call site's intent), internally calls `resolve_startup(...)` (or directly constructs `ServerSecrets` + computes `AuthMode` if the helper bypasses settings — auditor's choice per call site), and passes the resulting `ServerSecrets` and `AuthMode` into `build_app_state`. Tests that set up specific auth modes (e.g. dev-token enabled) now thread settings + `EnvSource` through the helper rather than relying on a single closure to drive both secret resolution and auth-mode computation. +- Test: existing files plus new snapshot-semantics tests in `server_secrets.rs`. + +**Approach:** +- `EnvSource::snapshot()` is called exactly once during `ServerSecrets::load`. The trait reference is dropped immediately after; only the materialized `HashMap` is retained. No risk of live env reads via trait object dispatch. +- Object-store credential resolution at `serve.rs:340-401, 520-522, 540-542` uses `std::env::var(...)` directly (daemon child inherits parent env, so AWS names are present). + +**Test scenarios:** +- Happy path: `ServerSecrets::load(path, &StubEnv([("SESSION_SECRET", "from-env")].into()))` returns "from-env" even when file has a different value. +- Edge: env empty, file has the value → file wins (single fallback path). +- Edge: neither env nor file has the value → `get` returns `None`. +- Edge: env has it, file does too, different values → env wins (12-factor). +- Edge: missing file path → empty `file_entries`, env-only resolution. +- Snapshot semantics: after construction, mutating the source `EnvSource` (or process env, if production source) does not affect `get` returns. The snapshot is owned and immutable. +- Auth-mode happy path: `resolve_auth_mode_with_lookup` resolves correctly when secrets come from env, file, or both. +- Migration: `cli_auth_token`, `routing`, `tcp`, `auth_harness` tests pass after migration to `EnvSource` stubs. + +**Verification:** +- `cargo nextest run -p fabro-server -p fabro-cli` passes. +- `grep -rn 'std::env::var\|std::env::vars' lib/crates/fabro-server/src/ | grep -v 'serve.rs\|object_store\|spawn_env\|server_secrets.rs'` shows no live env reads on the server-secret resolution path (the only `std::env::vars()` is inside `ProcessEnv::snapshot`, called once at construction). + +--- + +- [ ] **Unit 2: Drop foreground `set_var`; drop daemon `cmd.env(SESSION_SECRET)`** + +**Goal:** Eliminate parent-process env mutation. Daemon parent stops passing SESSION_SECRET via `cmd.env`; daemon child reads env+file at its own boot. + +**Requirements:** R3, R4 + +**Dependencies:** Unit 1. + +**Files:** +- Modify: `lib/crates/fabro-cli/src/commands/server/start.rs:265-274` — delete the `set_var`/scopeguard block in `execute_foreground`. +- Modify: `lib/crates/fabro-cli/src/commands/server/start.rs:350,352` — `execute_daemon` removes `cmd.env("SESSION_SECRET", ...)`. Keep existing `cmd.env_remove("FABRO_JSON")` (output-format hygiene). +- Modify: `lib/crates/fabro-test/src/lib.rs:931` — drop `cmd.env("SESSION_SECRET", ...)` for spawned test servers; tests provision via `server.env` or by setting env on the spawned `Command` explicitly when the test simulates a 12-factor PaaS scenario. +- Test: existing `cmd/server_start.rs` tests cover both modes. + +**Approach:** +- Unit 1's snapshot semantics make both `set_var` and `cmd.env(SESSION_SECRET)` unnecessary. The in-process foreground server's `ServerSecrets::load(path, &ProcessEnv)` snapshots whatever env the parent CLI had. The daemon child inherits parent env (today's behavior — unchanged) and snapshots it at its own boot. +- A `SESSION_SECRET` exported in the operator's parent shell does flow through to both modes — that's the 12-factor design intent. + +**Test scenarios:** + +Tests proving env behavior must use construction-time `EnvSource` stubs or explicit child-process `Command::env` — *not* `std::env::set_var`. Any old test using process-env mutation is migrated as part of this unit (or Unit 7's clippy enforcement will fail it). + +- Happy path (foreground, env source): in-process server constructed with `&StubEnv([("SESSION_SECRET", "from-env")].into())`; running server uses that value. +- Happy path (foreground, file source): empty `EnvSource`, value in `server.env`; running server uses the file value. +- Happy path (foreground, env-wins): env and file have different values; env wins. +- Happy path (daemon, env source): test passes `SESSION_SECRET` to spawned daemon via explicit `Command::env`; daemon child inherits and snapshots it. NOT via `std::env::set_var` in the test process. +- Happy path (daemon, file source): no env on the spawned `Command`, value in `server.env`; daemon child uses the file value. + +**Verification:** +- `grep -rn 'std::env::set_var\|remove_var' lib/crates/fabro-cli/src/` returns no hits in production code. +- `grep -rn 'cmd\.env."SESSION_SECRET"' lib/crates/fabro-cli/src/` returns no hits in production code (test code may still use `Command::env` for daemon spawn simulation; that's fine — it's setting child env, not parent env). + +--- + +- [ ] **Unit 3: Worker and render-graph spawns use `env_clear` + strict fail-closed allowlists** + +**Goal:** Worker and render-graph subprocesses inherit only an explicit allowlist. The lists are strict fail-closed — new env vars require demonstrated need (a failing test that proves a workflow execution path needs them) plus intentional addition. Authority-bearing values re-injected explicitly. Daemon child spawn is unchanged (inherits parent env per 12-factor). + +Proxy and TLS env vars (`HTTPS_PROXY`, `SSL_CERT_FILE`, etc.) are *not* inherited unless usage-proven by a failing test and intentionally added — they were never part of the worker's documented contract. + +**Requirements:** R5 + +**Dependencies:** None. + +**Files:** +- Create: `lib/crates/fabro-server/src/spawn_env.rs` — defines two helpers: + - `apply_worker_env(&mut tokio::process::Command)` — `env_clear` + worker list. + - `apply_render_graph_env(&mut tokio::process::Command)` — `env_clear` + render-graph list. +- Modify: `lib/crates/fabro-server/src/server.rs:3776-3834` — `worker_command` calls `apply_worker_env(&mut cmd)` first, then existing `cmd.env("FABRO_DEV_TOKEN", token)` re-injection. Remove existing `env_remove("FABRO_JSON")` and `env_remove("FABRO_DEV_TOKEN")` (covered by `env_clear`). +- Modify: `lib/crates/fabro-server/src/server.rs:7232-7235` — `__render-graph` spawn calls `apply_render_graph_env(&mut cmd)`. Keep explicit `cmd.env("FABRO_TELEMETRY", "off")`. +- Test: new tests in `spawn_env.rs`. + +**Approach:** + +```text +WORKER list (each entry has // reason: ... in the source): + PATH, HOME, TMPDIR, USER // process essentials + RUST_LOG, RUST_BACKTRACE // diagnostics + FABRO_HOME, FABRO_STORAGE_ROOT // worker reads its own state ++ explicit cmd.env("FABRO_DEV_TOKEN", token) when dev-token auth enabled + +RENDER_GRAPH list: + PATH, HOME, TMPDIR ++ explicit cmd.env("FABRO_TELEMETRY", "off") + +DAEMON spawn: no helper. Inherits parent env. Keeps existing cmd.env_remove("FABRO_JSON"). +``` + +The lists are constants in `spawn_env.rs`. Each entry is one named env var with a one-line `//` comment. A worker that needs a new env var must be amended in source — that's the entire mechanism. + +**Test scenarios:** +- Happy path (worker): with allowlisted names in parent env, all reach the worker. Random names (`MY_API_KEY`, `NEW_RELIC_LICENSE_KEY`, `DATABASE_URL`, `SESSION_SECRET=leak`) do not. +- Happy path (worker, dev-token enabled): `FABRO_DEV_TOKEN` is set to the install-provisioned value via explicit re-injection. +- Negative (worker): parent's `FABRO_DEV_TOKEN=garbage` does not reach the worker; explicit re-injection sets the correct value. +- Happy path (render-graph): `PATH` and `HOME` reach the child; arbitrary parent vars do not. +- Integration (render-graph): with `FABRO_TELEMETRY=on` in parent env, child sees `off` (explicit override after `env_clear`). +- Integration: real worker subprocess executes a workflow end-to-end with leak probes (`MY_API_TOKEN=leak`, `NEW_RELIC_LICENSE_KEY=leak`) exported in parent env. Workflow completes; leak probes do not appear in worker logs or in stage env (Sandbox-configured). + +**Verification:** +- `cargo nextest run -p fabro-server` passes. +- A test asserts the worker spawn sees *only* names in the allowlist plus the explicit `FABRO_DEV_TOKEN` — i.e. "no ambient inheritance except allowlisted names." Same for render-graph. The check operates by enumerating the child's actual env, not by greping `env_remove` calls. +- `grep -rn 'env_remove' lib/crates/fabro-server/src/` returns no hits for the worker (`server.rs:3776-3834`) or render-graph (`server.rs:7232-7235`) spawn sites — both routes are now via `env_clear` + the helpers. +- `grep -rn 'env_remove' lib/crates/fabro-cli/src/commands/server/start.rs` returns the single intentional `cmd.env_remove("FABRO_JSON")` on the daemon-spawn path (output-format hygiene; daemon child unchanged per Unit 2). No other `env_remove` calls in CLI server code. + +--- + +- [ ] **Unit 4: Shared startup validation via snapshot-backed `ServerSecrets`** + +**Goal:** Server start no longer auto-generates. Daemon preflight constructs the same `ServerSecrets` snapshot foreground startup uses and runs the *same* server-side auth/startup validation logic. No separate parent-CLI validator that could drift from the server-side rules. + +**Requirements:** R1, R2, R7 + +**Dependencies:** Units 1-3. + +**Files:** +- Create: `lib/crates/fabro-server/src/startup.rs` (or extend an existing module) — define the shared validation logic. Two entry points around it: a public preflight wrapper (CLI calls this; never sees `ServerSecrets`), and a crate-internal full-resolution function (`serve_command` calls this; consumes the snapshot directly). Both share a single internal implementation so there is no possibility of drift. + + ```text + // Crate-internal: returns full state for in-process consumption. + pub(crate) struct StartupResolution { + pub(crate) auth_mode: AuthMode, + pub(crate) server_secrets: ServerSecrets, + } + + pub(crate) fn resolve_startup( + env_path: &Path, + env: &dyn EnvSource, + settings: &ResolvedServerSettings, + ) -> Result + + // Public: thin wrapper for CLI preflight. Calls resolve_startup, drops the + // StartupResolution after validating, returns just the success/failure. + pub fn validate_startup( + env_path: &Path, + env: &dyn EnvSource, + settings: &ResolvedServerSettings, + ) -> Result<(), StartupValidationError> + ``` + + Internally `resolve_startup` constructs `ServerSecrets::load(env_path, env)`, then runs the existing `jwt_auth::resolve_auth_mode_with_lookup` against a closure delegating to that snapshot, then returns both. `validate_startup` is `resolve_startup(...).map(|_| ())` — the literal sharing of code makes drift impossible. + + Function takes `&ResolvedServerSettings` (not just `&ServerAuthSettings`) because validation already depends on `server.web.enabled` and `server.integrations.github.client_id` (`jwt_auth.rs:63`) — narrowing would silently weaken the validation surface. + + `StartupValidationError` *wraps or re-uses the existing error type* returned by `jwt_auth::resolve_auth_mode_with_lookup` plus the secret-loading errors from `ServerSecrets::load`. It must cover the full surface that path rejects today: missing required secret (`SESSION_SECRET`, `FABRO_DEV_TOKEN` when dev-token auth enabled, `GITHUB_APP_CLIENT_SECRET` when GitHub auth enabled), invalid secret value (e.g., malformed `FABRO_DEV_TOKEN`), empty auth methods, GitHub auth configured with `server.web.enabled = false`, missing `server.integrations.github.client_id`. Implementer audits `jwt_auth.rs:67` to enumerate the full variant set; the plan does not invent a narrow new one. + + CLI never sees `ServerSecrets`. `ServerSecrets` and `resolve_startup` stay `pub(crate)`. Only `validate_startup` + `EnvSource` + `ProcessEnv` + `StartupValidationError` cross the crate boundary. +- Modify: `lib/crates/fabro-server/src/lib.rs` — re-export `startup::{validate_startup, EnvSource, ProcessEnv, StartupValidationError}` from the crate root. **Not** `resolve_startup` or `StartupResolution`. +- Modify: `lib/crates/fabro-cli/src/commands/server/start.rs:229-250` — delete `load_or_create_local_session_secret`. Daemon preflight calls `fabro_server::validate_startup(runtime_directory.env_path(), &fabro_server::ProcessEnv, &resolved_settings)`. On `Err`: surface the error to stderr exactly as returned (text comes from the shared error type's `Display`). +- Modify: `lib/crates/fabro-cli/src/commands/server/start.rs:264` — `execute_foreground` does not run a separate validator. The in-process server's `serve_command` calls `resolve_startup` internally and threads the returned `(ServerSecrets, AuthMode)` into `build_app_state` (replacing today's separate `ServerSecrets::with_env_lookup` + `resolve_auth_mode_with_lookup` calls at `serve.rs:594-607`). Single source of truth: foreground's resolution and daemon's preflight share the same internal `resolve_startup`; only the calling surface differs. +- Modify: `lib/crates/fabro-server/src/serve.rs:594-607` — replace today's ad-hoc two-step construction with a single `resolve_startup(...)` call. The returned `ServerSecrets` and `AuthMode` are passed into `build_app_state` (per Unit 1's revised signature). +- Modify: `lib/crates/fabro-cli/src/commands/server/start.rs:350` — `execute_daemon` runs `validate_startup` before spawning the child. +- Test: `tests/it/cmd/server_start.rs` adds missing-secret tests for each required secret across both modes, plus tests demonstrating env-source success, file-source success, and each non-missing-key rejection (empty auth methods, web-disabled with GitHub auth, missing client_id, invalid dev token). A unit test in `fabro-server` asserts `validate_startup` and `resolve_startup` return identical accept/reject decisions for the same inputs. + +**Approach:** +- Required secrets remain *exactly* the current startup-critical set, encoded once in the shared validation: SESSION_SECRET (always), FABRO_DEV_TOKEN (when dev-token auth is enabled), GITHUB_APP_CLIENT_SECRET (when GitHub auth is enabled). GITHUB_APP_PRIVATE_KEY and GITHUB_APP_WEBHOOK_SECRET remain in-scope server secrets but are NOT new universal boot blockers — they continue to surface where they did before (conditional route mounts, per-request errors). +- Daemon preflight and foreground startup run the same validation function with the same `ServerSecrets` snapshot type; "drift" is impossible by construction. + +**Test scenarios:** +- Happy path: required secrets in env → boots (both modes). +- Happy path: required secrets in file → boots (both modes). +- Happy path: env wins when both present. +- Error path (each required secret): missing in both → fail fast naming the secret and both sources (both modes get identical error text because they call the same function). +- Negative regression: post-Unit-1 snapshot is built once; mutating env after boot doesn't change the resolved value. + +**Verification:** +- `fabro server start` against an empty env and uninstalled storage exits non-zero with the same error message in both `--foreground` and daemon modes. +- `fabro server start` with secrets in env (12-factor simulation) boots without any install flow having run. +- `grep -rn 'generate_session_secret' lib/crates/fabro-cli/src/commands/server/` returns no hits. +- `grep -rn 'ServerSecrets\|server_secrets::\|resolve_startup\|StartupResolution' lib/crates/fabro-cli/` returns no hits — CLI never sees `ServerSecrets` or the internal resolution. Only `validate_startup` is reachable from outside `fabro-server`. +- The public secret-related surface from `fabro-server`'s crate root is exactly: `validate_startup`, `EnvSource`, `ProcessEnv`, `StartupValidationError`. Nothing else. +- Foreground startup at `serve.rs:594-607` makes exactly one call to compute `(ServerSecrets, AuthMode)` — `resolve_startup(...)` — not a separate `ServerSecrets::with_env_lookup` followed by `resolve_auth_mode_with_lookup`. Both values are passed into `build_app_state`. +- `build_app_state` no longer constructs `ServerSecrets`; it accepts pre-built `ServerSecrets` and `AuthMode` from the caller. Verified by reading the signature. + +--- + +- [ ] **Unit 5: Shared install orchestration for coordinated `server.env` writes and restart handoff** + +**Goal:** CLI install and web install share a single orchestration layer for `server.env` persistence. Both flows take the same path through generation, persistence, removal of legacy entries, and restart handoff. Web install keeps its existing `/install/finish` restart-handoff contract; CLI install joins the same orchestration. No blanket refuse-while-running policy. + +**Requirements:** R8 + +**Dependencies:** Pairs naturally with Unit 6 (legacy `FABRO_JWT_*` removal hooks into the same orchestration). Either order works. + +**Files:** +- Modify: `lib/crates/fabro-install/src/lib.rs` — establish the shared orchestration entry points. Both CLI install and web install call into these. Orchestration owns: + - input gathering (no `server.env` write, no serialization) + - persistence (atomic `server.env` write via existing `envfile::merge_env_file` at `envfile.rs:204-256`) + - legacy entry removal (Unit 6 hook for `FABRO_JWT_*`) + - restart/handoff (web install via `/install/finish`'s existing restart URL contract; CLI install determines its own handoff — for the in-process case, exit cleanly; for daemon mode, respect today's `fabro server stop` + restart pattern) +- Modify: `lib/crates/fabro-cli/src/commands/install.rs:1864, 1213` — route `server.env` writes through the shared orchestration in `fabro-install`. CLI-specific UX (prompts, progress display) remains in `commands/install.rs`; persistence does not. +- Modify: `lib/crates/fabro-server/src/install.rs:1313, 1343-1353, 1380` — server-side `/install/finish` handlers route through the same orchestration. The existing `/install/finish` API contract (returns restart URL; SPA polls until server returns) is preserved exactly — no `restart_required` flag, no API surface change. +- Test: parity tests in `lib/crates/fabro-install/tests/` prove CLI install and web install take the same persistence/removal path with the same outputs given the same inputs. + +**Approach:** +- The shared orchestration in `fabro-install` is the single chokepoint for `server.env` writes from install flows. Manual edits to `server.env` outside the orchestration still require operator restart discipline (documented). +- Coordinated install flows MAY write while a server is running — they own restart/handoff. Web install already has the handoff via `/install/finish`; CLI install gets the same primitives. +- Two-phase shape: `gather inputs (no serialization)` → `persist + restart-handoff (atomic)`. Serialization window is sub-second regardless of OAuth duration. +- No PID-comparison logic, no refuse-while-running predicate, no new chokepoint helper in `fabro-config`. The earlier `write_server_env_serialized` proposal is dropped. + +**Test scenarios:** +- Parity: identical install inputs produce identical `server.env` contents and identical removed entries via CLI and web paths. +- Happy path (CLI): install on stopped server succeeds; `server.env` updated atomically. +- Happy path (web): install on running server completes via `/install/finish`; restart URL returned; SPA reconnects after restart. +- Legacy removal: install (CLI or web) on a `server.env` containing `FABRO_JWT_*` entries leaves the file without them (Unit 6 hook). +- Negative regression: install-then-start works in the common single-shell CLI sequence. + +**Verification:** +- `cargo nextest run -p fabro-install -p fabro-cli` passes including parity tests. +- `grep -rn 'envfile::write_env_entries\|envfile::merge_env_file' lib/crates/fabro-cli/src lib/crates/fabro-server/src` shows `server.env` writes routed through `fabro-install` orchestration; no direct calls outside it. +- `/install/finish` API contract unchanged (existing OpenAPI schema and SPA reconnect tests pass without modification). + +--- + +- [ ] **Unit 6: Remove legacy `FABRO_JWT_*` drift** + +**Goal:** Stop generating `FABRO_JWT_PRIVATE_KEY` / `FABRO_JWT_PUBLIC_KEY`, stop reading them in diagnostics, remove operator/docs references that describe them as auth inputs, and actively remove existing entries from `server.env` during install flows. SESSION_SECRET is the sole auth master. + +**Requirements:** R9 + +**Dependencies:** None for code shape; the active-removal hook plugs into Unit 5's shared install orchestration. + +**Files:** +- Modify: `lib/crates/fabro-cli/src/commands/install.rs:1853-1855` — delete `generate_jwt_keypair()` call and the corresponding `("FABRO_JWT_PRIVATE_KEY", ...)` / `("FABRO_JWT_PUBLIC_KEY", ...)` entries from `generated_server_env_pairs`. +- Modify: `lib/crates/fabro-server/src/install.rs` (whichever lines mirror the CLI side, surfaced in research) — same deletion on the web install side. +- Modify: `lib/crates/fabro-install/src/lib.rs` (Unit 5's shared orchestration) — add `FABRO_JWT_PRIVATE_KEY` and `FABRO_JWT_PUBLIC_KEY` to a `legacy_keys_to_remove` set; the persistence step writes these as removals on every install run. +- Modify: `lib/crates/fabro-server/src/diagnostics.rs:540, 552` — remove the `state.server_secret("FABRO_JWT_PUBLIC_KEY")` and `state.server_secret("FABRO_JWT_PRIVATE_KEY")` checks. Adjust diagnostics output and tests accordingly. +- Modify: **all** operator-facing references to `FABRO_JWT_PRIVATE_KEY` / `FABRO_JWT_PUBLIC_KEY` across `docs/`, `apps/marketing/`, README, and any in-repo runbook. The criterion is "any mention," not "mention as auth input." Concrete known-stale targets: + - `docs/administration/server-configuration.mdx:297` — remove the "future CLI login flows" reference and any surrounding prose treating these as install/runtime secrets. + - `docs/administration/server-configuration.mdx:331` — remove the row(s) from the "Server authentication" table. + - Implementer must also `grep -rn 'FABRO_JWT_PRIVATE_KEY\|FABRO_JWT_PUBLIC_KEY' docs/ apps/ README*` and either delete or rewrite every hit. Surviving mentions should appear only in (a) the new strategy doc's "removed" section or (b) historical plans under `docs/plans/`. +- Modify: install snapshot tests, server.env fixture files, and any insta snapshots that assert on `FABRO_JWT_*` lines — regenerate. +- Test: install run against a `server.env` pre-seeded with `FABRO_JWT_*` entries — assert they are absent after install completes. + +**Approach:** +- Deletion is the entire change. No deprecation period; no compatibility shim. The keys haven't participated in runtime auth since the CLI auth login migration (`2026-04-19-003`). +- Operators upgrading run install once; legacy entries are silently cleaned up. The strategy doc records the cleanup for any operator who notices. + +**Test scenarios:** +- Happy path: install on a fresh storage dir produces a `server.env` with no `FABRO_JWT_*` entries. +- Happy path (cleanup): install on a `server.env` containing `FABRO_JWT_PRIVATE_KEY=...` and `FABRO_JWT_PUBLIC_KEY=...` produces an updated `server.env` without those keys, with other entries preserved. +- Diagnostics: `fabro doctor` (or whichever command surfaces diagnostics) does not mention `FABRO_JWT_*`. + +**Verification:** +- `grep -rn 'FABRO_JWT_PRIVATE_KEY\|FABRO_JWT_PUBLIC_KEY\|generate_jwt_keypair' lib/crates apps/ docs/ README*` returns hits *only* in (a) the new strategy doc's "removed" section, (b) historical plan files under `docs/plans/`. No hits in operator-facing docs (`docs/administration/`, `docs/quickstart/`, marketing) or production crate code. +- `cargo nextest run -p fabro-cli -p fabro-server` passes with regenerated snapshots. +- Mintlify docs build succeeds with the removed entries (no broken anchors/links from other pages that referenced the JWT-key sections). + +--- + +- [ ] **Unit 7: Strategy doc + workspace-wide clippy enforcement** + +**Goal:** Document the design and encode the rules as compile-time enforcement applied to *all* code including tests. + +**Requirements:** R10 + +**Dependencies:** Units 1-6. + +**Files:** +- Create: `docs-internal/server-secrets-strategy.md`. +- Modify: `clippy.toml` — add `std::env::set_var` and `std::env::remove_var` to `disallowed-methods` with reason text pointing at the strategy doc. The ban is workspace-wide and applies to tests as well as production code. +- Modify: `lib/crates/fabro-telemetry/src/spawn.rs:73-76` — add `#[expect(clippy::disallowed_methods, reason = "post-fork pre-execvp env mutation; safe because grandchild is single-threaded and about to be replaced via exec")]`. +- Modify: any other production caller surfaced during implementation (the count is small per Unit 1's grep) — same `#[expect]` pattern with documented reason. +- Modify: `CLAUDE.md` (and `AGENTS.md` if separate) — add "Strategy docs" entry pointing at the new doc. + +**Approach:** + +Strategy doc covers: +- **`ServerSecrets` is the resolved-secrets API.** Sources are process env (12-factor) and `/server.env` (install/local convenience). Snapshotted at boot via `EnvSource` trait that materializes immediately into owned `HashMap`. Env wins on conflict. +- **The five active server-level secrets** and their consumers (table from this plan). `FABRO_JWT_*` is removed (Unit 6); SESSION_SECRET is the sole auth master via HKDF. +- **Provisioning paths:** CLI install, web install (shared orchestration in `fabro-install`), or platform env. Server start does not auto-generate. +- **Tests must not mutate process env.** Enforced workspace-wide by clippy. Tests inject via construction-time `EnvSource` stubs (e.g. `StubEnv`) for in-process resolution, or via explicit child-process `Command::env` for subprocess simulation. +- **Worker and render-graph env:** `env_clear` + strict fail-closed allowlist. Daemon child inherits parent env (12-factor pattern). New worker env entries require demonstrated need + intentional addition. +- **Install while running:** allowed only through the shared install orchestration which owns restart/handoff. Manual `server.env` edits still require restart discipline. +- **Rotation:** restart required. Live rotation intentionally not supported. Compliance-driven N+1 rotation (overlap windows) is a known limitation tracked as follow-up. +- **Out of scope (with reasons):** Vault + `ProviderCredentials`, `vault_or_env` for run-level credentials (different track), `{{ env.FOO }}` config interpolation, workflow-stage env (Sandbox's job), `validate_api_key` `set_var` smell, Tailscale spawns, `bun --watch-web`. +- **Adding a new server-level secret:** (1) provision via the install orchestration or platform env; (2) consume via `state.server_secret(...)`; (3) do not touch env in any other layer; (4) decide if it joins the startup-critical set (most don't). +- **Adding a new worker env var:** add to the worker list in `spawn_env.rs` with a one-line reason and a failing-without test that proves need. + +**Verification:** +- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` passes. +- Adding a new `std::env::set_var(...)` in any production OR test file produces a clippy denial that names the strategy doc. + +## System-Wide Impact + +- **Interaction graph:** all active server-level secrets (the five remaining after Unit 6) converge through `state.server_secret(name)` after Unit 1, which returns from the env+file snapshot. `resolve_auth_mode_with_lookup` migrates in the same unit. Legacy `FABRO_JWT_*` is removed across install, diagnostics, and docs (Unit 6). +- **Error propagation:** daemon preflight (`validate_startup`) and foreground startup (`resolve_startup`, called from `serve_command`) both delegate to the same shared validation logic introduced in Unit 4 — the public preflight wrapper is literally `resolve_startup(...).map(|_| ())`, so error messages and accept/reject decisions are identical by construction. Unit 1 provides only the snapshot machinery (`ServerSecrets` + `EnvSource`) that Unit 4's validation consumes. Object-store credential failures still surface from the AWS SDK / object_store crate (unchanged — daemon inherits AWS env). +- **State lifecycle:** `ServerSecrets` snapshot built once at boot from env+file; both immutable for process lifetime. Rotating any in-scope secret requires restart. Install flows MAY update `server.env` while a server is running when they own restart/handoff via the shared install orchestration (Unit 5). +- **Subprocess env:** workers and render-graph processes inherit only an explicit fail-closed allowlist. Daemon child inherits parent env — that's how 12-factor SESSION_SECRET reaches the daemon. +- **API surface:** no public API change. `state.server_secret(...)` signature unchanged. `ServerSecrets::with_env_lookup` removed; replaced by `load(path, &dyn EnvSource)`. `/install/finish` API contract unchanged (no `restart_required` flag, no new fields). +- **Unchanged invariants:** `ProviderCredentials`, Vault REST API, `vault_or_env` for run-level credentials, `{{ env.FOO }}` interpolation, workflow stages (configured by `Sandbox`) all behave exactly as before. + +## Risks & Dependencies + +| Risk | Mitigation | +|---|---| +| Snapshot semantics surprise: operator changes env after boot, expects the running server to pick it up | Documented behavior. Live rotation is intentionally not supported; restart required. Strategy doc is explicit. | +| Worker list is missing something a workflow stage runner inside Sandbox needs | Worker process itself only does HTTP callbacks — Sandbox handles stage env. Real workflow integration test in Unit 3 verification catches false negatives. | +| Existing deployments without `server.env` AND without env-set secrets fail to start | Intended behavior; error message names both sources. Per repo policy, accept the breakage. | +| Test migration: replacing `std::env::set_var` calls with `EnvSource` stubs touches many files | `EnvSource` + `StubEnv` pattern is mechanical; one helper per pattern. Unit 7's clippy enforcement catches stragglers at compile time so nothing slips through. | +| Shared install orchestration regression breaks CLI/web parity | Parity tests in Unit 5 explicitly assert identical persistence behavior across CLI and web paths. Both flows route through the same `fabro-install` entry points. | +| Automatic `FABRO_JWT_*` removal surprises operators who believed those keys were authoritative | Strategy doc names the cleanup explicitly. Keys haven't participated in runtime auth since `2026-04-19-003`; cleanup is overdue, not novel. Operators upgrading run install once and the cleanup happens silently. | +| Rust 2024 edition migration is a separate effort | Removing `set_var` is a prerequisite; this work doesn't gate on the edition migration. Workspace-wide clippy enforcement (Unit 7) prevents reintroduction including in tests. | + +## Documentation / Operational Notes + +- **Operator-facing change:** `fabro server start` against an empty env AND uninstalled storage now fails fast naming both sources. Document in install/quickstart. +- **12-factor PaaS deployments (Railway, Heroku, Fly):** unchanged ergonomics — set secrets in platform env, run `fabro server start`. Works without `fabro install` having run on the platform. +- **Container/k8s deployments:** if secrets are mounted as files (e.g. via projected volume into `/server.env`) or set as env vars (k8s Secret → env), both work. +- **Cloud object-store (S3 / IRSA / ECS):** unchanged — daemon inherits AWS env from parent, object-store layer reads ambient credentials. +- **Install while server running:** install flows may update `server.env` on a running server only when they coordinate restart/handoff via the shared install orchestration (CLI install or web install via `/install/finish`). Manual edits to `server.env` outside the orchestration still require restart discipline. +- **`FABRO_JWT_*` removal:** `FABRO_JWT_PRIVATE_KEY` / `FABRO_JWT_PUBLIC_KEY` are removed from the runtime auth model; SESSION_SECRET is the sole auth master for cookie and JWT derivation. Existing legacy entries are cleaned up automatically on subsequent install flows. +- **Rotation:** edit env (and restart) or edit `server.env` (and restart). Live rotation not supported. +- **Logging:** the fail-fast error appears in operator log aggregators. Pair human-readable message with a structured error code (e.g., `error_code=missing_session_secret`) so log searches match without depending on the exact string. + +## Sources & References + +- Related plans: + - `docs/plans/2026-04-05-server-canonical-secrets-doctor-repo-plan.md` — architectural anchor + - `docs/plans/2026-04-19-003-feat-cli-auth-login-plan.md` — SESSION_SECRET as HKDF master + - `docs/plans/2026-04-18-001-feat-webhook-strategy-plan.md` — webhook secret precedent + - `docs/plans/2026-04-02-001-feat-server-daemon-management-plan.md` — origin of daemon/foreground split +- Related commits: + - `7cb6c65d5` — "Remove dev-token minting from server start" (precedent for Unit 4) +- Strategy doc precedent: `docs-internal/logging-strategy.md`, `docs-internal/events-strategy.md` diff --git a/docs/plans/2026-04-22-004-refactor-worker-jwt-auth-plan.md b/docs/plans/2026-04-22-004-refactor-worker-jwt-auth-plan.md new file mode 100644 index 000000000..bdd7ad4e4 --- /dev/null +++ b/docs/plans/2026-04-22-004-refactor-worker-jwt-auth-plan.md @@ -0,0 +1,518 @@ +--- +title: "refactor: Server-issued per-run JWT for worker subprocess auth" +type: refactor +status: active +date: 2026-04-22 +deepened: 2026-04-22 +--- + +# refactor: Server-issued per-run JWT for worker subprocess auth + +## Overview + +Server mints one per-run JWT (HS256, 72h, claims include `run_id`) at every worker subprocess spawn, passes it to the worker via the `FABRO_WORKER_TOKEN` env var, and every run-scoped route accepts it. Worker stops reading `~/.fabro/auth.json`. Existing artifact-upload-token mechanism is folded into the new worker token (one credential covers all run-scoped routes). End-user auth (dev-token / github) is now strictly orthogonal to worker auth. + +## Problem Frame + +Workers POST back to the server (events, state, blobs, stage artifacts). Today the worker only authenticates because it inherits the CLI user's OAuth session from `~/.fabro/auth.json` (`fabro-cli/src/server_client.rs:312` → `AuthStore::default()` at `fabro-client/src/auth_store.rs:107`). Side effects: + +- (a) worker authenticates *as the user* — events emitted by the worker get user identity, not "system"; +- (b) any deployment where the worker doesn't share a home dir with an authenticated CLI user (containerized server, `fabro` system user, remote worker, multi-tenant) silently fails; +- (c) worker auth is implicitly coupled to end-user auth strategy when conceptually independent. + +Today's GitHub-only install (`auth.methods = ["github"]`) writes no `FABRO_DEV_TOKEN` and `worker_command` (`server.rs:3836-3845`) injects nothing — the worker has no documented credential at all. Only the home-dir steal makes it work. + +The artifact-upload-token mechanism (`server.rs:758`, `server.rs:861`) already proves the right pattern for one route. Generalize it. + +## Requirements Trace + +- R1. Worker subprocess authenticates to server with a credential the server explicitly issued, not one stolen from the user's home directory. +- R2. Credential is per-run (claim `run_id` must match path `run_id`); cross-run reuse rejected. +- R3. Credential survives server restart up to its natural 72h expiry (no operational events shortening the ceiling). +- R4. Both `start` and `resume` spawn paths re-mint a fresh 72h credential. +- R5. Worker auth works in any deployment topology, including GitHub-only installs with no `~/.fabro/auth.json` on the worker host. +- R6. Worker-emitted events stamped as a system principal (`system:worker`); originator user identity remains discoverable on the run record. +- R7. Run-scoped routes still accept end-user JWTs for non-worker callers (CLI, web UI) — fall-through, not replacement. + +## Scope Boundaries + +- Out: refresh tokens for workers (decided: hard 72h ceiling per spawn, fresh mint on resume). +- Out: multi-host / remote worker spawning (this plan makes it *possible*, doesn't deliver it). +- Out: changes to end-user auth methods (`ServerAuthMethod::DevToken | Github`). +- Out: any new `RunAuthMethod` variant — worker token bypasses `AuthenticatedSubject` entirely. +- Out: extending `RunSummary` with provenance — originator already on `RunSpec.provenance.subject` and that's enough. +- Out: SSE attach routes (`/runs/{id}/attach`, `/attach`). These are end-user read paths (web UI / CLI tail). The worker is an event *producer*, not consumer; it never calls them. They keep `AuthenticatedService` (user-JWT-only). +- Out: lifecycle endpoints (`/runs/{id}/cancel`, `/pause`, `/unpause`, `/archive`, `/unarchive`, `DELETE /runs/{id}`). The worker has no business invoking these — they remain user-JWT-only and the worker token is explicitly rejected on them (Unit 3). +- Out: any `Display` impl on `Credential::Worker` payload. Plan keeps redacted `Debug` only; do not add `Display`. Token must never be `format!`-able as a side effect. + +## Threat Model + +State the assumptions explicitly so reviewers and operators can challenge them. + +- **Trust boundary:** the server process and any worker subprocess running under the same OS user are mutually trusted. The plan does not provide isolation between workers running as the same UID — a same-UID attacker (or a workflow stage that compromises the worker process) can read `FABRO_WORKER_TOKEN` from `/proc//environ` on Linux. Multi-tenant deployments must use per-tenant OS users, per-tenant containers, or per-tenant namespaces for tenant isolation. Cross-run isolation between same-UID workers is NOT a property of this design. +- **`SESSION_SECRET` is the master key.** It signs both user JWTs (via existing `derive_jwt_key` HKDF) and worker JWTs (via new `derive_worker_jwt_key` HKDF, distinct context label). Any leak vector — backup including `server.env`, environment dump in logs, ECS task definition exposure, accidental commit, breadcrumb capture — gives the attacker the ability to mint both kinds of tokens for any user / any run. Operational guidance: store `SESSION_SECRET` in a secrets manager, exclude from logs/Sentry, document a rotation procedure (rotation invalidates ALL outstanding worker tokens AND ALL user sessions — accept as the cost of compromise response). A separate `WORKER_JWT_SECRET` could narrow this — out of scope, called out in Open Questions. +- **Worker token compromise (single run):** an attacker who exfiltrates a single worker token gains read/write on that one run's events/blobs/state for up to 72h or until terminal-status revocation, whichever first. Run-id binding limits cross-run damage. Server-side revocation set narrows the post-completion window (with the caveat in Risks: in-memory revocation does not survive server restart). +- **`SESSION_SECRET` rotation as defense:** rotating `SESSION_SECRET` is the only operator-facing mechanism today to invalidate all outstanding worker tokens. Acceptable for emergency response; not a regular rotation cadence. + +## Context & Research + +### Relevant Code and Patterns + +- `lib/crates/fabro-server/src/server.rs:291-293, 758-826, 828-869` — artifact-upload-token: claims struct, key generation (`OsRng` per boot), mint, "service token first, else user JWT" check (`authorize_artifact_upload`). This is the exact shape of the new worker token, generalized to all run-scoped routes. +- `lib/crates/fabro-server/src/server.rs:3797-3851` — `worker_command`: single spawn site for both `start` and `resume`. Already passes `--artifact-upload-token` via argv. Replace with `--worker-token`. +- `lib/crates/fabro-server/src/server.rs:4760` — `execute_run_subprocess` calls `worker_command` once per spawn; `RunExecutionMode` flows from `start_run` (`server.rs:4385`) and `create_run` (`server.rs:4156`). Single mint site covers both modes. +- `lib/crates/fabro-server/src/auth/keys.rs:41` — `derive_jwt_key(secret: &[u8])` — existing HKDF helper for the user-JWT key. Mirror for worker JWT with distinct context label `b"fabro-worker-jwt-v1"` so worker keys survive server restarts. +- `lib/crates/fabro-cli/src/commands/run/runner.rs:55-125, 239-295` — `__run-worker` entry, `HttpRunStore`, `HttpArtifactUploader`. Only seven server endpoints touched (catalogued below). +- `lib/crates/fabro-cli/src/server_client.rs:51-58, 133-137, 312-335` — `connect_server_target_direct` → `connect_target_api_client_bundle` → `resolve_target_credential` → `AuthStore::default()`. Sole worker caller is `runner.rs:66`. Can be replaced for worker only via a sibling constructor. +- `lib/crates/fabro-client/src/credential.rs:6-30` — `Credential` enum. Add `Worker(String)` variant; `bearer_token()` returns the string. +- `lib/crates/fabro-types/src/run_event/mod.rs:29-81` — `ActorRef`/`ActorKind { User | Agent | System }`. No new variant needed — stamp worker events with `ActorKind::System`. +- `lib/crates/fabro-types/src/run.rs:34-49, 68` — `RunProvenance`/`RunSubjectProvenance` already on `RunSpec`. Originator preserved at run-creation time; no schema change. +- `lib/crates/fabro-workflow/src/event.rs:1340-1493, 2580-2603` — `stored_event_fields`/`to_run_event_at`: where `actor` is set on emitted events. Today most worker events ship `actor: None`; lifecycle events get user actor server-side; agent events get `ActorKind::Agent`. Default-fill at conversion time is the surgical change. + +### Worker → server endpoint surface (the surface that needs `authorize_run_scoped`) + +| Worker call | HTTP | Path | Server handler | Auth today | +|---|---|---|---|---| +| `client.get_run_state` | GET | `/runs/{id}/state` | `get_run_state` (`server.rs:5179`) | `AuthenticatedService` | +| `client.list_run_events` | GET | `/runs/{id}/events` | `list_run_events` (`server.rs:5249`) | `AuthenticatedService` | +| `client.append_run_event` | POST | `/runs/{id}/events` | `append_run_event` (`server.rs:5199`) | `AuthenticatedService` | +| `client.write_run_blob` | POST | `/runs/{id}/blobs` | `write_run_blob` (`server.rs:5455`) | `AuthenticatedService` | +| `client.read_run_blob` | GET | `/runs/{id}/blobs/{blobId}` | `read_run_blob` (`server.rs:5482`) | `AuthenticatedService` | +| `client.upload_stage_artifact_file` | POST | `/runs/{id}/stages/{stageId}/artifacts` (octet-stream) | `put_stage_artifact` (`server.rs:5941`) | `authorize_artifact_upload` | +| `client.upload_stage_artifact_batch` | POST | same path (multipart) | same handler | same | + +### Coordination with concurrent plans + +- `docs/plans/2026-04-22-003-refactor-lock-down-server-secrets-plan.md` partially landed: `apply_worker_env` exists at `lib/crates/fabro-server/src/spawn_env.rs:22` and is already invoked from `worker_command` at `server.rs:3835`. The allowlist keeps `HOME`, so the env scrub alone does NOT fix the OAuth-from-disk steal. This plan is the worker-side fix. Per the user's "share" decision, both `apply_worker_env` (server-side) and the new `apply_sandbox_env` (worker-side) move into `fabro-util` so they share a single denylist constant — coordinate with whatever else of `2026-04-22-003` is still in flight. +- `docs/plans/2026-04-19-003-feat-cli-auth-login-plan.md` Unit 8 created `lib/crates/fabro-server/src/auth/jwt.rs` (`Claims`, `issue`, `verify`, `JwtError`) and `auth/keys.rs::derive_jwt_key`. Reuse the HKDF derivation pattern (distinct context label) and the `jsonwebtoken` primitives directly — do not route worker-token claims through user-`JwtSubject`. +- `docs/plans/2026-04-20-001-fix-cli-server-same-host-assumptions-plan.md` deliberately closed the "trust local files because same host" pattern. This plan preserves that closure — no new same-host exceptions; the worker uses an explicitly-passed token. + +### Institutional Learnings + +- No `docs/solutions/` directory exists. Prior decisions live in `docs/plans/` (see above). +- Artifact-upload-token TTL precedent is 24h (`server.rs:293`). New worker-token TTL is 72h (3× expansion) — justified because worker tokens must survive long human-in-the-loop pauses and there's no in-process refresh. + +## Key Technical Decisions + +| Decision | Rationale | +|---|---| +| Replace artifact-upload-token entirely; one JWT per run covers all run-scoped routes | Two parallel per-run JWTs is bookkeeping. Run-id binding gives the same blast-radius constraint without a separate scope. | +| Pass JWT to worker via env var `FABRO_WORKER_TOKEN`, NOT CLI arg | Symmetric with existing `FABRO_DEV_TOKEN` re-injection model (`worker_command` at `server.rs:3836-3845`). Plays naturally with the `env_clear` + explicit re-injection model in `2026-04-22-003`. Avoids token strings in `ps` output. | +| HS256, key derived from `SESSION_SECRET` via HKDF (context `b"fabro-worker-jwt-v1"`) | Workers must survive server restarts up to natural 72h expiry. `OsRng`-per-boot (artifact-upload precedent) defeats the long TTL. Distinct context label keeps it isolated from the user-JWT key. Operator rotation of `SESSION_SECRET` invalidates outstanding worker tokens — accepted. | +| 72h TTL, no refresh | Decided by user. Long-paused runs need a generous outer ceiling. Resume re-mints. Workers running > 72h continuously fail loudly — acceptable outer bound. | +| Per-run `run_id` claim, path-vs-claim check | Mirrors `maybe_authorize_artifact_upload_token`. Cross-run reuse → 403. | +| Add `Credential::Worker(String)` variant (not reuse `DevToken`) | Debug printing stays accurate; type lets us prove "worker code only constructs `Worker`" structurally. | +| New worker-only client constructor `connect_server_target_with_bearer(target, token)`, bypasses `AuthStore`/`OAuthSession` entirely | Worker should never read user OAuth. Surgical to fix at the worker callsite (one caller, `runner.rs:66`) rather than gating `resolve_target_credential` with a "are you a worker" flag. | +| Worker default-fills `actor` on emitted events to `ActorRef { kind: System, id: Some("worker"), display: Some("system:worker") }` only when variant doesn't already set it | Lifecycle events keep user actor (set server-side at the lifecycle endpoint, not by worker). Agent events keep `ActorKind::Agent`. Surgical change in `to_run_event_at`. | +| `authorize_run_scoped(parts, state, run_id)` is the single helper for all worker-touched routes | Replaces five `_auth: AuthenticatedService` extractors and the existing `authorize_artifact_upload`. One helper, one fall-through behavior. | +| Delete artifact-upload-token mechanism atomically (no transition period) | Single-server, single-codebase change. No external consumers of the old token shape. In-flight workers survive deploy via user-JWT fall-through (they hold OAuth from `auth.json`); only newly-spawned post-deploy workers exercise the new contract — those start cleanly. Atomic swap. | +| Server-only secrets (`SESSION_SECRET`, `FABRO_JWT_PRIVATE_KEY`, `GITHUB_APP_*`) must NOT leak to the worker process | **Already structurally mitigated**: `apply_worker_env` at `spawn_env.rs:22` does `env_clear` + an 8-name allowlist that excludes all of these. `worker_allowlist_is_fail_closed` test (`spawn_env.rs:80-113`) asserts `SESSION_SECRET` doesn't leak. Without this protection, an inherited `SESSION_SECRET` would let the worker derive `WorkerTokenKeys` locally and mint tokens for any run — defeating per-run binding. Verification: extend the existing fail-closed test to also assert `FABRO_JWT_PRIVATE_KEY`, `FABRO_JWT_PUBLIC_KEY`, `GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET` don't leak. | +| Server-side per-run revocation set populated when run reaches terminal status | 72h compromise window is severe under the multi-tenant threat model. In-memory `DashSet` checked in `authorize_worker_token` cuts post-completion blast radius to zero without changing the TTL ceiling. ~10 lines. | +| Worker-side env scrubbing via single chokepoint helper, not per-call `env_remove` | Per-call rots: every new spawn site forgets. Codebase already uses chokepoint pattern in `LocalSandbox::execute` (allowlist + suffix filter). Mirror with `fabro-sandbox/src/spawn_env.rs::apply_sandbox_env(&mut Command)`, route remaining unscrubbed sites through it. `LocalSandbox::execute`'s incidental `_token`-suffix filter already catches `FABRO_WORKER_TOKEN` today — make that explicit (denylist entry, not coincidence). | +| Worker exits with distinct non-zero code (`78` / `EX_NOPERM`) on missing/rejected `FABRO_WORKER_TOKEN`; emits `tracing::error!(target = "worker_auth", ...)` | Lets operators distinguish auth failures from generic crashes during deploy windows. Today all worker exit codes look identical at the server. | + +### Worker-token vs artifact-upload-token (delta) + +| Property | Artifact-upload-token (today) | Worker-token (new) | +|---|---|---| +| Coverage | One route (`/runs/{id}/stages/{stageId}/artifacts`) | All run-scoped routes the worker hits | +| TTL | 24h | 72h | +| Signing key | `OsRng` at server boot, in-memory only | HKDF from `SESSION_SECRET`, context `b"fabro-worker-jwt-v1"` | +| Survives server restart | No | Yes (up to natural expiry) | +| Issuer string | `"fabro-server-artifact-upload"` | `"fabro-server-worker"` | +| Scope claim | `"stage_artifacts:upload"` | `"run:worker"` | +| Passed to worker | `--artifact-upload-token` argv | `FABRO_WORKER_TOKEN` env var | +| Worker uses it as | Per-call method arg on the client | Client's bearer for every server call | + +## Open Questions + +### Resolved During Planning + +- TTL: 72h (user). Refresh: none in-process; fresh mint at every spawn (start AND resume). +- Key derivation: HKDF from `SESSION_SECRET` with context `b"fabro-worker-jwt-v1"` (user, this session). +- Replace artifact-upload-token entirely vs. keep both: replace (user, this session). +- New `RunAuthMethod::Worker` variant: no — worker token bypasses `AuthenticatedSubject` entirely. +- Stamp worker events server-side vs. worker-side: worker-side, in a dedicated `SystemWorkerActorSink` wrapper inside the worker's `RunEventSink::fanout` chain (NOT in the shared `to_run_event_at` converter, which is also called server-side). Keeps the converter pure (passthrough semantics preserved) and avoids mis-stamping server-emitted events. + +### Deferred to Implementation + +- Exact module name for new server-side worker-token machinery — likely `fabro-server/src/worker_token.rs`, decide at implementation. +- Exact name of the new `Credential::Worker` variant on `fabro-client` — `Worker` likely, confirm against existing naming when implementing. +- Whether to enforce "worker module never imports `AuthStore`" structurally (clippy `disallowed_types` on the `commands::run` module). Nice-to-have; defer. +- Mechanism for the compile-time "no `Display` for `Credential::Worker`" guard — `static_assertions::assert_not_impl_any!` is the natural fit, but choose at implementation time based on whether the crate already pulls that dep. +- Reaper for the in-memory revocation set — defer; entries are bounded by terminal-state runs and process lifetime is the natural reaper. Add only if real workloads show unbounded growth. +- Core dump disable (`setrlimit(RLIMIT_CORE, 0)`) on the worker process to prevent token capture in crash dumps. Same-UID attacker assumption holds today; nice-to-have, defer. +- Centralizing terminal-status writes through a single `mark_run_terminal(run_id, status)` chokepoint that both writes the status AND inserts into the revocation set (instead of grep-and-wire-by-hand). Cleaner, more auditable; defer pending implementation discovery of how dispersed the current writes are. +- Whether `WORKER_JWT_SECRET` should be a separate operator-rotated secret (narrower than `SESSION_SECRET`) — defer; the SESSION_SECRET-as-master-key design is acceptable for the current threat model per Threat Model section, but the gap is documented for follow-up. + +### Decisions surfaced by review (resolved) + +- **Revocation persistence:** **document the gap, accept for now (option c).** In-memory revocation set, lost on restart. Combined with HKDF-derived signing key surviving restart, this leaves a known re-enablement window for tokens of completed runs across restarts. Documented in Risks and System-Wide Impact. Follow-up plan can add persistence if multi-tenant production demand surfaces it. +- **Multi-token-per-run on rapid pause/resume:** **accept and document (option c).** Each resume mints a fresh 72h token; prior tokens stay valid up to natural `exp` or run terminal status. Multiplied compromise window is bounded by run-id (still scoped to one run). Documented in Risks. Follow-up plan can add per-spawn nonce if it becomes a real problem. +- **Clippy lint extension to `tokio::process::Command::new`:** **no.** Do not extend the lint. Tokio-Command sites rely on code-review discipline to use `apply_sandbox_env`. Plan keeps `clippy.toml`'s existing `std::process::Command::new` denial only. +- **Share `apply_sandbox_env` / `apply_worker_env`:** **share.** Place the helper in `fabro-util` (or a new dedicated crate if `fabro-util` becomes too dumping-ground), expose two thin wrappers — `fabro_util::process::apply_worker_env` (server-side, used by `worker_command`) and `fabro_util::process::apply_sandbox_env` (worker-side, used by all worker-reachable spawn sites). Both call the same underlying scrub function with the same denylist constant. Coordinate with `2026-04-22-003` Unit 3 — whichever plan lands first creates the helper; the other plan adds the second wrapper. +- **Line-number drift in plan references:** addressed in this revision pass. + +## High-Level Technical Design + +> *This illustrates the intended approach and is directional guidance for review, not implementation specification. The implementing agent should treat it as context, not code to reproduce.* + +```mermaid +sequenceDiagram + participant Op as Operator + participant Srv as fabro-server + participant W as worker subprocess + participant Child as sandbox/agent child + + Op->>Srv: start (SESSION_SECRET in env) + Note over Srv: HKDF-derive WorkerTokenKeys
context "fabro-worker-jwt-v1" + Srv->>Srv: spawn scheduled (start or resume) + Note over Srv: issue_worker_token(run_id)
HS256 + claims{run_id, scope, 72h} + Srv->>W: spawn with env_clear + FABRO_WORKER_TOKEN injected
(SESSION_SECRET / JWT_PRIVATE_KEY / GITHUB_* removed) + W->>W: read FABRO_WORKER_TOKEN from env
build Client with Credential::Worker(token)
(no AuthStore, no OAuthSession) + W->>Srv: POST /runs/{id}/events (Authorization: Bearer ...) + Srv->>Srv: authorize_run_scoped:
1) try worker token (run_id match + revocation check)
2) else fall through to user-JWT extractor + Srv-->>W: 200 OK + W->>Child: spawn (apply_sandbox_env: scrub FABRO_WORKER_TOKEN) + Child-->>W: result (no token in env) + Note over W,Srv: ... run completes ... + W->>Srv: POST /runs/{id}/events (terminal) + Srv->>Srv: insert run_id into revocation set + Note over Srv: server restart: HKDF re-derives same key,
outstanding tokens still verify (up to natural exp) +``` + +## Implementation Units + +- [ ] **Unit 1: Worker JWT primitives (claims, keys, mint)** + +**Goal:** Server can mint a per-run worker JWT signed with a key derived from `SESSION_SECRET`. No callers yet. + +**Requirements:** R2, R3. + +**Dependencies:** None. + +**Files:** +- Create: `lib/crates/fabro-server/src/worker_token.rs` +- Modify: `lib/crates/fabro-server/src/auth/keys.rs` (add `derive_worker_jwt_key`) +- Modify: `lib/crates/fabro-server/src/lib.rs` (module declaration) +- Modify: `lib/crates/fabro-server/src/server.rs` (`AppState` field for `WorkerTokenKeys`, construction in `build_app_state`) +- Test: `lib/crates/fabro-server/src/worker_token.rs` (unit tests inline) + +**Approach:** +- Constants: `WORKER_TOKEN_ISSUER = "fabro-server-worker"`, `WORKER_TOKEN_SCOPE = "run:worker"`, `WORKER_TOKEN_TTL_SECS = 72 * 60 * 60`. +- `WorkerTokenClaims { iss, iat, exp, run_id, scope, jti }` — mirrors `ArtifactUploadClaims` plus a `jti` (random 128-bit hex) claim. `jti` enables per-token audit correlation in the future and lets logs distinguish two tokens minted for the same run (start vs resume) without leaking the token itself. +- `WorkerTokenKeys { encoding, decoding, validation }` — mirrors `ArtifactUploadTokenKeys`. Built from a 32-byte HKDF output keyed by `SESSION_SECRET`, context `b"fabro-worker-jwt-v1"`. +- `pub fn issue_worker_token(keys: &WorkerTokenKeys, run_id: &RunId) -> Result` — `jsonwebtoken::encode` with HS256. +- Add `worker_tokens: WorkerTokenKeys` field on `AppState` next to `artifact_upload_tokens` (keep both during this unit; the artifact field is deleted in Unit 3). +- `derive_worker_jwt_key(secret: &[u8]) -> [u8; 32]` in `auth/keys.rs` — same HKDF construction as `derive_jwt_key`, distinct `info` parameter. + +**Patterns to follow:** +- `server.rs:291-293, 322-329, 758-780, 813-826` (artifact-upload-token, end-to-end). +- `auth/keys.rs:41` (`derive_jwt_key` HKDF construction). + +**Test scenarios:** +- Happy path: `issue_worker_token` produces a token; `jsonwebtoken::decode` with the same `WorkerTokenKeys` returns the expected `WorkerTokenClaims` (iss, scope, run_id). +- Edge case: token issued with `WorkerTokenKeys` derived from secret S verifies under a *fresh* `WorkerTokenKeys` derived from the same S — proves restart survival (R3). +- Edge case: token issued under secret S1 fails to verify under keys derived from secret S2 (rotation invalidation). +- Edge case: derivation context label `b"fabro-worker-jwt-v1"` produces a key materially different from `derive_jwt_key(secret)` (no accidental cross-acceptance with user JWTs). + +**Verification:** +- All worker-token unit tests pass. +- `cargo build -p fabro-server` succeeds. +- No production callers of new symbols yet — Unit 1 is purely additive infrastructure. + +--- + +- [ ] **Unit 2: Server `authorize_run_scoped` helper (with revocation) + client `Credential::Worker` variant** + +**Goal:** Single server-side helper accepts worker token (run-id-bound, not revoked) OR falls back to user JWT. Client crate gains a typed worker credential with no `Display` and redacted `Debug`. + +**Requirements:** R2, R7. + +**Dependencies:** Unit 1. + +**Files:** +- Modify: `lib/crates/fabro-server/src/worker_token.rs` (add `authorize_worker_token`, `authorize_run_scoped`, `RevokedRunSet`) +- Modify: `lib/crates/fabro-server/src/server.rs` (`AppState` field for `revoked_runs: RevokedRunSet`; populate in run-terminal-status sites) +- Modify: `lib/crates/fabro-server/src/lib.rs` (re-export `authorize_run_scoped` if needed) +- Modify: `lib/crates/fabro-client/src/credential.rs` (add `Worker(String)` variant — no `Display`) +- Test: `lib/crates/fabro-server/src/worker_token.rs` (authorize helper tests inline) +- Test: `lib/crates/fabro-client/src/credential.rs` (Debug + bearer_token tests inline) + +**Approach:** +- `authorize_worker_token(parts: &Parts, state: &AppState, run_id: &RunId) -> Result` — mirror `maybe_authorize_artifact_upload_token` (`server.rs:828-859`): on decode fail return `Ok(false)`; on scope mismatch / run_id mismatch / `state.revoked_runs.contains(run_id)` return `Err(ApiError::forbidden())`; on success `Ok(true)`. +- `pub fn authorize_run_scoped(parts: &Parts, state: &AppState, run_id: &RunId) -> Result<(), ApiError>` — mirror `authorize_artifact_upload` (`server.rs:861-869`): try worker token first, else `authenticate_service_parts(parts)`. +- `RevokedRunSet` — wraps `Arc>` (or equivalent concurrent set). Bounded by terminal-state runs; entries can age out via a periodic reaper or simply persist for the process lifetime (simpler; bounded memory unless workload is pathological — defer reaper to a follow-up). +- Wire revocation insertion at every site that transitions a run to a terminal status (find via grep for `RunStatus::Succeeded | Failed | Cancelled` writes in `fabro-server` and `fabro-workflow`-reduced events on the server side). +- `Credential::Worker(String)` — `bearer_token() -> &str` returns the string; `Debug` prints `Credential::Worker()`. **Do NOT implement `Display`.** Compile-time hardening. +- **Audit logging at authorize time:** on successful worker-token auth, emit `tracing::info!(target = "worker_auth", run_id = %run_id, jti = %claims.jti, "worker token accepted")`. On rejection (wrong run_id, wrong scope, revoked, bad signature), emit `tracing::warn!(target = "worker_auth", reason = %rejection_reason, "worker token rejected")`. Never log the token string itself — only `jti`. Supports incident response: operators can correlate accepted/rejected tokens to specific runs without a token database. + +**Patterns to follow:** +- `server.rs:828-869` (artifact-upload-token authorize pattern). +- `fabro-client/src/credential.rs:6-40` (existing `DevToken`/`OAuth` variants). + +**Test scenarios:** +- Happy path (server): valid worker token for path run_id → `authorize_run_scoped` returns `Ok(())`. +- Error path (server): worker token with `claims.run_id != path.run_id` → 403. +- Error path (server): worker token with wrong scope → 403. +- Error path (server): expired worker token → falls through to user-JWT path (returns false from worker check), and user-JWT extractor then rejects → 401. +- Error path (server): bad signature → falls through to user-JWT path; user-JWT also rejects → 401. +- Error path (server): `alg=none` JWT → rejected (validation requires HS256). +- Error path (server, revocation): valid token but `run_id` in `revoked_runs` set → 403. Insert run_id into set, then re-attempt with a fresh-issued token for the same run → 403. +- Integration (server): no `Authorization` header → falls through to user-JWT extractor → 401 (no implicit acceptance). +- Integration (server): valid user JWT, no worker token → user-JWT path accepts (R7). +- Happy path (client): `Credential::Worker(s).bearer_token()` returns `s`. +- Edge case (client): `Debug` impl prints `Credential::Worker()` — token string never appears in debug output. +- Compile-time guard (client): assert `Credential::Worker` does not implement `Display` (e.g. via a `static_assertions::assert_not_impl_any!` or equivalent — defer exact mechanism to implementation, but the test must exist). + +**Verification:** +- All `authorize_run_scoped` tests pass with both branches and revocation exercised. +- `cargo nextest run -p fabro-server -p fabro-client` succeeds. + +--- + +- [ ] **Unit 3: Wire all run-scoped routes through `authorize_run_scoped`; replace artifact-upload-token at the spawn** + +**Goal:** Every endpoint the worker hits accepts the worker token. Server spawn passes `--worker-token`. Old artifact-upload-token machinery deleted atomically. + +**Requirements:** R1, R2, R4, R7. + +**Dependencies:** Unit 1, Unit 2. + +**Files:** +- Modify: `lib/crates/fabro-server/src/server.rs` + - Replace `_auth: AuthenticatedService` with explicit `authorize_run_scoped(&parts, &state, &id)?` call inside handler bodies for: `get_run_state` (5162), `list_run_events` (5232), `append_run_event` (5182), `write_run_blob` (5438), `read_run_blob` (5465). Add `parts: Parts` extractor where needed. + - `put_stage_artifact` (5924): swap `authorize_artifact_upload` → `authorize_run_scoped`. + - `worker_command` (3797-3851): replace `state.issue_artifact_upload_token` → `state.issue_worker_token`. Drop the `--artifact-upload-token ` arg entirely. Set `cmd.env("FABRO_WORKER_TOKEN", token)` unconditionally (always, regardless of auth method). Also `cmd.env_remove("FABRO_WORKER_TOKEN")` before re-injection (defense against parent-env leakage). Delete the conditional `cmd.env("FABRO_DEV_TOKEN", token)` block (3836-3845) and the preceding `cmd.env_remove("FABRO_DEV_TOKEN")` (keep an unconditional `env_remove` if Unit 3 of `2026-04-22-003` hasn't landed yet — coordinate; once `apply_worker_env` lands, `FABRO_WORKER_TOKEN` becomes the only authority-bearing re-injection). + - `worker_command`: ensure `SESSION_SECRET`, `FABRO_JWT_PRIVATE_KEY`, `FABRO_JWT_PUBLIC_KEY`, `GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET` are all stripped from the worker env. The existing `apply_worker_env` at `spawn_env.rs:22` (already wired in at `server.rs:3835`) does `env_clear` + allowlist, so as long as the allowlist excludes these names (verify), they're already structurally absent. Add explicit `env_remove`s as belt-and-suspenders only if the audit reveals any leak. **Critical context**: without these strips, an inherited `SESSION_SECRET` lets the worker derive `WorkerTokenKeys` locally and mint tokens for any run. Verify allowlist excludes them — this is the highest-impact verification in the plan. + - **Lifecycle/admin routes**: enumerate the routes the worker token must NOT reach and confirm none of them call `authorize_run_scoped` (they keep `_auth: AuthenticatedSubject` / `AuthenticatedService` directly): `cancel_run`, `pause_run`, `unpause_run`, `archive_run`, `unarchive_run`, `delete_run`, `submit_answer` (interview answers are end-user actions), `start_run`/`create_run`. Add a one-line code comment at each such handler: "// Worker token intentionally not accepted; this is a user/admin action." + - Delete: `ARTIFACT_UPLOAD_TOKEN_*` constants (290-292), `ArtifactUploadClaims` (322-329), `ArtifactUploadTokenKeys` (315-320), `artifact_upload_token_keys` (813-826), `AppState::issue_artifact_upload_token` (758-780), `AppState::artifact_upload_tokens` field (568), `maybe_authorize_artifact_upload_token` (827-858), `authorize_artifact_upload` (860-869). +- Test: existing tests in `lib/crates/fabro-server/src/server.rs` test module (rename / replace `worker_command_injects_dev_token_only_when_enabled` at 7911). + +**Approach:** +- All handler signature changes are mechanical: `_auth: AuthenticatedService` → take `parts: Parts` (or use `axum::extract::Request` + `into_parts`), call `authorize_run_scoped(&parts, &state, &id)?` near the top of the body. +- Atomic swap, no transition period — cargo + tests catch any missed callsite. +- **Pre-implementation audit (must run BEFORE coding starts):** + 1. Grep all `client.*` and `api.*` callsites under `lib/crates/fabro-cli/src/commands/run/` (and any helpers it transitively uses) → confirm each resolves to a server handler in the worker-touched table above. Adds rows for any newly-discovered handlers (e.g. heartbeat, status report) and wires them through `authorize_run_scoped`. + 2. Grep all internal repos and `docs/api-reference/fabro-api.yaml` for `artifact-upload-token`, `artifact_upload_token`, `--artifact-upload-token`, and `stage_artifacts:upload`. Document zero hits in the PR description before merging. If any external consumer exists, add a deprecation cycle instead of atomic delete. +- **Structural rule:** `authorize_run_scoped` is invoked ONLY from handlers whose path contains `{id}` (`RunId`). Never invoke from non-run-scoped routes (list endpoints, admin endpoints). Verify by grep: every `authorize_run_scoped` callsite must be preceded by a path-extracted `RunId`. + +**Patterns to follow:** +- `put_stage_artifact` handler (`server.rs:5941`) is the existing model: takes `parts: Parts`, calls `authorize_artifact_upload(&parts, &state, &id)?`. + +**Test scenarios:** +- Happy path: each of the 5 newly-wired routes accepts a worker token whose `claims.run_id` matches the path `id` → expected response. +- Error path: each route with worker token whose `claims.run_id` ≠ path `id` → 403. +- Integration: each route with valid user JWT and no worker token → still works (R7 fall-through). +- Replacement test for `worker_command_injects_dev_token_only_when_enabled`: rename to `worker_command_always_sets_worker_token_env`. Build `worker_command` for both `methods=["github"]` and `methods=["dev-token"]` settings; assert `FABRO_WORKER_TOKEN` env is set to a valid token in BOTH cases (no longer conditional on auth method). Assert `FABRO_DEV_TOKEN` env is NOT set in either case. Assert no `--artifact-upload-token` or `--worker-token` arg appears in argv (env-only). +- Edge case (server secrets stripped): extend the existing `worker_allowlist_is_fail_closed` test (`spawn_env.rs:80-113`, currently asserts `SESSION_SECRET` and `MY_API_KEY` don't leak) to ALSO assert `FABRO_JWT_PRIVATE_KEY`, `FABRO_JWT_PUBLIC_KEY`, `GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET` don't leak. Critical: prevents the worker-mints-any-token escalation. The existing structure (env_clear + allowlist) already provides this; the test just needs to enumerate the additional names so future allowlist edits can't silently add them back. +- Negative path (route enumeration): for each lifecycle/admin route (`cancel_run`, `pause_run`, `unpause_run`, `archive_run`, `unarchive_run`, `delete_run`, `submit_answer`), assert that presenting a valid worker token for the same `run_id` is rejected (handler still requires user JWT). Use the existing JWT-extractor test pattern (`jwt_auth.rs::valid_jwt_bearer_authenticates_with_identity` shape) as a model. +- Negative path (SSE attach + non-run-scoped): assert a worker token is rejected on `/runs/{id}/attach`, `/attach`, `GET /runs` (list), `GET /usage`, `GET /sessions`, and any other non-run-scoped route discovered by the structural-rule grep above. Documents that worker authority does not bleed into list/admin surfaces. +- Audit logging: assert that successful auth emits a `target = "worker_auth"` info span with `run_id` and `jti`; assert that rejection emits a `warn` span with `reason`. Use a tracing test subscriber. Confirms incident-response observability. +- Edge case: assert deleted symbols (`ArtifactUploadClaims`, `authorize_artifact_upload`, etc.) no longer exist — covered implicitly by `cargo build`. + +**Verification:** +- `cargo build --workspace` succeeds (no references to deleted artifact-upload-token symbols). +- `cargo nextest run -p fabro-server` passes. +- Server test for `worker_command` confirms `--worker-token` always present, `FABRO_DEV_TOKEN` never set. + +--- + +- [ ] **Unit 4: Worker (CLI) — use injected worker token from env, stop reading `AuthStore`** + +**Goal:** Worker subprocess reads its credential from `FABRO_WORKER_TOKEN` env at startup, uses it as its sole bearer for every server call, never constructs `AuthStore::default()`. Artifact uploads use the same client credential. + +**Requirements:** R1, R5. + +**Dependencies:** Unit 2 (`Credential::Worker` variant), Unit 3 (server sets `FABRO_WORKER_TOKEN` env on the worker subprocess). + +**Files:** +- Modify: `lib/crates/fabro-cli/src/args.rs:815` — DELETE the `artifact_upload_token: Option` field on `RunWorkerArgs`. Do NOT add a replacement clap arg — the worker reads from env directly. +- Modify: `lib/crates/fabro-cli/src/commands/run/mod.rs:88-100` — drop the `artifact_upload_token` plumbing on the dispatch path. +- Modify: `lib/crates/fabro-cli/src/server_client.rs` — add `pub(crate) async fn connect_server_target_with_bearer(target: &ServerTarget, bearer: &str) -> Result`. Builds `Client` with `.credential(Credential::Worker(bearer.to_owned()))`, no `oauth_session`, no `resolve_target_credential` call, no `AuthStore` access. +- Modify: `lib/crates/fabro-cli/src/commands/run/runner.rs` + - At top of `execute`: read `FABRO_WORKER_TOKEN` from env via `std::env::var` and **immediately wrap in a `WorkerToken(SecretString)` newtype** (using `secrecy::SecretString` or a local equivalent: redacted `Debug`, no `Display`, no `Deref`). Never let the raw `String` escape the read site. Validate non-empty; bail with a clear error mentioning `FABRO_WORKER_TOKEN` if missing or empty. Drop `artifact_upload_token` from `execute`'s signature. +- Modify: `lib/crates/fabro-util/src/exit.rs` — add `ExitClass::WorkerAuthRequired` mapping to integer `78` (`EX_NOPERM`). The runner's missing-token error is classified as this variant via `.classify(ExitClass::WorkerAuthRequired)` so the existing `exit_code_for` mapper produces 78 cleanly. Avoids a divergent `std::process::exit` path that would bypass the runner's tracing/JSON-output flow. + - Line 66: replace `connect_server_target_direct(&server)` with `connect_server_target_with_bearer(&target, &worker_token)`. + - Lines 76-80: build the artifact uploader without a separate `bearer_token` field — the client already carries the credential. + - Delete `MissingArtifactUploadTokenUploader` (298-315) and the `match artifact_upload_token { Some/None }` fork (244-251); always construct `HttpArtifactUploader`. + - `HttpArtifactUploader`: drop the `bearer_token: String` field; `upload_stage_artifacts` no longer threads a per-call bearer. +- Modify: `lib/crates/fabro-client/src/client.rs:1053, 1091` — `upload_stage_artifact_*` no longer takes `bearer_token` parameter; uses the client's credential. (Confirm signature change is workable; if the client API forces per-call bearer for legacy reasons, leave the parameter and pass `client.credential().bearer_token()` from the worker side instead.) +- Modify: `runner::execute` exits with code `78` (`EX_NOPERM`) via `ExitClass::WorkerAuthRequired` when `FABRO_WORKER_TOKEN` is missing or invalid; emits `tracing::error!(target = "worker_auth", ...)` so operators can grep for it. Distinct from generic crash exits. +- Compile-time guards on `WorkerToken`: `static_assertions::assert_not_impl_any!(WorkerToken: Display, std::fmt::Display)` (or equivalent) — token cannot be `format!`-ed by accident. Test asserts `format!("{:?}", worker_token)` contains no substring of the actual token. + +**Child-process env scrub (chokepoint pattern, NOT per-call):** + +The codebase already has a chokepoint at `LocalSandbox::execute` (`lib/crates/fabro-sandbox/src/local.rs:223-242`): `env_clear` + `should_filter_env_var` heuristic with safelist + suffix denylist. `FABRO_WORKER_TOKEN` is incidentally caught by the `_token` suffix filter today — make this explicit (denylist entry, not coincidence). Then add a sibling helper for the unscrubbed sites. + +- Modify: `lib/crates/fabro-sandbox/src/local.rs:43-66` — add `"FABRO_WORKER_TOKEN"` (and `SESSION_SECRET`, `FABRO_JWT_PRIVATE_KEY`, `FABRO_JWT_PUBLIC_KEY`, `GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET`) to an explicit denylist alongside the suffix heuristic. Comment why: future rename like `FABRO_WORKER_AUTH` would silently leak under the suffix heuristic alone. +- Create or extend: `lib/crates/fabro-util/src/process.rs` (new module) exposing both `pub fn apply_worker_env(cmd: &mut Command)` (server-side, used by `worker_command`) AND `pub fn apply_sandbox_env(cmd: &mut Command)` (worker-side, used by spawn sites below). Both call the same underlying scrub against the same denylist constant. **Shared with `2026-04-22-003`** — whichever plan lands first creates the module; the other adds the second wrapper. The denylist constant lives here too, single source of truth for "secret env names that must not leak across the worker boundary." Both `tokio::process::Command` and `std::process::Command` supported (the function takes a trait or has two overloads). +- Modify: route the following unscrubbed worker-reachable spawn sites through `apply_sandbox_env`: + - `lib/crates/fabro-sandbox/src/local.rs:334, 355, 504` — `rg`, `grep`, `uname`. + - `lib/crates/fabro-workflow/src/git.rs:35` (`git_cmd` helper — single chokepoint for git lines `342, 347, 414`). + - `lib/crates/fabro-workflow/src/transforms/file_inlining.rs:124, 129` — git invocations. + - `lib/crates/fabro-workflow/src/sandbox_git.rs` — production git sites (skip `#[cfg(test)]` blocks at 800-1053). + - `lib/crates/fabro-workflow/src/pipeline/initialize.rs:417` — devcontainer `initializeCommand` (`sh -c`). + - `lib/crates/fabro-devcontainer/src/features.rs:67, 80, 113, 150, 199` — `which`, `brew`, `sh -c curl|tar`, `tar`, `oras pull`. + - `lib/crates/fabro-mcp/src/client.rs:47` — stdio MCP server subprocess. + - `lib/crates/fabro-github/src/lib.rs:129` — `gh auth token`. + - `lib/crates/fabro-hooks/src/executor.rs:187` — non-sandbox hook `sh -c`. +- Note: `fabro-agent` constructs zero `Command`s (all exec routes through `Sandbox::exec_command`). Fixing `LocalSandbox` covers the entire agent surface. +- Note: existing `clippy.toml` denies `std::process::Command::new` workspace-wide but does NOT deny `tokio::process::Command::new`. Most worker-reachable spawn sites use tokio Command, so the lint nudge is partial — tokio sites rely on code-review discipline to use `apply_sandbox_env`. Decision: do not extend the lint here (out of scope; bigger workspace-wide audit). Code review enforces tokio-side hygiene. + +**Approach:** +- `connect_server_target_with_bearer` is the smallest possible surface: it skips the `AuthStore`/`OAuthSession` machinery entirely. The user-facing `connect_server_target` and `connect_server_with_settings` are unchanged. +- The new constructor is the *only* path the worker takes; verify by grep that `commands::run::runner` and `commands::run::mod` are the only modules importing it. + +**Patterns to follow:** +- `server_client.rs:80-110` (`connect_managed_unix_socket_api_client_bundle`) for the client-builder shape; new constructor is a stripped-down version. + +**Test scenarios:** +- Happy path: `connect_server_target_with_bearer` builds a `Client` whose outgoing requests carry `Authorization: Bearer `. +- Edge case: `connect_server_target_with_bearer` does NOT call `AuthStore::default()` — verify by injecting a `FABRO_AUTH_FILE=/nonexistent` env override and confirming construction succeeds (the helper must not even attempt to read the file). +- Edge case: `connect_server_target_with_bearer` does NOT install an `OAuthSession` (no refresh attempts on 401). +- Integration: `runner::execute` with `FABRO_WORKER_TOKEN=` set in env POSTs an event using only the injected token; no fallback to `auth.json`. +- Error path: `runner::execute` invoked without `FABRO_WORKER_TOKEN` in env returns a clear error mentioning the variable name; does NOT silently fall back to user OAuth. +- Edge case: `RunWorkerArgs` no longer has `artifact_upload_token` field — `cargo build` confirms. +- Integration (env hygiene, sandbox path): a workflow stage Bash command `env | grep -E "FABRO_WORKER_TOKEN|SESSION_SECRET|FABRO_JWT_PRIVATE_KEY"` prints empty output. Covers `LocalSandbox::execute` denylist correctness. +- Integration (env hygiene, non-sandbox path): a unit test on `apply_sandbox_env` builds a `Command`, sets the denylist names in the parent test env, calls the helper, then asserts each name is `Removed` in the resulting `Command`'s env overrides. +- Error path (exit code): `runner::execute` invoked with no `FABRO_WORKER_TOKEN` exits with status 78 and emits a `target = "worker_auth"` tracing line. Verify via process spawn in a small integration test. + +**Verification:** +- `cargo build --workspace` succeeds. +- `cargo nextest run -p fabro-cli` passes. +- grep for `AuthStore` from `commands/run/` returns no hits. + +--- + +- [ ] **Unit 5: Stamp `system:worker` actor on worker-emitted events** + +**Goal:** Events the worker emits without a typed actor get a `system:worker` stamp at the *worker-side sink layer*, not in shared event-conversion infrastructure. User identity stays on the run record (`RunSpec.provenance.subject`), where it already lives. + +**Requirements:** R6. + +**Dependencies:** Should land WITH the auth changes (Units 1-4), not in isolation. Landing this alone produces a wire-visible behavior change (worker events flip from `actor: None` to `actor: System(worker)`) without the auth context that justifies it. Sequence: land Unit 5 in the same release as Units 1-4 to keep the rationale visible in git history. + +**Files:** +- Modify: `lib/crates/fabro-cli/src/commands/run/runner.rs` — install a worker-only sink wrapper in the `RunEventSink::fanout` chain at `runner.rs:100` that default-fills `actor` for events emitted by the worker. +- Test: `lib/crates/fabro-cli/src/commands/run/runner.rs` (test module inline). + +**Approach:** +- **Critical: do NOT add the default-fill to `lib/crates/fabro-workflow/src/event.rs::to_run_event_at` or `stored_event_fields`.** Those helpers are shared infrastructure called by both the server (e.g. `server.rs:6805` lifecycle/error event flush via `workflow_event::to_run_event`) AND the worker. Default-filling there mis-stamps server-emitted events as `system:worker`. +- Helper function (not `const` — `ActorRef::id`/`display` are `Option`, heap-allocated, not `const`-constructible): `fn system_worker_actor() -> ActorRef { ActorRef { kind: ActorKind::System, id: Some("worker".to_string()), display: Some("system:worker".to_string()) } }` lives in `runner.rs` (worker-local). +- Wrap `RunEventSink::backend(http_store)` in a `SystemWorkerActorSink` adapter that, before forwarding to the inner sink, applies the rule: **if `event.actor.is_none()`, fill with `system_worker_actor()`** (value-based, not variant-based). Variants like `RunArchived { actor: Some(user_actor) }` keep their actor unchanged because `actor.is_some()`. Variants like worker self-cancel `RunCancelRequested { actor: None }` correctly get the system actor. +- Agent events (`AssistantMessage`) are constructed with `actor: Some(ActorRef::agent(...))` upstream — they retain `ActorKind::Agent` because `actor.is_some()`. + +**Patterns to follow:** +- `RunEventSink::fanout` composition (`fabro-workflow/src/event.rs` sink layer) — sink wrappers are the existing pattern for per-deployment behavior. +- Existing actor-stamping for lifecycle events at the server endpoints (`actor_from_subject` at `server.rs:6278`) — server-side stamping for user actions; worker-side wrapper for worker events. Symmetric. + +**Test scenarios:** +- Happy path: a stage-execution `RunEvent { actor: None, ... }` flows through `SystemWorkerActorSink` → forwarded event has `actor: Some(ActorRef { kind: System, id: Some("worker"), display: Some("system:worker") })`. +- Edge case: `RunEvent { actor: Some(user_actor), ... }` (e.g. lifecycle event mirrored back) → forwarded event retains the user actor (`actor.is_some()` → no-op). +- Edge case: agent message `RunEvent { actor: Some(ActorKind::Agent), ... }` → forwarded event retains `ActorKind::Agent`. +- Edge case: worker self-cancel `RunEvent { actor: None, body: RunCancelRequested { ... } }` → forwarded event gets `system:worker`. (Catches the variant-vs-value-based bug from earlier draft.) +- Regression: server-side event flush via `workflow_event::to_run_event` (`server.rs:6805`) is unchanged — `to_run_event_at` retains its passthrough semantics. Verify by leaving `to_run_event_at` tests untouched. +- Regression: `create_hydrates_provenance_into_store_state` (`fabro-workflow/src/operations/create.rs:1037`) still passes — originator user identity still on `RunSpec.provenance.subject`. + +**Verification:** +- `cargo nextest run -p fabro-workflow` passes. +- New tests for the default-fill and the override-protections both pass. + +--- + +- [ ] **Unit 6: End-to-end regression — github-only worker run with no `~/.fabro/auth.json`** + +**Goal:** Lock in the bug fix: a github-only deployment can spawn a worker that completes a run, with no user OAuth artifact present on the worker host. + +**Requirements:** R1, R5. + +**Dependencies:** Units 1-5. + +**Files:** +- Create: an integration test under `lib/crates/fabro-cli/tests/it/cmd/` (existing test scaffolding) — file name per local convention (e.g. `worker_auth.rs` or `cmd/run_worker_auth_test.rs`). + +**Approach:** +- Test fixture: server configured with `auth.methods = ["github"]` only; no `FABRO_DEV_TOKEN`; `FABRO_HOME` redirected to a fresh tempdir with no `auth.json`. +- Run a tiny workflow end-to-end via the daemon → worker spawn path. +- Assert the worker successfully POSTs at least one `RunEvent` and the run reaches a terminal status. +- Assert no `auth.json` file is touched (timestamp / non-existence). + +**Patterns to follow:** +- Existing integration tests in `lib/crates/fabro-cli/tests/it/cmd/` (per `support.rs` helpers like `daemon.bind.to_target()` at `support.rs:718`). +- CLAUDE.md note: tests must use `.no_proxy()` HTTP clients. + +**Test scenarios:** +- Integration: github-only server + no `auth.json` + minimal workflow → run completes successfully; events visible via `GET /runs/{id}/events`. +- Integration: same setup but worker token tampered (e.g. spawn with a manually-replaced bogus token) → worker fails fast on first server call. + +**Verification:** +- `cargo nextest run -p fabro-cli --test it` passes the new test. +- Test fails on `main` (pre-Units 1-5) — confirms it covers the regression. + +## System-Wide Impact + +- **Interaction graph:** Worker subprocess no longer reads `~/.fabro/auth.json`. CLI user-facing commands (`fabro run`, `fabro ps`, `fabro auth`, etc.) unchanged — they still go through `connect_server_target` / `connect_server_with_settings`. SSE attach endpoints (`/runs/{id}/attach`, `/attach`) unchanged: worker is a producer, not a consumer; user-JWT-only auth on those routes preserved. +- **Error propagation:** Worker token expiry mid-run → next server call returns 401, worker exits 78 (`EX_NOPERM`), server marks run failed via existing `pump_worker_*` paths (`server.rs:4880`+). Distinct exit code separates auth failures from generic crashes. Worker token rejected as revoked after run terminal status → 403, same exit path. +- **State lifecycle risks:** Server restart with HKDF-derived key: outstanding worker tokens remain valid up to natural expiry. Server restart with `SESSION_SECRET` rotated: outstanding workers fail at next call (acceptable; matches user-session invalidation). Server-side revocation set is in-memory and lost on restart — combined with token survival across restart, this creates a post-restart re-enablement window for tokens of completed runs. See Risks for mitigation options. The previous draft's claim that revocation "cuts blast radius to zero" was wrong; the honest claim is "cuts post-completion blast radius to zero **between server restarts**." +- **Event sink uniformity:** Worker `RunEventSink::fanout([Store(http), Callback])` (`runner.rs:100`) — default-fill of `actor` happens upstream in `to_run_event_at`, so all sinks (HTTP backend, local callback, future telemetry) see the same `system:worker` actor. No per-sink divergence. +- **API surface parity:** `RunEvent.actor` shape unchanged (`ActorRef` already has `ActorKind::System`); worker-emitted events newly carry `system:worker` instead of `None`. Web UI audit (`apps/fabro-web/app/`) found ZERO references to `actor` or `author` — there's nothing to break or filter today. Risk of UI breakage was overstated in earlier draft; confirmed safe. +- **Worker-process trust degradation:** A workflow stage that compromises the worker process (malicious shell, code injection) gains read access to `FABRO_WORKER_TOKEN` for that worker's lifetime + 72h until natural expiry, *or* until the run reaches terminal status (revocation). Blast radius bounded by run-id claim: only the compromised run's blobs/events/state are accessible. Not cross-run. `SESSION_SECRET` is NOT in the worker's env (Unit 3) so the worker cannot mint cross-run tokens. +- **Integration coverage:** Unit 6 covers github-only-no-auth-store regression; existing CLI integration tests cover dev-token deployments. +- **Unchanged invariants:** End-user auth (dev-token / github) unchanged. `RunAuthMethod` enum unchanged. `RunSpec.provenance` shape unchanged. Webhook auth unchanged. Artifact upload route's behavior from a worker's perspective unchanged (worker still authenticates and uploads — just via the unified token). `OpenAPI` / `fabro-api-client` (TypeScript) DTOs unchanged. + +## Risks & Dependencies + +| Risk | Mitigation | +|------|------------| +| Worker process inherits `SESSION_SECRET` → can mint tokens for any run, defeating per-run binding | Unit 3 explicitly `env_remove`s `SESSION_SECRET` (and other server-only secrets) on every worker spawn. Verified by `worker_command_strips_server_secrets_from_worker_env` test. **Highest-impact mitigation in the plan.** | +| Token leaks via `format!`/`Display`/`tracing` of `Credential::Worker` payload | `Credential::Worker` has redacted `Debug`, no `Display`. Compile-time guard test in Unit 2 asserts `!impl Display`. Code review must reject any `format!("... {token}")` pattern in worker code paths. | +| Sentry / panic capture serializes the worker's env or backtrace locals | Sentry panic hook (`fabro-telemetry/src/panic.rs`) captures only panic message + stacktrace, not env or frame variables. Confirmed safe today. New panics in worker code with token in the formatted message would leak — code review responsibility, no structural fix. | +| 72h token compromised mid-run, attacker uses it from any host on network | Run-id binding limits blast radius to one run. Server-side revocation set (Unit 2) cuts post-completion blast radius to zero **between server restarts** — see the restart re-enablement risk row below. If the threat model demands more (multi-tenant production), follow-up plan should add IP binding, proof-of-possession, or persisted revocation — out of scope here. | +| `client.upload_stage_artifact_*` API forces per-call bearer parameter | Fallback in Unit 4: keep parameter, pass `client.credential().bearer_token()` from the worker side. | +| `2026-04-22-003-refactor-lock-down-server-secrets-plan.md` lands first; its `apply_worker_env` allowlist must include `FABRO_WORKER_TOKEN` and EXCLUDE `SESSION_SECRET`/`FABRO_JWT_PRIVATE_KEY`/`GITHUB_APP_*` | Coordinate at land time. Both plans converge on env_clear + explicit re-injection. `FABRO_WORKER_TOKEN` replaces `FABRO_DEV_TOKEN` as the single re-injected secret. Verify the allowlist excludes server-only secrets — if not, this plan's `env_remove`s in `worker_command` are the safety net. | +| Server restart mid-run with `SESSION_SECRET` rotated → outstanding workers die | Accepted. Same UX as user sessions. Operators who rotate `SESSION_SECRET` already accept session invalidation. | +| `FABRO_AUTH_FILE` env var present in worker subprocess somehow re-introduces user OAuth | Worker explicitly does not call `AuthStore::default()`; the env var is only consulted by that constructor. Unit 4 removes the worker callsite. | +| Worker child processes (sandbox, agent, devcontainer) inherit `FABRO_WORKER_TOKEN` | Chokepoint helper `apply_sandbox_env` (Unit 4) + explicit denylist in `LocalSandbox::should_filter_env_var` covers all worker-reachable spawn sites. Note: `clippy.toml` denies `std::process::Command::new` only — extend to `tokio::process::Command::new` (Unit 4) so the lint nudge actually catches the dominant async spawn pattern. | +| `FABRO_WORKER_TOKEN` readable via `/proc//environ` to same-UID processes on Linux | Documented in Threat Model: env-var transport does not protect against same-UID reads. Multi-tenant deployments must isolate per-tenant via separate UIDs / containers / namespaces. NOT a property of this design. | +| Revocation set is in-memory; lost on server restart, but worker tokens survive restart by design (HKDF key persists) — creates post-restart re-enablement window for tokens belonging to terminated runs | **Known limitation, called out honestly.** A token captured pre-completion can be replayed for up to 72h after a routine deploy if the run completed before the deploy and the attacker waits out the restart. Mitigation options: (a) persist revocation set to a small KV (Redis or a SlateDB key), (b) at authorize time, look up the run's terminal status from the run store and reject if `claims.iat < run.terminal_at`. **Pick at implementation time** — see Open Questions. The plan explicitly does NOT claim revocation "cuts post-completion blast radius to zero" anymore. | +| Same-run concurrent worker spawn (scheduler race, manual operator action) → two valid tokens for one `run_id` racing on event/state appends | Scheduler's at-most-one-worker-per-run guarantee is assumed but not verified by this plan. Verification step in Unit 3 audit must check `start_run` / `resume_run` / `unarchive_run` paths for race conditions. If not enforced today, follow-up plan adds a server-side spawn lock or a per-spawn nonce. Out of scope for this plan to fix the scheduler; in scope to flag the assumption. | +| Rapid pause/resume cycles leave multiple valid tokens per run (each resume mints fresh, prior tokens not revoked) | Each prior worker token remains valid up to its 72h `exp`. Multiplicative compromise window. **Pick at implementation time:** (a) revoke prior tokens on every new spawn (requires tracking active tokens per run), (b) bind via `spawn_id` nonce that the server replaces on each spawn, (c) accept and document. See Open Questions. | +| Web UI renders `ActorKind::System` poorly (assumed `User` only) | **Downgraded.** Audit of `apps/fabro-web/app/` found zero references to `actor`/`author` today — nothing to break. If future UI surfaces author display, that's additive work, not a regression. | + +## Documentation / Operational Notes + +- `docs-internal/` — if any internal doc describes worker auth (search before landing), update to reflect: "worker → server auth uses a server-issued per-run JWT, independent of end-user auth method." +- No external user-facing doc impact (no public API change; CLI args on `__run-worker` are internal-only, hidden via `#[command(hide = true)]`). + +### Deploy story (atomic swap, no shim) + +Verified: in-flight workers spawned pre-deploy continue working post-deploy via user-JWT fall-through (R7) — they still hold OAuth from `auth.json`, and `authorize_run_scoped` accepts user JWTs. The artifact-upload route (the only worker-touched route that today does NOT use bare `AuthenticatedService`) already implements the same fall-through pattern — `authorize_artifact_upload` at `lib/crates/fabro-server/src/server.rs:868` calls `authenticate_service_parts(parts)` after the upload-token check, so existing workers' user JWTs authenticate against artifact uploads post-deploy. Only newly-spawned workers post-deploy exercise the new contract; those start cleanly with `FABRO_WORKER_TOKEN`. Resumed runs re-spawn via `worker_command`, get a fresh token, no orphaning. **No drain or backwards-compat shim required.** + +**Pre-deploy checklist:** +- Confirm `SESSION_SECRET` is set and stable across the restart (HKDF key derives from it). +- Verify `apply_worker_env` (if `2026-04-22-003` landed) excludes `SESSION_SECRET` and other server-only secrets from the allowlist. +- Grep all internal repos and `docs/api-reference/fabro-api.yaml` for `artifact-upload-token`, `artifact_upload_token`, `--artifact-upload-token`, `stage_artifacts:upload`. Document zero hits in the PR description before merging — the atomic-delete decision depends on no external consumer existing. +- Baseline: record `count(runs where status=running)`. + +**Post-deploy (within 5 min):** +- `count(runs where status=running)` matches baseline ± natural completions. +- grep server logs for `target=worker_auth` — expect zero hits (any hit means token-injection bug). +- `WorkflowRunFailed` rate vs. 7-day baseline. +- Spawn one new run end-to-end; confirm completion. +- If any HITL-paused runs exist, resume one and confirm event emission. + +**Rollback:** redeploy old binary. Workers spawned during the new-binary window fail at next call → marked failed → user re-runs. No data restoration needed; `FABRO_WORKER_TOKEN` is env-only, never persisted. + +## Sources & References + +- Worker auth surface inventory: `lib/crates/fabro-cli/src/commands/run/runner.rs:55-125` +- Artifact-upload-token model to generalize: `lib/crates/fabro-server/src/server.rs:291-293, 757-869` +- Worker spawn site: `lib/crates/fabro-server/src/server.rs:3797-3851` +- Existing HKDF key derivation: `lib/crates/fabro-server/src/auth/keys.rs:41` +- `Credential` variants: `lib/crates/fabro-client/src/credential.rs:6-30` +- `ActorRef` / `ActorKind`: `lib/crates/fabro-types/src/run_event/mod.rs:29-81` +- `RunProvenance`: `lib/crates/fabro-types/src/run.rs:34-49` +- Coordinated plans: `docs/plans/2026-04-22-003-refactor-lock-down-server-secrets-plan.md`, `docs/plans/2026-04-19-003-feat-cli-auth-login-plan.md`, `docs/plans/2026-04-20-001-fix-cli-server-same-host-assumptions-plan.md` +- Origin of artifact-upload-token pattern: `docs/plans/2026-04-06-object-backed-artifact-uploads.md:42-45` +- Worker subprocess history: `docs/plans/2026-04-06-subprocess-run-workers-signal-control-plan.md`, `docs/plans/2026-04-07-worker-http-only-run-store-migration-plan.md`