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