# 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 `ignore`d 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.