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/.github/workflows/rust.yml b/.github/workflows/rust.yml index ac96ed440..d47092a01 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -47,6 +47,7 @@ jobs: with: persist-credentials: false - run: bin/dev/check-boundary.sh + - run: bin/dev/check-env-mutation.sh fmt: name: Format diff --git a/AGENTS.md b/AGENTS.md index 2df3d2b6e..adc9b5d79 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,6 +100,7 @@ When working on Rust crates, read the relevant strategy doc **before** making ch - **`docs-internal/logging-strategy.md`** — read when adding `tracing` calls (`info!`, `debug!`, `warn!`, `error!`), working on error handling paths, or adding new operations that should be observable - **`docs-internal/events-strategy.md`** — read when adding or modifying `Event` variants, touching `Emitter`/`emit()`, changing `progress.jsonl` output, or adding new workflow stage types - **`files-internal/testing-strategy.md`** — read when adding or reorganizing tests, choosing between unit vs `tests/it`, deciding whether a test belongs in `cmd` vs `workflow` vs `scenario`, or deciding how to structure snapshots and fixtures +- **`docs-internal/server-secrets-strategy.md`** — read when adding or changing server-level secrets, startup validation, install-time secret persistence, or subprocess env inheritance/scrubbing ## Shell quoting in sandbox code diff --git a/bin/dev/check-env-mutation.sh b/bin/dev/check-env-mutation.sh new file mode 100755 index 000000000..6b95cb8f7 --- /dev/null +++ b/bin/dev/check-env-mutation.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +set -euo pipefail + +cd "$(dirname "$0")/../.." + +if command -v rg >/dev/null 2>&1; then + matches=$(rg -n 'std::env::(set_var|remove_var)' --glob '*.rs' || true) +else + matches=$(grep -R -n -E 'std::env::(set_var|remove_var)' . --include='*.rs' --exclude-dir=target --exclude-dir=.git || true) +fi + +fail=0 +while IFS= read -r match; do + [[ -z "$match" ]] && continue + + path=${match%%:*} + rest=${match#*:} + line=${rest#*:} + line=${line#"${line%%[![:space:]]*}"} + + case "$path:$line" in + "lib/crates/fabro-telemetry/src/spawn.rs:std::env::set_var(key, value);" | \ + "lib/crates/fabro-telemetry/src/spawn.rs:std::env::remove_var(key);" | \ + 'lib/crates/fabro-server/src/install.rs:std::env::set_var("FABRO_TEST_IN_MEMORY_STORE", "1");') + continue + ;; + esac + + echo "process env mutation check failed: $match" >&2 + fail=1 +done <<< "$matches" + +if [[ $fail -ne 0 ]]; then + cat >&2 <<'EOF' + +Do not mutate process-wide env with std::env::set_var/remove_var. +Inject env at construction time or on child-process Command values instead. +See docs-internal/server-secrets-strategy.md. +EOF + exit 1 +fi + +echo "Process env mutation checks passed." diff --git a/clippy.toml b/clippy.toml index 0a19186f3..033174d94 100644 --- a/clippy.toml +++ b/clippy.toml @@ -20,6 +20,8 @@ disallowed-methods = [ { path = "std::fs::File::create", reason = "Blocking open; prefer tokio::fs::File::create on Tokio paths. Document intentional sync I/O with #[expect(clippy::disallowed_methods, reason = \"...\")]" }, { path = "std::fs::File::create_new", reason = "Blocking open; prefer tokio::fs::File::create_new on Tokio paths. Document intentional sync I/O with #[expect(clippy::disallowed_methods, reason = \"...\")]" }, { path = "std::fs::OpenOptions::open", reason = "Blocking open; prefer tokio::fs::OpenOptions::open on Tokio paths. OS file-lock semantics may require spawn_blocking instead. Document intentional sync I/O with #[expect(clippy::disallowed_methods, reason = \"...\")]" }, + { path = "std::env::set_var", reason = "Server/process env must be injected at construction or child-process spawn time, not mutated globally. See docs-internal/server-secrets-strategy.md" }, + { path = "std::env::remove_var", reason = "Server/process env must be injected at construction or child-process spawn time, not mutated globally. See docs-internal/server-secrets-strategy.md" }, { path = "reqwest::Client::new", reason = "Use fabro_http::http_client() or fabro_http::test_http_client()", allow-invalid = true }, { path = "reqwest::Client::builder", reason = "Use fabro_http::HttpClientBuilder::new()", allow-invalid = true }, { path = "reqwest::blocking::Client::new", reason = "Use fabro_http::blocking_http_client() or fabro_http::blocking_test_http_client()", allow-invalid = true }, diff --git a/docs-internal/cli-workflow-coupling-audit.md b/docs-internal/cli-workflow-coupling-audit.md index 00f9c295b..a58c2c828 100644 --- a/docs-internal/cli-workflow-coupling-audit.md +++ b/docs-internal/cli-workflow-coupling-audit.md @@ -34,7 +34,7 @@ | Path | Direct dependency | Why it still exists | Suggested handling | | --- | --- | --- | --- | -| `lib/crates/fabro-cli/src/commands/store/dump.rs` test module | `event::{Event, append_event}` | Unit tests synthesize workflow events directly. | Low priority; keep until a lighter-weight event fixture helper exists. | +| `lib/crates/fabro-cli/src/commands/dump.rs` test module | `event::{Event, append_event}` | Unit tests synthesize workflow events directly. | Low priority; keep until a lighter-weight event fixture helper exists. | | `lib/crates/fabro-cli/src/commands/run/wait.rs` test module | `outcome::StageStatus`, `records::Conclusion`, `run_status::RunStatusRecord` | Output tests construct workflow-owned records directly. | Replace with shared fixture builders once status/conclusion DTOs move out. | | `lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs` test module | `event::{Event, RunNoticeLevel, to_run_event, to_run_event_at}`, `outcome::billed_model_usage_from_llm` | Progress tests build engine events directly. | Replace with shared event fixture helpers after event DTO extraction. | | `lib/crates/fabro-cli/src/commands/run/run_progress/event.rs` test module | `event::{Event, to_run_event}` | Event rendering tests depend on engine event constructors. | Replace with shared event fixture helpers after event DTO extraction. | diff --git a/docs-internal/run-directory-keys.md b/docs-internal/run-directory-keys.md index 4d5adc8e7..990e57bfe 100644 --- a/docs-internal/run-directory-keys.md +++ b/docs-internal/run-directory-keys.md @@ -33,7 +33,7 @@ These paths are local runtime state, not canonical event projections. These names are still real, but they are no longer live scratch files by default: - Metadata branch files such as `run.json`, `start.json`, `checkpoint.json`, and `retro.json` -- `fabro store dump` exports such as `run.json`, `start.json`, `status.json`, `checkpoint.json`, `conclusion.json`, `retro.json`, `events.jsonl`, and per-node prompt/response/status/stdout/stderr files +- `fabro dump` exports such as `run.json`, `start.json`, `status.json`, `checkpoint.json`, `conclusion.json`, `retro.json`, `events.jsonl`, and per-node prompt/response/status/stdout/stderr files - Retro-agent temp uploads named `progress.jsonl`, `checkpoint.json`, `run.json`, and `start.json` inside the retro sandbox ## Notes diff --git a/docs-internal/server-secrets-strategy.md b/docs-internal/server-secrets-strategy.md new file mode 100644 index 000000000..804795484 --- /dev/null +++ b/docs-internal/server-secrets-strategy.md @@ -0,0 +1,68 @@ +# Server Secrets Strategy + +This document defines how Fabro handles server-level secrets. + +## Core Rules + +- `ServerSecrets` is the canonical server-secret reader. +- It reads from `process env` and `/server.env`. +- Resolution is snapshot-based: env and file are read once at construction, then treated as immutable for the life of the process. +- `process env` wins over `server.env` on conflicts. +- `fabro server start` never generates secrets. Missing required secrets are a startup error. +- `std::env::set_var` and `std::env::remove_var` are banned workspace-wide. Tests are not exempt. CI enforces this with `bin/dev/check-env-mutation.sh` so broad clippy suppressions cannot bypass it. + +## Active Server Secrets + +These values belong to the server runtime and are read via `state.server_secret(...)`: + +| Secret | Used by | +|---|---| +| `SESSION_SECRET` | Cookie encryption and JWT signing derivation | +| `FABRO_DEV_TOKEN` | Dev-token auth for worker/server interactions | +| `GITHUB_APP_PRIVATE_KEY` | GitHub App credentials | +| `GITHUB_APP_WEBHOOK_SECRET` | GitHub webhook verification | +| `GITHUB_APP_CLIENT_SECRET` | GitHub OAuth login | + +`FABRO_JWT_PRIVATE_KEY` and `FABRO_JWT_PUBLIC_KEY` are removed. `SESSION_SECRET` is the single auth root. + +## Startup + +- Foreground and daemon startup use the same validation path. +- Required-at-startup secrets are: + - `SESSION_SECRET` + - `FABRO_DEV_TOKEN` when dev-token auth is enabled + - `GITHUB_APP_CLIENT_SECRET` when GitHub auth is enabled +- Other server secrets remain lazy/feature-specific rather than universal boot blockers. + +## Provisioning + +Server secrets come from one of two sources: + +- Platform env for 12-factor deployments +- `server.env` written by install flows + +There is no compatibility layer for removed secrets and no startup-time secret generation. + +## Subprocess Boundaries + +- Worker and render-graph subprocesses start from `env_clear()` and re-add only explicit allowlisted variables. +- Authority-bearing values are re-injected intentionally. +- The daemon child inherits the parent env unchanged except for output-format hygiene (`FABRO_JSON` removal). + +## Tests + +- In-process tests must inject server secrets with construction-time stubs (`EnvSource`, `StubEnv`) or by writing `server.env`. +- Subprocess tests must set child env with `Command::env`. +- Tests must not mutate the process-wide environment. + +## Rotation + +- Secret rotation requires restart. +- Live rotation is intentionally unsupported. + +## Adding A New Server Secret + +1. Provision it through platform env or install-written `server.env`. +2. Read it through `state.server_secret(...)`. +3. Decide explicitly whether startup should fail when it is absent. +4. If a worker or render subprocess needs it, re-inject it explicitly rather than broadening inheritance casually. diff --git a/docs/administration/security.mdx b/docs/administration/security.mdx index 2a3feec24..437c2b5ac 100644 --- a/docs/administration/security.mdx +++ b/docs/administration/security.mdx @@ -30,7 +30,7 @@ Fabro is single-tenant software designed for small, trusted teams. The following - **Enable authentication.** Fabro supports `dev-token` and GitHub OAuth. Do not disable auth outside of local development or controlled demos. - **Configure a username allowlist for GitHub OAuth.** `[server.auth.github].allowed_usernames` should contain the exact GitHub users allowed to log in. An empty list rejects everyone. -- **Configure the session secret used by the web flow.** `SESSION_SECRET` should be provisioned with a strong value on long-lived deployments. If you also provision `FABRO_JWT_PRIVATE_KEY` and `FABRO_JWT_PUBLIC_KEY`, treat them as server runtime secrets, but they are not what currently gates browser auth. +- **Configure the session secret used by the web flow.** `SESSION_SECRET` should be provisioned with a strong value on long-lived deployments. - **Terminate HTTPS or mTLS upstream when needed.** Fabro's listener is plain HTTP/Unix only. If CI, scripts, or a browser must connect over HTTPS, terminate TLS at a reverse proxy or load balancer and keep the Fabro listener on a private network. ### Secrets diff --git a/docs/administration/server-configuration.mdx b/docs/administration/server-configuration.mdx index dce6b3ae7..1d96421d8 100644 --- a/docs/administration/server-configuration.mdx +++ b/docs/administration/server-configuration.mdx @@ -299,7 +299,6 @@ For the auth model above, the main server runtime secrets are: - `SESSION_SECRET` when the web UI is enabled - `FABRO_DEV_TOKEN` when `"dev-token"` auth is enabled - `GITHUB_APP_CLIENT_SECRET` when `"github"` auth is enabled -- `FABRO_JWT_PRIVATE_KEY` / `FABRO_JWT_PUBLIC_KEY`, provisioned during install for future CLI login flows - `AWS_ACCESS_KEY_ID` / `AWS_SECRET_ACCESS_KEY` when the install wizard or a manual config uses static S3 object-store credentials @@ -332,8 +331,6 @@ Fabro resolves these from `process env -> server.env`. | Variable | Description | |---|---| -| `FABRO_JWT_PRIVATE_KEY` | Ed25519 private key (base64-encoded PEM) for JWT signing | -| `FABRO_JWT_PUBLIC_KEY` | Ed25519 public key (base64-encoded PEM) for JWT verification | | `SESSION_SECRET` | Session encryption secret (64-character hex string) | ### Object store runtime secrets (optional) diff --git a/docs/agents/outputs.mdx b/docs/agents/outputs.mdx index e375ff405..3e95af24d 100644 --- a/docs/agents/outputs.mdx +++ b/docs/agents/outputs.mdx @@ -7,7 +7,7 @@ When an agent or prompt node finishes, Fabro captures its response text and prod ## Response capture -After an agent or prompt node completes, Fabro captures the full response text and persists it to `stages/{node_id}@{visit}/response.md` in metadata snapshots and `fabro store dump` output. It also writes the final outcome (status, context updates, routing directives) to `stages/{node_id}@{visit}/status.json`. +After an agent or prompt node completes, Fabro captures the full response text and persists it to `stages/{node_id}@{visit}/response.md` in metadata snapshots and `fabro dump` output. It also writes the final outcome (status, context updates, routing directives) to `stages/{node_id}@{visit}/status.json`. ## Context updates @@ -92,7 +92,7 @@ review -> approve [label="Approve"] ## Output logging -Fabro writes several files per stage to `stages/{node_id}@{visit}/` in metadata snapshots and `fabro store dump` output: +Fabro writes several files per stage to `stages/{node_id}@{visit}/` in metadata snapshots and `fabro dump` output: | File | Contents | |---|---| diff --git a/docs/agents/prompts.mdx b/docs/agents/prompts.mdx index 69404aa79..61cc58dc1 100644 --- a/docs/agents/prompts.mdx +++ b/docs/agents/prompts.mdx @@ -295,4 +295,4 @@ Use prompt nodes for analysis, classification, and summarization tasks where too ## Prompt logging -Fabro persists the assembled prompt to `stages/{node_id}@{visit}/prompt.md` in metadata snapshots and `fabro store dump` output for every agent and prompt stage. This includes the preamble (if any) and the expanded prompt text. Use these files for debugging when an agent behaves unexpectedly. +Fabro persists the assembled prompt to `stages/{node_id}@{visit}/prompt.md` in metadata snapshots and `fabro dump` output for every agent and prompt stage. This includes the preamble (if any) and the expanded prompt text. Use these files for debugging when an agent behaves unexpectedly. diff --git a/docs/changelog/2026-03-03.mdx b/docs/changelog/2026-03-03.mdx index 2b9137aea..1fc0f648c 100644 --- a/docs/changelog/2026-03-03.mdx +++ b/docs/changelog/2026-03-03.mdx @@ -47,5 +47,4 @@ If something is misconfigured, `fabro doctor` tells you exactly what's wrong and - New indicatif-based progress display for `fabro run start` shows real-time stage progress, tool calls, model names, and timing - Mercury provider updated to `mercury-2`; estimated output speed (tok/s) added to `fabro model list` - Run defaults in `server.toml` are inherited by all workflow runs, so you don't have to repeat sandbox, model, or concurrency settings -- `FABRO_JWT_PUBLIC_KEY` and `FABRO_JWT_PRIVATE_KEY` accept base64-encoded PEM strings for containerized deployments diff --git a/docs/changelog/2026-03-29.mdx b/docs/changelog/2026-03-29.mdx index e6e82bdcf..f89bbadab 100644 --- a/docs/changelog/2026-03-29.mdx +++ b/docs/changelog/2026-03-29.mdx @@ -1,14 +1,14 @@ --- -title: "Store dump export command" +title: "Dump export command" date: "2026-03-29" --- -## `fabro store dump` +## `fabro dump` -A new `fabro store dump` command exports the contents of the run store to a human-readable format for debugging and inspection. This is useful for diagnosing issues with run state, verifying data integrity after migrations, or extracting run data for external analysis. +A new `fabro dump` command exports the contents of the run store to a human-readable format for debugging and inspection. This is useful for diagnosing issues with run state, verifying data integrity after migrations, or extracting run data for external analysis. ```bash -fabro store dump +fabro dump ``` ## More diff --git a/docs/execution/observability.mdx b/docs/execution/observability.mdx index 80eff8ed8..d10a5ec91 100644 --- a/docs/execution/observability.mdx +++ b/docs/execution/observability.mdx @@ -81,7 +81,7 @@ jq '{from: .properties.from_node, to: .properties.to_node, label: .properties.la <(fabro logs 01JKXYZ...) | head ``` -If you need files on disk for offline analysis, `fabro store dump` exports `events.jsonl` plus run-state projections. +If you need files on disk for offline analysis, `fabro dump` exports `events.jsonl` plus run-state projections. ## Event categories @@ -132,6 +132,6 @@ Post-run analysis surfaces include: |---|---| | `fabro logs ` | Full event envelope stream as NDJSON | | `fabro inspect ` | Current durable run state, including run/start/checkpoint/conclusion records | -| `fabro store dump --output ` | Exported `events.jsonl` plus reconstructed JSON and node files | +| `fabro dump --output ` | Exported `events.jsonl` plus reconstructed JSON and node files | See [retros](/execution/retros), [stages](/api-reference/run-internals/list-run-stages), and [turns](/api-reference/run-internals/list-stage-turns) for higher-level analysis views built on top of this event stream. diff --git a/docs/execution/retros.mdx b/docs/execution/retros.mdx index 879190803..e5a8437f4 100644 --- a/docs/execution/retros.mdx +++ b/docs/execution/retros.mdx @@ -143,4 +143,4 @@ Retros are also available via the REST API. See the [list retros](/api-reference ## Storage -Retros are stored in durable run state. If you need files on disk, `fabro store dump` materializes retro text under `stages/retro/` alongside `run.json`, stage files, and the rest of the exported run data. +Retros are stored in durable run state. If you need files on disk, `fabro dump` materializes retro text under `stages/retro/` alongside `run.json`, stage files, and the rest of the exported run data. 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` diff --git a/docs/reference/cli.mdx b/docs/reference/cli.mdx index 8a1f33e6e..bac80bc2e 100644 --- a/docs/reference/cli.mdx +++ b/docs/reference/cli.mdx @@ -947,13 +947,13 @@ fabro secret rm ANTHROPIC_API_KEY --- -## `fabro store dump` +## `fabro dump` -Export the contents of a run's store-backed state to a directory for debugging and inspection. +Export the contents of a run's durable state to a directory for debugging and inspection. ```bash -fabro store dump -fabro store dump abc123 -o ./debug-output +fabro dump +fabro dump abc123 -o ./debug-output ``` | Argument / Flag | Description | diff --git a/docs/reference/run-directory.mdx b/docs/reference/run-directory.mdx index d707afa35..f463697b3 100644 --- a/docs/reference/run-directory.mdx +++ b/docs/reference/run-directory.mdx @@ -30,18 +30,18 @@ These paths are local runtime state and caches, not the canonical run state. - **`runtime/`** — Local runtime files. Today this is mainly materialized blob payloads under `runtime/blobs/`. - **`nodes/{manager_node}_{visit}/child/`** — Nested scratch directories for manager-loop child workflows. -Large durable values, event streams, checkpoints, diffs, conclusions, and retros are no longer projected into live scratch by default. Use `fabro logs`, `fabro inspect`, the API, or `fabro store dump` for those surfaces. +Large durable values, event streams, checkpoints, diffs, conclusions, and retros are no longer projected into live scratch by default. Use `fabro logs`, `fabro inspect`, the API, or `fabro dump` for those surfaces. ## Reconstructed and export-only layouts -Reconstructed metadata branches and `fabro store dump` exports now use the same core layout: +Reconstructed metadata branches and `fabro dump` exports now use the same core layout: - `run.json` for the current projection snapshot, including the current checkpoint - `graph.fabro` for workflow source - `stages/retro/*.md` for retro prompt/response text - `stages/{node_id}@{visit}/...` for per-stage prompt, response, status, diff, stdout, and stderr files -`fabro store dump` adds export-only history surfaces on top of that shared layout: +`fabro dump` adds export-only history surfaces on top of that shared layout: - `events.jsonl` for the durable event stream - `checkpoints/*.json` for checkpoint history snapshots diff --git a/docs/superpowers/specs/2026-04-18-web-install-design.md b/docs/superpowers/specs/2026-04-18-web-install-design.md index ce5f96ae0..05b4b7466 100644 --- a/docs/superpowers/specs/2026-04-18-web-install-design.md +++ b/docs/superpowers/specs/2026-04-18-web-install-design.md @@ -260,7 +260,7 @@ The web wizard produces **the same on-disk state** as the CLI install. The TOML- Files written: - `~/.fabro/settings.toml` — server config, auth methods, GitHub integration strategy. -- `/server.env` — `FABRO_JWT_PRIVATE_KEY`, `FABRO_JWT_PUBLIC_KEY`, `SESSION_SECRET`, `FABRO_DEV_TOKEN`, plus GitHub App env pairs (`GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET`) if the App strategy was chosen. +- `/server.env` — `SESSION_SECRET`, `FABRO_DEV_TOKEN`, plus GitHub App env pairs (`GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET`) if the App strategy was chosen. - `/vaults/default/secrets.json` — vault entries for LLM API key credentials and (if Token strategy) `GITHUB_TOKEN`. Path matches `Storage::secrets_path()` at `lib/crates/fabro-config/src/storage.rs:38`. - `/server.dev-token` — the per-storage dev token, written via `Storage::server_state().dev_token_path()` at `storage.rs:103`. The CLI install also writes a home-level mirror at `Home::from_env().dev_token_path()` (`install.rs:1994-1999`); the web flow does the same to keep parity, since the home-level file is what tooling outside the storage dir expects to find. - Artifact store metadata stamped with `FABRO_VERSION` via `write_artifact_store_metadata` (`install.rs:1458`). diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index 44e4160e3..67ccdc5eb 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -514,7 +514,7 @@ pub(crate) struct InspectArgs { } #[derive(Args)] -pub(crate) struct StoreDumpArgs { +pub(crate) struct DumpArgs { #[command(flatten)] pub(crate) server: ServerTargetArgs, @@ -1001,8 +1001,8 @@ pub(crate) enum Commands { Parse(ParseArgs), /// Inspect and copy run artifacts (screenshots, reports, traces) Artifact(ArtifactNamespace), - /// Export store-backed run state for debugging - Store(StoreNamespace), + /// Export a run's durable state to a directory + Dump(DumpArgs), #[command(flatten)] RunsCmd(RunsCommands), /// List and test LLM models @@ -1085,9 +1085,7 @@ impl Commands { ArtifactCommand::List(_) => "artifact list", ArtifactCommand::Cp(_) => "artifact cp", }, - Self::Store(ns) => match &ns.command { - StoreCommand::Dump(_) => "store dump", - }, + Self::Dump(_) => "dump", Self::Exec(_) => "exec", Self::RunCmd(cmd) => cmd.name(), Self::Preflight(_) => "preflight", @@ -1199,18 +1197,6 @@ pub(crate) enum ArtifactCommand { Cp(ArtifactCpArgs), } -#[derive(Args)] -pub(crate) struct StoreNamespace { - #[command(subcommand)] - pub(crate) command: StoreCommand, -} - -#[derive(Subcommand)] -pub(crate) enum StoreCommand { - /// Export a run's durable state to a directory - Dump(StoreDumpArgs), -} - #[derive(Args)] pub(crate) struct SecretNamespace { #[command(flatten)] diff --git a/lib/crates/fabro-cli/src/commands/store/dump.rs b/lib/crates/fabro-cli/src/commands/dump.rs similarity index 98% rename from lib/crates/fabro-cli/src/commands/store/dump.rs rename to lib/crates/fabro-cli/src/commands/dump.rs index cdb9aa82d..0748ae671 100644 --- a/lib/crates/fabro-cli/src/commands/store/dump.rs +++ b/lib/crates/fabro-cli/src/commands/dump.rs @@ -1,6 +1,6 @@ #![expect( clippy::disallowed_methods, - reason = "CLI `store dump` command: sync file I/O for dump outputs" + reason = "CLI `dump` command: sync file I/O for dump outputs" )] use std::io::ErrorKind; @@ -12,18 +12,18 @@ use bytes::Bytes; use fabro_store::{ArtifactStore, RunDatabase}; use fabro_store::{EventEnvelope, RunProjection, StageId}; use fabro_types::{RunBlobId, RunId}; +use fabro_workflow::run_dump::RunDump; use futures::future::BoxFuture; #[cfg(test)] use serde::de::DeserializeOwned; use tokio::task::spawn_blocking; -use super::run_export::StoreRunExport; -use crate::args::StoreDumpArgs; +use crate::args::DumpArgs; use crate::command_context::CommandContext; use crate::server_client::Client; use crate::shared::{absolute_or_current, print_json_pretty}; -pub(crate) async fn dump_command(args: &StoreDumpArgs, base_ctx: &CommandContext) -> Result<()> { +pub(crate) async fn run(args: &DumpArgs, base_ctx: &CommandContext) -> Result<()> { let ctx = base_ctx.with_target(&args.server)?; let printer = ctx.printer(); let client = ctx.server().await?; @@ -215,7 +215,7 @@ async fn export_run_from_source( .with_context(|| format!("failed to create {}", staging_parent.display()))?; let staging_dir = tempfile::Builder::new() - .prefix(".fabro-store-dump-") + .prefix(".fabro-dump-") .tempdir_in(staging_parent) .with_context(|| { format!( @@ -241,7 +241,7 @@ async fn write_run_dump( output_dir: &Path, ) -> Result { let events = source.list_events().await?; - let mut dump = StoreRunExport::from_store_state_and_events(state, &events)?; + let mut dump = RunDump::from_store_state_and_events(state, &events)?; dump.hydrate_referenced_blobs_with_reader(|blob_id| source.read_blob(blob_id)) .await?; diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index d2aad2df0..60c4b016b 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -26,8 +26,8 @@ use fabro_config::daemon::ServerDaemon; use fabro_config::user::{SETTINGS_CONFIG_FILENAME, default_storage_dir}; use fabro_config::{Storage, envfile}; use fabro_install::{ - InstallListenConfig, generate_jwt_keypair, merge_server_settings as merge_server_settings_impl, - write_github_app_settings, write_token_settings, + InstallListenConfig, PendingSettingsWrite, merge_server_settings as merge_server_settings_impl, + persist_install_outputs_direct, write_github_app_settings, write_token_settings, }; use fabro_model::Provider; use fabro_server::serve; @@ -883,13 +883,6 @@ enum PendingGitHubSettings { }, } -#[derive(Clone, Copy)] -struct PendingSettingsWrite<'a> { - path: &'a Path, - contents: &'a str, - previous_contents: Option<&'a str>, -} - async fn setup_github_app( s: &Styles, web_url: &str, @@ -1148,15 +1141,24 @@ fn credential_secret_request(credential: &AuthCredential) -> Result Result<()> { - if secrets.is_empty() { - return Ok(()); - } +fn server_env_updates(secrets: &[(String, String)]) -> Vec { + secrets + .iter() + .map(|(key, value)| envfile::EnvFileUpdate { + key: key.clone(), + value: value.clone(), + comment: None, + }) + .collect() +} - let env_path = Storage::new(storage_dir).runtime_directory().env_path(); - envfile::merge_env_file(&env_path, secrets.iter().cloned()) - .with_context(|| format!("merging server env secrets into {}", env_path.display()))?; - Ok(()) +fn server_env_removals(keys: &[&'static str]) -> Vec { + keys.iter() + .map(|key| envfile::EnvFileRemoval { + key: (*key).to_string(), + comment: None, + }) + .collect() } async fn persist_install_outputs( @@ -1221,20 +1223,15 @@ fn persist_github_install_changes( let previous_vault = std::fs::read_to_string(&vault_path).ok(); let result = (|| -> Result<()> { - let mut server_env = envfile::read_env_file(&server_env_path) - .with_context(|| format!("reading env file {}", server_env_path.display()))?; - for key in &writes.server_env_remove { - server_env.remove(*key); - } - for (key, value) in &writes.server_env_set { - server_env.insert(key.clone(), value.clone()); - } - if server_env.is_empty() { - restore_optional_file(&server_env_path, None)?; - } else { - envfile::write_env_file(&server_env_path, &server_env) - .with_context(|| format!("writing env file {}", server_env_path.display()))?; - } + let server_env_writes = server_env_updates(&writes.server_env_set); + let server_env_removals = server_env_removals(&writes.server_env_remove); + persist_install_outputs_direct( + storage_dir, + &server_env_writes, + &server_env_removals, + &[], + Some(&writes.settings_write), + )?; let mut vault = Vault::load(vault_path.clone()).map_err(anyhow::Error::from)?; for key in &writes.vault_remove { @@ -1249,14 +1246,6 @@ fn persist_github_install_changes( .map_err(anyhow::Error::from)?; } - std::fs::write(writes.settings_write.path, writes.settings_write.contents).with_context( - || { - format!( - "writing settings file {}", - writes.settings_write.path.display() - ) - }, - )?; Ok(()) })(); @@ -1293,12 +1282,16 @@ async fn persist_install_outputs_with_settings( connect_server: impl for<'a> Fn(&'a Path) -> BoxFuture<'a, Result>, stop_server: impl for<'a> Fn(&'a Path, Duration) -> BoxFuture<'a, bool>, ) -> Result<()> { - persist_server_env_secrets(storage_dir, server_env_secrets)?; - - if let Some(write) = settings_write { - std::fs::write(write.path, write.contents) - .with_context(|| format!("writing settings file {}", write.path.display()))?; - } + let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); + let previous_server_env = std::fs::read_to_string(&server_env_path).ok(); + let settings_write_ref = settings_write.as_ref(); + persist_install_outputs_direct( + storage_dir, + &server_env_updates(server_env_secrets), + &[], + &[], + settings_write_ref, + )?; let persist_result = persist_vault_secrets_with( storage_dir, @@ -1310,6 +1303,7 @@ async fn persist_install_outputs_with_settings( .await; if let Err(err) = persist_result { + restore_optional_file(&server_env_path, previous_server_env.as_deref())?; if let Some(write) = settings_write { match write.previous_contents { Some(previous) => std::fs::write(write.path, previous) @@ -1799,13 +1793,6 @@ async fn run_install_inner(args: &InstallArgs, ctx: &CommandContext) -> Result<( s.green.apply_to("✔") ); - let (jwt_private_pem, jwt_public_pem) = generate_jwt_keypair()?; - fabro_util::printerr!( - printer, - " {} Ed25519 JWT keypair generated", - s.green.apply_to("✔") - ); - let dev_token = if fabro_config::dev_token_auth_enabled(&install_settings) { let token = dev_token::read_or_mint_dev_token_for_install( &fabro_util::Home::from_env().dev_token_path(), @@ -1826,14 +1813,7 @@ async fn run_install_inner(args: &InstallArgs, ctx: &CommandContext) -> Result<( None }; - let jwt_private_b64 = BASE64_STANDARD.encode(jwt_private_pem.as_bytes()); - let jwt_public_b64 = BASE64_STANDARD.encode(jwt_public_pem.as_bytes()); - - let mut generated_server_env_pairs = vec![ - ("FABRO_JWT_PRIVATE_KEY".to_string(), jwt_private_b64), - ("FABRO_JWT_PUBLIC_KEY".to_string(), jwt_public_b64), - ("SESSION_SECRET".to_string(), session_secret), - ]; + let mut generated_server_env_pairs = vec![("SESSION_SECRET".to_string(), session_secret)]; if let Some(token) = dev_token { generated_server_env_pairs.push(("FABRO_DEV_TOKEN".to_string(), token)); } @@ -2018,39 +1998,6 @@ mod tests { assert!(secret.chars().all(|c| !c.is_ascii_uppercase())); } - // -- JWT keypair -- - - #[tokio::test] - async fn jwt_keypair_private_pem_header() { - let (private, _) = generate_jwt_keypair().unwrap(); - assert!( - private.starts_with("-----BEGIN PRIVATE KEY-----"), - "private PEM: {private}" - ); - } - - #[tokio::test] - async fn jwt_keypair_public_pem_header() { - let (_, public) = generate_jwt_keypair().unwrap(); - assert!( - public.starts_with("-----BEGIN PUBLIC KEY-----"), - "public PEM: {public}" - ); - } - - #[tokio::test] - async fn jwt_keypair_public_parses() { - let (_, public) = generate_jwt_keypair().unwrap(); - jsonwebtoken::DecodingKey::from_ed_pem(public.as_bytes()).expect("public key should parse"); - } - - #[tokio::test] - async fn jwt_keypair_private_parses() { - let (private, _) = generate_jwt_keypair().unwrap(); - jsonwebtoken::EncodingKey::from_ed_pem(private.as_bytes()) - .expect("private key should parse"); - } - // -- Config TOML generation -- #[test] @@ -2503,11 +2450,8 @@ client_id = "client-id" #[tokio::test] async fn persist_install_outputs_persists_vault_secrets_via_server_when_autostarting() { let dir = tempfile::tempdir().unwrap(); - let server_env_pairs = vec![ - ("SESSION_SECRET".to_string(), "session".to_string()), - ("FABRO_JWT_PUBLIC_KEY".to_string(), "public-key".to_string()), - ]; - let vault_secrets = vec![ + let server_env_pairs = [("SESSION_SECRET".to_string(), "session".to_string())]; + let vault_secrets = [ CreateSecretRequest { name: "GITHUB_TOKEN".to_string(), value: "gh-token".to_string(), @@ -2541,7 +2485,8 @@ client_id = "client-id" .await; let stop_called = Arc::new(AtomicBool::new(false)); - persist_server_env_secrets(dir.path(), &server_env_pairs).unwrap(); + let env_path = Storage::new(dir.path()).runtime_directory().env_path(); + envfile::merge_env_file(&env_path, server_env_pairs.iter().cloned()).unwrap(); persist_vault_secrets_with( dir.path(), &vault_secrets, @@ -2568,7 +2513,6 @@ client_id = "client-id" std::fs::read_to_string(Storage::new(dir.path()).runtime_directory().env_path()) .unwrap(); assert!(server_env.contains("SESSION_SECRET=session")); - assert!(server_env.contains("FABRO_JWT_PUBLIC_KEY=public-key")); assert_eq!(created.calls_async().await, 2); assert!(stop_called.load(Ordering::SeqCst)); assert!(!Storage::new(dir.path()).secrets_path().exists()); @@ -2577,7 +2521,7 @@ client_id = "client-id" #[tokio::test] async fn persist_vault_secrets_with_leaves_running_server_up() { let dir = tempfile::tempdir().unwrap(); - let vault_secrets = vec![CreateSecretRequest { + let vault_secrets = [CreateSecretRequest { name: "GITHUB_TOKEN".to_string(), value: "gh-token".to_string(), type_: ApiSecretType::Environment, @@ -2785,10 +2729,10 @@ client_id = "client-id" } #[tokio::test] - async fn persist_install_outputs_with_settings_does_not_write_settings_on_secret_failure() { + async fn persist_install_outputs_with_settings_rolls_back_new_files_on_secret_failure() { let dir = tempfile::tempdir().unwrap(); - let server_env_pairs = vec![("SESSION_SECRET".to_string(), "session".to_string())]; - let vault_secrets = vec![CreateSecretRequest { + let server_env_pairs = [("SESSION_SECRET".to_string(), "session".to_string())]; + let vault_secrets = [CreateSecretRequest { name: "GITHUB_CLI_TOKEN".to_string(), value: "gh-token".to_string(), type_: ApiSecretType::Environment, @@ -2823,7 +2767,7 @@ client_id = "client-id" assert!(result.is_err()); assert!( - Storage::new(dir.path()) + !Storage::new(dir.path()) .runtime_directory() .env_path() .exists() @@ -2835,8 +2779,8 @@ client_id = "client-id" #[tokio::test] async fn persist_install_outputs_with_settings_restores_previous_contents_on_secret_failure() { let dir = tempfile::tempdir().unwrap(); - let server_env_pairs = vec![("SESSION_SECRET".to_string(), "session".to_string())]; - let vault_secrets = vec![CreateSecretRequest { + let server_env_pairs = [("SESSION_SECRET".to_string(), "session".to_string())]; + let vault_secrets = [CreateSecretRequest { name: "GITHUB_CLI_TOKEN".to_string(), value: "gh-token".to_string(), type_: ApiSecretType::Environment, diff --git a/lib/crates/fabro-cli/src/commands/mod.rs b/lib/crates/fabro-cli/src/commands/mod.rs index 68eece40f..f9408d15b 100644 --- a/lib/crates/fabro-cli/src/commands/mod.rs +++ b/lib/crates/fabro-cli/src/commands/mod.rs @@ -2,6 +2,7 @@ pub(crate) mod artifact; pub(crate) mod auth; pub(crate) mod config; pub(crate) mod doctor; +pub(crate) mod dump; pub(crate) mod exec; pub(crate) mod graph; pub(crate) mod install; @@ -10,6 +11,7 @@ pub(crate) mod parse; pub(crate) mod pr; pub(crate) mod preflight; pub(crate) mod provider; +pub(crate) mod rebuild; pub(crate) mod render_graph; pub(crate) mod repo; pub(crate) mod run; @@ -17,7 +19,6 @@ pub(crate) mod runs; pub(crate) mod sandbox; pub(crate) mod secret; pub(crate) mod server; -pub(crate) mod store; pub(crate) mod system; pub(crate) mod uninstall; pub(crate) mod upgrade; diff --git a/lib/crates/fabro-cli/src/commands/pr/create.rs b/lib/crates/fabro-cli/src/commands/pr/create.rs index 2844cd503..688e03a73 100644 --- a/lib/crates/fabro-cli/src/commands/pr/create.rs +++ b/lib/crates/fabro-cli/src/commands/pr/create.rs @@ -13,7 +13,7 @@ use tracing::info; use crate::args::PrCreateArgs; use crate::command_context::CommandContext; -use crate::commands::store::rebuild::rebuild_run_store; +use crate::commands::rebuild::rebuild_run_store; use crate::shared::print_json_pretty; use crate::shared::repo::ensure_matching_repo_origin; use crate::user_config; diff --git a/lib/crates/fabro-cli/src/commands/store/rebuild.rs b/lib/crates/fabro-cli/src/commands/rebuild.rs similarity index 100% rename from lib/crates/fabro-cli/src/commands/store/rebuild.rs rename to lib/crates/fabro-cli/src/commands/rebuild.rs diff --git a/lib/crates/fabro-cli/src/commands/run/fork.rs b/lib/crates/fabro-cli/src/commands/run/fork.rs index 9a2a4a5cf..cf5dc7296 100644 --- a/lib/crates/fabro-cli/src/commands/run/fork.rs +++ b/lib/crates/fabro-cli/src/commands/run/fork.rs @@ -6,7 +6,7 @@ use git2::Repository; use crate::args::ForkArgs; use crate::command_context::CommandContext; -use crate::commands::store::rebuild::rebuild_run_store; +use crate::commands::rebuild::rebuild_run_store; use crate::shared::print_json_pretty; use crate::shared::repo::ensure_matching_repo_origin; diff --git a/lib/crates/fabro-cli/src/commands/run/rewind.rs b/lib/crates/fabro-cli/src/commands/run/rewind.rs index e3539c1f0..1a4556026 100644 --- a/lib/crates/fabro-cli/src/commands/run/rewind.rs +++ b/lib/crates/fabro-cli/src/commands/run/rewind.rs @@ -15,7 +15,7 @@ use serde::Serialize; use crate::args::RewindArgs; use crate::command_context::CommandContext; -use crate::commands::store::rebuild::rebuild_run_store; +use crate::commands::rebuild::rebuild_run_store; use crate::server_client::Client; use crate::shared::repo::ensure_matching_repo_origin; use crate::shared::{color_if, print_json_pretty}; diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index 9e87dcc50..6380cf1a9 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -7,15 +7,15 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use anyhow::{Context, Result, anyhow, bail}; +use fabro_config::RuntimeDirectory; use fabro_config::bind::{Bind, BindRequest}; use fabro_config::daemon::ServerDaemon; use fabro_config::user::{FABRO_CONFIG_ENV, default_settings_path, load_settings_config}; -use fabro_config::{RuntimeDirectory, envfile}; use fabro_server::jwt_auth::auth_method_name; -use fabro_server::serve::{DEFAULT_TCP_PORT, ServeArgs}; +use fabro_server::serve::{DEFAULT_TCP_PORT, ServeArgs, resolve_runtime_server_settings_for_start}; +use fabro_server::{process_env_snapshot, validate_startup}; use fabro_types::settings::ServerAuthMethod; use fabro_util::printer::Printer; -use fabro_util::session_secret; use fabro_util::terminal::Styles; use tokio::net::{TcpStream, UnixStream}; use tokio::process::Command as TokioCommand; @@ -222,33 +222,6 @@ fn configured_auth_methods(config_path: Option<&Path>) -> Vec .unwrap_or_default() } -fn valid_session_secret(secret: &str) -> bool { - session_secret::validate_session_secret(secret).is_ok() -} - -fn load_or_create_local_session_secret(runtime_directory: &RuntimeDirectory) -> Result { - if let Some(secret) = std::env::var("SESSION_SECRET") - .ok() - .filter(|secret| valid_session_secret(secret)) - { - return Ok(secret); - } - - let server_env_path = runtime_directory.env_path(); - if let Some(secret) = envfile::read_env_file(&server_env_path) - .ok() - .and_then(|entries| entries.get("SESSION_SECRET").cloned()) - .filter(|secret| valid_session_secret(secret)) - { - return Ok(secret); - } - - let secret = session_secret::generate_session_secret(); - envfile::merge_env_file(&server_env_path, [("SESSION_SECRET", secret.as_str())]) - .with_context(|| format!("merging session secret into {}", server_env_path.display()))?; - Ok(secret) -} - // --------------------------------------------------------------------------- // Foreground mode // --------------------------------------------------------------------------- @@ -261,18 +234,6 @@ async fn execute_foreground( styles: &'static Styles, _printer: Printer, ) -> Result<()> { - let session_secret = load_or_create_local_session_secret(&RuntimeDirectory::new(&storage_dir))?; - let prior_session_secret = std::env::var_os("SESSION_SECRET"); - std::env::set_var("SESSION_SECRET", &session_secret); - let _env_guard = - scopeguard::guard( - prior_session_secret, - |prior_session_secret| match prior_session_secret { - Some(value) => std::env::set_var("SESSION_SECRET", value), - None => std::env::remove_var("SESSION_SECRET"), - }, - ); - super::foreground::serve_with_daemon_record(serve_args, bind, storage_dir, styles).await } @@ -303,6 +264,13 @@ async fn execute_daemon( return Ok(()); } + let resolved_settings = resolve_runtime_server_settings_for_start(serve_args, storage_dir)?; + validate_startup( + runtime_directory.env_path().as_path(), + process_env_snapshot(), + &resolved_settings, + )?; + let log_path = runtime_directory.log_path(); if let Some(parent) = log_path.parent() { std::fs::create_dir_all(parent) @@ -347,9 +315,7 @@ async fn execute_daemon( cmd.arg("--watch-web"); } - let session_secret = load_or_create_local_session_secret(&runtime_directory)?; cmd.arg("--storage-dir").arg(storage_dir); - cmd.env("SESSION_SECRET", &session_secret); cmd.env_remove("FABRO_JSON"); cmd.stdout(stdout_log) diff --git a/lib/crates/fabro-cli/src/commands/store/mod.rs b/lib/crates/fabro-cli/src/commands/store/mod.rs deleted file mode 100644 index 07089c243..000000000 --- a/lib/crates/fabro-cli/src/commands/store/mod.rs +++ /dev/null @@ -1,14 +0,0 @@ -pub(crate) mod dump; -pub(crate) mod rebuild; -mod run_export; - -use anyhow::Result; - -use crate::args::{StoreCommand, StoreNamespace}; -use crate::command_context::CommandContext; - -pub(crate) async fn dispatch(ns: StoreNamespace, base_ctx: &CommandContext) -> Result<()> { - match ns.command { - StoreCommand::Dump(args) => dump::dump_command(&args, base_ctx).await, - } -} diff --git a/lib/crates/fabro-cli/src/commands/store/run_export.rs b/lib/crates/fabro-cli/src/commands/store/run_export.rs deleted file mode 100644 index 42738384d..000000000 --- a/lib/crates/fabro-cli/src/commands/store/run_export.rs +++ /dev/null @@ -1 +0,0 @@ -pub(super) use fabro_workflow::run_dump::RunDump as StoreRunExport; diff --git a/lib/crates/fabro-cli/src/local_server.rs b/lib/crates/fabro-cli/src/local_server.rs index cdba3b556..72bcfc576 100644 --- a/lib/crates/fabro-cli/src/local_server.rs +++ b/lib/crates/fabro-cli/src/local_server.rs @@ -12,9 +12,16 @@ use fabro_server::serve::resolve_bind_request_from_settings; use fabro_types::settings::{ServerAuthMethod, SettingsLayer}; pub(crate) fn storage_dir(settings: &SettingsLayer) -> Result { + storage_dir_with_lookup(settings, &|name| std::env::var(name).ok()) +} + +pub(crate) fn storage_dir_with_lookup( + settings: &SettingsLayer, + lookup: &dyn Fn(&str) -> Option, +) -> Result { let storage_root = fabro_config::resolve_storage_root(settings); let resolved_root = storage_root - .resolve(|name| std::env::var(name).ok()) + .resolve(lookup) .map_err(|err| anyhow::anyhow!("failed to resolve {}: {err}", storage_root.as_source()))?; Ok(PathBuf::from(resolved_root.value)) } diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index c05961151..f9d58d510 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -225,8 +225,8 @@ async fn main_inner() -> (String, Result<()>) { Commands::Artifact(ns) => { commands::artifact::dispatch(ns, &base_ctx).await?; } - Commands::Store(ns) => { - commands::store::dispatch(ns, &base_ctx).await?; + Commands::Dump(args) => { + commands::dump::run(&args, &base_ctx).await?; } Commands::RunsCmd(cmd) => { commands::runs::dispatch(cmd, &base_ctx).await?; @@ -448,7 +448,7 @@ async fn prepare_server_bootstrap( mod tests { use args::{ AuthCommand, AuthNamespace, Commands, InstallGitHubStrategyArg, ModelsCommand, - ProviderCommand, ProviderNamespace, StoreCommand, StoreNamespace, + ProviderCommand, ProviderNamespace, }; use tokio::runtime::Runtime; @@ -883,13 +883,11 @@ level = "warn" } #[test] - fn parse_store_dump_command() { - let cli = Cli::try_parse_from(["fabro", "store", "dump", "ABC123", "-o", "./out"]) - .expect("should parse"); + fn parse_dump_command() { + let cli = + Cli::try_parse_from(["fabro", "dump", "ABC123", "-o", "./out"]).expect("should parse"); match *cli.command.unwrap() { - Commands::Store(StoreNamespace { - command: StoreCommand::Dump(args), - }) => { + Commands::Dump(args) => { assert_eq!(args.run, "ABC123"); assert_eq!(args.output, std::path::PathBuf::from("./out")); } diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 60ef4c21e..fc82ede62 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -500,21 +500,20 @@ mod tests { #[test] fn resolve_local_tcp_credential_does_not_fallback_to_home_dev_token() { let temp_home = tempfile::tempdir().unwrap(); - std::fs::write( - temp_home.path().join("dev-token"), - "fabro_dev_abababababababababababababababababababababababababababababababab", - ) - .unwrap(); - let original_home = std::env::var_os("FABRO_HOME"); - std::env::set_var("FABRO_HOME", temp_home.path()); - let _guard = scopeguard::guard(original_home, |original_home| match original_home { - Some(value) => std::env::set_var("FABRO_HOME", value), - None => std::env::remove_var("FABRO_HOME"), - }); - + let token = "fabro_dev_abababababababababababababababababababababababababababababababab"; + std::fs::write(temp_home.path().join("dev-token"), token).unwrap(); let target = ServerTarget::http_url("http://127.0.0.1:32276").unwrap(); + let store = AuthStore::new(temp_home.path().join("auth.json")); + assert_eq!( + load_cli_dev_token_from_sources(None, &Home::new(temp_home.path())).as_deref(), + Some(token) + ); - assert!(resolve_local_tcp_credential(&target).unwrap().is_none()); + assert!( + resolve_local_tcp_credential_with_store(&target, None, &store, Utc::now()) + .unwrap() + .is_none() + ); } #[test] diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 774d3c8a5..6131c2870 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -249,13 +249,13 @@ root = "{{ env.FABRO_STORAGE_ROOT }}" "#, ); let temp = tempfile::tempdir().unwrap(); - let original = std::env::var_os("FABRO_STORAGE_ROOT"); - std::env::set_var("FABRO_STORAGE_ROOT", temp.path()); - let _guard = scopeguard::guard(original, |original| match original { - Some(value) => std::env::set_var("FABRO_STORAGE_ROOT", value), - None => std::env::remove_var("FABRO_STORAGE_ROOT"), - }); - assert_eq!(storage_dir(&settings).unwrap(), temp.path()); + assert_eq!( + local_server::storage_dir_with_lookup(&settings, &|name| { + (name == "FABRO_STORAGE_ROOT").then(|| temp.path().display().to_string()) + }) + .unwrap(), + temp.path() + ); } } diff --git a/lib/crates/fabro-cli/tests/it/cmd/store_dump.rs b/lib/crates/fabro-cli/tests/it/cmd/dump.rs similarity index 89% rename from lib/crates/fabro-cli/tests/it/cmd/store_dump.rs rename to lib/crates/fabro-cli/tests/it/cmd/dump.rs index 1fedcf9e0..355dc7f0c 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/store_dump.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/dump.rs @@ -16,14 +16,14 @@ use crate::support::{LightweightCli, unique_run_id}; fn help() { let context = test_context!(); let mut cmd = context.command(); - cmd.args(["store", "dump", "--help"]); + cmd.args(["dump", "--help"]); fabro_snapshot!(context.filters(), cmd, @" success: true exit_code: 0 ----- stdout ----- Export a run's durable state to a directory - Usage: fabro store dump [OPTIONS] --output + Usage: fabro dump [OPTIONS] --output Arguments: Run ID prefix or workflow name @@ -42,7 +42,7 @@ fn help() { } #[test] -fn store_dump_accepts_server_target_from_separate_home() { +fn dump_accepts_server_target_from_separate_home() { let context = test_context!(); let run = setup_completed_dry_run(&context); let cli = LightweightCli::new(); @@ -51,7 +51,6 @@ fn store_dump_accepts_server_target_from_separate_home() { let mut cmd = cli.command(); cmd.args([ - "store", "dump", "--server", &server, @@ -63,10 +62,10 @@ fn store_dump_accepts_server_target_from_separate_home() { cmd.env("FABRO_DEV_TOKEN", dev_token); } - let output = cmd.output().expect("store dump should execute"); + let output = cmd.output().expect("dump should execute"); assert!( output.status.success(), - "store dump via remote server target failed\nstdout:\n{}\nstderr:\n{}", + "dump via remote server target failed\nstdout:\n{}\nstderr:\n{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); @@ -74,7 +73,7 @@ fn store_dump_accepts_server_target_from_separate_home() { } #[test] -fn store_dump_exports_large_command_output_backed_by_blob_refs() { +fn dump_exports_large_command_output_backed_by_blob_refs() { let context = test_context!(); let workflow = context.temp_dir.join("large-output.fabro"); fs::write( @@ -130,17 +129,11 @@ fn store_dump_exports_large_command_output_backed_by_blob_refs() { let output_dir = context.temp_dir.join("export"); let mut dump_cmd = context.command(); - dump_cmd.args([ - "store", - "dump", - "--output", - output_dir.to_str().unwrap(), - &run_id, - ]); - let dump_output = dump_cmd.output().expect("store dump should execute"); + dump_cmd.args(["dump", "--output", output_dir.to_str().unwrap(), &run_id]); + let dump_output = dump_cmd.output().expect("dump should execute"); assert!( dump_output.status.success(), - "store dump failed\nstdout:\n{}\nstderr:\n{}", + "dump failed\nstdout:\n{}\nstderr:\n{}", String::from_utf8_lossy(&dump_output.stdout), String::from_utf8_lossy(&dump_output.stderr) ); @@ -153,7 +146,7 @@ fn store_dump_exports_large_command_output_backed_by_blob_refs() { } #[test] -fn store_dump_exports_blob_refs_and_artifacts_together() { +fn dump_exports_blob_refs_and_artifacts_together() { let context = test_context!(); let workspace_dir = context.temp_dir.join("mixed-export"); fs::create_dir_all(&workspace_dir).unwrap(); @@ -233,17 +226,11 @@ include = ["assets/**"] let output_dir = context.temp_dir.join("export-mixed"); let mut dump_cmd = context.command(); - dump_cmd.args([ - "store", - "dump", - "--output", - output_dir.to_str().unwrap(), - &run_id, - ]); - let dump_output = dump_cmd.output().expect("store dump should execute"); + dump_cmd.args(["dump", "--output", output_dir.to_str().unwrap(), &run_id]); + let dump_output = dump_cmd.output().expect("dump should execute"); assert!( dump_output.status.success(), - "store dump failed\nstdout:\n{}\nstderr:\n{}", + "dump failed\nstdout:\n{}\nstderr:\n{}", String::from_utf8_lossy(&dump_output.stdout), String::from_utf8_lossy(&dump_output.stderr) ); @@ -260,14 +247,13 @@ include = ["assets/**"] } #[test] -fn store_dump_exports_completed_run_snapshot() { +fn dump_exports_completed_run_snapshot() { let context = test_context!(); let run = setup_completed_dry_run(&context); let output_dir = context.temp_dir.join("export"); let mut cmd = context.command(); cmd.args([ - "store", "dump", "--output", output_dir.to_str().unwrap(), @@ -298,7 +284,7 @@ fn store_dump_exports_completed_run_snapshot() { } #[test] -fn store_dump_rejects_non_empty_output_dir() { +fn dump_rejects_non_empty_output_dir() { let context = test_context!(); let run = setup_completed_dry_run(&context); let output_dir = context.temp_dir.join("nonempty"); @@ -307,7 +293,6 @@ fn store_dump_rejects_non_empty_output_dir() { let mut cmd = context.command(); cmd.args([ - "store", "dump", "--output", output_dir.to_str().unwrap(), diff --git a/lib/crates/fabro-cli/tests/it/cmd/fabro.rs b/lib/crates/fabro-cli/tests/it/cmd/fabro.rs index ee823c0f5..a8fbc255d 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/fabro.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/fabro.rs @@ -25,7 +25,7 @@ fn help() { validate Validate a workflow graph Render a workflow graph as SVG artifact Inspect and copy run artifacts (screenshots, reports, traces) - store Export store-backed run state for debugging + dump Export a run's durable state to a directory rm Remove one or more workflow runs inspect Show detailed information about a workflow run archive Mark terminal runs as archived (reviewed, no further action needed). Archived runs are hidden from default listings diff --git a/lib/crates/fabro-cli/tests/it/cmd/mod.rs b/lib/crates/fabro-cli/tests/it/cmd/mod.rs index 8cae23120..880ef3d99 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/mod.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/mod.rs @@ -9,6 +9,7 @@ mod diff; mod discord; mod docs; mod doctor; +mod dump; mod exec; mod fabro; mod fork; @@ -53,8 +54,6 @@ mod server_start; mod server_status; mod server_stop; mod start; -mod store; -mod store_dump; pub(crate) mod support; mod system; mod system_df; diff --git a/lib/crates/fabro-cli/tests/it/cmd/ps.rs b/lib/crates/fabro-cli/tests/it/cmd/ps.rs index a6d021a3b..4642010d1 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/ps.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/ps.rs @@ -9,12 +9,17 @@ use crate::support::{fatal_error_line, unique_run_id}; const TEST_DEV_TOKEN: &str = "fabro_dev_abababababababababababababababababababababababababababababababab"; +const TEST_SESSION_SECRET: &str = + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; fn provision_local_server_auth(context: &fabro_test::TestContext, storage_dir: &std::path::Path) { context.ensure_home_server_auth_methods(); let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); - envfile::merge_env_file(&server_env_path, [("FABRO_DEV_TOKEN", TEST_DEV_TOKEN)]) - .expect("merging FABRO_DEV_TOKEN into server.env"); + envfile::merge_env_file(&server_env_path, [ + ("FABRO_DEV_TOKEN", TEST_DEV_TOKEN), + ("SESSION_SECRET", TEST_SESSION_SECRET), + ]) + .expect("merging server auth into server.env"); dev_token::write_dev_token( &context.home_dir.join(".fabro").join("dev-token"), TEST_DEV_TOKEN, diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index 6cf0ad28c..337c5b213 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -13,16 +13,18 @@ use std::time::{Duration, Instant}; use fabro_store::EventEnvelope; use fabro_test::{assert_reqwest_status, expect_reqwest_json, fabro_snapshot, test_context}; -use fabro_types::{EventBody, FailureReason, RunEvent}; +use fabro_types::{EventBody, FailureReason, RunEvent, StageId}; use httpmock::MockServer; use super::support::{ - local_dev_token, output_stderr, run_events, run_state, server_endpoint, server_target, - wait_for_event_names, wait_for_status, write_gated_workflow, + find_run_dir, local_dev_token, output_stderr, run_events, run_state, server_endpoint, + server_target, wait_for_event_names, wait_for_status, write_gated_workflow, }; use crate::support::{fabro_json_snapshot, unique_run_id}; const SHARED_DAEMON_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); +const LEAKED_WORKER_PARENT_TOKEN: &str = "leak-worker-parent-token"; +const LEAKED_NEW_RELIC_LICENSE: &str = "leak-new-relic-license"; fn auth_context() -> fabro_test::TestContext { let context = test_context!(); @@ -126,6 +128,20 @@ fn worker_command(context: &fabro_test::TestContext) -> assert_cmd::Command { cmd } +fn assert_no_worker_env_leak(scope: &str, content: &str) { + for needle in [ + "MY_API_TOKEN=", + "NEW_RELIC_LICENSE_KEY=", + LEAKED_WORKER_PARENT_TOKEN, + LEAKED_NEW_RELIC_LICENSE, + ] { + assert!( + !content.contains(needle), + "{scope} leaked {needle:?}:\n{content}" + ); + } +} + async fn wait_for_server_question( client: &fabro_http::HttpClient, base_url: &str, @@ -373,6 +389,120 @@ digraph DetachedStoreOnly { assert_worker_succeeded(&run_dir, &output); } +#[test] +fn server_dispatched_worker_does_not_inherit_parent_secret_env() { + let mut context = test_context!(); + let server_root = tempfile::tempdir_in("/tmp").unwrap(); + let storage_dir = server_root.path().join("storage"); + let socket_path = server_root.path().join("fabro.sock"); + let config_path = server_root.path().join("settings.toml"); + context.manage_storage_dir(&storage_dir); + std::fs::write( + &config_path, + format!( + r#"_version = 1 + +[server.storage] +root = "{}" + +[server.auth] +methods = ["dev-token"] +"#, + storage_dir.display() + ), + ) + .expect("writing leak-probe server settings"); + + let start_output = context + .command() + .env("MY_API_TOKEN", LEAKED_WORKER_PARENT_TOKEN) + .env("NEW_RELIC_LICENSE_KEY", LEAKED_NEW_RELIC_LICENSE) + .args(["server", "start"]) + .arg("--storage-dir") + .arg(&storage_dir) + .arg("--bind") + .arg(&socket_path) + .arg("--config") + .arg(&config_path) + .output() + .expect("server start should execute"); + assert!( + start_output.status.success(), + "server start failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&start_output.stdout), + String::from_utf8_lossy(&start_output.stderr) + ); + + let workflow_path = context.temp_dir.join("worker-leak-probe.fabro"); + std::fs::write( + &workflow_path, + r#"digraph WorkerLeakProbe { + graph [goal="Verify worker subprocess env isolation", default_max_retries=0] + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + probe [shape=parallelogram, label="Probe", script="echo probe-ran; for key in $(printf 'MY%s NEW%s' '_API_TOKEN' '_RELIC_LICENSE_KEY'); do value=$(printenv \"$key\" || true); if [ -n \"$value\" ]; then echo \"$key=$value\"; fi; done"] + start -> probe -> exit +} +"#, + ) + .expect("writing leak-probe workflow"); + + let run_id = unique_run_id(); + let dev_token = local_dev_token(&storage_dir).expect("managed server should have a dev token"); + let run_output = context + .run_cmd() + .env("FABRO_DEV_TOKEN", dev_token) + .args([ + "--server", + socket_path.to_str().expect("socket path should be UTF-8"), + "--run-id", + run_id.as_str(), + "--detach", + "--auto-approve", + "--no-retro", + "--sandbox", + "local", + workflow_path + .to_str() + .expect("workflow path should be UTF-8"), + ]) + .output() + .expect("detached leak-probe run should execute"); + assert!( + run_output.status.success(), + "detached run failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&run_output.stdout), + String::from_utf8_lossy(&run_output.stderr) + ); + + let run_dir = find_run_dir(&storage_dir, &run_id).expect("leak-probe run dir should exist"); + wait_for_status(&run_dir, &["succeeded"]); + + let state = run_state(&run_dir); + let _probe = state + .node(&StageId::new("probe", 1)) + .expect("probe node state should exist"); + let stdout = state + .checkpoint + .as_ref() + .and_then(|checkpoint| checkpoint.context_values.get("command.output")) + .and_then(serde_json::Value::as_str) + .expect("probe command output should exist"); + assert!( + stdout.contains("probe-ran"), + "probe stage should have executed, got stdout:\n{stdout}" + ); + assert_no_worker_env_leak("probe stdout", stdout); + assert_no_worker_env_leak( + "run state", + &serde_json::to_string(&state).expect("run state should serialize"), + ); + + let server_log = + std::fs::read_to_string(storage_dir.join("logs/server.log")).unwrap_or_default(); + assert_no_worker_env_leak("server log", &server_log); +} + #[test] fn runner_resume_rejects_completed_run_without_mutating_it() { let context = auth_context(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs index 13996832b..b5c54475b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -14,13 +14,15 @@ use std::time::{Duration, Instant}; use fabro_config::{Storage, envfile}; use fabro_test::{ - apply_test_isolation, fabro_snapshot, isolated_storage_dir, server_log_files, test_context, - wait_for_log_line, wait_for_path, + TestContext, apply_test_isolation, fabro_snapshot, isolated_storage_dir, server_log_files, + test_context, wait_for_log_line, wait_for_path, }; use fabro_util::dev_token; const TEST_DEV_TOKEN: &str = "fabro_dev_abababababababababababababababababababababababababababababababab"; +const TEST_SESSION_SECRET: &str = + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; fn write_dev_token_server_settings(config_path: &std::path::Path, rest: &str) { std::fs::write( @@ -32,12 +34,115 @@ fn write_dev_token_server_settings(config_path: &std::path::Path, rest: &str) { fn provision_dev_token_auth(home_dir: &std::path::Path, storage_dir: &std::path::Path) { let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); - envfile::merge_env_file(&server_env_path, [("FABRO_DEV_TOKEN", TEST_DEV_TOKEN)]) - .expect("merging FABRO_DEV_TOKEN into server.env"); + envfile::merge_env_file(&server_env_path, [ + ("FABRO_DEV_TOKEN", TEST_DEV_TOKEN), + ("SESSION_SECRET", TEST_SESSION_SECRET), + ]) + .expect("merging server auth into server.env"); dev_token::write_dev_token(&home_dir.join(".fabro").join("dev-token"), TEST_DEV_TOKEN) .expect("writing home dev-token"); } +#[derive(Clone, Copy, Debug)] +enum ServerStartMode { + Foreground, + Daemon, +} + +impl ServerStartMode { + const ALL: [Self; 2] = [Self::Foreground, Self::Daemon]; + + fn name(self) -> &'static str { + match self { + Self::Foreground => "foreground", + Self::Daemon => "daemon", + } + } + + fn add_args(self, cmd: &mut assert_cmd::Command) { + if matches!(self, Self::Foreground) { + cmd.arg("--foreground"); + } + } +} + +struct StartupFailureCase { + name: &'static str, + settings: &'static str, + server_env: &'static [(&'static str, &'static str)], + expected_error: &'static str, +} + +fn run_startup_failure(context: &TestContext, mode: ServerStartMode, case: &StartupFailureCase) { + let storage_root = isolated_storage_dir(); + let storage_dir = storage_root + .path() + .join(format!("{}-{}", case.name, mode.name())); + let socket_path = storage_root + .path() + .join(format!("{}-{}.sock", case.name, mode.name())); + let config_dir = tempfile::tempdir_in("/tmp").expect("creating startup failure config dir"); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write(&config_path, case.settings).expect("writing startup failure settings"); + if !case.server_env.is_empty() { + envfile::merge_env_file( + &Storage::new(&storage_dir).runtime_directory().env_path(), + case.server_env.iter().copied(), + ) + .expect("writing startup failure server.env"); + } + + let mut cmd = context.command(); + cmd.args(["server", "start"]); + mode.add_args(&mut cmd); + cmd.arg("--storage-dir") + .arg(&storage_dir) + .arg("--bind") + .arg(&socket_path) + .arg("--config") + .arg(&config_path); + let output = cmd + .output() + .expect("server start failure command should run"); + + assert!( + !output.status.success(), + "server start should reject {} in {} mode\nstdout:\n{}\nstderr:\n{}", + case.name, + mode.name(), + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + output.stdout.is_empty(), + "server start rejection should not write stdout for {} in {} mode:\n{}", + case.name, + mode.name(), + String::from_utf8_lossy(&output.stdout) + ); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + format!("error: {}\n", case.expected_error), + "unexpected stderr for {} in {} mode", + case.name, + mode.name() + ); + + let log_path = storage_dir.join("logs/server.log"); + match mode { + ServerStartMode::Foreground => assert!( + log_path.exists(), + "foreground validation intentionally runs after log bootstrap for {}", + case.name + ), + ServerStartMode::Daemon => assert!( + !log_path.exists(), + "daemon validation should fail before creating server.log for {}", + case.name + ), + } +} + #[test] fn help() { let context = test_context!(); @@ -96,6 +201,120 @@ fn help() { "); } +#[test] +fn start_rejects_invalid_startup_configuration_in_foreground_and_daemon() { + const DEV_TOKEN_SETTINGS: &str = r#"_version = 1 + +[server.auth] +methods = ["dev-token"] +"#; + const GITHUB_SETTINGS: &str = r#"_version = 1 + +[server.web] +enabled = true + +[server.auth] +methods = ["github"] + +[server.auth.github] +allowed_usernames = ["octocat"] + +[server.integrations.github] +client_id = "Iv1.testclient" +"#; + const GITHUB_WITHOUT_CLIENT_ID_SETTINGS: &str = r#"_version = 1 + +[server.web] +enabled = true + +[server.auth] +methods = ["github"] + +[server.auth.github] +allowed_usernames = ["octocat"] +"#; + const GITHUB_WEB_DISABLED_SETTINGS: &str = r#"_version = 1 + +[server.web] +enabled = false + +[server.auth] +methods = ["github"] + +[server.auth.github] +allowed_usernames = ["octocat"] + +[server.integrations.github] +client_id = "Iv1.testclient" +"#; + const EMPTY_AUTH_METHODS_SETTINGS: &str = r"_version = 1 + +[server.auth] +methods = [] +"; + + let context = test_context!(); + let cases = [ + StartupFailureCase { + name: "missing-session-secret", + settings: DEV_TOKEN_SETTINGS, + server_env: &[("FABRO_DEV_TOKEN", TEST_DEV_TOKEN)], + expected_error: "Fabro server refuses to start: auth is configured but SESSION_SECRET is not set.", + }, + StartupFailureCase { + name: "missing-dev-token", + settings: DEV_TOKEN_SETTINGS, + server_env: &[("SESSION_SECRET", TEST_SESSION_SECRET)], + expected_error: "Fabro server refuses to start: dev-token auth is enabled but FABRO_DEV_TOKEN is not set.", + }, + StartupFailureCase { + name: "missing-github-client-secret", + settings: GITHUB_SETTINGS, + server_env: &[("SESSION_SECRET", TEST_SESSION_SECRET)], + expected_error: "Fabro server refuses to start: github auth is enabled but GITHUB_APP_CLIENT_SECRET is not set.", + }, + StartupFailureCase { + name: "empty-auth-methods", + settings: EMPTY_AUTH_METHODS_SETTINGS, + server_env: &[], + expected_error: "failed to resolve server settings:\n server.auth.methods: invalid value - must not be empty", + }, + StartupFailureCase { + name: "github-web-disabled", + settings: GITHUB_WEB_DISABLED_SETTINGS, + server_env: &[ + ("SESSION_SECRET", TEST_SESSION_SECRET), + ("GITHUB_APP_CLIENT_SECRET", "github-client-secret"), + ], + expected_error: "Fabro server refuses to start: github auth is enabled but server.web.enabled is false.", + }, + StartupFailureCase { + name: "github-missing-client-id", + settings: GITHUB_WITHOUT_CLIENT_ID_SETTINGS, + server_env: &[ + ("SESSION_SECRET", TEST_SESSION_SECRET), + ("GITHUB_APP_CLIENT_SECRET", "github-client-secret"), + ], + expected_error: "Fabro server refuses to start: github auth is enabled but server.integrations.github.client_id is not configured.", + }, + StartupFailureCase { + name: "invalid-dev-token", + settings: DEV_TOKEN_SETTINGS, + server_env: &[ + ("SESSION_SECRET", TEST_SESSION_SECRET), + ("FABRO_DEV_TOKEN", "not-a-valid-dev-token"), + ], + expected_error: "Fabro server refuses to start: FABRO_DEV_TOKEN has invalid format.", + }, + ]; + + for case in &cases { + for mode in ServerStartMode::ALL { + run_startup_failure(&context, mode, case); + } + } +} + #[test] fn start_already_running_exits_with_error() { let context = test_context!(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/store.rs b/lib/crates/fabro-cli/tests/it/cmd/store.rs deleted file mode 100644 index 082ce6ba3..000000000 --- a/lib/crates/fabro-cli/tests/it/cmd/store.rs +++ /dev/null @@ -1,29 +0,0 @@ -use fabro_test::{fabro_snapshot, test_context}; - -#[test] -fn help() { - let context = test_context!(); - let mut cmd = context.command(); - cmd.args(["store", "--help"]); - fabro_snapshot!(context.filters(), cmd, @" - success: true - exit_code: 0 - ----- stdout ----- - Export store-backed run state for debugging - - Usage: fabro store [OPTIONS] - - Commands: - dump Export a run's durable state to a directory - help Print this message or the help of the given subcommand(s) - - Options: - --json Output as JSON [env: FABRO_JSON=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help - ----- stderr ----- - "); -} diff --git a/lib/crates/fabro-cli/tests/it/scenario/server_lifecycle.rs b/lib/crates/fabro-cli/tests/it/scenario/server_lifecycle.rs index 5abc94115..5906cc6da 100644 --- a/lib/crates/fabro-cli/tests/it/scenario/server_lifecycle.rs +++ b/lib/crates/fabro-cli/tests/it/scenario/server_lifecycle.rs @@ -13,10 +13,16 @@ fn start_status_stop_lifecycle() { let server_env_path = fabro_config::Storage::new(&storage_dir) .runtime_directory() .env_path(); - fabro_config::envfile::merge_env_file(&server_env_path, [( - "FABRO_DEV_TOKEN", - "fabro_dev_abababababababababababababababababababababababababababababababab", - )]) + fabro_config::envfile::merge_env_file(&server_env_path, [ + ( + "FABRO_DEV_TOKEN", + "fabro_dev_abababababababababababababababababababababababababababababababab", + ), + ( + "SESSION_SECRET", + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + ), + ]) .unwrap(); fabro_util::dev_token::write_dev_token( &context.home_dir.join(".fabro").join("dev-token"), diff --git a/lib/crates/fabro-cli/tests/it/support/auth_harness.rs b/lib/crates/fabro-cli/tests/it/support/auth_harness.rs index a8a81b40b..6f69c096f 100644 --- a/lib/crates/fabro-cli/tests/it/support/auth_harness.rs +++ b/lib/crates/fabro-cli/tests/it/support/auth_harness.rs @@ -22,7 +22,8 @@ use fabro_server::auth::GithubEndpoints; use fabro_server::ip_allowlist::IpAllowlistConfig; use fabro_server::jwt_auth::resolve_auth_mode_with_lookup; use fabro_server::server::{ - RouterOptions, build_router_with_options, create_app_state_with_env_lookup, + RouterOptions, build_router_with_options, + create_app_state_with_env_lookup_and_server_secret_env, }; use fabro_test::{GitHubAppState, TestContext, apply_test_isolation}; use fabro_types::RunAuthMethod; @@ -79,12 +80,21 @@ impl RealAuthHarness { _ => None, }) .expect("auth mode should resolve"); - let state = create_app_state_with_env_lookup(settings, 5, move |name| match name { - "SESSION_SECRET" => Some(TEST_SESSION_SECRET.to_string()), - "GITHUB_APP_CLIENT_SECRET" => Some(github_client_secret.clone()), - "FABRO_DEV_TOKEN" => dev_token.clone(), - _ => None, - }); + let mut secrets = std::collections::HashMap::from([ + ( + "SESSION_SECRET".to_string(), + TEST_SESSION_SECRET.to_string(), + ), + ( + "GITHUB_APP_CLIENT_SECRET".to_string(), + github_client_secret.clone(), + ), + ]); + if let Some(token) = dev_token.clone() { + secrets.insert("FABRO_DEV_TOKEN".to_string(), token); + } + let state = + create_app_state_with_env_lookup_and_server_secret_env(settings, 5, |_| None, &secrets); let github_base = github_base_url(&twin.base_url); let router = build_router_with_options( state, diff --git a/lib/crates/fabro-cli/tests/it/workflow/command_agent_mixed.rs b/lib/crates/fabro-cli/tests/it/workflow/command_agent_mixed.rs index ee9ba5be8..d0fbabe4f 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/command_agent_mixed.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/command_agent_mixed.rs @@ -6,8 +6,8 @@ use fabro_test::test_context; use super::{ - completed_nodes, find_run_dir, fixture, read_conclusion, run_id_for, sandbox_tests, - store_dump_export, timeout_for, + completed_nodes, dump_export, find_run_dir, fixture, read_conclusion, run_id_for, + sandbox_tests, timeout_for, }; sandbox_tests!(command_agent_mixed, keys = ["ANTHROPIC_API_KEY"]); @@ -48,7 +48,7 @@ fn scenario_command_agent_mixed(sandbox: &str) { "verify should be completed" ); - let export_dir = store_dump_export(&context, &run_id_for(&run_dir)); + let export_dir = dump_export(&context, &run_id_for(&run_dir)); let stdout = std::fs::read_to_string(export_dir.join("stages/verify@1/stdout.log")) .expect("verify stdout.log should exist"); assert!( diff --git a/lib/crates/fabro-cli/tests/it/workflow/command_pipeline.rs b/lib/crates/fabro-cli/tests/it/workflow/command_pipeline.rs index 5a46d92e4..7c1d7eae7 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/command_pipeline.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/command_pipeline.rs @@ -6,8 +6,8 @@ use fabro_test::test_context; use super::{ - completed_nodes, find_run_dir, fixture, read_conclusion, run_id_for, sandbox_tests, - store_dump_export, timeout_for, + completed_nodes, dump_export, find_run_dir, fixture, read_conclusion, run_id_for, + sandbox_tests, timeout_for, }; sandbox_tests!(command_pipeline); @@ -47,7 +47,7 @@ fn scenario_command_pipeline(sandbox: &str) { "step2 should be completed" ); - let export_dir = store_dump_export(&context, &run_id_for(&run_dir)); + let export_dir = dump_export(&context, &run_id_for(&run_dir)); let stdout1 = std::fs::read_to_string(export_dir.join("stages/step1@1/stdout.log")) .expect("step1 stdout.log should exist"); assert!( diff --git a/lib/crates/fabro-cli/tests/it/workflow/full_stack.rs b/lib/crates/fabro-cli/tests/it/workflow/full_stack.rs index 4b03f1155..a2a1feff4 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/full_stack.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/full_stack.rs @@ -6,8 +6,8 @@ use fabro_test::test_context; use super::{ - completed_nodes, find_run_dir, fixture, has_event, read_conclusion, read_run_spec, run_id_for, - sandbox_tests, store_dump_export, timeout_for, + completed_nodes, dump_export, find_run_dir, fixture, has_event, read_conclusion, read_run_spec, + run_id_for, sandbox_tests, timeout_for, }; sandbox_tests!(full_stack, keys = ["ANTHROPIC_API_KEY"]); @@ -73,7 +73,7 @@ fn scenario_full_stack(sandbox: &str) { } // Verify node stdout should contain PASS - let export_dir = store_dump_export(&context, &run_id_for(&run_dir)); + let export_dir = dump_export(&context, &run_id_for(&run_dir)); let stdout = std::fs::read_to_string(export_dir.join("stages/verify@1/stdout.log")) .expect("verify stdout.log should exist"); assert!( diff --git a/lib/crates/fabro-cli/tests/it/workflow/mod.rs b/lib/crates/fabro-cli/tests/it/workflow/mod.rs index 5cf1005a4..b7911837a 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/mod.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/mod.rs @@ -59,17 +59,16 @@ pub(super) fn has_event(run_dir: &Path, event_name: &str) -> bool { .any(|event| event.event.event_name() == event_name) } -pub(super) fn store_dump_export(context: &TestContext, run_id: &str) -> PathBuf { - let output_dir = context.temp_dir.join(format!("store-dump-{run_id}")); +pub(super) fn dump_export(context: &TestContext, run_id: &str) -> PathBuf { + let output_dir = context.temp_dir.join(format!("dump-{run_id}")); context .command() .args([ - "store", "dump", "--output", output_dir .to_str() - .expect("store dump output path should be valid UTF-8"), + .expect("dump output path should be valid UTF-8"), run_id, ]) .assert() diff --git a/lib/crates/fabro-config/src/envfile.rs b/lib/crates/fabro-config/src/envfile.rs index 403cf5595..576877827 100644 --- a/lib/crates/fabro-config/src/envfile.rs +++ b/lib/crates/fabro-config/src/envfile.rs @@ -358,7 +358,7 @@ mod tests { let entries = merge_env_file(&path, [ ("SESSION_SECRET", "secret"), - ("FABRO_JWT_PUBLIC_KEY", "jwt"), + ("FABRO_DEV_TOKEN", "token"), ]) .unwrap(); @@ -368,8 +368,8 @@ mod tests { Some("secret") ); assert_eq!( - entries.get("FABRO_JWT_PUBLIC_KEY").map(String::as_str), - Some("jwt") + entries.get("FABRO_DEV_TOKEN").map(String::as_str), + Some("token") ); } diff --git a/lib/crates/fabro-install/src/lib.rs b/lib/crates/fabro-install/src/lib.rs index 7cf5c222f..29d03911a 100644 --- a/lib/crates/fabro-install/src/lib.rs +++ b/lib/crates/fabro-install/src/lib.rs @@ -6,17 +6,8 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; -use base64::Engine as _; -use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use fabro_config::{Storage, envfile}; use fabro_vault::{SecretType as VaultSecretType, Vault}; -use ring::rand::SystemRandom; -use ring::signature::{Ed25519KeyPair, KeyPair as _}; - -const ED25519_SPKI_PREFIX: [u8; 12] = [ - 0x30, 0x2A, 0x30, 0x05, 0x06, 0x03, 0x2B, 0x65, 0x70, 0x03, 0x21, 0x00, -]; -const ED25519_PUBLIC_KEY_LEN: usize = 32; pub struct PendingSettingsWrite<'a> { pub path: &'a Path, @@ -95,47 +86,6 @@ impl std::error::Error for PersistInstallOutputsError { } } -fn pem_encode(label: &str, bytes: &[u8]) -> String { - let body = BASE64_STANDARD.encode(bytes); - let mut pem = String::new(); - pem.push_str("-----BEGIN "); - pem.push_str(label); - pem.push_str("-----\n"); - for chunk in body.as_bytes().chunks(64) { - pem.push_str(std::str::from_utf8(chunk).expect("base64 output should be valid UTF-8")); - pem.push('\n'); - } - pem.push_str("-----END "); - pem.push_str(label); - pem.push_str("-----\n"); - pem -} - -fn ed25519_public_key_spki(public_key: &[u8]) -> Result> { - anyhow::ensure!( - public_key.len() == ED25519_PUBLIC_KEY_LEN, - "generated Ed25519 public key had unexpected length" - ); - - let mut spki = Vec::with_capacity(ED25519_SPKI_PREFIX.len() + public_key.len()); - spki.extend_from_slice(&ED25519_SPKI_PREFIX); - spki.extend_from_slice(public_key); - Ok(spki) -} - -pub fn generate_jwt_keypair() -> Result<(String, String)> { - let pkcs8 = Ed25519KeyPair::generate_pkcs8(&SystemRandom::new()) - .map_err(|_| anyhow::anyhow!("failed to generate Ed25519 keypair"))?; - let keypair = Ed25519KeyPair::from_pkcs8(pkcs8.as_ref()) - .map_err(|_| anyhow::anyhow!("failed to parse generated Ed25519 keypair"))?; - let public_der = ed25519_public_key_spki(keypair.public_key().as_ref())?; - - Ok(( - pem_encode("PRIVATE KEY", pkcs8.as_ref()), - pem_encode("PUBLIC KEY", &public_der), - )) -} - pub fn default_web_url() -> String { "http://127.0.0.1:32276".to_string() } diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 926f4c9b3..7ea025eee 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -540,30 +540,6 @@ fn check_crypto(state: &AppState) -> CheckResult { } } - if let Some(raw) = state.server_secret("FABRO_JWT_PUBLIC_KEY") { - if let Err(err) = decode_pem_value("FABRO_JWT_PUBLIC_KEY", &raw).and_then(|pem| { - jsonwebtoken::DecodingKey::from_ed_pem(pem.as_bytes()) - .map(|_| ()) - .map_err(|e| format!("invalid JWT public key: {e}")) - }) { - errors.push(err); - } else { - details.push(CheckDetail::new("FABRO_JWT_PUBLIC_KEY valid".to_string())); - } - } - - if let Some(raw) = state.server_secret("FABRO_JWT_PRIVATE_KEY") { - if let Err(err) = decode_pem_value("FABRO_JWT_PRIVATE_KEY", &raw).and_then(|pem| { - jsonwebtoken::EncodingKey::from_ed_pem(pem.as_bytes()) - .map(|_| ()) - .map_err(|e| format!("invalid JWT private key: {e}")) - }) { - errors.push(err); - } else { - details.push(CheckDetail::new("FABRO_JWT_PRIVATE_KEY valid".to_string())); - } - } - if errors.is_empty() { CheckResult { name: "Crypto".to_string(), diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index e1dbe6969..301d7a943 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -18,9 +18,8 @@ use fabro_config::bind::{Bind, BindRequest}; use fabro_config::envfile::EnvFileUpdate; use fabro_install::{ InstallListenConfig, OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_SECRET_ACCESS_KEY_ENV, - PendingSettingsWrite, VaultSecretWrite, generate_jwt_keypair, merge_server_settings, - persist_install_outputs_direct, write_github_app_settings, write_object_store_settings, - write_token_settings, + PendingSettingsWrite, VaultSecretWrite, merge_server_settings, persist_install_outputs_direct, + write_github_app_settings, write_object_store_settings, write_token_settings, }; use fabro_model::Provider; use fabro_store::ArtifactStore; @@ -43,7 +42,7 @@ use zeroize::Zeroizing; use crate::error::ApiError; use crate::serve::{self, DEFAULT_TCP_PORT}; -use crate::server_secrets::ServerSecrets; +use crate::server_secrets::{ServerSecrets, process_env_snapshot}; use crate::{security_headers, static_files}; #[derive(Clone)] @@ -113,6 +112,11 @@ impl InstallAppState { reason = "test-only: set FABRO_TEST_IN_MEMORY_STORE to a constant so install tests \ don't hang on real S3; parallel tests race on the same value" )] + #[expect( + clippy::disallowed_methods, + reason = "test-only: forces the in-memory object store for install tests so they \ + don't contact real S3" + )] pub fn for_test_with_paths(token: &str, storage_dir: &Path, config_path: &Path) -> Self { // Install-flow tests verify persistence and redaction, not S3 // reachability. Force the in-memory object store shortcut so @@ -973,7 +977,8 @@ async fn validate_install_object_store_selection( let server_env_path = Storage::new(state.storage_dir.as_ref()) .runtime_directory() .env_path(); - let server_secrets = ServerSecrets::load(server_env_path).map_err(|err| err.to_string())?; + let server_secrets = ServerSecrets::load(server_env_path, process_env_snapshot()) + .map_err(|err| err.to_string())?; let build_options = serve::ObjectStoreBuildOptions { client_options, retry_config: RetryConfig { @@ -1373,23 +1378,7 @@ async fn post_install_finish( }; let session_secret = session_secret::generate_session_secret(); - let (jwt_private_pem, jwt_public_pem) = match generate_jwt_keypair() { - Ok(value) => value, - Err(err) => { - return install_error_response(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()); - } - }; - server_env_writes.extend([ - make_env_write( - "FABRO_JWT_PRIVATE_KEY", - BASE64_STANDARD.encode(jwt_private_pem.as_bytes()), - ), - make_env_write( - "FABRO_JWT_PUBLIC_KEY", - BASE64_STANDARD.encode(jwt_public_pem.as_bytes()), - ), - make_env_write("SESSION_SECRET", session_secret), - ]); + server_env_writes.push(make_env_write("SESSION_SECRET", session_secret)); if let Some(token) = dev_token.as_ref() { server_env_writes.push(make_env_write("FABRO_DEV_TOKEN", token.clone())); } @@ -1968,6 +1957,7 @@ async fn wait_for_shutdown(mut shutdown_rx: watch::Receiver) { #[cfg(test)] mod tests { + use std::collections::HashMap; use std::io; use std::sync::atomic::AtomicBool; use std::sync::{Arc, Mutex}; @@ -2108,7 +2098,7 @@ AWS_SESSION_TOKEN=ambient-session\n\ AWS_WEB_IDENTITY_TOKEN_FILE=/tmp/fabro-web-identity-token\n", ) .unwrap(); - let server_secrets = ServerSecrets::with_env_lookup(env_path.clone(), |_| None).unwrap(); + let server_secrets = ServerSecrets::load(env_path.clone(), HashMap::new()).unwrap(); let manual_credentials = InstallAwsCredentialPair::new("submitted-access", "submitted-secret"); diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index 55b6e3db0..96e48a8c1 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -31,7 +31,11 @@ pub mod security_headers; pub mod serve; pub mod server; mod server_secrets; +mod spawn_env; +mod startup; pub mod static_files; pub mod web_auth; pub use error::{ApiError, Error, Result}; +pub use server_secrets::process_env_snapshot; +pub use startup::validate_startup; diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 3705d5437..955c9d955 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -32,12 +32,12 @@ use tracing::{error, info, warn}; use crate::canonical_origin::resolve_canonical_origin; use crate::github_webhooks::{TailscaleFunnelManager, WEBHOOK_ROUTE, WEBHOOK_SECRET_ENV}; use crate::ip_allowlist::{GitHubMetaResolver, IpAllowlistConfig, resolve_ip_allowlist_config}; -use crate::jwt_auth::resolve_auth_mode_with_lookup; use crate::server::{ AppState, AppStateConfig, RouterOptions, build_app_state, build_router_with_options, reconcile_incomplete_runs_on_startup, shutdown_active_workers, spawn_scheduler, }; -use crate::server_secrets::ServerSecrets; +use crate::server_secrets::{ServerSecrets, process_env_snapshot}; +use crate::startup::resolve_startup; const TEST_IN_MEMORY_STORE_ENV: &str = "FABRO_TEST_IN_MEMORY_STORE"; const AWS_SESSION_TOKEN_ENV: &str = "AWS_SESSION_TOKEN"; @@ -475,6 +475,15 @@ fn resolve_server_settings(file: &SettingsLayer) -> anyhow::Result anyhow::Result { + let disk_settings = load_settings_config(args.config.as_deref())?; + let effective_settings = apply_runtime_settings(&disk_settings, args, data_dir); + resolve_server_settings(&effective_settings) +} + pub fn resolve_bind_request_from_settings( settings: &SettingsLayer, explicit_bind: Option<&str>, @@ -533,7 +542,7 @@ fn resolve_interp_path(value: &InterpString) -> anyhow::Result { fn load_server_secrets_for_settings(settings: &ServerNamespace) -> anyhow::Result { let storage_root = resolve_interp_path(&settings.storage.root)?; let server_env_path = Storage::new(&storage_root).runtime_directory().env_path(); - ServerSecrets::load(server_env_path).map_err(anyhow::Error::from) + ServerSecrets::load(server_env_path, process_env_snapshot()).map_err(anyhow::Error::from) } pub(crate) fn build_artifact_object_store_with_server_secrets( @@ -615,24 +624,21 @@ where let storage = Storage::new(&data_dir); let vault_path = storage.secrets_path(); let server_env_path = storage.runtime_directory().env_path(); - let server_secrets = ServerSecrets::load(server_env_path.clone())?; - let webhook_secret_present = server_secrets.get(WEBHOOK_SECRET_ENV).is_some(); - // Shared config for live reloading let effective_settings = apply_runtime_settings(&disk_settings, &args, &data_dir); let resolved_server_settings = resolve_server_settings(&effective_settings)?; + let (auth_mode, server_secrets) = resolve_startup( + &server_env_path, + process_env_snapshot(), + &resolved_server_settings, + )?; + let webhook_secret_present = server_secrets.get(WEBHOOK_SECRET_ENV).is_some(); let bind_request = resolve_bind_request_from_settings(&effective_settings, args.bind.as_deref())?; let shared_settings = Arc::new(RwLock::new(effective_settings)); std::fs::create_dir_all(&data_dir) .with_context(|| format!("creating data directory {}", data_dir.display()))?; - let (auth_mode, max_concurrent_runs) = { - let auth_mode = resolve_auth_mode_with_lookup(&resolved_server_settings, |name| { - server_secrets.get(name) - })?; - let max_concurrent_runs = resolved_server_settings.scheduler.max_concurrent_runs; - (auth_mode, max_concurrent_runs) - }; + let max_concurrent_runs = resolved_server_settings.scheduler.max_concurrent_runs; let web_enabled = resolved_server_settings.web.enabled; let github_meta_resolver = GitHubMetaResolver::from_cache_dir(&storage.cache_dir())?; @@ -665,7 +671,7 @@ where store, artifact_store, vault_path, - server_env_path, + server_secrets, env_lookup, http_client: None, })?; diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index fc3ee36f3..27e1d1e39 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -125,6 +125,7 @@ use crate::run_selector::{ResolveRunError, resolve_run_by_selector}; use crate::server_secrets::{ LlmClientResult, ProviderCredentials, ServerSecrets, auth_issue_message, }; +use crate::spawn_env::{apply_render_graph_env, apply_worker_env}; use crate::{demo, diagnostics, run_manifest, security_headers, static_files, web_auth}; pub(crate) type EnvLookup = Arc Option + Send + Sync>; @@ -572,7 +573,7 @@ pub struct AppState { pub(crate) files_in_flight: FilesInFlight, pub(crate) vault: Arc>, - pub(crate) server_secrets: ServerSecrets, + pub(super) server_secrets: ServerSecrets, pub(crate) provider_credentials: ProviderCredentials, pub(crate) settings: Arc>, pub(crate) server_settings: RwLock>, @@ -591,7 +592,7 @@ pub(crate) struct AppStateConfig { pub(crate) store: Arc, pub(crate) artifact_store: ArtifactStore, pub(crate) vault_path: PathBuf, - pub(crate) server_env_path: PathBuf, + pub(crate) server_secrets: ServerSecrets, pub(crate) env_lookup: EnvLookup, pub(crate) http_client: Option, } @@ -2407,6 +2408,21 @@ pub fn create_app_state_with_env_lookup( settings: SettingsLayer, max_concurrent_runs: usize, env_lookup: impl Fn(&str) -> Option + Send + Sync + 'static, +) -> Arc { + create_app_state_with_env_lookup_and_server_secret_env( + settings, + max_concurrent_runs, + env_lookup, + &HashMap::new(), + ) +} + +#[doc(hidden)] +pub fn create_app_state_with_env_lookup_and_server_secret_env( + settings: SettingsLayer, + max_concurrent_runs: usize, + env_lookup: impl Fn(&str) -> Option + Send + Sync + 'static, + server_secret_env: &HashMap, ) -> Arc { let (store, artifact_store) = test_store_bundle(); let env_lookup: EnvLookup = Arc::new(env_lookup); @@ -2415,6 +2431,8 @@ pub fn create_app_state_with_env_lookup( let mut config = default_test_app_state_config(settings, max_concurrent_runs, env_lookup); config.store = store; config.artifact_store = artifact_store; + let server_env_path = config.vault_path.with_file_name("server.env"); + config.server_secrets = load_test_server_secrets(server_env_path, server_secret_env.clone()); build_app_state(config).expect("test app state should build") } @@ -2450,7 +2468,7 @@ pub(crate) fn create_test_app_state_with_session_key( store, artifact_store, vault_path, - server_env_path, + server_secrets: load_test_server_secrets(server_env_path, HashMap::new()), env_lookup, http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), }) @@ -2485,7 +2503,7 @@ fn default_test_app_state_config( store, artifact_store, vault_path, - server_env_path, + server_secrets: load_test_server_secrets(server_env_path, HashMap::new()), env_lookup, http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), } @@ -2542,6 +2560,10 @@ fn default_env_lookup() -> EnvLookup { Arc::new(|name| std::env::var(name).ok()) } +fn load_test_server_secrets(path: PathBuf, env: HashMap) -> ServerSecrets { + ServerSecrets::load(path, env).expect("test server secrets should load") +} + pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result> { let AppStateConfig { settings, @@ -2550,16 +2572,12 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result Router { let secret = TEST_WEBHOOK_SECRET.to_string(); - let state = create_app_state_with_env_lookup(SettingsLayer::default(), 5, move |name| { - (name == WEBHOOK_SECRET_ENV).then(|| secret.clone()) - }); + let state = create_app_state_with_env_lookup_and_server_secret_env( + SettingsLayer::default(), + 5, + |_| None, + &HashMap::from([(WEBHOOK_SECRET_ENV.to_string(), secret)]), + ); build_router_with_options( state, &auth_mode, @@ -7782,12 +7802,11 @@ root = "/srv/new" ) .unwrap(); - let secrets = - ServerSecrets::with_env_lookup(dir.path().join("server.env"), |name| match name { - "SESSION_SECRET" => Some("env-value".to_string()), - _ => None, - }) - .unwrap(); + let secrets = ServerSecrets::load( + dir.path().join("server.env"), + HashMap::from([("SESSION_SECRET".to_string(), "env-value".to_string())]), + ) + .unwrap(); assert_eq!(secrets.get("SESSION_SECRET").as_deref(), Some("env-value")); assert_eq!( @@ -7811,7 +7830,7 @@ root = "/srv/new" .unwrap(); assert_eq!( command_env_value(&github_cmd, "FABRO_DEV_TOKEN"), - EnvOverride::Removed + EnvOverride::Unchanged ); let dev_token = tempfile::tempdir().unwrap(); @@ -7867,10 +7886,15 @@ allowed_usernames = ["octocat"] .write(&runtime_directory) .unwrap(); - create_app_state_with_env_lookup(settings, 5, move |name| match name { - "FABRO_DEV_TOKEN" => dev_token.clone(), - _ => None, - }) + let server_secret_env = dev_token + .map(|token| HashMap::from([("FABRO_DEV_TOKEN".to_string(), token)])) + .unwrap_or_default(); + create_app_state_with_env_lookup_and_server_secret_env( + settings, + 5, + |_| None, + &server_secret_env, + ) } #[cfg(unix)] diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs index d296f0f0b..b09ed710b 100644 --- a/lib/crates/fabro-server/src/server_secrets.rs +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -1,5 +1,5 @@ use std::collections::HashMap; -use std::path::PathBuf; +use std::path::Path; use std::sync::Arc; use fabro_auth::{CredentialResolver, CredentialUsage, ResolveError, ResolvedCredential}; @@ -11,6 +11,10 @@ use tokio::sync::RwLock as AsyncRwLock; type EnvLookup = Arc Option + Send + Sync>; +pub fn process_env_snapshot() -> HashMap { + std::env::vars().collect() +} + #[derive(Debug, thiserror::Error)] pub(crate) enum Error { #[error(transparent)] @@ -18,36 +22,33 @@ pub(crate) enum Error { } pub(crate) struct ServerSecrets { - path: PathBuf, + env_entries: HashMap, file_entries: HashMap, - env_lookup: EnvLookup, } impl ServerSecrets { - pub(crate) fn load(path: PathBuf) -> Result { - Self::with_env_lookup(path, |name| std::env::var(name).ok()) - } - - pub(crate) fn with_env_lookup(path: PathBuf, env_lookup: F) -> Result - where - F: Fn(&str) -> Option + Send + Sync + 'static, - { + pub(crate) fn load( + path: impl AsRef, + env_entries: HashMap, + ) -> Result { Ok(Self { - file_entries: envfile::read_env_file(&path)?, - path, - env_lookup: Arc::new(env_lookup), + env_entries, + file_entries: envfile::read_env_file(path.as_ref())?, }) } pub(crate) fn get(&self, name: &str) -> Option { - (self.env_lookup)(name).or_else(|| self.file_entries.get(name).cloned()) + self.env_entries + .get(name) + .cloned() + .or_else(|| self.file_entries.get(name).cloned()) } } impl std::fmt::Debug for ServerSecrets { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("ServerSecrets") - .field("path", &self.path) + .field("env_entries", &self.env_entries.keys().collect::>()) .field( "file_entries", &self.file_entries.keys().collect::>(), @@ -149,13 +150,15 @@ impl std::fmt::Debug for ProviderCredentials { #[cfg(test)] mod tests { + use std::collections::HashMap; use std::sync::Arc; use fabro_auth::{AuthCredential, AuthDetails}; + use fabro_config::envfile; use fabro_vault::{SecretType, Vault}; use tokio::sync::RwLock as AsyncRwLock; - use super::ProviderCredentials; + use super::{ProviderCredentials, ServerSecrets}; use crate::server_secrets::Provider; #[tokio::test] @@ -198,4 +201,33 @@ mod tests { Provider::Anthropic ]); } + + #[test] + fn server_secrets_snapshot_prefers_env_over_file() { + let dir = tempfile::tempdir().unwrap(); + let env_path = dir.path().join("server.env"); + envfile::write_env_file( + &env_path, + &HashMap::from([ + ("SESSION_SECRET".to_string(), "file-value".to_string()), + ( + "GITHUB_APP_CLIENT_SECRET".to_string(), + "file-client".to_string(), + ), + ]), + ) + .unwrap(); + + let secrets = ServerSecrets::load( + env_path, + HashMap::from([("SESSION_SECRET".to_string(), "env-value".to_string())]), + ) + .unwrap(); + + assert_eq!(secrets.get("SESSION_SECRET").as_deref(), Some("env-value")); + assert_eq!( + secrets.get("GITHUB_APP_CLIENT_SECRET").as_deref(), + Some("file-client") + ); + } } diff --git a/lib/crates/fabro-server/src/spawn_env.rs b/lib/crates/fabro-server/src/spawn_env.rs new file mode 100644 index 000000000..cd55bd18a --- /dev/null +++ b/lib/crates/fabro-server/src/spawn_env.rs @@ -0,0 +1,125 @@ +use std::ffi::OsString; + +use tokio::process::Command; + +const WORKER_ENV_ALLOWLIST: &[&str] = &[ + "PATH", + "HOME", + "TMPDIR", + "USER", + "RUST_LOG", + "RUST_BACKTRACE", + "FABRO_HOME", + "FABRO_STORAGE_ROOT", +]; + +const RENDER_GRAPH_ENV_ALLOWLIST: &[&str] = &["PATH", "HOME", "TMPDIR"]; + +pub(crate) fn apply_worker_env(cmd: &mut Command) { + apply_allowlist(cmd, WORKER_ENV_ALLOWLIST, &|name| std::env::var_os(name)); +} + +pub(crate) fn apply_render_graph_env(cmd: &mut Command) { + apply_allowlist(cmd, RENDER_GRAPH_ENV_ALLOWLIST, &|name| { + std::env::var_os(name) + }); +} + +fn apply_allowlist(cmd: &mut Command, keys: &[&str], lookup: &dyn Fn(&str) -> Option) { + cmd.env_clear(); + for key in keys { + if let Some(value) = lookup(key) { + cmd.env(key, value); + } + } +} + +#[cfg(all(test, unix))] +mod tests { + use std::collections::HashMap; + use std::ffi::OsString; + use std::path::Path; + + use super::{RENDER_GRAPH_ENV_ALLOWLIST, WORKER_ENV_ALLOWLIST, apply_allowlist}; + + fn env_command() -> tokio::process::Command { + assert!(Path::new("/usr/bin/env").exists()); + tokio::process::Command::new("/usr/bin/env") + } + + async fn env_output(mut cmd: tokio::process::Command) -> HashMap { + let output = cmd.output().await.expect("running env subprocess"); + assert!(output.status.success()); + String::from_utf8(output.stdout) + .expect("parsing env subprocess output as UTF-8") + .lines() + .filter_map(|line| { + let (key, value) = line.split_once('=')?; + Some((key.to_string(), value.to_string())) + }) + .collect() + } + + #[tokio::test] + async fn worker_allowlist_is_fail_closed() { + let env = HashMap::from([ + ("PATH".to_string(), "/bin".to_string()), + ("HOME".to_string(), "/tmp/home".to_string()), + ("TMPDIR".to_string(), "/tmp".to_string()), + ("USER".to_string(), "alice".to_string()), + ("RUST_LOG".to_string(), "debug".to_string()), + ("FABRO_HOME".to_string(), "/tmp/fabro-home".to_string()), + ( + "FABRO_STORAGE_ROOT".to_string(), + "/tmp/fabro-storage".to_string(), + ), + ("SESSION_SECRET".to_string(), "leak".to_string()), + ("FABRO_DEV_TOKEN".to_string(), "garbage".to_string()), + ("MY_API_KEY".to_string(), "blocked".to_string()), + ]); + let mut cmd = env_command(); + apply_allowlist(&mut cmd, WORKER_ENV_ALLOWLIST, &|name| { + env.get(name).map(OsString::from) + }); + cmd.env( + "FABRO_DEV_TOKEN", + "fabro_dev_abababababababababababababababababababababababababababababababab", + ); + + let actual = env_output(cmd).await; + + assert_eq!(actual.get("PATH").map(String::as_str), Some("/bin")); + assert_eq!(actual.get("HOME").map(String::as_str), Some("/tmp/home")); + assert_eq!( + actual.get("FABRO_DEV_TOKEN").map(String::as_str), + Some("fabro_dev_abababababababababababababababababababababababababababababababab") + ); + assert!(!actual.contains_key("SESSION_SECRET")); + assert!(!actual.contains_key("MY_API_KEY")); + } + + #[tokio::test] + async fn render_graph_allowlist_is_fail_closed() { + let env = HashMap::from([ + ("PATH".to_string(), "/bin".to_string()), + ("HOME".to_string(), "/tmp/home".to_string()), + ("TMPDIR".to_string(), "/tmp".to_string()), + ("FABRO_TELEMETRY".to_string(), "on".to_string()), + ("SESSION_SECRET".to_string(), "leak".to_string()), + ]); + let mut cmd = env_command(); + apply_allowlist(&mut cmd, RENDER_GRAPH_ENV_ALLOWLIST, &|name| { + env.get(name).map(OsString::from) + }); + cmd.env("FABRO_TELEMETRY", "off"); + + let actual = env_output(cmd).await; + + assert_eq!(actual.get("PATH").map(String::as_str), Some("/bin")); + assert_eq!( + actual.get("FABRO_TELEMETRY").map(String::as_str), + Some("off") + ); + assert!(!actual.contains_key("SESSION_SECRET")); + } +} diff --git a/lib/crates/fabro-server/src/startup.rs b/lib/crates/fabro-server/src/startup.rs new file mode 100644 index 000000000..6bd2c957f --- /dev/null +++ b/lib/crates/fabro-server/src/startup.rs @@ -0,0 +1,87 @@ +use std::collections::HashMap; +use std::path::Path; + +use fabro_types::settings::ServerNamespace; + +use crate::jwt_auth::{AuthMode, resolve_auth_mode_with_lookup}; +use crate::server_secrets::ServerSecrets; + +pub(crate) fn resolve_startup( + env_path: &Path, + env_entries: HashMap, + settings: &ServerNamespace, +) -> anyhow::Result<(AuthMode, ServerSecrets)> { + let server_secrets = ServerSecrets::load(env_path, env_entries)?; + let auth_mode = resolve_auth_mode_with_lookup(settings, |name| server_secrets.get(name))?; + Ok((auth_mode, server_secrets)) +} + +pub fn validate_startup( + env_path: &Path, + env_entries: HashMap, + settings: &ServerNamespace, +) -> anyhow::Result<()> { + resolve_startup(env_path, env_entries, settings).map(|_| ()) +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use fabro_config::parse_settings_layer; + use fabro_types::settings::ServerNamespace; + + use super::validate_startup; + + fn resolved_settings(auth_methods: &[&str]) -> ServerNamespace { + let settings = parse_settings_layer(&format!( + r" +_version = 1 + +[server.auth] +methods = [{}] +", + auth_methods + .iter() + .map(|method| format!("\"{method}\"")) + .collect::>() + .join(", ") + )) + .unwrap(); + fabro_config::resolve_server_from_file(&settings).unwrap() + } + + #[test] + fn validate_startup_accepts_configured_secrets() { + let dir = tempfile::tempdir().unwrap(); + let env = HashMap::from([ + ( + "SESSION_SECRET".to_string(), + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef".to_string(), + ), + ( + "FABRO_DEV_TOKEN".to_string(), + "fabro_dev_abababababababababababababababababababababababababababababababab" + .to_string(), + ), + ]); + let settings = resolved_settings(&["dev-token"]); + + assert!(validate_startup(dir.path().join("server.env").as_path(), env, &settings).is_ok()); + } + + #[test] + fn validate_startup_rejects_missing_secrets() { + let dir = tempfile::tempdir().unwrap(); + let settings = resolved_settings(&["dev-token"]); + + assert!( + validate_startup( + dir.path().join("server.env").as_path(), + HashMap::new(), + &settings, + ) + .is_err() + ); + } +} diff --git a/lib/crates/fabro-server/tests/it/api/docs.rs b/lib/crates/fabro-server/tests/it/api/docs.rs index e20617f90..006fe67db 100644 --- a/lib/crates/fabro-server/tests/it/api/docs.rs +++ b/lib/crates/fabro-server/tests/it/api/docs.rs @@ -23,8 +23,12 @@ fn security_doc_does_not_require_jwt_keys_for_the_current_web_flow() { "security doc should still mention the session secret" ); assert!( - !security.contains("`FABRO_JWT_PRIVATE_KEY`, `FABRO_JWT_PUBLIC_KEY`, and `SESSION_SECRET`"), - "security doc should not describe JWT keys as required for the current web flow" + !security.contains("FABRO_JWT_PRIVATE_KEY"), + "security doc should not mention removed JWT key settings" + ); + assert!( + !security.contains("FABRO_JWT_PUBLIC_KEY"), + "security doc should not mention removed JWT key settings" ); } diff --git a/lib/crates/fabro-server/tests/it/api/install.rs b/lib/crates/fabro-server/tests/it/api/install.rs index c5bc618d0..95bc8d4d6 100644 --- a/lib/crates/fabro-server/tests/it/api/install.rs +++ b/lib/crates/fabro-server/tests/it/api/install.rs @@ -764,8 +764,6 @@ async fn token_install_finish_persists_settings_env_and_vault() { .env_path(), ) .unwrap(); - assert!(server_env.contains("FABRO_JWT_PRIVATE_KEY=")); - assert!(server_env.contains("FABRO_JWT_PUBLIC_KEY=")); assert!(server_env.contains("SESSION_SECRET=")); assert!(server_env.contains("FABRO_DEV_TOKEN=")); assert!(!server_env.contains("AWS_ACCESS_KEY_ID=")); diff --git a/lib/crates/fabro-server/tests/it/openapi_conformance.rs b/lib/crates/fabro-server/tests/it/openapi_conformance.rs index 898d6fc2a..a12b067bf 100644 --- a/lib/crates/fabro-server/tests/it/openapi_conformance.rs +++ b/lib/crates/fabro-server/tests/it/openapi_conformance.rs @@ -12,7 +12,7 @@ use axum::body::Body; use axum::http::{Method, Request, StatusCode}; use fabro_server::install::{InstallAppState, build_install_router}; use fabro_server::jwt_auth::AuthMode; -use fabro_server::server::{build_router, create_app_state_with_env_lookup}; +use fabro_server::server::{build_router, create_app_state_with_env_lookup_and_server_secret_env}; use serde_yaml::Value; use tower::ServiceExt; @@ -146,9 +146,12 @@ fn github_webhook_spec_and_sdk_describe_a_json_body() { async fn github_webhook_spec_route_is_routable_when_webhook_secret_is_present() { let secret = "test-webhook-secret".to_string(); let app = build_router( - create_app_state_with_env_lookup(test_settings(), 5, move |name| { - (name == "GITHUB_APP_WEBHOOK_SECRET").then(|| secret.clone()) - }), + create_app_state_with_env_lookup_and_server_secret_env( + test_settings(), + 5, + |_| None, + &std::collections::HashMap::from([("GITHUB_APP_WEBHOOK_SECRET".to_string(), secret)]), + ), AuthMode::Disabled, ); diff --git a/lib/crates/fabro-telemetry/src/spawn.rs b/lib/crates/fabro-telemetry/src/spawn.rs index 741ea8748..08e4abe05 100644 --- a/lib/crates/fabro-telemetry/src/spawn.rs +++ b/lib/crates/fabro-telemetry/src/spawn.rs @@ -37,7 +37,7 @@ pub fn spawn_detached(args: &[&str], env: &[(&str, &str)], env_remove: &[&str]) #[expect( clippy::disallowed_types, clippy::disallowed_methods, - reason = "Detaching must flush stdio synchronously before the double-fork." + reason = "Detaching must flush stdio synchronously before the double-fork; post-fork pre-exec env mutation is the one allowed exception to the workspace env-mutation ban." )] fn spawn_detached_unix(args: &[&str], env: &[(&str, &str)], env_remove: &[&str]) { // Flush stdout/stderr before forking so the child process doesn't inherit diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 05b5cca44..4c9f690bf 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -631,8 +631,11 @@ fn write_settings_file(path: &Path, storage_dir: &Path, rest: &str) { fn write_test_server_dev_token(storage_dir: &Path) { let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); - envfile::merge_env_file(&server_env_path, [("FABRO_DEV_TOKEN", TEST_DEV_TOKEN)]) - .unwrap_or_else(|err| panic!("failed to write {}: {err}", server_env_path.display())); + envfile::merge_env_file(&server_env_path, [ + ("FABRO_DEV_TOKEN", TEST_DEV_TOKEN), + ("SESSION_SECRET", TEST_SESSION_SECRET), + ]) + .unwrap_or_else(|err| panic!("failed to write {}: {err}", server_env_path.display())); } fn write_test_home_dev_token(settings_path: &Path) { diff --git a/test/twin/openai/src/config.rs b/test/twin/openai/src/config.rs index 1bb77929a..f7cd31dbd 100644 --- a/test/twin/openai/src/config.rs +++ b/test/twin/openai/src/config.rs @@ -11,20 +11,21 @@ pub struct Config { impl Config { pub fn from_env() -> Result { - let bind_addr = std::env::var("TWIN_OPENAI_BIND_ADDR") - .ok() + Self::from_lookup(&|name| std::env::var(name).ok()) + } + + pub fn from_lookup(lookup: &dyn Fn(&str) -> Option) -> Result { + let bind_addr = lookup("TWIN_OPENAI_BIND_ADDR") .map(|value| value.parse().context("invalid TWIN_OPENAI_BIND_ADDR")) .transpose()? .unwrap_or_else(|| SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 3000)); - let require_auth = std::env::var("TWIN_OPENAI_REQUIRE_AUTH") - .ok() + let require_auth = lookup("TWIN_OPENAI_REQUIRE_AUTH") .map(|value| parse_bool_env(&value, "TWIN_OPENAI_REQUIRE_AUTH")) .transpose()? .unwrap_or(true); - let enable_admin = std::env::var("TWIN_OPENAI_ENABLE_ADMIN") - .ok() + let enable_admin = lookup("TWIN_OPENAI_ENABLE_ADMIN") .map(|value| parse_bool_env(&value, "TWIN_OPENAI_ENABLE_ADMIN")) .transpose()? .unwrap_or(true); diff --git a/test/twin/openai/tests/config_contract.rs b/test/twin/openai/tests/config_contract.rs index e49564332..283c14538 100644 --- a/test/twin/openai/tests/config_contract.rs +++ b/test/twin/openai/tests/config_contract.rs @@ -2,30 +2,14 @@ use twin_openai::config::Config; #[test] fn config_loads_from_environment() { - let prior_bind = std::env::var("TWIN_OPENAI_BIND_ADDR").ok(); - let prior_auth = std::env::var("TWIN_OPENAI_REQUIRE_AUTH").ok(); - let prior_admin = std::env::var("TWIN_OPENAI_ENABLE_ADMIN").ok(); - - std::env::set_var("TWIN_OPENAI_BIND_ADDR", "127.0.0.1:4100"); - std::env::set_var("TWIN_OPENAI_REQUIRE_AUTH", "false"); - std::env::set_var("TWIN_OPENAI_ENABLE_ADMIN", "false"); - - let config = Config::from_env().expect("config should load"); + let config = Config::from_lookup(&|name| match name { + "TWIN_OPENAI_BIND_ADDR" => Some("127.0.0.1:4100".to_string()), + "TWIN_OPENAI_REQUIRE_AUTH" | "TWIN_OPENAI_ENABLE_ADMIN" => Some("false".to_string()), + _ => None, + }) + .expect("config should load"); assert_eq!(config.bind_addr.to_string(), "127.0.0.1:4100"); assert!(!config.require_auth); assert!(!config.enable_admin); - - match prior_bind { - Some(value) => std::env::set_var("TWIN_OPENAI_BIND_ADDR", value), - None => std::env::remove_var("TWIN_OPENAI_BIND_ADDR"), - } - match prior_auth { - Some(value) => std::env::set_var("TWIN_OPENAI_REQUIRE_AUTH", value), - None => std::env::remove_var("TWIN_OPENAI_REQUIRE_AUTH"), - } - match prior_admin { - Some(value) => std::env::set_var("TWIN_OPENAI_ENABLE_ADMIN", value), - None => std::env::remove_var("TWIN_OPENAI_ENABLE_ADMIN"), - } }