PiCloud/security_audit/REVIEW.md

Review — Security remediation fix/audit-2026-06-11 (Tier 1+2)

Reviewer: independent audit pass (not the implementing agent). Date: 2026-06-12. Branch: fix/audit-2026-06-11 (11 commits off main, unpushed). Scope reviewed: the 2 Criticals + 8 Highs claimed closed in the handback.

Verdict: APPROVE — ready to ff-merge into main.

All 10 Tier-1+2 findings are correctly implemented, the security logic is sound, and the production test suites are green. The single failing test is pre-existing and out of scope (disclosed honestly in the handback). The findings below are non-blocking follow-ups.

---

What I verified (independently, not from the handback)

Build gates

• cargo clippy --all-targets --all-features -- -D warnings → clean (exit 0).
• cargo fmt --all --check → clean.

Tests (run against throwaway Postgres 16 containers)

Suite	Result	Notes
picloud --test api	53 passed	in-process server boot
picloud --test authz	28 passed, 1 failed	the 1 failure is pre-existing — see below
picloud --test email_inbound	8 passed	directly exercises H-B2
manager-core --test schema_snapshot	1 passed	migration 0042 applies; hand-edited expected_schema.txt matches the real migrated DB exactly
picloud-cli --test cli	75 passed (fresh DB)	see "test-harness artifacts"
unit tests (manager-core 343, executor-core 74, shared, orchestrator, …)	all passed
executor-core doctests	ignored doctest correctly skipped under normal cargo test

Net: every suite the handback claimed green is green when run correctly.

Per-finding code verification

