docs(audit-2026-06-11/tier3): handback + independent review artifacts

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

security_audit/HANDBACK_tier3.md

@@ -0,0 +1,135 @@
+# Handback — Security remediation Tier-3 "safety batch" (`fix/audit-2026-06-11-tier3`)
+
+Implementer: assistant (inline, with iterative testing).
+Date: 2026-06-12.
+Branch: `fix/audit-2026-06-11-tier3` (8 commits off `main`, unpushed).
+Scope: the user-selected "Safety batch" from the post-merge review of `fix/audit-2026-06-11`.
+
+## Verdict from my own testing: green, ready for your review.
+
+- `cargo fmt --all --check` → clean.
+- `cargo clippy --all-targets --all-features -- -D warnings` → clean (exit 0).
+- Full workspace `--include-ignored` against a fresh Postgres 16: **824 passed**; the
+  previously-failing `authz::deactivating_user_revokes_their_api_keys` now **passes**.
+- Full `picloud-cli` suite in isolation (fresh DB + dev key): **76 passed**.
+- Two "failures" in the *combined* `--no-fail-fast` run are the same harness artifacts the
+  original REVIEW.md documented — see "Test-harness artifacts" below. Neither is a code defect.
+
+## Commits (one finding per commit)
+
+| Commit | Finding | What landed |
+|---|---|---|
+| `31ecfae` | PrincipalCache revocation-lag (+ H-E1, test rename) | `evict_user`/`evict_token` on the cache; one shared cache Arc across AuthState/AdminsState/ApiKeysState; evict on deactivation, password change, logout, single API-key delete. Greens the failing test. |
+| `95fddf3` | C-2 sanitizer + F-SE-H-03 | `sanitize_stored_content_type` rejects control chars up-front (was passable via an `image/` prefix → header-injection/panic); `disable_symbol("debug")` to match `print`. |
+| `e676eb9` | F-SE-H-04 | Scrub `ExecError::Runtime` detail from public data-plane responses; log full text under a correlation id. |
+| `d9fb0e7` | F-FS-002 | `guard_collection` on repo `head/get/list/delete`; `validate_files_collection` on the three admin endpoints (clean 422). |
+| `3ac1022` | H-1 | Caddy `request_body { max_size 12MB }` (both Caddyfiles, validated); explicit `DefaultBodyLimit::max(1MB)` on the public email-inbound webhook. |
+| `bdcc9a6` | H2 (+ M2) | Reject inline `--password`/`--token` (stdin-only); `rpassword` no-echo prompt for admins. |
+| `e8cc3af` | H-B1 follow-up | `extract_remote_ip` uses the **last** X-Forwarded-For hop (Caddy-appended real client) instead of the spoofable first hop. |
+| `ec4a2aa` | — | `cargo fmt`. |
+
+## Detail per item
+
+### 1. PrincipalCache revocation-lag (`31ecfae`) — greens the failing test
+The cache (`token-hash → Principal`, 60s TTL) had no eviction hook, so deactivation /
+password change / API-key revocation flipped the DB rows while the cache kept serving the
+stale principal until TTL. Added `PrincipalCache::evict_user(user_id)` and `evict_token(hash)`.
+Wired one **shared** cache `Arc` (hoisted in `lib.rs`) into `AuthState`, `AdminsState`, and
+`ApiKeysState` — a per-state cache would let the middleware's own copy keep authenticating a
+revoked principal. Evict points: deactivation + password change (`evict_user`), logout
+(`evict_token`, precise), single API-key delete (`evict_user`). Unit tests for both methods
+plus the existing `authz::deactivating_user_revokes_their_api_keys` integration test now pass.
+
+**H-E1 note (folded in):** the DB-write invalidation stays best-effort by design (the
+deactivation path deliberately doesn't undo on a DB blip; I kept password-change consistent).
+The cache evict is what now makes revocation take effect on the *next* request rather than
+after the TTL. I did **not** convert it to a hard abort — that would contradict the existing
+deliberate design and isn't clearly better.
+
+### 2. C-2 sanitizer + F-SE-H-03 (`95fddf3`)
+`sanitize_stored_content_type("image/png\r\nX-Injected: 1")` previously matched the `image/`
+prefix and returned the original (CRLF intact) → response-header injection / `.expect()` panic
+on download. Now any control byte (`<0x20` or `0x7f`) coerces to `application/octet-stream`
+first. Also `disable_symbol("debug")` (the comment already claimed both were disabled).
+
+### 3. F-SE-H-04 (`e676eb9`)
+`ExecError::Runtime` strings (FS paths, `"blocked by SSRF policy: link-local"`, pool-exhaustion
+timing, panic fragments, attacker-thrown strings) were returned verbatim in the public
+`/api/v1/execute` + user-route 502 bodies. This handler only serves anonymous data-plane
+callers (the authenticated script-test path is in manager-core and keeps verbose errors), so
+the full text is logged under a `correlation_id` and the client gets `"script execution error
+(ref: <uuid>)"`. New test asserts a thrown marker string doesn't leak.
+
+### 4. F-FS-002 (`d9fb0e7`)
+Admin files endpoints hit `FsFilesRepo` directly (not via the validating `FilesServiceImpl`).
+Added `guard_collection` to repo `head/get/list/delete` (create/update already had it) and
+`validate_files_collection` to the three admin endpoints for a clean 422 instead of the repo
+guard surfacing as a 500.
+
+### 5. H-1 (`3ac1022`)
+Caddy `request_body { max_size 12MB }` in both Caddyfiles (12MB clears the orchestrator's
+10 MiB user-route read; replaces Caddy's multi-GB default). Both validated with
+`caddy validate`. Email-inbound webhook gets an explicit `DefaultBodyLimit::max(1MB)`. Admin /
+execute keep Axum's 2MB extractor default; user routes keep their 10 MiB manual read.
+
+### 6. H2 + M2 (`bdcc9a6`)
+`--password`/`--token` reject inline values (leak into shell history / `ps` / `/proc`); only
+`-` (stdin) is accepted, `PICLOUD_TOKEN` env still covers CI for login. Interactive admins
+prompt switched to `rpassword` (true no-echo; the old `read_line` echoed despite the claim).
+New CLI test asserts inline `--password hunter2` is rejected.
+
+### 7. H-B1 follow-up (`e8cc3af`)
+`extract_remote_ip` took the **first** X-Forwarded-For entry; Caddy appends the real client as
+the **last** hop, so earlier entries are client-spoofable — an attacker could rotate the first
+entry to evade the per-IP login limiter. Now takes the last hop. Three unit tests (spoof,
+single-hop, missing-header).
+
+## Deferred (with rationale)
+
+- **Bootstrap sentinel (H-E2)** — deliberately NOT done. The API can't empty `admin_users`
+  (the last-active-admin guard blocks delete *and* deactivate of the final active admin), so
+  `count_active() > 0` always holds via normal operation. The only way to `count_active() == 0`
+  is direct SQL deletion, which already implies DB-write compromise (at which point an attacker
+  just inserts their own admin row — bootstrap re-seed is moot). A persistent sentinel would
+  only defend that niche **and** needs a durable marker = a schema migration, which the chosen
+  "no new schema" batch scope excludes. Re-fold into the v1.2 schema batch if desired.
+
+Everything else in the audit's Tier-3/4 (per-app quotas, audit-log table, AAD re-encryption
+sweep, absolute session expiry, outbox GC, per-execution SDK counters, per-app trigger/cron
+caps, etc.) remains deferred per the chosen scope.
+
+## Operational notes / behaviour changes
+
+- `pic admins create/set --password <inline>` and `pic login --token <inline>` now **error**.
+  Scripts must use `--password -` / `--token -` (piped) or `PICLOUD_TOKEN`.
+- Public-route runtime errors no longer echo detail; operators correlate via the logged
+  `correlation_id`.
+- Caddy now rejects request bodies > 12 MB at the proxy.
+
+## Test-harness artifacts (NOT regressions — same as the original review)
+
+A combined `cargo test --workspace --no-fail-fast -- --include-ignored` against one DB shows 77
+"failures":
+- **76 × picloud-cli** — the CLI fixture spawns the `picloud` binary and bootstraps an admin
+  into the shared base DB; run concurrently with the rest of the workspace it collides and the
+  fixture `LazyLock` poisons. **In isolation against a fresh DB the full suite is 76/76 green.**
+- **1 × engine doctest** (`set_self_weak`, line 172) — an `ignore`d pseudo-code example that
+  `--include-ignored` forces to compile. Under a normal `cargo test` it's skipped.
+
+How to reproduce a clean run:
+```
+# fresh DB
+docker run -d --rm --name pg -p 127.0.0.1:15495:5432 -e POSTGRES_USER=picloud \
+  -e POSTGRES_PASSWORD=picloud -e POSTGRES_DB=picloud postgres:16-alpine
+export DATABASE_URL=postgres://picloud:picloud@127.0.0.1:15495/picloud
+export PICLOUD_DEV_MODE=true PICLOUD_DEV_INSECURE_KEY=i-understand-this-is-insecure
+# everything except CLI (CLI needs its own DB):
+cargo test --workspace --exclude picloud-cli -- --include-ignored
+# CLI on its own fresh DB:
+cargo test -p picloud-cli --test cli -- --include-ignored
+```
+
+## Suggested next step
+
+ff-merge `fix/audit-2026-06-11-tier3` into `main` after your review. The branch is off the
+current `main` (which already has the Tier-1+2 work) and fast-forwards cleanly.

security_audit/REVIEW_tier3.md

@@ -0,0 +1,133 @@
+# Independent review — Tier-3 safety batch
+
+Verdict: APPROVE WITH NITS
+
+Scope reviewed: `git diff main...fix/audit-2026-06-11-tier3` (8 commits), every touched
+source file traced end-to-end, targeted non-DB unit tests run green, clippy clean on the
+four touched Rust crates. DB-backed `#[ignore]` integration tests were read but not run.
+
+## Per-fix verdict
+
+### 1. PrincipalCache revocation-lag — PASS
+Traced the Arc end to end. `crates/picloud/src/lib.rs:503` constructs ONE
+`Arc<PrincipalCache>` and `.clone()`s it into `AuthState` (510), `AdminsState` (523),
+and `ApiKeysState` (537). The middleware reads via `resolve_principal` →
+`state.principal_cache.get(...)` (`auth_middleware.rs:242`) where `state` is the same
+`auth_state` wired into both `require_authenticated` (lib.rs:571-573) and `auth_router`
+(lib.rs:596). It is genuinely one shared instance, so an evict on the revocation side is
+visible to the resolve side. No second `AuthState` is constructed anywhere.
+Keying is correct: `evict_user(user_id)` (`retain(p.user_id != user_id)`) on deactivation
+(`admin_users_api.rs:339`), password change (`admin_users_api.rs:266`), and single
+API-key delete (`api_keys_api.rs:170` — coarse-by-design because the cache is hashed by
+token, not key id); `evict_token(&hash)` (precise, single session) on logout
+(`auth_api.rs:201`). Unit tests `evict_user_drops_only_that_users_entries` and
+`evict_token_drops_only_that_entry` pass and actually assert the targeting (Bob's entry
+survives; the user's other session survives logout).
+Note: this remains a single-process cache; in cluster mode (deferred) revocation would
+still lag on peer nodes. Acceptable for MVP/single-node.
+
+### 2. content-type sanitizer — PASS
+`shared/src/files.rs:280` — `if ct.bytes().any(|b| b < 0x20 || b == 0x7f) { return
+SAFE_RENDER_FALLBACK }` runs FIRST, before the `split(';')` / allowlist logic. Byte check
+is exactly `< 0x20 || == 0x7f`. Test `control_chars_are_coerced_even_under_an_allowlisted
+_prefix` covers `image/png\r\n…`, bare `\n`, `\u{7f}`, `\u{0}` and passes. CRLF in an
+allowlisted prefix now coerces to `application/octet-stream`. Closes the header-injection
+/ HeaderValue-panic vector.
+
+### 3. disable debug — PASS
+`engine.disable_symbol("debug")` sits next to `disable_symbol("print")` inside
+`build_engine` (`executor-core/src/engine.rs:412`), which is the per-call builder invoked
+on every execution path (incl. the real run path at engine.rs:283). Test
+`debug_and_print_symbols_are_disabled` asserts both `print("x")` and `debug("x")` fail as
+`ExecError::Parse`; passes.
+
+### 4. runtime-error scrub — PASS
+The scrub is in `orchestrator-core/src/api.rs:753` inside `impl IntoResponse for ApiError`
+— the public data-plane response path (`/api/v1/execute/{id}` + user-route fallback). It
+logs full `detail` under a `correlation_id` (warn) and returns `"script execution error
+(ref: <uuid>)"`. I checked there is NO admin/authenticated path reusing this `IntoResponse`
+— the only other `ExecError` consumer in manager-core is `dispatcher.rs` (queue/cron
+background, stores `last_error` internally, not a public HTTP body). `Parse` /
+`InvalidResponse` / `Timeout` / `OperationBudgetExceeded` stay verbose. `Parse` can echo
+script-syntax detail, but that is the caller's own script and is deterministic/compile-
+shape, not internal infra detail — acceptable. The handback's claim of a separate "verbose
+admin script-test path" appears moot (no such endpoint found), but that is harmless — the
+public path is the one that mattered and it is scrubbed.
+
+### 5. files collection validation — PASS
+All four repo READ/DELETE methods now guard at method entry: `head`
+(`files_repo.rs:353`), `get` (373), `delete` (439), `list` (485); create/update (321,397)
+already guarded. `guard_collection` rejects empty / `/` / `\` / `..` / `\0`. The three
+admin endpoints call `validate_files_collection` (a re-export of `shared::validate_
+collection`, same reject-set) BEFORE touching the repo — `list_files` (files_api.rs:91),
+`get_file` (153), `delete_file` (198) — yielding a clean 422 instead of a repo-surfaced
+500. Belt-and-suspenders is real, both layers verified.
+
+### 6. body limits — PASS
+`DefaultBodyLimit::max(1 MiB)` is applied with `.layer(...)` on the email-inbound `Router`
+BEFORE `.with_state` (`email_inbound_api.rs:184-186`) — correct axum ordering, layer wraps
+the route. Caddy `request_body { max_size 12MB }` is placed inside the site block in both
+`caddy/Caddyfile` and `caddy/Caddyfile.prod` (valid placement). 12 MB (Caddy decimal =
+12,000,000) ≥ the orchestrator's 10 MiB user-route read (10,485,760, `api.rs:219`), so
+legit large invokes pass while the multi-GB Caddy default is replaced. Admin/execute keep
+axum's default extractor limit.
+
+### 7. CLI argv-secret rejection — PASS
+`resolve_password` (`admins.rs:51`) rejects `Some(non-dash)` with a clear error, accepts
+only `Some("-")` (stdin) and errors on `None`. `login.rs:30` rejects inline `--token`,
+accepts `--token -` (stdin) and still honors `PICLOUD_TOKEN` env. Interactive prompts use
+`rpassword::prompt_password` (true no-echo) for both password and token; the old echoing
+`read_line` trait was removed. `rpassword = "7"` is a declared dep. No internal caller
+passes an inline secret: `pic secrets set` already reads value from stdin only
+(`secrets.rs`), and the only `resolve_password` callers are `create`/`set`. New test
+`create_with_inline_password_is_rejected` asserts `--password hunter2` fails with "inline
+is not allowed".
+
+### 8. X-Forwarded-For last-hop — PASS
+`extract_remote_ip` now uses `s.split(',').next_back()` (`auth_api.rs:268`), trims, falls
+back to `"unknown"` on absent/empty/whitespace header (verified: `xff("   ")` → trimmed
+empty → falls through to `"unknown"`). Trust model is correct for the documented single-
+Caddy topology: Caddy's reverse_proxy APPENDS the real peer as the last XFF hop, so earlier
+entries are client-spoofable and the last entry is authoritative. Three unit tests (spoof-
+stable, single-hop, missing/empty) pass. The doc-comment honestly flags the multi-proxy
+caveat as deferred to cluster work.
+
+## Bootstrap-sentinel deferral — VALID
+Verified BOTH guards exist in `admin_users_api.rs`:
+- Deactivation path: `patch_admin` at 298-302 returns `LastActiveAdmin` when
+  `count_active_excluding(id) == 0` (plus a last-owner guard at 310-314).
+- Delete path: `delete_admin` at 372-385 mirrors it (`count_active_excluding == 0` →
+  `LastActiveAdmin`, plus last-owner guard).
+The delete guard only fires `if target.is_active`, but an inactive admin is not counted by
+`count_active`, so deleting one cannot drive `count_active()` to 0. There is no API path to
+`count_active() == 0`; reaching it requires direct SQL (= DB-write compromise, at which
+point re-seed is moot). The deferral rationale holds. Sentinel needs a durable marker
+(schema migration) which is out of the chosen no-schema scope — reasonable to defer.
+
+## New issues / regressions found
+None blocking. Nits:
+- **(nit, stale doc)** `auth_middleware.rs:141-142` still says the `principal_cache` field
+  is "`None` for tests / harnesses that don't wire it", but the field is a non-Option
+  `Arc<PrincipalCache>`. Cosmetic leftover; update or drop the sentence.
+- **(nit)** `crates/picloud-cli/tests/admins.rs` `create_with_inline_password_is_rejected`
+  is gated `#[ignore = "needs DATABASE_URL"]`, but the inline rejection happens in
+  `resolve_password` before any network/DB call, so the test would pass without Postgres.
+  Harmless over-gating; could be un-ignored for cheap coverage.
+- **(observation, out of scope)** The API-key-delete eviction is coarse (`evict_user`,
+  dropping all of that user's cached sessions+keys) because the cache is keyed by token
+  hash and the key id can't derive it. Documented and acceptable; a per-key revocation
+  would need a secondary index. Not a defect.
+
+## Tests adequacy
+Adequate. Each fix has a test that exercises behaviour, not just compilation:
+content-type coercion under allowlisted prefix; both `print`+`debug` rejection; XFF last-
+hop incl. spoof-stability; `evict_user`/`evict_token` targeting; inline-password rejection;
+and a DB-backed `runtime_error_detail_is_scrubbed_from_public_response` that throws a marker
+string and asserts it never appears in the public body while a `ref:` id does. The four
+non-DB suites I ran are green (5 + 1 + 9 + 3). DB-backed tests read as correct but unrun.
+
+## Bottom line
+All 8 fixes correctly close their stated gaps with no regression or new vulnerability found.
+The critical shared-Arc invariant for fix #1 is genuinely satisfied. The bootstrap-sentinel
+deferral rationale is verified accurate. Only three cosmetic nits, none blocking. Approve
+and ff-merge.