134 lines
8.6 KiB
Markdown
134 lines
8.6 KiB
Markdown
# 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.
|