• C-1 (cookie CSRF) — extract_token cookie path fully removed; SESSION_COOKIE const deleted; login response no longer emits Set-Cookie (build_cookie + SET_COOKIE gone). Tests now assert cookie rejection. Dashboard already uses Bearer. ✅
• C-2 (stored XSS) — download forces Content-Disposition: attachment + X-Content-Type-Options: nosniff + default-src 'none'; sandbox; frame-ancestors 'none' CSP + Referrer-Policy: no-referrer; content-type re-sanitized through an allowlist at serve time; write path coerces dangerous types at create/update. Allowlist correctly coerces text/html, image/svg+xml, application/javascript, application/xhtml+xml, flash, xml. ✅ (one residual edge below)
• H-A1/A2/A3 (headers) — Caddy header layer adds CSP/X-Frame-Options→frame-ancestors/Permissions-Policy/no-store/HSTS; ?-operator lets the C-2 per-download CSP win on file routes. ✅
• H-B1 (login DoS) — per-(ip, username) + per-username bucket gates before acquiring a bounded Argon2 semaphore (correct ordering — attackers can't queue), fails closed on a closed semaphore. ✅
• H-B2 (email webhook) — bad-signature limiter checked before verification; no-secret path fails closed (401) as defense-in-depth; new triggers require a secret; email-trigger secret correctly stays on the open_legacy (v0, no-AAD) path so the AAD migration didn't break it. ✅
• H-C1 (runaway script) — thread-local DeadlineGuard consulted by engine.on_progress, returning Some(Dynamic::UNIT) to halt eval; both spawn_blocking sites pass Some(deadline); invoke re-entries inherit the deadline; default None preserves test/validation behavior. ✅
• H-D1 (AES-GCM AAD) — encrypt_with_aad/decrypt_with_aad added with round-trip + AAD-mismatch tests; secrets/app_secrets seal/open bind app_id(+name) AAD; read path dispatches on the version column (v0 → no-AAD legacy decrypt, v1 → AAD, else Corrupted); migration 0042 adds version SMALLINT NOT NULL DEFAULT 0 so existing rows keep decrypting. The dev correctly pinned the subtle RustCrypto fact that v0 == empty-AAD-v1, so version-column dispatch (not ABI) is the discriminator. ✅
• H-E1 (password change) — now delete_for_user + expire_all_for_user on password change, mirroring the deactivation path. ✅ (best-effort caveat below)
• H-F1 (dispatcher cross-app) — queue arm dead-letters on script.app_id != claimed.app_id (observable, doesn't execute); HTTP arm returns ResolveTrigger error. Mirrors build_invoke_request. ✅
• H-H1 (email::send) — send() enforces a per-message recipient cap (TooManyRecipients) + per-(app_id, recipient) rate limiter (RateLimited). Per-app, as the audit asked. ✅

The one failing test is genuinely pre-existing

picloud --test authz deactivating_user_revokes_their_api_keys asserts 401 but gets 200 (authz.rs:651). This is the PrincipalCache revocation-lag Medium the audit itself flagged as deferred (expire_all_for_user flips the DB row, but the in-process cache serves the stale principal until TTL). Confirmed pre-existing via git: the branch leaves both authz.rs (the test) and the deactivation revocation path byte-identical to main — H-E1 only added invalidation to the password-change path, not deactivation. The handback's disclosure is accurate.

Test-harness artifacts in my combined run (not code defects)

When I ran cargo test --workspace --no-fail-fast -- --include-ignored against a single shared DB, I saw 77 "failures." All but the one above are harness artifacts:

• 75 × picloud-cli — the fixture spawns the picloud binary as a subprocess; bootstrap only seeds the fixture admin when admin_users is empty, but the earlier suites had already populated that table in the shared DB → fixture login 401 → poisoned LazyLock cascade. Against a fresh DB, all 75 pass. server.rs is unchanged by the branch.
• 1 × engine doctest (set_self_weak, line 172) — a ```ignore pseudo-code example. --include-ignored forced it to compile; under a normal cargo test it is correctly skipped. Unchanged by the branch.

---

Non-blocking findings (recommend as follow-ups, not merge blockers)

1. C-2 sanitizer lets CRLF through an allowlisted prefix. sanitize_stored_content_type("image/png\r\nX-Injected: yes") matches the image/ prefix and returns the original string (CRLF intact). NewFile::validate is length-only, so such a value can be stored; on download it would reach .header(CONTENT_TYPE, …) → .expect(...) and panic the task (Axum catches it per-task → 500, file's download poisoned). This is the pre-existing agent-4 Medium — not introduced by C-2, and C-2 narrows the surface — but the function named sanitize_* gives false confidence. One-line fix: reject/strip bytes < 0x20/0x7f in sanitize_stored_content_type (or in validate).

2. H-E1 invalidation is best-effort. delete_for_user / expire_all_for_user errors are logged but don't fail the password-change response. If delete_for_user errors, the password rotates while sessions survive — the exact gap being closed. Consider surfacing the failure (or retrying) rather than only tracing::error!. Same shape exists on the pre-existing deactivation path, so it's a consistency point, not a regression.

3. H-B2 lockout is trigger-global. An attacker flooding bad signatures locks out legitimate inbound email to that trigger for the 60s window. Inherent to lockout-style limiters and acceptable for single-node MVP; worth a comment so it's a known trade-off.

4. Operational breaking change (intended). Existing email triggers with a null inbound_secret now return 401 (the H-B2 fix). Operators must add a secret to any pre-audit secret-less email trigger. Call this out in release notes.

5. Cosmetic. bearer_and_cookie_produce_same_principal no longer tests cookies (its body uses Authorization: Bearer). Rename to avoid implying cookie auth still exists.

6. Confirm remote_ip source for H-B1. Behind Caddy the socket peer is Caddy's IP (so the per-IP bucket degrades to global; the per-username bucket still protects). If it instead trusts an unvalidated X-Forwarded-For, that's spoofable. Worth a one-line check of how remote_ip is derived. Not a blocker either way.

---

Recommendation

ff-merge fix/audit-2026-06-11 into main. The Tier-1+2 security work is correct, tested, and honestly documented. File the six items above as Tier-3 follow-ups alongside the already-deferred work (per-app quotas, body limits, disable_symbol("debug"), anon error-scrub, email-trigger AAD upgrade, PrincipalCache eviction).