# 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.