--- title: "feat: fabro auth login CLI command with GitHub OAuth + JWT" type: feat status: active date: 2026-04-19 origin: docs/superpowers/specs/2026-04-19-cli-auth-login-design.md --- # `fabro auth login` — CLI authentication via GitHub OAuth ## Overview Adds a `fabro auth login` / `logout` / `status` command group that lets CLI users authenticate to a Fabro server without a shared dev-token. The flow is OAuth 2.0 Authorization Code with PKCE against a loopback listener, driven through the server's existing GitHub OAuth for web. The server issues a short-lived HS256 JWT access token (10 min) plus an opaque rotating refresh token (sliding 30 d) persisted in SlateDB. Dev-token stays unchanged and is preserved as a fallback. ## Problem Frame Today the CLI authenticates only via a shared `fabro_dev_*` secret. Servers that enable GitHub OAuth but disable dev-token (production and shared environments) leave CLI users with no way in. The existing web OAuth flow mints an encrypted session cookie that is browser-only, and the file at `lib/crates/fabro-server/src/jwt_auth.rs` is misleadingly named — it does no JWT issuance or verification today. We need per-user, per-device CLI credentials that are server-side revocable, short-lived on the wire, and forward-compatible with Google Workspace and GitHub Enterprise Server logins. See origin: `docs/superpowers/specs/2026-04-19-cli-auth-login-design.md`. ## Requirements Trace - **R1.** CLI users can log in with `fabro auth login` against a server that accepts only GitHub OAuth (no dev-token). (origin §Problem, §CLI UX) - **R2.** Access credential is a short-lived (10 min) HS256 JWT; refresh credential is an opaque 32-byte secret rotated on every use with 30 d sliding expiry. (origin §Token model) - **R3.** Refresh-token rotation is atomic: two concurrent refreshes cannot both succeed. Reuse detection deletes the entire chain. (origin §Revocation semantics) - **R4.** Identity is OIDC-style `(idp_issuer, idp_subject)` — not GitHub username — forward-compatible with Google/GHES. `login` is display-only. (origin §Identity model) - **R5.** `server.web.url` is the server's single canonical origin. `fabro auth login` opens the browser against the resolved HTTP(S) `--server` target directly; no `/api/v1/auth/cli/config` preflight exists. (origin §Architecture overview, §Canonical origin) - **R6.** `fabro auth login` requires an HTTP(S) server target. Unix-socket targets use the local dev-token flow instead of browser OAuth. Plain HTTP is allowed; operator-managed TLS is a deployment concern. (origin §Canonical origin) - **R7.** Dev-token flow is unchanged. Bearer-priority order: `FABRO_DEV_TOKEN` env → `AuthStore` JWT → dev-token file fallback. (origin §CLI UX, §Bearer priority) - **R8.** Reactive auto-refresh triggers on 401 with `code == "access_token_expired"` in the `ApiError` envelope; refresh failures surface as "session expired — run `fabro auth login`". (origin §Bearer priority) - **R9.** Server-side errors in the browser flow reach the CLI via OAuth-style redirect to loopback (`?error=&state=&error_description=`) when `redirect_uri`+`state` have validated; plain HTML page otherwise. (origin §Browser-to-CLI error handoff) - **R10.** GitHub allowlist rejection at `/auth/callback/github` happens **before** a session is minted; `/auth/cli/resume` must forward the callback's `?error=` to the CLI loopback without checking session first. (origin §/auth/cli/resume algorithm) - **R11.** `SESSION_SECRET` is reused as an HKDF master; cookie and JWT keys are domain-separated subkeys. No new env vars. (origin §Settings and secrets) - **R12.** `server.auth.methods=[github]` combined with `web.enabled=false` fails at server startup with a clear config error. When web is disabled and `/auth/*` is not mounted, CLI login is unsupported and `/auth/cli/start` returns `404`. (origin §Web mode and config validation) - **R13.** IP allowlist, when enabled, covers every new endpoint (no carve-outs). (origin §IP allowlist) - **R14.** `fabro auth status` runs fully offline against local state (no server call); `--json` emits structured output; credentials and their expiry windows are shown per server. (origin §CLI UX) - **R15.** Integration tests exercise the real `web_auth.rs` OAuth glue end-to-end against a black-box `twin-github` extended with OAuth endpoints, not via `#[cfg(test)]` session injection. (origin §Prerequisite: twin-github OAuth extension) ## Scope Boundaries `fabro-server` is a single-node process. There is no multi-node deployment model to design against — the per-hash and per-code in-process mutexes in Units 9 and 10 provide the full atomicity guarantees R3 demands. No distributed-coordination design is needed. Explicit non-goals for this plan (deferred to fast-follow work): - **Windows support for `fabro auth login` writes.** `fabro auth login` returns a clear error on Windows; users work around via WSL or dev-token. Dropped to avoid DPAPI/ACL complexity that doesn't clearly improve on the 0600 file baseline for the same-user threat model. Note: `fabro auth status` and `fabro auth logout` DO work on Windows (see R14 — status must remain cross-platform for dev-token detection and logged-out-state reporting). - **Idempotency grace on refresh.** Network failures during refresh-response delivery force re-login. A future grace window (if needed) must key by stable per-install identifier, not UA/source-IP. - **`--browser-url` override for split-network topologies.** Browser always opens against the resolved HTTP(S) `--server` target in v1. - **Public `oauth_base_url` / `api_base_url` settings** for GHES / self-hosted GitHub. Test harness uses a test-only injection point; public settings land when GHES itself lands. - Device flow (`--device`). Same token model, different exchange endpoint. - OS keychain storage. Plain 0600 file is the v1 Unix storage baseline. - Rate limiting. Out of scope; operators can add a reverse-proxy-level limiter. - Per-device session UI (`fabro auth sessions`, revoke-by-device). - Emergency per-user kick independent of allowlist removal. - Configurable token TTLs via TOML. - RS256/ES256 signing. - Google Workspace or GHES IdPs (identity model supports them; plumbing is future work). - Renames of `SESSION_SECRET` → `FABRO_AUTH_SECRET` or `ServerAuthMethod::Github` → IdP-agnostic name. - Per-IdP allowlists (current `[server.auth.github].allowed_usernames` stays; note the GitHub login-reuse risk — see §Security properties). - Audit log table for logins/logouts (`tracing::info!`/`warn!` only in v1). ## Context & Research ### Relevant Code and Patterns - **`lib/crates/fabro-oauth/`** — already implements `PkceCodes`, `generate_pkce()`, `generate_state()`, `build_authorize_url()`, `start_callback_server()`, `run_browser_flow()`. CLI login reuses this rather than reimplementing PKCE/state/loopback. - **`lib/crates/fabro-server/src/web_auth.rs`** — `SessionCookie` struct at `:24`; `login_github` at `:285`; `callback_github` at `:359` (allowlist check at `:522-526` — rejection returns `Redirect::to("/login?error=unauthorized")` without minting a session); `read_private_session` at `:127`; `parse_cookie_header` at `:113`; session cookie name `__fabro_session` at `:20`; cookie key derived from `SESSION_SECRET` via `state.session_key()`. **GitHub URLs are hardcoded** at `:337` (authorize), `:428` (token exchange), `:474` (/user), `:509` (/user/emails) — no config override today; the plan adds one (see Unit 2b). - **`lib/crates/fabro-server/src/jwt_auth.rs`** — `AuthMode` at `:37`; `resolve_auth_mode_with_lookup` at `:46`; `AuthenticatedService` / `AuthenticatedSubject` extractors at `:181`/`:197`; existing session-cookie injection test pattern at `:430-460` (reusable in new tests). Misleadingly named — does no JWT work today. - **`lib/crates/fabro-server/src/server.rs`** — router construction at `build_router_with_options` (`:904`); `real_routes()` at `:1087`; `demo_routes()` at `:1007`; `AppState` at `:535`; web-auth routes nested under `/auth` at `:917`. `.nest("/api/v1", api_common)` is always mounted; the `/auth/*` prefix only mounts when `options.web_enabled`. - **`lib/crates/fabro-server/src/error.rs`** — `ApiError` with `IntoResponse` at `:121`; serializes to `{"errors":[{"status","title","detail"}]}`. Constructors: `::new`, `::bad_request`, `::unauthorized`, `::forbidden`, `::not_found`. - **`lib/crates/fabro-store/src/keys.rs`** — `SlateKey::new(ns).with(seg)` builder, null-separated; `.into_prefix()` adds trailing null for prefix scans. **`SlateKey::new` and `.with` are `pub(crate)`** — fabro-server cannot construct keys directly. Values are serialized with `serde_json::to_vec` (not bincode as the spec said; plan corrects this). - **`lib/crates/fabro-store/src/slate/mod.rs`** — `Database` wraps `slatedb::Db`; `db.scan_prefix(&prefix).await?` iterates keyed rows; new keyspaces live as sibling modules under `slate/`. **`Database` exposes only run-centric public methods** (`create_run`, `open_run`, `list_runs`, `runs()`, `get`, `find`, `list`) — no public raw `put`/`get`/`scan_prefix`. The refresh-token store lives inside `fabro-store` as a new sibling module (matching `run_store.rs` and `catalog.rs`), exposing typed methods to `fabro-server`. - **`lib/crates/fabro-config/src/resolve/server.rs`** — `resolve_auth` at `:103`; pushes `ResolveError::Invalid { path, reason }` into an `errors` vec. This is the right layer for structural validation, but the cross-field check involving the env-var-driven `web.enabled` is in `jwt_auth::resolve_auth_mode_with_lookup` at `jwt_auth.rs:46-65`. - **`lib/crates/fabro-cli/src/args.rs`** — `ProviderNamespace` namespace pattern at `:1437`: outer `Args` struct with `#[command(subcommand)]`, inner `Subcommand` enum. Top-level variant `Commands::Provider(ProviderNamespace)` at `:1025`. Dispatch in `main.rs:326`. - **`lib/crates/fabro-cli/src/commands/provider/login.rs`** — canonical login-command shape: takes clap args + `CliSettings` + `CliLayer` + `Printer`, gets `ctx.server().await?`, calls generated `fabro_api` client. - **`lib/crates/fabro-cli/src/server_client.rs`** — `ServerStoreClient` at `:33`; client construction in `connect_api_client_bundle` at `:132`; `apply_bearer_token_auth` at `:336` sets `Authorization` as a default header (one-shot at client build) — this is a real constraint: progenitor's generated `fabro_api::Client` holds the prebuilt `reqwest::Client` by value and its headers are baked in at construction. Dynamic-per-request auth means (a) a reqwest middleware reading an `Arc>`, (b) rebuilding `fabro_api::Client` on refresh (drops nothing since it's a thin wrapper), or (c) a hand-written `AuthedApiClient` that wraps each operation. Plan chooses (b) — see Unit 21. `map_api_error` at `:1022-1047` collapses `progenitor_client::Error::ErrorResponse` to `anyhow::Error` by extracting only `errors[0].detail` — this is the specific gap auto-refresh must bridge. - **`lib/crates/fabro-server/tests/it/helpers.rs`** — `test_app_with_scheduler()` (and `test_app_state_with_options`) builds in-process routers via `build_router(state, AuthMode::Disabled)`; tests drive via `tower::ServiceExt::oneshot(Request)`. No insta at this layer; CLI tests use insta (inline). - **`test/twin/github/src/`** — handler registration in `handlers/mod.rs:15`; `SharedState = Arc>` (`server.rs:11`); state seeded in `state.rs`; fixtures in `fixtures.rs`. No OAuth endpoints today. - **`lib/crates/fabro-util/src/dev_token.rs`** — file-write pattern with mode 0600 via `OpenOptionsExt` and atomic temp-then-rename. Reuse shape for `~/.fabro/auth.json`. - **`docs/api-reference/fabro-api.yaml`** — OpenAPI source of truth. Build flow per CLAUDE.md §API workflow: edit yaml → `cargo build -p fabro-api` regenerates Rust client → `cd lib/packages/fabro-api-client && bun run generate` for TS. - **`AGENTS.md`** (project root) — no glob imports; `fabro_test::test_http_client()` or `cli_http_client_builder().no_proxy()` in tests; `docs-internal/logging-strategy.md` for new `tracing` calls. ### Institutional Learnings `docs/solutions/` does not exist in this repo. No prior OAuth/JWT/refresh-token solutions to draw from. Seed `docs/solutions/` during this work if we encounter novel problems worth compounding. ### External References Design-time external work was consolidated in the spec (OAuth 2.0 RFC 6749 §4.1.2.1 for error redirects; OAuth 2.0 Security BCP for rotation-with-reuse-detection; comparisons to `gcloud`, `wrangler`, `gh auth login`, `aws sso login`). No new external research required at planning time. ## Key Technical Decisions - **Reuse `fabro-oauth` crate for CLI-side PKCE + loopback, extending its callback server.** The existing `start_callback_server` swallows `error` params and hangs on state mismatch. The new `start_callback_server_with_errors` function fires the oneshot only when the state matches — with either `code` (success) or `error` (server-authored failure) payload. State-mismatch requests still return HTTP 400 without firing the oneshot (preserves the property that any local process can't abort an in-progress login by probing the callback port). The CLI relies on `--timeout` (default 5 min) for the state-mismatch-no-legitimate-callback case. - **Piggyback on the existing web GitHub OAuth flow; do not fork it.** Add `return_to` query param to `/auth/login/github` (strict whitelist: `^/auth/cli/(start|resume)$`). `/auth/cli/resume` is the continuation point after web OAuth completes. `/auth/callback/github` preserves `return_to` on all error paths, not just success. Avoids duplicating ~200 lines of token exchange + user fetch + allowlist logic. - **Reuse `SESSION_SECRET` as HKDF master, full Extract-and-Expand.** Two domain-separated subkeys derived via HKDF-SHA256 (`Extract` then `Expand`) with context labels `b"fabro-cookie-v1"` and `b"fabro-jwt-hs256-v1"`. HKDF-Extract applied first means operator-supplied `SESSION_SECRET` doesn't need to be uniformly random (it never is). Startup validation also requires `SESSION_SECRET` to be at least 32 bytes (64 hex chars) when `github` is in `auth.methods` — documented as "this secret now signs JWTs; a weak value allows offline forgery." - **`SlateAuthTokenStore` is a concrete struct (no trait), held as `Arc` in `AuthServices`.** Single instance per server; `Clone` yields another Arc handle to the same DashMap, not a duplicate. `DashMap::entry(...).or_insert_with(|| Arc::new(Mutex::new(())))` acquires the per-hash mutex atomically (not get-then-insert). Mutex-entry cleanup only occurs while holding the mutex and only when no other Arc strong references exist. - **No idempotency grace in v1.** `consume_and_rotate` burns the old token immediately on success; any second presentation is treated as theft. Network failures during `/auth/cli/refresh` response delivery (server rotated, client disconnected) force the user to re-login on their next command — surfaced as "session ended during refresh. Run `fabro auth login`." Simpler semantics, smaller attack surface than the spoofable fingerprint-based grace that was previously proposed. If production telemetry shows frequent network-blip-triggered logouts, reintroduce a grace window keyed by a stable per-install identifier (stored in `auth.json`), NOT by UA/source-IP which are attacker-controlled. - **Identity uses a type-enforced enum with serde validation.** `SessionCookie` carries `identity: Option` where `IdpIdentity { issuer: String, subject: String }` validates non-empty fields via `#[serde(try_from = "IdpIdentityWire")]` — closes the serde-bypass gap where `#[derive(Deserialize)]` would otherwise reconstitute the type without invariant checks. The same `IdpIdentity` type is used by `AuthCode` and `RefreshToken` so the invariant flows through the entire auth lifecycle, not just at the cookie boundary. `None` = dev-token session; `Some(_)` = IdP-authenticated. Eligibility gate is a single `session.identity.as_ref()` pattern match. Cookie v1 → v2 bump; v1 cookies force re-login (acceptable given 30 d TTL). - **Unix-only CLI login in v1.** Windows support deferred. `fabro auth login` on Windows exits with a clear message: "CLI OAuth login is not supported on Windows in this release. Use WSL, or use a dev-token server." Drops the DPAPI / NTFS-ACL machinery, the `windows` crate dependency, and Windows-specific tests. Unix continues to use mode 0600 atomic writes via `fs2` advisory locking. - **Error shape sanitation.** Server emits only fixed `error_description` strings chosen from a closed enumeration per `error` code; inbound `error_description` is NEVER forwarded verbatim (would be a terminal-injection / log-injection vector). HTML error pages render only static server-authored text; query values are never echoed; `Content-Type: text/html; charset=utf-8`, `X-Content-Type-Options: nosniff`. - **Cookie attributes explicit.** `fabro_cli_flow`: `HttpOnly; Secure` (auto-relaxed for `http://127.0.0.1` dev); `SameSite=Lax`; `Path=/auth`; 10 min max_age. `fabro_oauth_state`: existing attributes; max_age raised from 10 min to 30 min to cover slow GitHub authorize (MFA, org approvals). - **Endpoint-appropriate error envelopes.** `/auth/cli/token`, `/refresh`, `/logout` return flat RFC 6749 `{error, error_description}`. `/auth/cli/start`, `/resume` return either a 302-redirect-with-error-param (when `redirect_uri`+`state` validated) or a plain HTML error page (otherwise). Protected endpoints continue to use the existing `ApiError` envelope, extended with an optional `code: Option` field (backwards compatible via `skip_serializing_if = "Option::is_none"`). OpenAPI spec version bumped (minor) to document the new field. - **SlateDB value encoding matches repo convention: `serde_json::to_vec`.** Fields are small fixed-size, the encoding cost is negligible, and JSON is easier to inspect in logs. - **Refresh-token storage lives in `fabro-store` as a typed module** (`lib/crates/fabro-store/src/slate/auth_tokens.rs`), parallel to `run_store.rs` and `catalog.rs`. Exposes a typed `SlateAuthTokenStore` with domain methods (`insert_refresh_token`, `find_refresh_token`, `delete_chain`, `gc_expired`). Keeps `SlateKey` encapsulation intact; fabro-server consumes the typed API. - **Authorization codes live in SlateDB**, keyspace `auth/code/` (opaque 32-byte random code, base64url-encoded). 60 s TTL enforced at read time; reaper task every 30 s purges expired entries. `consume` is single-use via a per-code in-process mutex (fabro-server is single-node, so an in-process mutex is the correct primitive). PKCE provides a secondary defense at `/auth/cli/token` (Unit 15 step 5) as belt-and-suspenders against stolen codes without verifiers. - **Reactive-only auto-refresh (no pre-flight).** CLI refreshes on 401 with `code == "access_token_expired"` and retries once. No clock-based pre-flight check. Avoids concurrent-refresh storms across parallel CLI invocations, removes clock-sync assumptions, and shrinks the refresh code path by half. Latency cost is at most one extra round-trip per ~10 min window of activity. - **HTTPS-or-loopback-or-unix-socket-only for refresh traffic, checked by URL parsing, not string matching.** CLI parses the server target as `url::Url`. Accepts: `scheme == "https"`; OR `scheme == "http"` with `url.host()` parsing to an `IpAddr` that `is_loopback()` returns true (covers `127.0.0.0/8`, `::1`, and `::ffff:127.0.0.1`); OR the Unix-socket target type. Rejects literal `localhost` (DNS-overridable), any host containing dots after a loopback prefix (e.g., `127.0.0.1.evil.com`), any encoded form (decimal `2130706433`, hex `0x7f000001`, octal). Surfaces an actionable error pointing at the scheme or host. Prevents silent refresh-token leaks over plaintext. - **`map_api_error_structured` wraps `progenitor_client::Error` into `ApiFailure { status, code, detail }`.** Existing `map_api_error` is refactored to call `map_api_error_structured` and discard `code`, reducing drift risk. Auto-refresh wrapper uses the structured helper. - **No OpenAPI preflight surface.** The CLI constructs browser URLs from its resolved HTTP(S) target directly. The OAuth-style `/auth/cli/{token,refresh,logout}` endpoints remain outside the canonical API surface and are hand-wired. - **Browser URL always equals `config.web_url` in v1.** Split-network topologies (dockerized dev, VPN mismatches, SSH-plus-browser-on-different-hosts) are deferred to a fast-follow `--browser-url` flag. For v1, users whose `server.web.url` is not reachable from their browser cannot complete login; the preflight handler returns the exact URL the CLI will open so the failure mode is at least visible. ## Open Questions ### Resolved During Planning - **SlateDB value encoding.** Resolved: `serde_json::to_vec` per existing repo convention. - **Where startup validation lives.** Resolved: `jwt_auth::resolve_auth_mode_with_lookup` at `jwt_auth.rs:46-65`, extended with the `github + !web.enabled` check. Matches the existing `SESSION_SECRET` and `GITHUB_APP_CLIENT_SECRET` checks at the same boundary. - **Whether to extend `fabro-api.yaml` for all six new endpoints.** Resolved: only `GET /api/v1/auth/cli/config` goes in the OpenAPI spec. The five `/auth/cli/*` OAuth endpoints use a flat OAuth envelope (incompatible with `ApiError`) and live outside the progenitor-generated client — CLI calls them via a dedicated `fabro_http` helper. - **How much to extend twin-github.** Resolved: happy path + explicit denial + wrong-client-secret. Not a fidelity clone. ### Deferred to Implementation - **Exact `error_description` strings.** User-visible copy decided at implementation time per endpoint; not architecturally load-bearing. - **GC reaper cadence tuning.** Initial values: AuthCode reaper every 30 s, refresh-token GC every 6 h. Tune if logs show pressure. - **Whether `fabro auth login` reuses a single `AuthStore` file lock across preflight + token POST + write.** Implementation detail; spec requires lock around read-modify-write, not across network I/O. - **Whether `VerifiedAuth` adds `idp_issuer`/`idp_subject` fields or exposes them via a separate method.** Shape decision during Unit 11; handlers largely treat `AuthenticatedService` as a unit marker. ## 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.* ### Flow shape ```mermaid sequenceDiagram autonumber participant CLI participant Browser participant API as API router
(--server target) participant Web as Web router
(server.web.url) participant GH as GitHub
(or twin-github in tests) CLI->>API: GET /api/v1/auth/cli/config API-->>CLI: {enabled, web_url, methods} Note over CLI: Abort if enabled=false CLI->>CLI: generate PKCE + state
bind loopback :PORT CLI->>Browser: open web_url/auth/cli/start?... Browser->>Web: GET /auth/cli/start alt eligible GitHub session exists Web-->>Browser: 302 redirect_uri?code=&state= else no eligible session Web-->>Browser: 302 /auth/login/github?return_to=/auth/cli/resume Browser->>Web: GET /auth/login/github Web->>GH: redirect to authorize GH-->>Browser: redirect to /auth/callback/github?code=&state= Browser->>Web: GET /auth/callback/github alt allowlist ok Web-->>Browser: set __fabro_session; 302 /auth/cli/resume Browser->>Web: GET /auth/cli/resume Web-->>Browser: 302 redirect_uri?code=&state= else allowlist rejects / GitHub error Web-->>Browser: 302 /auth/cli/resume?error= Browser->>Web: GET /auth/cli/resume?error= Web-->>Browser: 302 redirect_uri?error=&state= end end Browser->>CLI: GET loopback/callback?code=&state= | ?error=&state= CLI->>API: POST /auth/cli/token {code, verifier} API-->>CLI: {access_token, refresh_token, ...} CLI->>CLI: persist ~/.fabro/auth.json (0600, fs2-locked) ``` ### Module tree ``` lib/crates/fabro-server/src/ ├── auth/ │ ├── mod.rs AuthServices bundle + re-exports │ ├── cli_flow.rs /auth/cli/* handlers (config, start, resume, token, refresh, logout) │ ├── jwt.rs HS256 encode/verify, claims, HKDF subkey derivation │ ├── (refresh tokens live in fabro-store; fabro-server re-exports via auth/mod.rs) │ └── (auth codes live in fabro-store; fabro-server re-exports via auth/mod.rs) ├── web_auth.rs SessionCookie migration; return_to plumbing; error-path preservation ├── jwt_auth.rs extractor extended; resolve_auth_mode gets github+!web.enabled check └── server.rs route registration + AuthServices wiring lib/crates/fabro-cli/src/ ├── commands/auth/ │ ├── mod.rs AuthNamespace / AuthCommand dispatch │ ├── login.rs preflight + fabro-oauth browser flow + POST /token + persist │ ├── logout.rs POST /logout + AuthStore remove │ └── status.rs local-only status formatter (text + JSON) ├── auth_store.rs AuthStore + AuthEntry + ServerTargetKey normalization + fs2 lock └── server_client.rs bearer priority + auto-refresh wrapper; map_api_error_structured ``` ### `AuthMode` extension shape Adds `jwt_key: Option>` alongside existing `dev_token` and `session_methods`. Construction goes through `resolve_auth_mode_with_lookup` (HKDF subkey derivation from `SESSION_SECRET` happens there). ## Implementation Units Organized into five phases. Early phases stand alone (no behavior changes). Later phases deliver observable features. ### Phase 1 — Foundations (no user-visible behavior) - [ ] **Unit 1: OpenAPI spec — preflight config endpoint** **Goal:** Add `GET /api/v1/auth/cli/config` to the OpenAPI spec and regenerate Rust + TypeScript clients. **Requirements:** R5, R6, R12 **Dependencies:** none **Files:** - Modify: `docs/api-reference/fabro-api.yaml` — add operation under `/auth/cli/config`; add `CliAuthConfig` schema with `enabled: bool`, `web_url: nullable string`, `methods: [string]`, `reason: nullable string`. **`reason` is an open string (NOT an OpenAPI `enum`)** so future server versions can add new values without breaking older CLI clients. The schema description enumerates the currently known values (`"github_not_enabled"`, `"web_not_enabled"`) for documentation, but the type is a plain string. This ensures the progenitor-generated client accepts any string value; forward-compat handling lives in the CLI (Unit 18), which pattern-matches known values and falls back to a generic "CLI login is not available on this server" message for anything unrecognized. No `reason_description` (CLI renders text for known values locally). - Run: `cargo build -p fabro-api` (build.rs regenerates Rust types + client). - Run: `cd lib/packages/fabro-api-client && bun run generate` (regenerates TS client). - Test: rely on existing `tests/it/api/openapi_conformance.rs` once Unit 8 (the handler) lands. **Approach:** - Endpoint is `GET`, unauthenticated, no request body. - `operationId: getCliAuthConfig` (camelCase to match existing ops). - Response 200 schema carries the five fields above. - `reason` is an open string (not a closed OpenAPI `enum`) — the schema description lists the currently-known values (`github_not_enabled`, `web_not_enabled`) but the generated Rust type is `Option`, so an older CLI deserializing a future server's new `reason` value succeeds and falls through to Unit 18's generic-unknown-value branch. **Patterns to follow:** - Mirror existing unauthenticated ops in `fabro-api.yaml` (e.g., `/health` if present, or the anonymous route pattern). **Test scenarios:** - Test expectation: none — pure schema change; conformance is asserted transitively when Unit 8 registers the route. **Verification:** - Rust client exposes a `get_cli_auth_config()` builder method. - TS client gets a matching method. - `cargo build --workspace` succeeds. --- - [ ] **Unit 2: Workspace dependency additions** **Goal:** Add `hkdf` and `fs2` as workspace dependencies. **Requirements:** R11, R14 **Dependencies:** none **Files:** - Modify: `Cargo.toml` (workspace root) — add `hkdf = "0.12"` and `fs2 = "0.4"` under `[workspace.dependencies]`. - Modify: `lib/crates/fabro-server/Cargo.toml` — declare `hkdf = { workspace = true }`. - Modify: `lib/crates/fabro-cli/Cargo.toml` — declare `fs2 = { workspace = true }`. **Approach:** - Pin to current minor versions. - No crate-level code lands here — just Cargo files. **Patterns to follow:** - Existing workspace deps in `Cargo.toml` use `{ workspace = true }` at crate level. **Test scenarios:** - Test expectation: none — dependency addition only. **Verification:** - `cargo build --workspace` succeeds. --- - [ ] **Unit 3: `ApiError` extended with optional `code` field** **Goal:** Extend the `ApiError` envelope with an optional machine-readable `code` while preserving backwards compatibility. **Requirements:** R8 **Dependencies:** none **Files:** - Modify: `lib/crates/fabro-server/src/error.rs` — add `code: Option` to `ErrorEntry` with `#[serde(skip_serializing_if = "Option::is_none")]`; add `ApiError::with_code(status, detail, code)` constructor; add `unauthorized_with_code` and `forbidden_with_code` convenience constructors used by the JWT extractor. - Modify: `docs/api-reference/fabro-api.yaml` — update the error-response schema component to document `code` as an optional string; document the values `access_token_expired`, `access_token_invalid`, `unauthorized` as the initial set. - Test: `lib/crates/fabro-server/src/error.rs` (inline `#[cfg(test)] mod tests`). **Approach:** - `code` is purely additive: when `None`, the serialized JSON is identical to today's shape (verified by test). - Regenerate clients after the yaml update so `fabro-api`'s generated `Error` type includes the field. **Patterns to follow:** - `#[serde(skip_serializing_if = "Option::is_none")]` on optional response fields is common elsewhere in `server.rs` DTOs. **Test scenarios:** - Happy path: `ApiError::with_code(StatusCode::UNAUTHORIZED, "token expired", "access_token_expired")` serializes to `{"errors":[{"status":"401","title":"Unauthorized","detail":"token expired","code":"access_token_expired"}]}`. - Edge case: `ApiError::unauthorized()` (no code) serializes without a `code` key — byte-exact match with today's output (guard against accidental envelope breakage). **Verification:** - Round-trip test confirms `code` absent when `None`. - `cargo build --workspace` succeeds with regenerated client. --- - [ ] **Unit 4: HKDF subkey derivation helper (Extract-and-Expand, entropy-validated)** **Goal:** Derive two domain-separated symmetric subkeys from `SESSION_SECRET` — one for the existing `cookie` crate, one for HS256 JWT signing — with full HKDF-Extract-and-Expand so operator-supplied secrets don't need to be uniformly random. **Requirements:** R11 **Dependencies:** Unit 2 **Files:** - Create: `lib/crates/fabro-server/src/auth/mod.rs` — module root (stub for now; later units add re-exports). - Create: `lib/crates/fabro-server/src/auth/keys.rs` — `derive_cookie_key(master: &[u8]) -> Result` and `derive_jwt_key(master: &[u8]) -> Result`; HKDF context labels `b"fabro-cookie-v1"` and `b"fabro-jwt-hs256-v1"`. `KeyDeriveError` variants: `TooShort { got_bytes, min_bytes }`, `Empty`. - Modify: `lib/crates/fabro-server/src/lib.rs` — declare `pub mod auth;`. - Modify: `lib/crates/fabro-server/src/web_auth.rs` — existing `state.session_key()` callers now receive the derived subkey (internally — call site untouched; the derivation happens inside `state.session_key()` wiring or at `AuthMode` construction, matching Unit 5). - Test: `lib/crates/fabro-server/src/auth/keys.rs` (inline `#[cfg(test)] mod tests`). **Approach:** - HKDF-Extract-and-Expand (two steps) with SHA-256: Extract with a zero-byte salt and the raw `SESSION_SECRET` as IKM to produce a 32-byte PRK; then Expand the PRK with `info = context label` to 64 bytes for cookie or 32 bytes for JWT. This is the correct primitive for operator-supplied master secrets; HKDF-Expand-only (what the spec originally said) requires the input to already be uniformly random, which `SESSION_SECRET` may not be. - Minimum-entropy check: reject a `SESSION_SECRET` shorter than 32 bytes with `KeyDeriveError::TooShort`. Documented as: "`SESSION_SECRET` must be at least 32 bytes (64 hex characters). This secret now signs JWTs as well as session cookies — a weak value allows offline JWT forgery if any JWT is captured." - `JwtSigningKey` is a thin newtype wrapping the raw 32 bytes + helper `encoding_key() / decoding_key()` methods that bridge to `jsonwebtoken::EncodingKey::from_secret`. - Keep the helper private to `fabro-server` (no public re-export). **Patterns to follow:** - `cookie::Key::from(&[u8])` accepts a 64-byte derived key (the `cookie` crate itself derives internal sub-keys for encryption vs MAC from those 64 bytes). - The `hkdf` crate exposes `Hkdf::::new(salt, ikm)` for Extract and `.expand(info, &mut okm)` for Expand. **Test scenarios:** - Happy path: same master + same label yields identical subkey bytes across two calls (determinism). - Happy path: different labels yield different subkeys for the same master (domain separation). - Happy path: 32-byte master + cookie label produces 64 bytes; + JWT label produces 32 bytes (shape). - Error path: empty master → `Empty`. - Error path: 31-byte master → `TooShort { got_bytes: 31, min_bytes: 32 }`. - Integration (with the `cookie` crate): a key derived via Extract-and-Expand round-trips a private cookie (encrypt → decrypt) successfully. - Edge case: a purposefully low-entropy 32-byte master (e.g., `[0x61; 32]`) derives distinct JWT and cookie subkeys — the test just confirms domain separation still holds; we do NOT attempt to detect weak secrets beyond the length check. **Verification:** - Tests pass in isolation. --- - [ ] **Unit 5: Startup validation — `github + !web.enabled` fails at boot** **Goal:** Reject the incoherent config combination `server.auth.methods=[..., github, ...]` with `server.web.enabled=false` at startup. Also enforce the `SESSION_SECRET` ≥32-byte floor required for HKDF key derivation when github auth is enabled. **Requirements:** R12 **Dependencies:** Unit 4 **Files:** - Modify: `lib/crates/fabro-server/src/jwt_auth.rs` — extend `resolve_auth_mode_with_lookup` (`:46`) with the cross-field check; produce a `ResolveError::Invalid { path: "server.auth.methods", reason: "GitHub auth is enabled but server.web.enabled is false" }`. - Modify: `lib/crates/fabro-server/src/jwt_auth.rs` — store the HKDF-derived JWT key on `AuthMode::Enabled` (field `jwt_key: Option`); derive it in the same resolver using Unit 4's helper when `SESSION_SECRET` is present and github is enabled. - Test: `lib/crates/fabro-server/src/jwt_auth.rs` (existing `#[cfg(test)] mod tests`). **Approach:** - Add three checks alongside the existing `SESSION_SECRET` and `GITHUB_APP_CLIENT_SECRET` validations at `:60-65`: 1. `github in methods` with `web.enabled=false` → `ResolveError::Invalid { path: "server.auth.methods", reason: "GitHub auth requires server.web.enabled = true" }`. 2. `github in methods` but `SESSION_SECRET` missing → (existing behavior, unchanged). 3. `github in methods` with `SESSION_SECRET` present but shorter than 32 bytes → `ResolveError::Invalid { path: "SESSION_SECRET", reason: "SESSION_SECRET must be at least 32 bytes (64 hex characters) when github auth is enabled — it now signs JWTs as well as session cookies. Current length: {got} bytes." }`. Uses the `KeyDeriveError::TooShort` from Unit 4's helper. - `jwt_key` is `None` when github is not enabled — keeps non-github deployments clean. **Patterns to follow:** - Existing `ResolveError::Invalid` pushes in `fabro-config/src/resolve/server.rs:103`. **Test scenarios:** - Happy path: `methods=[dev-token, github]` + `web.enabled=true` + 32-byte `SESSION_SECRET` → resolves successfully with `jwt_key = Some(_)`. - Error path: `methods=[github]` + `web.enabled=false` → returns `ResolveError::Invalid` with the web-enabled reason text. - Error path: `methods=[github]` + `web.enabled=true` + 31-byte `SESSION_SECRET` → returns `ResolveError::Invalid` with a path-of-`SESSION_SECRET` and a reason mentioning minimum length. - Edge case: `methods=[dev-token]` + `web.enabled=false` + weak `SESSION_SECRET` → resolves successfully (no github, no JWT key, entropy rule doesn't apply) with `jwt_key = None`. - Edge case: `methods=[github]` + `web.enabled=true` but no `SESSION_SECRET` → existing error wins (no regression). **Verification:** - New tests pass; existing `jwt_auth` tests still pass. --- - [ ] **Unit 6: `SessionCookie` migration — type-safe `Option` representation** **Goal:** Migrate `SessionCookie` from `provider_id: Option` to a type-enforced IdP identity representation. Eligibility decisions become exhaustive pattern matches instead of empty-string checks, making it impossible to accidentally bootstrap a non-IdP session as CLI-eligible. **Requirements:** R4 **Dependencies:** none **Files:** - Create: `lib/crates/fabro-types/src/auth.rs` — `IdpIdentity` type; `IdpIdentityWire` DTO for serde; `IdpIdentityError` enum. **Placed in `fabro-types` (not `fabro-server`) because `fabro-store` also needs to use the type for `RefreshToken` (Unit 10), and `fabro-store` already depends on `fabro-types` but not on `fabro-server` — the reverse direction would create a dependency cycle.** Both `fabro-server` and `fabro-store` import from `fabro-types`. - Modify: `lib/crates/fabro-types/src/lib.rs` — declare `pub mod auth;` and re-export `IdpIdentity` at the crate root. - Modify: `lib/crates/fabro-server/src/web_auth.rs`: - `SessionCookie` struct (`:24`): bump `v: u8` sentinel from 1 → 2; drop `provider_id: Option`; add `identity: Option`. - Callback GitHub-login minting at `:533-545`: `identity = Some(IdpIdentity::new("https://github.com", profile.id.to_string())?)`. - Dev-token minting at `:246-257`: `identity = None` (dev sessions are non-IdP — semantic rather than stringly). - Modify: `lib/crates/fabro-server/src/jwt_auth.rs` — `read_private_session` and `AuthenticatedSubject` extraction: decoded cookies with `v != 2` are treated as absent (force re-login). - Modify: `lib/crates/fabro-server/src/web_auth.rs` — `SessionUser` response DTO (`:67`) and `/api/v1/auth/me`: flatten `identity` into `idp_issuer: Option` and `idp_subject: Option` in the serialized shape (keeping wire compatibility with anyone consuming the API). `login` remains the display field. - Test: `lib/crates/fabro-types/src/auth.rs` (inline `#[cfg(test)] mod tests`) — invariant + serde-bypass tests live next to the type. - Test: `lib/crates/fabro-server/src/web_auth.rs` (inline `#[cfg(test)] mod tests`) — cookie round-trip with the new `identity` field. **Approach:** - `IdpIdentity` shape (declared in `fabro-types/src/auth.rs`): ``` #[derive(Debug, Clone, PartialEq, Eq, Serialize)] #[serde(try_from = "IdpIdentityWire", into = "IdpIdentityWire")] pub struct IdpIdentity { issuer: String, // private field — inaccessible outside this module subject: String, // private field } impl IdpIdentity { pub fn new(issuer: impl Into, subject: impl Into) -> Result; pub fn issuer(&self) -> &str; pub fn subject(&self) -> &str; } #[derive(Deserialize, Serialize)] struct IdpIdentityWire { issuer: String, subject: String } // public fields, no invariants ``` - The `#[serde(try_from = "IdpIdentityWire")]` attribute forces deserialization to go through `IdpIdentity::try_from(wire)` which validates non-empty. This closes the serde-bypass gap: direct `Deserialize` would otherwise set the private fields blindly; `try_from` applies the constructor. `into = "IdpIdentityWire"` keeps the serialized form identical to today (flat `{issuer, subject}`). - One-way migration: v1 cookies are dropped, users re-login. Acceptable given 30 d TTL and this lands before CLI login GA. - The eligibility gate used in Units 14 and 15 becomes `matches!(session.identity, Some(_))` — compiler-enforced exhaustiveness. The **runtime 403-on-empty-strings check disappears** (the type makes it impossible). The **unreachability assertion** (returning 500 if somehow an invalid shape is reached — Unit 15 step 7) stays as a defensive guard against future bugs; it should be `debug_assert!(false, "unreachable: IdpIdentity invariant violated")` in debug builds and a 500 in release. - Dev-token sessions: `identity = None` means "no IdP authentication." `/auth/cli/*` eligibility gate rejects these by pattern-match. - Type invariant: `IdpIdentity::new` and `IdpIdentity::try_from(wire)` both reject empty `issuer` or `subject`; no public API can construct an invalid instance. The fields are `String` (not `pub`), the struct body has no public fields, so external crates can only construct via `new` or deserialize via `try_from`. - **Visibility:** `pub struct IdpIdentity`, `pub fn new`, `pub fn issuer`, `pub fn subject`, `pub struct IdpIdentityError` — required for `fabro-server` and `fabro-store` to both use the type from `fabro-types`. Invariants are enforced by private fields + validating constructor/`try_from`, NOT by `pub(crate)` visibility. **Patterns to follow:** - Existing `CookieJar::private_mut(&key).add(...)` cookie minting shape in callback at `web_auth.rs:549`. - Rust newtype-with-validation pattern paired with `#[serde(try_from = "...")]` is the standard way to enforce invariants across serde boundaries. **Test scenarios:** - Happy path: v2 GitHub cookie encodes with `identity: Some(IdpIdentity { issuer: "https://github.com", subject: "12345" })`, decodes round-trip. - Happy path: v2 dev-token cookie encodes with `identity: None`, decodes cleanly. - Error path: `IdpIdentity::new("", "12345")` → error. - Error path: `IdpIdentity::new("https://github.com", "")` → error. - **Error path — serde bypass guard:** deserialize a crafted JSON `{"issuer": "", "subject": "12345"}` directly into `IdpIdentity` → returns a serde error (NOT a successfully-constructed empty-issuer instance). This is the critical regression test — it verifies `#[serde(try_from)]` is wired correctly. - **Error path — serde bypass via SessionCookie:** mint a v2 session cookie with a tampered JSON payload carrying an empty `issuer` field directly (bypassing `IdpIdentity::new`) → `read_private_session` returns `None` or a deserialization error, NOT a successfully-decoded cookie with an invalid `IdpIdentity`. - Edge case: v1 cookie presented → `read_private_session` returns `None` (treated as absent). - Integration: session-cookie-injection test (existing pattern at `jwt_auth.rs:430-460`) updated to mint v2 cookies — no regressions in existing protected-endpoint tests. **Verification:** - Full `cargo nextest run -p fabro-server` green; the invariant test explicitly rejects empty strings. --- - [ ] **Unit 6b: GitHub base-URL override via test-only Axum extension (prerequisite for twin-github integration)** **Goal:** Make the four hardcoded `github.com` / `api.github.com` URLs in `web_auth.rs` redirectable to `twin-github` in integration tests, **without** introducing a public settings surface. Closes the test-integration gap without adding a production attack vector (a misconfigured public `oauth_base_url` would leak client-secret to an attacker-controlled host). **Requirements:** R15 **Dependencies:** none **Files:** - Create: `lib/crates/fabro-server/src/auth/github_endpoints.rs` — `GithubEndpoints { oauth_base: Url, api_base: Url }` with a `production_defaults()` constructor and a `with_bases(oauth: Url, api: Url)` test-only constructor (the test-only one is exposed via `pub(crate)` and only called from `#[cfg(test)]` code or the dedicated test harness in `fabro-server/tests/it/helpers.rs`). - Modify: `lib/crates/fabro-server/src/server.rs` — inject `Arc` as an Axum `Extension` layer at router build time. Production router uses `GithubEndpoints::production_defaults()`; test harness replaces it. - Modify: `lib/crates/fabro-server/src/web_auth.rs`: - `:337` authorize URL: read from `Extension>`, use `endpoints.oauth_base.join("login/oauth/authorize")`. - `:428` token exchange: `endpoints.oauth_base.join("login/oauth/access_token")`. - `:474` user fetch: `endpoints.api_base.join("user")`. - `:509` emails fetch: `endpoints.api_base.join("user/emails")`. - Modify: `lib/crates/fabro-server/tests/it/helpers.rs` — test-app builders accept an optional `GithubEndpoints` override; default (when not overridden) is production. - Test: `lib/crates/fabro-server/src/auth/github_endpoints.rs` (inline tests). **Approach:** - **No public config field.** The settings schema is unchanged. Operators cannot override these URLs from `settings.toml` or env. The only way to override is via the test-only constructor in test code. - `GithubEndpoints` is an Axum Extension, which means handlers pull it from the request extensions rather than from settings. Clean separation of concerns: production code path is unconditional; tests configure the stack at the router-build boundary. - When GHES / Google IdPs land (deferred scope), a proper settings surface for this override can be added with validation (HTTPS-only, allowlist, etc). For now, production has exactly one set of GitHub URLs — as hardcoded today — with no surface for an attacker to misdirect. - Use `Url::join()` not string concatenation — avoids trailing-slash footguns and validates shape. **Patterns to follow:** - Existing Axum Extension injection patterns in `build_router_with_options` at `server.rs:904`. - Existing `helpers.rs::test_app_state_with_options` for accepting per-test config overrides. **Test scenarios:** - Happy path: `GithubEndpoints::production_defaults()` yields the two URLs equal to what's currently hardcoded. - Happy path: `GithubEndpoints::with_bases("http://127.0.0.1:12345".parse()?, "http://127.0.0.1:12345/api".parse()?)` yields those URLs. - Edge case: `Url::join` with trailing-slash base produces a correctly single-slashed URL (regression guard for concatenation bugs). - Integration: `web_auth::login_github` in test mode with an injected `GithubEndpoints` redirects to the injected oauth base, not to github.com. Confirm by inspecting the 302 `Location` header. **Verification:** - All existing `web_auth.rs` tests pass (default behavior unchanged). - Integration tests in Phase 5 successfully redirect Fabro's OAuth calls to twin-github. --- - [ ] **Unit 7: `twin-github` OAuth + user endpoints (prerequisite for integration tests)** **Goal:** Extend `test/twin/github` with the four endpoints Fabro's `web_auth.rs` calls during the OAuth flow, so integration tests exercise the real glue end-to-end instead of stubbing session cookies. **Requirements:** R15 **Dependencies:** none **Files:** - Create: `test/twin/github/src/handlers/oauth.rs` — `GET /login/oauth/authorize` (auto-approve or configurable denial → redirect to `redirect_uri?code=&state=`), `POST /login/oauth/access_token` (validate `client_id`/`client_secret`/`code`, return `{access_token, token_type, scope}`). - Create: `test/twin/github/src/handlers/users.rs` — `GET /user` and `GET /user/emails`, returning fixture user/emails from state. - Modify: `test/twin/github/src/handlers/mod.rs` — register the four new routes. - Modify: `test/twin/github/src/state.rs` — extend `AppState` with `oauth_codes: HashMap`, `oauth_tokens: HashMap`, `seeded_user: GithubUser`, `seeded_emails: Vec`, `allow_authorize: bool` (for denial simulation). - Modify: `test/twin/github/src/fixtures.rs` — default seeded user + emails. - Modify: `test/twin/github/src/test_support.rs` — builder method `.with_oauth_user(user)` to seed before the test runs. - Test: `test/twin/github/src/handlers/oauth.rs` (inline `#[cfg(test)] mod tests`); cross-crate integration tests land in Phase 5. **Approach:** - Codes are single-use; exchanging invalidates the entry. - No token refresh, no scope validation. GitHub's OAuth doesn't rotate by default anyway. - Denial path: when `allow_authorize=false`, redirect to `redirect_uri?error=access_denied&state=`. **Patterns to follow:** - Existing handler shape in `test/twin/github/src/handlers/*.rs` — `State(state): State`, `state.read().await` / `state.write().await`. **Test scenarios:** - Happy path: authorize returns a code; exchanging the code with the correct client_secret returns an access token; `/user` with that token returns the seeded fixture. - Error path: wrong client_secret on token exchange → 400 `invalid_client`. - Edge case: replay an already-exchanged code → 400 `invalid_grant`. - Error path: `allow_authorize=false` → authorize redirects with `error=access_denied`. - Integration (within twin): full round-trip `authorize → exchange → /user → /user/emails` using a single seeded subject. **Verification:** - Twin-github's own test suite passes; twin is ready to be consumed by Fabro integration tests in Phase 5. ### Phase 2 — Auth internals (store + JWT) - [ ] **Unit 8: JWT issue + verify (`auth/jwt.rs`)** **Goal:** HS256 JWT encode and verify with strict algorithm pinning, `iss`/`aud`/`exp`/`iat` validation, and 5 s clock-skew tolerance. **Requirements:** R2, R4, R11 **Dependencies:** Unit 4, Unit 5 **Files:** - Create: `lib/crates/fabro-server/src/auth/jwt.rs` — `Claims` struct (all fields in origin §Token model — `iss`, `aud`, `sub`, `exp`, `iat`, `jti`, `idp_issuer`, `idp_subject`, `login`, `name`, `email`, `auth_method`); `issue(key, subject_snapshot, ttl) -> String`; `verify(key, expected_iss, token) -> Result`; `JwtError` enum with variants `AccessTokenExpired`, `AccessTokenInvalid`. - Modify: `lib/crates/fabro-server/src/auth/mod.rs` — re-export `Claims`, `issue`, `verify`, `JwtError`. - Test: `lib/crates/fabro-server/src/auth/jwt.rs` (inline `#[cfg(test)] mod tests`). **Approach:** - `Header` explicitly pinned to `HS256`; `Validation` constructed with `algorithms = vec![HS256]`. - Parse header before signature verify; reject any other `alg` (including `alg: none`). - **Clock-skew tolerance window is 5 seconds** (`Validation::leeway = 5`). Revised down from 30 seconds — fabro-server is single-node with NTP, not federated multi-region. A 5-second window accommodates normal clock drift without extending stolen-token replay windows unnecessarily. - `aud = "fabro-cli"` hardcoded constant. `iss` passed in by caller (the server's public URL). - **Reject any token with a `kid` header field** for now (no multi-key support in v1); forward-closes algorithm-confusion and key-selection abuse until we explicitly design multi-key rotation. **Patterns to follow:** - `jsonwebtoken` crate's `encode()` + `decode::()` shape. Existing usage in `fabro-github` shows the crate's ergonomics for this repo. **Test scenarios:** - Happy path: round-trip encode → verify yields the original claims. - Error path: `alg: none` header → `verify` returns `AccessTokenInvalid` before any signature comparison (guard against algorithm-confusion attack). - Error path: `alg: RS256` → rejected. - Error path: `exp` 10 s past now → `AccessTokenExpired`. - Happy path: `iat` 3 s in the future → accepted (within 5 s skew). - Error path: `iat` 10 s in the future → `AccessTokenInvalid`. - Error path: header contains `kid` → `AccessTokenInvalid`. - Error path: `iss` mismatch → `AccessTokenInvalid`. - Error path: `aud` mismatch → `AccessTokenInvalid`. - Edge case: signature tampered (valid header, valid claims, bad MAC) → `AccessTokenInvalid`. - Integration (with Unit 4): key derived from a given master produces verifiable tokens; changing the HKDF context label invalidates all tokens. **Verification:** - All tests pass. `jti` is a valid UUIDv4 in emitted tokens. --- - [ ] **Unit 9: `AuthCode` SlateDB store + reaper** **Goal:** Store authorization codes in SlateDB with 60 s TTL so they survive server restart. `consume` is single-use via a per-code in-process mutex (fabro-server is single-node — the in-process mutex is the right primitive). Reaper task prunes expired entries. **Requirements:** R1, R10 **Dependencies:** Unit 6 (for `IdpIdentity`) **Files:** - Create: `lib/crates/fabro-store/src/slate/auth_codes.rs` — typed SlateDB module parallel to `auth_tokens.rs` (Unit 10): - `AuthCode { identity: IdpIdentity, login: String, name: String, email: String, code_challenge: String, redirect_uri: String, expires_at: DateTime }` (uses the `IdpIdentity` type from `fabro-types` — no bare `idp_issuer`/`idp_subject` strings). - `SlateAuthCodeStore` struct wrapping `Arc` + `DashMap>>` (per-code in-process mutex map, mirroring Unit 10's pattern for refresh tokens). - Public methods: `insert(&self, code: &str, entry: AuthCode) -> Result<()>`, `consume(&self, code: &str) -> Result>` (single-use via per-code mutex — see Approach), `gc_expired(&self, cutoff: DateTime) -> Result`. - Modify: `lib/crates/fabro-store/src/slate/mod.rs` — declare the new module; expose `auth_codes()` accessor from `Database` returning `Arc`. - Modify: `lib/crates/fabro-store/src/keys.rs` — add `SlateKey::auth_code(code)` constructor and `SlateKey::auth_code_prefix()` for GC scans. - Modify: `lib/crates/fabro-server/src/auth/mod.rs` — re-export `AuthCode` via a thin façade. - Modify: `lib/crates/fabro-server/src/serve.rs` — spawn the reaper task alongside `auth_tokens` GC; reaper invokes `auth_codes.gc_expired(now)` every 30 s; cancel on graceful shutdown. - Test: `lib/crates/fabro-store/src/slate/auth_codes.rs` (inline `#[cfg(test)] mod tests`) — backed by `object_store::memory::InMemory`. **Approach:** - Code value is `base64url(32 bytes from OsRng)` — opaque server-internal identifier; the CLI treats it as a string. - **`consume` is single-use.** Implementation, mirroring Unit 10's per-hash mutex pattern: ``` let mutex = mutex_map.entry(code.to_string()).or_insert_with(|| Arc::new(Mutex::new(()))).value().clone(); let _guard = mutex.lock().await; // Critical section (holding the per-code mutex): let row = slatedb.get(auth_code_key(code)).await?; let entry = row.map(decode)?; if let Some(ref entry) = entry { if entry.expires_at <= now { return Ok(None); } // expired slatedb.delete(auth_code_key(code)).await?; } return Ok(entry); ``` Second caller within the same node blocks on the mutex, reads `None` after the first caller's delete, returns `None`. **Mutex cleanup:** same `Arc::strong_count == 2` check as Unit 10, never remove under the mutex. - Reaper scans `SlateKey::auth_code_prefix()` every 30 s and deletes rows with `expires_at <= now`. - Values encoded as `serde_json::to_vec` to match existing repo convention. **Patterns to follow:** - `lib/crates/fabro-store/src/slate/auth_tokens.rs` (Unit 10) — parallel pattern for refresh tokens; `auth_codes` mirrors the shape. - `lib/crates/fabro-store/src/slate/run_store.rs` for SlateDB CRUD. **Test scenarios:** - Happy path: insert → consume returns the entry; second consume returns `None`. - Edge case: insert → wait 61 s (via mocked time) → consume returns `None`. - **Integration — concurrent consume:** `tokio::task::JoinSet` with N=16 tasks all calling `consume(code)` on the same code → exactly one task receives `Some(entry)`; all others receive `None`. Asserts the per-code mutex + delete-under-mutex pattern holds. - Happy path: reaper run removes expired entries but preserves unexpired ones (table-driven). - Integration (with Unit 13): reaper is cancelled cleanly on shutdown — no leaked task. **Verification:** - Tests pass; no clippy warnings on the new module. --- - [ ] **Unit 10: `RefreshToken` + `SlateAuthTokenStore` with atomic `consume_and_rotate`** **Goal:** SlateDB-backed refresh-token persistence with `consume_and_rotate` as the only rotation primitive. Atomic via an in-process per-hash mutex (fabro-server is single-node — the in-process mutex is the correct primitive). No idempotency grace — network failures during response delivery force re-login (see Key Technical Decisions). **Requirements:** R2, R3 **Dependencies:** Unit 2 (workspace deps), Unit 6 (`IdpIdentity` type) **Files:** - Modify: `Cargo.toml` (workspace root) — add `dashmap = "6"` to `[workspace.dependencies]` (for the per-hash mutex map inside the store; extends Unit 2's scope). - Create: `lib/crates/fabro-store/src/slate/auth_tokens.rs` — typed module parallel to `run_store.rs` and `catalog.rs`: - `RefreshToken` struct with fields: `token_hash: [u8;32]`, `chain_id: Uuid`, `identity: IdpIdentity`, `login: String`, `name: String`, `email: String`, `issued_at: DateTime`, `expires_at: DateTime`, `last_used_at: DateTime`, `used: bool`, `user_agent: String`. `identity` uses the typed wrapper from Unit 6; the non-empty invariant flows through serde via `#[serde(try_from = ...)]`. - `SlateAuthTokenStore` struct wrapping `Arc` + `DashMap<[u8;32], Arc>>` for per-hash critical sections. No `recently_rotated` map (grace dropped for v1). - `ConsumeOutcome { Rotated(RefreshToken /* old */, RefreshToken /* new */), Reused(RefreshToken /* old */), Expired, NotFound }`. - Public methods: `consume_and_rotate(&self, presented_hash, new_token, now) -> Result`, `insert_refresh_token(&self, token) -> Result<()>` (primary insertion at first login), `find_refresh_token(&self, token_hash) -> Result>`, `delete_chain(&self, chain_id) -> Result`, `gc_expired(&self, cutoff) -> Result`. - Modify: `lib/crates/fabro-store/src/slate/mod.rs` — declare the new module; expose `auth_tokens()` accessor from `Database` (returns `Arc` — match existing `runs()` pattern). - Modify: `lib/crates/fabro-store/src/keys.rs` — add `SlateKey::auth_refresh(token_hash)` constructor and `SlateKey::auth_refresh_prefix()` for scans (keeps `SlateKey` encapsulated inside `fabro-store`). - Modify: `lib/crates/fabro-server/src/auth/mod.rs` — re-export `RefreshToken`, `ConsumeOutcome` via a thin façade so handlers don't need to import from `fabro-store` directly. - Modify: `lib/crates/fabro-server/src/serve.rs` — obtain `Arc` from `Database` at startup; spawn the GC reaper every 6 h with a 7-day grace window. - Test: `lib/crates/fabro-store/src/slate/auth_tokens.rs` (inline `#[cfg(test)] mod tests`) — backed by `object_store::memory::InMemory` (existing pattern in `fabro-store`). **Approach:** - **Concrete struct, not a trait.** `SlateAuthTokenStore` has no abstraction over the backing store. - **Singleton ownership model:** `AuthServices { refresh_tokens: Arc, ... }`. Every caller obtains the store via `Arc::clone(&services.refresh_tokens)` — sharing, not duplication. The DashMap lives inside the inner type, so every `Arc::clone` handle sees the same concurrency state. - **Per-hash mutex acquisition uses DashMap's atomic entry API:** ``` let mutex = mutex_map.entry(hash).or_insert_with(|| Arc::new(Mutex::new(()))).value().clone(); let _guard = mutex.lock().await; ``` (Conceptual — actual code uses the dashmap entry ref + value().clone() pattern; the plan names it so implementers don't accidentally write the racy get-then-insert variant.) - **Mutex cleanup:** after the critical section completes, attempt to remove the entry only if `Arc::strong_count == 2` (one from the map + one from the local `mutex` binding). Never remove under the mutex (implementation detail; documented inline). - **Critical section (holding the per-hash mutex):** 1. `find_refresh_token(presented_hash)` → if `None` → `NotFound`. 2. If `now >= expires_at` → `Expired`. 3. If `used == true` → `Reused(old)`. Do NOT mutate. The caller invokes `delete_chain(old.chain_id)` — which prefix-scans for every row sharing the chain, including this used row and every subsequent rotation. This is the theft-detection path; the row must remain queryable by `find_refresh_token` for it to fire, which is why step 4 does NOT delete the old row. 4. Else perform the rotation: **mark old `used = true, last_used_at = now`; insert new row.** The old row stays in SlateDB (with `used=true`) so that a subsequent replay presentation still hits step 3 and triggers theft detection. GC (`gc_expired`) purges used rows after `expires_at + 7d`. Return `Rotated(old, new)`. - **Multi-key write atomicity.** SlateDB exposes a `write(WriteBatch)` API for atomic multi-key commits. The rotation is now two writes (mark old `used=true` + insert new row); a single `WriteBatch` commits them atomically, eliminating the narrow crash window between them. If the implementer finds `WriteBatch` doesn't fit, fall back to sequential ops — a crash between mark-old and insert-new leaves `used=true` with no successor, which fails-closed on next presentation (replay is treated as theft; the one-time legitimate retry also forces re-login). Acceptable fail-mode. - Values encoded as `serde_json::to_vec` to match existing repo convention. - `delete_chain(chain_id)` is a prefix scan + filter (no secondary index in v1). **Patterns to follow:** - `lib/crates/fabro-store/src/slate/run_store.rs` for SlateDB CRUD shape. - `lib/crates/fabro-store/src/slate/mod.rs` for prefix-scan iteration and `Database::runs()` accessor pattern. - `dashmap::DashMap::entry().or_insert_with()` + `.value().clone()` on an `Arc>` is the standard atomic-upsert idiom. **Test scenarios:** - Happy path: `insert_refresh_token` then `find_refresh_token` round-trips. - Happy path: `consume_and_rotate` with a valid token returns `Rotated(old, new)`; the new token is findable; **the old token is still findable via `find_refresh_token` but now has `used=true`** (critical: required for theft detection on replay). - Error path: `consume_and_rotate` with unknown hash → `NotFound`. - Error path: `consume_and_rotate` with expired token → `Expired`. - Error path: `consume_and_rotate` with `used==true` token → `Reused(old)`; not mutated further (caller calls `delete_chain`). - **Error path — replay after successful rotation (the theft-detection invariant):** rotate once; re-call `consume_and_rotate` with the SAME `presented_hash` → returns `Reused(old)` with the old row's `chain_id`. This is the critical test — before the fix, this path returned `NotFound` because the old row had been deleted, silently collapsing theft detection to look-like-expiry. - **Integration — concurrent rotation (critical):** `tokio::task::JoinSet` with N=32 tasks all calling `consume_and_rotate` on the same presented token → exactly one sees `Rotated(old, new)`; all other 31 see `Reused(old)`. Chain deletion happens once (driven by the handler; see Unit 15). - **Integration — singleton sharing:** build `AuthServices`; clone the `Arc`; assert two tasks using different clone handles but contending on the same hash still see exactly one `Rotated`. Explicit regression guard against accidental per-clone DashMaps. - **Serde invariant guard (flows from Unit 6):** deserialize a crafted `RefreshToken` JSON with `identity.issuer = ""` → serde error, NOT a successfully-reconstituted invalid row. Validates that the `#[serde(try_from)]` invariant on `IdpIdentity` flows into `RefreshToken` via standard derive. - Happy path: `delete_chain(chain_id)` removes all rows sharing that chain and returns their count. - Happy path: `gc_expired(now - 7d)` removes rows older than the cutoff; preserves newer. **Verification:** - All tests pass, including the concurrent-rotation integration test (run with `cargo nextest run -p fabro-server --test-threads=8` to maximize contention). --- - [ ] **Unit 11: Extend `jwt_auth` extractor with JWT bearer path** **Goal:** Extend `AuthenticatedService` / `AuthenticatedSubject` to accept an HS256 JWT bearer alongside the existing dev-token bearer and session cookie. **Requirements:** R2, R4, R8 **Dependencies:** Unit 5, Unit 6, Unit 8 **Files:** - Modify: `lib/crates/fabro-server/src/jwt_auth.rs` — extend bearer dispatch with the following decision chain (order matters): 1. If bearer starts with `fabro_dev_` → dev-token validation (existing path, unchanged). 2. Else if bearer starts with `fabro_refresh_` → log `INFO` with the route path ("refresh token presented at protected endpoint") as a diagnostic signal for CLI bugs; return `ApiError::unauthorized_with_code(..., "unauthorized")`. Do NOT attempt JWT parsing. 3. Else attempt JWT parse via Unit 8's `verify()`. On success: produce `VerifiedAuth { idp_issuer, idp_subject, login, auth_method: Github, credential_source: JwtAccessToken }`. On `JwtError::AccessTokenExpired`: `ApiError::unauthorized_with_code(..., "access_token_expired")`. On `JwtError::AccessTokenInvalid`: `ApiError::unauthorized_with_code(..., "access_token_invalid")`. On parse failure (not a valid JWT structure): `ApiError::unauthorized_with_code(..., "unauthorized")`. - Modify: `lib/crates/fabro-server/src/jwt_auth.rs` — extend `CredentialSource` enum with `JwtAccessToken`. - Test: `lib/crates/fabro-server/src/jwt_auth.rs` (inline `#[cfg(test)] mod tests`). **Approach:** - `iss` compared against the server's resolved public URL (pulled from `AppState`). - Bearer dispatch uses explicit prefix checks for known-shaped tokens and JWT parse fallthrough for everything else — more robust than sniffing the base64 header prefix `eyJ` (which is format-dependent and future-fragile). - The `fabro_refresh_` branch emits an explicit diagnostic log so CLI bugs that pass the wrong bearer become visible (otherwise they'd look like an opaque 401 to the user). **Patterns to follow:** - Existing dev-token bearer validation at `jwt_auth.rs:99-145` (constant-time compare). **Test scenarios:** - Happy path: valid JWT bearer → extractor produces `VerifiedAuth` with correct `idp_issuer`/`idp_subject`. - Happy path: dev-token bearer still works (regression guard). - Happy path: session cookie still works (regression guard). - Error path: expired JWT → 401 `ApiError` with `code="access_token_expired"`. - Error path: JWT with bad signature → 401 `access_token_invalid`. - Error path: JWT with `alg: none` header → 401 `access_token_invalid` (no crash on crafted input). - Error path: refresh-token-shaped bearer (`fabro_refresh_...`) on a protected endpoint → 401 `unauthorized` AND an `INFO` log entry with the path (diagnostic for CLI bugs). - Error path: garbage bearer that happens to start with `eyJ` but is not a valid JWT → 401 `unauthorized` (not `access_token_invalid` — we distinguish malformed from tampered). - Error path: empty bearer → 401 `unauthorized`. - Integration: JWT issued by Unit 8 with key from Unit 4 verifies successfully end-to-end. **Verification:** - All existing protected-endpoint tests still pass; new JWT-bearer tests pass. ### Phase 3 — Server endpoints - [ ] **Unit 12: `/auth/login/github` and `/auth/callback/github` accept `return_to` and preserve it on error paths** **Goal:** Thread a strictly-whitelisted `return_to` query param through the existing GitHub OAuth flow, including on callback error paths (allowlist rejection, token-exchange failure, user-fetch failure). **Requirements:** R9, R10 **Dependencies:** none **Files:** - Modify: `lib/crates/fabro-server/src/web_auth.rs` — `login_github` (`:285`) accepts `?return_to=`; validate against regex `^/auth/cli/(start|resume)$` (reject anything else with `WARN` log of the redacted value, treat as absent). Store `return_to` in the existing `fabro_oauth_state` cookie alongside the state token. **Extend the `fabro_oauth_state` cookie `max_age` from 10 min to 30 min** to cover slow GitHub authorize flows (MFA, org approval, user delay). - Modify: `lib/crates/fabro-server/src/web_auth.rs` — `callback_github` (`:359`), error-path matrix: - **State-cookie missing/expired** (`:377-380`): the plan cannot trust `return_to` (we have no signed state to read it from). Render a plain HTML error page "Your login took too long or was tampered with. Please start again." and log WARN with the request-id. Do NOT redirect anywhere derived from query params. - **Config errors** (`session_key`/`web_url`/`client_id` resolution): existing JSON 409s stay (these are admin-facing, never CLI-facing). - **Success**: read `return_to` from state cookie, redirect there (default `/runs` when absent). - **Allowlist rejection** (`:522-526`): if state cookie carries CLI `return_to`, redirect to `?error=unauthorized&error_description=Login%20not%20permitted&state=`; no session minted. If no CLI `return_to`, preserve existing `/login?error=unauthorized`. - **GitHub token-exchange or user-fetch failure**: same shape, `error=server_error&error_description=Could%20not%20complete%20GitHub%20sign-in`. - **User denial at GitHub** (`?error=access_denied` in callback query string): forward as `?error=access_denied&error_description=Authorization%20denied`. - Every `error_description` value is a **closed enumeration of server-authored strings**, never a forwarded-from-upstream value (defense against log/terminal injection — see Key Technical Decisions). - Modify: `lib/crates/fabro-server/src/web_auth.rs` — extend `OauthStateCookie` struct (look near `add_oauth_state_cookie`) to carry `return_to: Option`. - Test: `lib/crates/fabro-server/tests/it/api/` — add `cli_auth_return_to.rs` for end-to-end checks via `tower::ServiceExt::oneshot`. **Approach:** - `return_to` whitelist is strict: must be an absolute path starting with `/`, must match the exact regex. Any other value is ignored and logged at WARN (with redaction of the raw value to avoid log pollution). - Error-path redirect only occurs when a CLI `return_to` is present on the state cookie — existing non-CLI behavior is preserved. - Every CLI-visible error is mapped from the server-side cause to one of a closed set of `{error, error_description}` pairs. Inbound attacker-controlled query strings never flow through verbatim. **Patterns to follow:** - `add_oauth_state_cookie` / `remove_oauth_state_cookie` around `web_auth.rs:348-353` show the cookie-signing pattern. **Test scenarios:** - Happy path: `login_github?return_to=/auth/cli/resume` → state cookie carries the return_to; success callback redirects to `/auth/cli/resume`. - Error path: `return_to=http://evil.com` → treated as absent, WARN logged, redirect goes to default `/runs`. - Error path: `return_to=/` (not a CLI path) → treated as absent. - Error path: allowlist rejection with CLI return_to → 302 to `?error=unauthorized&error_description=Login+not+permitted&state=...`, no session minted. - Error path: simulated GitHub token-exchange failure (via Unit 6b's override pointing at a failing twin-github) → 302 to `?error=server_error&...`. - Error path: callback query carries `?error=access_denied` (user clicked Cancel at GitHub) → forwarded to CLI via `?error=access_denied&...`. - **Error path — state cookie missing**: browser hits `/auth/callback/github` with valid GitHub code+state but no `fabro_oauth_state` cookie → plain HTML error page, CLI loopback times out (documented behavior — not a new redirect target). - **Error path — state cookie expired**: extend max_age to 30 min means this is less common; test verifies a 35-min-delayed callback still fails cleanly (plain HTML; CLI timeout). - **Error path — inbound error_description from attacker**: an attacker crafts `/auth/callback/github?error=access_denied&error_description=...&state=` → server returns `error=access_denied&error_description=Authorization%20denied` (the server-authored string), NOT the attacker string. Sanitization test. - Regression: non-CLI login (no `return_to`) — existing happy path and rejection paths unchanged. **Verification:** - All tests pass; existing GitHub OAuth tests unchanged. --- - [ ] **Unit 13: `/api/v1/auth/cli/config` preflight handler** **Goal:** Implement the unauthenticated preflight endpoint that tells the CLI whether OAuth login is available and, if so, which origin to open in the browser. **Requirements:** R5, R6, R12 **Dependencies:** Unit 1, Unit 5 **Files:** - Create: `lib/crates/fabro-server/src/auth/cli_flow.rs` — `config()` handler; registers under `/api/v1/auth/cli/config`. Always mounted (regardless of `web.enabled`), as it lives on the API router. - Modify: `lib/crates/fabro-server/src/server.rs` — add route registration in `api_common` (the API sub-router defined at `server.rs:902`). - Modify: `lib/crates/fabro-server/src/auth/mod.rs` — re-export the handler. - Test: `lib/crates/fabro-server/tests/it/api/cli_auth_config.rs` (new). **Approach:** - Read `AuthMode` and `server.web` settings from `AppState`. - `enabled = web.enabled && auth.methods.contains(Github)`. - When `enabled`: `{enabled: true, web_url: server.web.url, methods: [...]}` with `reason` absent. - When `!enabled`: `web_url: null`, `reason ∈ {"github_not_enabled", "web_not_enabled"}`. The CLI renders the user-visible description from the enum value locally — no `reason_description` on the wire. - **Mount-site split** (implementation footgun): `config()` registers in `api_common` (unauthenticated preflight, always mounted). Units 14 and 15's `/auth/cli/*` handlers register in the `/auth` nest (only when `web.enabled`). The `auth/cli_flow.rs` module exports two route-set functions: `api_routes()` for Unit 13's preflight, and `web_routes()` for Units 14/15's browser + token endpoints. `server.rs` calls each at its correct mount point. This keeps the file unified while making the mount asymmetry explicit. **Patterns to follow:** - Existing unauthenticated-handler shape in `web_auth.rs:auth_config` (`:278`). - Existing `api_routes()` / `routes()` split in `web_auth.rs:97`, `:105`. **Test scenarios:** - Happy path: `web.enabled=true`, `methods=[github]` → `{enabled: true, web_url: "http://...", methods: ["github"]}` (no `reason` key). - Error path: `methods=[dev-token]` → `{enabled: false, web_url: null, methods: ["dev-token"], reason: "github_not_enabled"}`. - Error path: `web.enabled=false`, `methods=[dev-token]` (no startup conflict since github is absent) → `{enabled: false, web_url: null, methods: ["dev-token"], reason: "web_not_enabled"}`. - Edge case: IP allowlist enabled + request from a non-allowlisted IP → 403 (allowlist runs before this handler; regression guard). - **Integration — mount split**: boot with `web.enabled=false` (no github); verify `GET /api/v1/auth/cli/config` returns 200 with `enabled=false`, while `GET /auth/cli/start` returns 404 (not mounted). - Integration: OpenAPI conformance test confirms the serialized shape matches the spec added in Unit 1. **Verification:** - Tests pass; conformance test passes. --- - [ ] **Unit 14: `/auth/cli/start` and `/auth/cli/resume` browser-flow handlers** **Goal:** Implement the two browser-consumed endpoints of the CLI OAuth flow, including strict `redirect_uri` validation, session eligibility gate, error passthrough on `/resume`, and `github_auth_not_configured` HTML short-circuit. **Requirements:** R5, R9, R10, R12, R13 **Dependencies:** Unit 4, Unit 9, Unit 12, Unit 13 **Files:** - Modify: `lib/crates/fabro-server/src/auth/cli_flow.rs` — add `start()` and `resume()` handlers; single static HTML helper `static_error_page(title: &'static str, body: &'static str) -> Response` that emits server-authored text only (no query-value echoing) with `Content-Type: text/html; charset=utf-8`, `X-Content-Type-Options: nosniff`, `Cache-Control: no-store`. Callers pass `&'static str` pairs — the `'static` lifetime bound is load-bearing: it makes echoing untrusted runtime data into the HTML a compile-time error. Constants for the five cases: `GITHUB_NOT_CONFIGURED`, `INVALID_REDIRECT_URI`, `INVALID_OR_MISSING_STATE`, `MISSING_FLOW_COOKIE`, `LOGIN_SUCCESSFUL`. - Modify: `lib/crates/fabro-server/src/server.rs` — register both routes under the existing `/auth` nest (only mounted when `web.enabled`). - Modify: `lib/crates/fabro-server/src/auth/cli_flow.rs` — private (authenticated-encryption) `fabro_cli_flow` cookie helpers via `cookie::PrivateJar` with the cookie key from Unit 4, matching the existing `__fabro_session` pattern. Cookie attributes: `HttpOnly; SameSite=Lax; Path=/auth; MaxAge=10min; Secure` (Secure relaxed only for `http://127.0.0.1` dev deployments, detected via `server.web.url.starts_with("https://")` — same pattern as existing `session_cookie_secure` helper at `web_auth.rs:215`). - Test: `lib/crates/fabro-server/tests/it/api/cli_auth_browser.rs` (new). **Approach:** - **Session eligibility gate** (used by `/start` and `/resume`, exhaustively pattern-matched on Unit 6's typed `Option`): ``` match session.and_then(|s| s.identity.as_ref()) { Some(idp) => eligible, None => ineligible, } ``` No empty-string checks; the type system enforces the invariant. - `/auth/cli/start` query validation: 1. If `github` not in `methods` → static HTML error (template `GithubAuthNotConfigured`). NOT a flat JSON envelope. 2. `redirect_uri` must match `http://127.0.0.1:/callback` or `http://[::1]:/callback`. Else → static HTML error (`InvalidRedirectUri`) — the redirect target cannot be trusted, so no redirect. 3. `state` is 16–512 URL-safe chars. Missing/malformed → static HTML error (`InvalidOrMissingState`). 4. `code_challenge` present; `code_challenge_method` = `S256`. Missing/wrong → 302 to `redirect_uri?error=invalid_request&error_description=&state=`. - Eligible session: mint `AuthCode` (Unit 9), 302 to `redirect_uri?code=&state=`. - Ineligible or absent session: set `fabro_cli_flow` cookie with `{redirect_uri, state, code_challenge}` (10 min TTL, attributes above); 302 to `/auth/login/github?return_to=/auth/cli/resume`. - `/auth/cli/resume` algorithm (order matters — enforced by tests): 1. If `github` not in `methods` → static HTML error (`GithubAuthNotConfigured`). 2. Read `fabro_cli_flow` cookie. If missing/expired → static HTML error (`MissingFlowCookie`). 3. **Error passthrough** (before any session check): if query carries `?error=`, map the inbound `error` value to a closed enum (valid values: `unauthorized`, `server_error`, `access_denied`; any other → `server_error`). Clear `fabro_cli_flow`, 302 to `redirect_uri?error=&error_description=&state=`. Do NOT inspect `__fabro_session`. **Never forward the inbound `error_description` verbatim** — the server authors the user-visible text (closed set). 4. Apply session eligibility gate. If ineligible: clear `fabro_cli_flow`, 302 to `redirect_uri?error=github_session_required&error_description=&state=`. 5. Mint `AuthCode` keyed to the session's `(identity.issuer, identity.subject, login, name, email, code_challenge, redirect_uri)`; clear `fabro_cli_flow`; 302 to `redirect_uri?code=&state=`. **Patterns to follow:** - `remove_cookie` + `add_cookie` pair at `web_auth.rs:549-561`. - `session_cookie_secure` helper at `web_auth.rs:215` (reuse for Secure-flag detection). **Test scenarios:** - Happy path (/start): eligible session → 302 to `redirect_uri?code=&state=`; code is a fresh entry in the `AuthCodeStore`. - Happy path (/start): no session → `fabro_cli_flow` cookie set with the exact attributes above; 302 to `/auth/login/github?return_to=/auth/cli/resume`. - Happy path (/resume): after a valid GitHub callback, session is present + eligible → 302 to `redirect_uri?code=&state=`. - Error path (/start): invalid `redirect_uri` host → static HTML error (status 400, HTML content-type, `X-Content-Type-Options: nosniff` set). - Error path (/start): missing `state` → static HTML error. - Error path (/start): missing `code_challenge` → 302 with `error=invalid_request`. - Error path (/start): `github` not in methods → static HTML error, not JSON. - Error path (/resume): inbound `?error=unauthorized` (from allowlist rejection per Unit 12) → 302 to `redirect_uri?error=unauthorized&error_description=&state=...` regardless of session state. **Assert ordering explicitly** — dev-token session must NOT mask this as `github_session_required`. - Error path (/resume): inbound `?error=` → mapped to `server_error`; inbound `?error_description=