110 lines
8.4 KiB
Markdown
110 lines
8.4 KiB
Markdown
# 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).
|