PiCloud/security_audit/REVIEW_tier3.md

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.