51f14fa2b1
Closes the gaps and the one security finding from the second end-to-end
CLI test (E2E_STASH_REPORT.md), plus the H1 boot-regression found while
re-reviewing those fixes.
Security
- S6: reserved-path validation (`check_reserved`) now case-folds before
comparing, so `/API/v2/x`, `/HEALTHZ`, `/Admin/x` are rejected like
their lowercase forms. Request-time matching stays case-sensitive.
- S10: "public route != public data" callout in sdk-shape.md (script_gate
skips authz when the principal is anonymous).
Observability / features
- G1: trigger executions now write `execution_logs`. Migration 0043 adds
a `source` column (CHECK mirrors ExecutionSource/OutboxSourceKind,
DEFAULT 'http' backfills history); a shared `build_execution_log` helper
in executor-core; dispatcher logging for outbox triggers + queue
consumers (skips sync-HTTP rows the orchestrator already logs). `pic
logs` gains a source column + `--source` filter.
- G5: dev-only in-memory email capture under PICLOUD_DEV_MODE with no SMTP
(email::send succeeds locally), readable at GET /api/v1/admin/dev/emails
(Owner/Admin only; route mounted only in capture mode).
- G6: generalized the Rhai in-place-mutation footgun note (trim/replace/
make_upper/make_lower/crop/truncate/pad return ()).
- G2/G3/G4 (CLI): `pic members`, `pic files`, `pic queues`, read-only
`pic kv` (+ new kv_api.rs); `pic deploy --timeout/--memory/--kind/
--sandbox`; first-class `pic triggers create-{docs,files,pubsub,queue,
email}` wrappers. All new client path segments percent-encoded via seg().
H1 regression fix (found in re-review)
- The S6 change also runs in `compile_routes`, which compiles every stored
route at boot and on each route CRUD. A single stored route the new
validation rejects (creatable while the S6 gap existed) made the whole
compile Err and aborted startup. `compile_routes` is now lenient: it
skips an un-compilable row with a warning instead of bricking boot
(route creation still validates separately). Migration 0044 sweeps
pre-existing reserved-path routes on upgrade (WHERE mirrors
check_reserved exactly). Added regression tests for both.
Verified: cargo fmt, clippy --all-targets --all-features -D warnings, the
schema_snapshot test, and the new S6/lenient-compile unit tests all pass;
boot-resilience and G1/G5 confirmed live.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
139 lines
11 KiB
Markdown
139 lines
11 KiB
Markdown
# E2E Test #2 — "Stash" (paste + file-drop) + security matrix
|
||
|
||
**Date:** 2026-06-13 · **Build:** v1.1.9 · **Instance:** host `picloud` on `127.0.0.1:8099`
|
||
(dev mode, `PICLOUD_FILES_MAX_FILE_SIZE_BYTES=1048576`, SSRF guard on, SMTP unset), Postgres in
|
||
Docker. Driven entirely through the `pic` CLI; data-plane traffic via `curl`.
|
||
|
||
## Goal
|
||
|
||
Second end-to-end pass. The first test (To-Do, `E2E_TODO_REPORT.md`) covered users/docs/pubsub/
|
||
cron/routes/domains/topics. This one builds a **new** app to exercise the *untested* half of the
|
||
platform — **kv, files, queue, invoke, secrets, api-keys, dead-letters, async + two-param routes**
|
||
— and then runs a **10-probe security matrix**. Findings: feature/CLI gaps + security verdicts.
|
||
|
||
## Verdict
|
||
|
||
**Every feature worked** end-to-end, and **every security control held except one Low-severity
|
||
gap** (reserved-path validation is case-sensitive). Highlights of what's *good*: cross-app
|
||
isolation is airtight, the SSRF guard blocks cloud-metadata, size/op caps fire before authz, and
|
||
internal script errors are scrubbed from responses with a correlation id. Two notable
|
||
**observability/ergonomics gaps** (trigger executions invisible to `pic logs`; the `.trim()`/`.replace()`
|
||
return-`()` footgun) and the expected CLI-coverage gaps round it out.
|
||
|
||
---
|
||
|
||
## The app — "Stash" (built CLI-only, no raw admin curl)
|
||
|
||
A pastebin with file attachments and a background word-count worker. 9 scripts, 6 routes, a queue
|
||
consumer trigger, a dead-letter trigger, 2 API keys, 1 secret — all via `pic`.
|
||
|
||
| Feature exercised | How | Result |
|
||
|---|---|---|
|
||
| **kv** | paste store, hit counter, `/stats` aggregates, event log | ✅ get/set/list round-trip |
|
||
| **files** | `files::create` from base64 blob + `get`/`head`, two-param download route | ✅ bytes round-trip (`"hello attachment bytes"`, size 22) |
|
||
| **queue** | `queue::enqueue("ingest")` + `create-from-json --kind queue` consumer | ✅ worker fired |
|
||
| **invoke** | worker → `invoke("analyzer", #{text})` function-to-function | ✅ returns word count |
|
||
| **secrets** | `pic secrets set` (stdin) + `secrets::get` to gate `/stats` | ✅ 401 w/o token, 200 with |
|
||
| **async route** | `routes create --dispatch async` for `POST /events` | ✅ **202** + execution_id |
|
||
| **dead-letters** | poison job → 3 attempts → DL row; `pic dead-letters replay` | ✅ captured + replayed (+9 words) |
|
||
| **api-keys** | `pic api-keys mint --scope … [--app …]` | ✅ minted, scopes enforced (below) |
|
||
| **two-param route** | `/pastes/:code/files/:fid` | ✅ both params captured |
|
||
|
||
End-to-end stats after the run: `{total_pastes: 2, total_words: 13}` — proving queue → worker →
|
||
invoke → kv all fired on real Postgres rows + on-disk file bytes.
|
||
|
||
---
|
||
|
||
## Security matrix (10 probes, all reproduced live)
|
||
|
||
| # | Probe | Verdict | Evidence |
|
||
|---|---|---|---|
|
||
| **S1** | Cross-app isolation | ✅ HOLDS | A script in app `evil` doing `kv…list()` / `secrets::get("admin-token")` / `files…list()` returned `{paste_keys:[], attachment_count:0, stolen_secret:null, my_secret_names:[]}`. `app_id` derives from `cx.app_id`; no SDK call takes a script-passed app_id. |
|
||
| **S2** | Secret confidentiality | ✅ HOLDS | `pic secrets ls` shows names only; the plaintext `sup3r-s3cret-admin` does **not** appear anywhere in the server log even though `/stats` reads it each request. |
|
||
| **S3** | API-key scope + app-binding | ✅ HOLDS | `script:read` key → **403** on `POST /apps` and `GET /admins`, **200** on in-scope `GET /scripts`. `app:admin` key bound to `stash` → **403** on app `evil`. `instance:admin` + `--app` → **422**. Unknown scope `bogus:scope` → rejected by CLI. |
|
||
| **S4** | Sandbox op-budget | ✅ HOLDS | `loop {}` → **HTTP 507 "execution exceeded operation budget"** in **0.30 s**; `/healthz` still `ok` (no hang). |
|
||
| **S5** | Value/size caps | ✅ HOLDS | Oversized kv write rejected (HTTP 502). Notably the detail (`"Length of string too large"`) was **scrubbed from the response** and logged with `correlation_id` — good error hygiene. Caps are checked before authz, so anonymous callers can't DoS. |
|
||
| **S6** | Reserved-path prefixes | ⚠️ **GAP (Low)** | `/api/x`, `/admin/x`, `/healthz`, `/version` → correctly **422**. But case variants `/API/v2/x`, `/Admin/x`, `/HEALTHZ` were **accepted**. See finding below. |
|
||
| **S7** | Files path traversal | ✅ HOLDS | `files::collection("../../../etc")` → `"invalid collection name: must not contain '/', '\\', '..', or NUL"`; a non-UUID file id → not found. No FS escape. |
|
||
| **S8** | docs filter injection | ✅ HOLDS | `docs::find(#{ "x'); DROP TABLE docs;--": 1 })` → ran safely, `count=0`; the `docs` table still exists (`to_regclass` = true). Values/paths are bound as `$N` params. |
|
||
| **S9** | SSRF via `http::` | ✅ HOLDS | `http::get("http://127.0.0.1:8099/healthz")` → `"blocked by SSRF policy: loopback"`; `http://169.254.169.254/…` (cloud metadata) → `"blocked by SSRF policy: link-local"`. |
|
||
| **S10** | Anonymous data-plane access | ℹ️ By design (caveat) | Every Stash route is public (no auth) yet freely uses kv/secrets/files/queue. `script_gate` returns `Ok` when `principal.is_none()` — the *script* is the gate. Correct per design, but a sharp threat-model edge (see note). |
|
||
|
||
### S6 — reserved-path validation is case-sensitive (Low)
|
||
**What:** route-creation rejects the exact reserved prefixes but accepts case variants:
|
||
```
|
||
pic routes create --path /api/x -> 422 "path '/api/x' is reserved"
|
||
pic routes create --path /API/v2/x -> Created route … (GET * /API/v2/x) # accepted
|
||
pic routes create --path /HEALTHZ -> Created route … # accepted
|
||
pic routes create --path /Admin/x -> Created route … # accepted
|
||
```
|
||
**Current impact — Low, *not* a full bypass:** path matching is case-*sensitive*, so a request to
|
||
the real lowercase `/api/v2/x` still 404s (the system namespace is safe); only the exact
|
||
mixed-case path serves the script (`GET /API/v2/x` → 200, `/HEALTHZ` → 200). Real `/healthz`
|
||
still returns `ok` from the top-level handler.
|
||
**Why it still matters:** (1) inconsistent enforcement of a stated security boundary; (2) lets a
|
||
tenant publish convincing look-alike paths (`/Admin/login`, `/API/v1/…`) for phishing or log
|
||
confusion; (3) **fragile** — method matching is already case-insensitive (`matcher.rs:275`); if
|
||
path matching is ever made case-insensitive too, this instantly becomes a real shadowing bypass
|
||
of `/api/`, `/admin/`, `/healthz`. The validation should be the durable guard.
|
||
**Fix:** normalize case before the reserved check in `orchestrator-core/src/routing/pattern.rs`
|
||
(`check_reserved` ~line 110) — compare `raw.to_ascii_lowercase()` against the reserved list (or
|
||
reject any case-insensitive match).
|
||
|
||
### S10 — anonymous public scripts have full app data-plane access (design caveat)
|
||
Not a bug, but worth surfacing for users: a *public* (unauthenticated) route's script can read/write
|
||
**all** of the app's kv, docs, files, secrets, and queue — the platform only gates *authenticated*
|
||
principals; for anonymous ingress the script itself must enforce access. A developer who assumes
|
||
"this route is public" ≠ "this code can read every secret in my app" could over-expose data. Worth
|
||
a prominent doc callout near the SDK auth model.
|
||
|
||
---
|
||
|
||
## Feature / CLI gaps
|
||
|
||
- **G1 — trigger executions are invisible to `pic logs`.** After successful queue-worker and
|
||
analyzer runs, `pic logs <worker>` and `pic logs <analyzer>` were **empty**. Trigger-dispatched
|
||
executions (queue/cron/dead-letter/invoke) don't surface in the per-script log tail — the only
|
||
built-in visibility into worker activity is **dead-letters (failures only)** or the script's own
|
||
side effects. This is a real observability gap for background workloads. *Suggest: include
|
||
trigger executions in the logs surface, or add a `pic logs --trigger`/events view.*
|
||
- **G2 — no `pic kv` / `files` / `queues` / `members` commands.** Data-plane stores are
|
||
script/HTTP-only (no admin CLI to inspect kv/files/queue contents; queues are read-only HTTP).
|
||
App **membership** (`/apps/{id}/members`) also has no CLI — multi-user app roles can't be managed
|
||
with `pic`. *(Confirmed absent via `pic <cmd> --help`.)*
|
||
- **G3 — no per-script sandbox/timeout flags in `pic deploy`/`scripts`.** `timeout_seconds`,
|
||
`memory_limit_mb`, and sandbox overrides are only settable via the raw scripts API or instance
|
||
env (`PICLOUD_SANDBOX_MAX_*`). A developer can't cap a single script's runtime from the CLI.
|
||
- **G4 — uneven trigger wrappers.** Only `kv`, `cron`, `dead-letter` have first-class
|
||
`pic triggers create-*` wrappers; `docs`/`files`/`pubsub`/`queue`/`email` require hand-built
|
||
`create-from-json --kind … --body '{…}'`. Works, but the developer must know each body schema.
|
||
- **G5 — `email::send` unusable in dev.** Returns `"email is not configured: set
|
||
PICLOUD_SMTP_HOST/USER/PASSWORD"`. Expected (no silent drop), but the email feature can't be
|
||
exercised at all without an SMTP relay — worth a documented local-dev fake/sink.
|
||
- **G6 — Rhai `.trim()` returns `()` (in-place mutation footgun).** Same family as `.replace()`
|
||
(already documented). `let t = text.trim()` sets `t` to unit; my analyzer dead-lettered with
|
||
`Function not found: split((), …)` until rewritten to `text.trim();` as a statement. *Suggest:
|
||
extend the existing stdlib footgun note to list `trim`/`to_upper`/`to_lower` etc., not just
|
||
`replace`.*
|
||
|
||
## Things done especially well (no action)
|
||
- Cross-app isolation has no script-controlled `app_id` anywhere — the boundary is structural.
|
||
- SSRF guard covers loopback **and** link-local (cloud-metadata `169.254.169.254`), and blocks at
|
||
every redirect hop.
|
||
- Size/operation caps are enforced **before** authz, so anonymous public scripts can't DoS the DB.
|
||
- Script runtime errors are **scrubbed** from the HTTP response and logged with a `correlation_id`
|
||
— no internal detail leaks to the caller.
|
||
- `deploy`/`apps create` honor `--output json` (the earlier F1 fix), so the whole build scripts
|
||
cleanly with captured ids.
|
||
|
||
## Reproduction / teardown
|
||
Scripts under `/tmp/stash/*.rhai`; apps `stash` + `evil` and their data persist in the dev
|
||
Postgres. Server: `target/debug/picloud` on `:8099` (see Phase 0 env). Teardown: kill the host
|
||
`picloud`; `docker compose down [-v]`. (Note: the S6 probe left harmless `/API/v2/x`, `/HEALTHZ`,
|
||
`/Admin/x`, `/Api/y` routes on `stash` — they vanish with the app.)
|
||
|
||
## Priority
|
||
**S6** is the only code-level finding (Low — confirm-and-harden the reserved-path check). **G1**
|
||
(trigger-execution observability) is the most impactful *developer-experience* gap for anyone
|
||
running background workers. Everything else is documented-behavior or known CLI-coverage gaps.
|