PiCloud/security_audit/HANDBACK_tier3.md

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