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>
Addresses findings from an independent review of the gap-closing commits:
- R1 (regression): add #[serde(default)] to HttpDispatchPayload.method so
async-HTTP outbox rows enqueued before the field existed still decode
after upgrade instead of dead-lettering on "missing field `method`".
Adds a regression test for the missing-key path.
- method case: uppercase ctx.request.method at the orchestrator boundary
so it honors its documented "uppercased" contract for extension verbs.
Route matching is already case-insensitive, so matching is unaffected.
- CLI hardening: percent-encode free-form path segments (app slug, topic
name, ids, secret name) in the reqwest client so a value containing
`/ ? #` can't break out of its URL segment. Adds a `seg()` helper +
unit test and applies it uniformly across all path-interpolating
methods (new and pre-existing).
- S1 (docs): correct the users::email_available enumeration framing — it
adds no new vector vs. create's uniqueness error, but is cheaper and
unthrottled; there is no built-in throttle/CAPTCHA primitive, so the
honest mitigation is a kv-based counter. Updated SDK doc-comments,
trait docs, and stdlib-reference.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`users::find_by_email` requires an authenticated principal (F-S-003,
anti-enumeration), so the natural check-then-create register pattern 502s
for anonymous public scripts. Add `users::email_available(email) -> bool`,
gated like `create` (the registration write path) but without the
anonymous rejection: it leaks only a single boolean — no more than
`create`'s own uniqueness error already does — so self-serve register
scripts can pre-check. Also document find_by_email's principal
requirement in the SDK doc-comment.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The request map exposed path/headers/body/params/query/rest but not the
HTTP method, forcing one script per verb (the E2E app needed 7 scripts
where ~3 would do). Add a `method` field to ExecRequest (populated from
the HTTP method on the sync-execute bypass and the HTTP outbox payload;
empty for non-HTTP triggers) and surface it as `ctx.request.method`,
uppercased. A single script can now branch GET/POST on one route.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A CLI-only walkthrough (E2E_TODO_REPORT.md) found two control-plane
operations with no `pic` command — every CLI-created app 404'd until it
claimed a domain, and external SSE feeds needed a raw topic-registration
call — plus several friction points. All admin APIs already existed; this
adds thin wrappers mirroring `routes`/`triggers`:
- B1: `pic apps domains {ls,add,rm}` over apps/{id}/domains.
- B2: `pic topics {ls,create,update,rm}` over apps/{id}/topics, and a
triggers help note distinguishing a pubsub trigger from topic
registration.
- F2: `pic users {ls,show,reset-password,revoke-sessions}` over the app
end-user admin surface (read + the two admin actions; create/invitations
deferred).
- F1: `apps create` and `deploy` now honor `--output json`, emitting the
created object so scripts can capture the id.
- F3: non-interactive login via `--username` + `--password-stdin` (inline
passwords still rejected, mirroring the `--token` rule).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review nit — the field is a non-optional Arc; the old '`None` for tests'
note was stale. Clarify that it's one shared Arc so revocation-side
evictions are visible to the middleware's resolve-side lookup.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
extract_remote_ip took the FIRST X-Forwarded-For entry, but Caddy (the
single trusted proxy) appends the real peer as the LAST hop — earlier
entries are client-supplied. An attacker could prepend a rotating
X-Forwarded-For value to get a fresh per-IP bucket every request and
evade the per-IP login limiter (the per-username bucket + global Argon2
semaphore still bounded it, but the per-IP layer was defeated). Take the
last hop so the key reflects the real client. Added unit tests covering
the spoof, the single-hop case, and the missing-header fallback.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pic admins create/set --password and pic login --token accepted secrets
on argv, where they leak into shell history, ps aux, and
/proc/<pid>/cmdline. Now only --password -/--token - (stdin) is allowed;
an inline value is rejected with guidance. PICLOUD_TOKEN env still covers
CI for login.
Also (M2): the interactive admins password prompt used read_line, which
echoes despite a 'no echo' claim — switched to rpassword::prompt_password
(already a dep via login). Removed the now-dead ReadLine trait + Write
import.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Caddy request_body { max_size 12MB } in both Caddyfiles — a hard
ceiling replacing Caddy's multi-GB default; 12MB clears the
orchestrator's 10 MiB user-route read. Validated with caddy validate.
- email-inbound router gets an explicit DefaultBodyLimit::max(1MB):
the public unauthenticated webhook previously rode Axum's 2MB
extractor default; tightened so a flood can't force large
allocation + JSON parse per request.
Admin / execute handlers keep Axum's 2MB extractor default (already
bounded); the user-route path keeps its explicit 10 MiB manual read.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The admin files endpoints hit FsFilesRepo directly (not via
FilesServiceImpl, which validates), so only create/update guarded the
collection — head/get/list/delete built FS paths from an unvalidated
value. Today nothing can store a traversal-shaped collection, but one
future bad migration / restore tool inserting collection='../../etc'
would give the read/delete paths arbitrary host-file reach.
- FsFilesRepo::{head,get,list,delete} now call guard_collection (create
and update already did).
- The three admin endpoints (list/get/delete) call
validate_files_collection up front for a clean 422 instead of the
repo guard surfacing as an opaque 500.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ExecError::Runtime strings (filesystem paths, "blocked by SSRF policy:
link-local" cloud-metadata reconnaissance, pool-exhaustion timing,
panic fragments, and 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 log the full text
server-side under a correlation id and return a stable generic message
plus the id for operator grep.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
C-2 follow-up: sanitize_stored_content_type previously let a CRLF pass
through an allowlisted prefix (e.g. "image/png\r\nX-Injected: 1" matched
the image/ branch and returned the original), which would inject a
response header / panic HeaderValue on download. Now any control byte
(<0x20 or 0x7f) coerces to application/octet-stream up front.
F-SE-H-03: disable_symbol("debug") to match "print" (the comment
already claimed both were disabled) so scripts can't write
attacker-controlled bytes to the operator's stderr.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The PrincipalCache (token-hash -> resolved Principal, 60s TTL) had no
eviction hook, so deactivation / password change / API-key revocation
flipped the DB rows but the cache kept serving the stale principal for
up to the TTL window. Closes the audit's PrincipalCache revocation-lag
Medium and greens authz::deactivating_user_revokes_their_api_keys.
- PrincipalCache::evict_user(user_id) + evict_token(hash).
- One shared cache Arc threaded into AuthState / AdminsState /
ApiKeysState (a per-state cache would let the middleware's own copy
keep authenticating a revoked principal).
- Evict on: deactivation, password change (both evict_user), logout
(evict_token, precise), single API-key delete (evict_user).
- H-E1 note: DB-write invalidation stays best-effort by design (a blip
must not undo the rotation); the cache evict is what makes it take
effect on the next request.
- Renamed bearer_and_cookie_produce_same_principal ->
session_token_and_api_key_produce_same_principal (cookie auth was
removed in C-1; the test never tested cookies).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Postgres-backed integration tests surfaced two assertion fixes while
verifying the audit branch:
* email_inbound::create_without_inbound_secret_is_rejected (added in the
H-B2 commit) asserted 400; TriggersApiError::Invalid maps to 422
(UNPROCESSABLE_ENTITY). Corrected.
* api::version_includes_public_base_url had two rotted assertions —
`schema` pinned at 6 (it's migrations::latest_version(), which had
climbed to 41 and is now 42 after 0042_secrets_envelope_version.sql)
and `sdk` pinned at "1.1" (the crate is at "1.10"). The test is
#[ignore]-gated so the rot went unnoticed in normal runs. Both pinned
to current reality. These were pre-existing failures, not caused by
the security changes.
Verified against a throwaway Postgres: schema_snapshot, api (53),
authz (28 of 29 — see below), dispatcher_e2e, email_inbound, invoke_e2e,
queue_e2e all green.
Known pre-existing failure (NOT fixed here, out of Tier 1+2 scope):
authz::deactivating_user_revokes_their_api_keys fails identically on the
pre-branch merge-base. It's the PrincipalCache revocation-lag Medium the
audit flagged as unfixed (resolve_principal caches by token-hash with no
eviction on deactivation; lag bounded by the 60s TTL). Deferred to a
Tier 3/4 pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
At-rest secrets were AES-256-GCM sealed with no Associated
Authentication Data, so anyone with Postgres write access could
ciphertext-swap rows across apps (or rename via row edit) and the
decrypt would silently succeed under the wrong identity, returning
attacker-chosen plaintext. This breaks the cross-app isolation boundary
the moment DB write access is achieved.
Adds an AAD-bound envelope (v1) alongside the legacy no-AAD layout (v0),
discriminated by a per-row `version` column:
* shared::crypto — new encrypt_with_aad / decrypt_with_aad using
aes_gcm::aead::Payload { msg, aad }. Originals retained for v0 reads.
Tests: AAD round-trip, AAD-mismatch fails, empty-AAD round-trip.
* migration 0042 — adds `secrets.version SMALLINT NOT NULL DEFAULT 0`
and `app_secrets.realtime_signing_key_version SMALLINT NOT NULL
DEFAULT 0`. Existing rows stay v0; new writes are v1.
* secrets (SDK + admin API) — seal() now binds AAD =
"secret:{app_id}:{name}" and emits v1; open() dispatches on version.
StoredSecret gains a `version` field; SecretsRepo::set takes it.
Both secrets_service::set and secrets_api::set_secret go through the
v1 path. Tests prove a cross-app swap and a cross-name swap both
surface Corrupted, and that a hand-built v0 row still decrypts.
* app_secrets (realtime signing key) — get_or_create_signing_key writes
v1 with AAD = "app_secret:{app_id}:realtime_signing_key"; decode
dispatches on version. Tests cover v0 decode, v1 round-trip, and v1
decode-under-wrong-app failing.
* email-trigger inbound secret — kept on v0 (seal_legacy/open_legacy)
and explicitly deferred: email_trigger_details has no version column
and the trigger_id isn't known at seal time. The audit classes the
email-trigger AAD gap as Medium; folded into v1.2's key-versioning
pass per SECURITY_AUDIT.md.
* expected_schema.txt re-blessed by hand (no local Postgres) for the two
new columns + migration 0042.
Also folds in a let-else clippy fix in auth_api.rs (login Argon2
semaphore acquire, from the H-B1 commit) and two cargo-fmt reflows.
No re-encryption sweep — v0 rows decrypt as-is; the sweep is deferred to
v1.2's key-versioning pass (audit "Notes on remediation methodology").
Audit ref: security_audit/03_crypto_secrets.md (H-D1).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Rhai SDK email::send path went straight to the SMTP relay with no
admission control. EmailRateLimiter lived in users_service.rs (gating
verification + password-reset emails) but was unreachable from the SDK
surface. Any anonymous-callable HTTP-route script could loop email::send
to burn the operator's SMTP quota or BCC-bomb thousands of recipients
per request.
Two new caps on EmailServiceImpl, both fire BEFORE message assembly +
SMTP connect:
1. Per-message recipient cap (to+cc+bcc combined). Default 20, override
with PICLOUD_EMAIL_MAX_RECIPIENTS. New EmailError::TooManyRecipients
variant. Closes the BCC-bomb amplification path.
2. EmailRateLimiter inlined into EmailServiceImpl:
* Per-(app, recipient): RECIPIENT_BURST=5 / RECIPIENT_WINDOW=60min.
* Per-app daily: APP_DAILY_CAP=200 / 24h.
The structure mirrors users_service's private limiter; defense-in-
depth redundancy is fine since the gates are identical and never
conflict. New EmailError::RateLimited(&'static str) variant; the
string names which bucket tripped so the operator can act.
users_service::map_email_error grows two cases (mapped to
EmailTransport for the script-facing error shape).
Tests:
* too_many_recipients_rejected — 30 recipients, default cap 20, rejected
with TooManyRecipients.
* per_recipient_burst_caps_repeated_send_to_same_address — 6 sends to
the same address; 6th returns RateLimited("per-recipient burst").
Audit ref: security_audit/09_external_integrations.md#f-ext-h-001.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two gaps:
1. /api/v1/admin/apps/{id}/triggers/email accepted `inbound_secret: null`
and treated it as "this trigger is unsigned". The runtime then
skipped HMAC verification entirely. URL discovery (logs, ops
dashboards, bruteforce on the 122-bit TriggerId space) then fan-out-
amplified into the outbox for free — anonymous DoS via the
dispatcher.
2. Even signed triggers had no per-trigger rate limit on signature-
verify failures, so an attacker who knew the URL could pump unsigned
POSTs to force Argon2-equivalent work (HMAC verify + nonce-dedup +
stored-secret decrypt) at line rate.
Fix:
* triggers_api::create_email_trigger now requires a non-empty
inbound_secret. 400 on missing / empty / whitespace-only.
* email_inbound_api::receive_inbound_email returns 401 immediately when
the resolved target has no inbound_secret_encrypted column (pre-fix
rows). The previous `if let Some(ct), Some(nonce)` branch is gone.
* New `BadSignatureLimiter`: per-`(app_id, trigger_id)` sliding-window
bucket (10 fails / 60 s). On lockout the receiver returns 429 with
Retry-After instead of 401. record_failure runs on both the
"no-secret" 401 path and any verify_signature failure.
Test fallout:
* email_inbound integration tests that relied on None secret: removed
the now-impossible `unsigned_trigger_accepts_without_signature` test,
replaced with `create_without_inbound_secret_is_rejected` covering
null / "" / " "; updated `malformed_body_is_422` and
`cross_app_path_is_404` to pass + sign with a secret.
* Two new unit tests pin the limiter behavior (burst lockout + per-
trigger independence).
Audit refs: security_audit/08_dos_resource.md#h-3,
security_audit/09_external_integrations.md#f-ext-m-001.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The login handler had no admission control before
`spawn_blocking(verify_password)`. On Pi-class hardware Argon2id is ~50-
100 ms per attempt, so a few dozen concurrent anonymous POSTs to
/auth/login park every blocking worker, wedging the entire admin API
and any other path that uses `spawn_blocking`.
Adds two cheap, in-process guards modeled on EmailRateLimiter:
* `LoginRateLimiter` — two sliding-window token buckets, one per
`(remote_ip, username)` (burst 5/60s) and one per `username`
(burst 10/15min). Per-(ip, user) defeats a single attacker pounding
one account; per-user defeats credential-stuffing distributed across
many IPs. Username is case-folded so case variants share a bucket.
Maps GC lazily when they cross a soft size cap.
* Per-process `tokio::sync::Semaphore` — caps concurrent Argon2 verifies
during login. Default 2 permits (Pi-class), overridable via
`PICLOUD_LOGIN_ARGON2_PARALLELISM`. Acquired AFTER the bucket check so
attackers can't queue.
Limit check fires before the DB credentials lookup, so even an unknown
username doesn't cost a query. Denied attempts return 429 + Retry-After.
Real client IP comes from the first X-Forwarded-For entry (Caddy is the
trusted single hop); falls back to "unknown" so the per-user bucket
still gates.
4 unit tests cover bucket burst exhaustion, per-user crossing IPs, case
folding, and per-user independence.
Audit ref: security_audit/08_dos_resource.md (H-2 / H-B1).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
build_invoke_request already asserts `script.app_id == row.app_id`
(dispatcher.rs:752) before constructing an ExecRequest. The queue
dispatcher (`dispatch_one_queue`) and the HTTP outbox arm
(`build_http_request`) did not. validate_trigger_target blocks cross-
app registration at write time, so the gap was latent; but a hand-
edited row, a partial backup restore, or a script re-pointed across
apps post-hoc could otherwise execute one app's script under another
app's SdkCallCx, breaking the cross-app isolation boundary the SDK
relies on.
Adds the runtime guard at both sites:
* dispatch_one_queue — on mismatch, dead-letter the message so the
misfire is observable rather than silently dropped, and short-
circuit.
* build_http_request — on mismatch, return ResolveTrigger error; the
outer dispatch arm logs + drops the row (same path as decode
failures).
Audit ref: security_audit/02_authz_isolation.md (H-F1).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PATCH /api/v1/admin/admins/{id} handler's password branch updated
the password hash but explicitly preserved every live session and API
key for the target user. That made password rotation a non-event for
credential compromise: a hijacked session that triggered a password
change kept its grip after the rotation, and a leaked API key survived
its owner's "I'm rotating because I think I'm compromised" reaction.
Now the password branch mirrors the deactivation branch:
1. update the password hash (unchanged),
2. `state.sessions.delete_for_user(id)` — wipes every live session
including the caller's, which logs them out and forces re-login
under the new password,
3. `state.keys.expire_all_for_user(id)` — revokes every API key.
This matches the `cmd_reset_password` CLI flow already in the codebase.
The dashboard's 401 handler redirects to the login screen, so the UX
on self-change is "you're logged out; sign back in".
Audit ref: security_audit/01_authn_session.md (H-E1).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tokio::time::timeout over JoinHandle only drops the future — the
spawn_blocking OS thread keeps running until the Rhai script self-
completes or hits the per-op budget. A `loop {}` body with a generous
max_operations could pin a blocking worker for tens of seconds; on Pi-
class hardware one anonymous-callable script call could permanently
subtract a worker from the pool.
Installs an `engine.on_progress` hook that consults a thread-local
deadline; when `Instant::now() >= deadline` the hook returns `Some(_)`,
triggering `ErrorTerminated` inside the Rhai loop. The deadline is set
by `Engine::execute_with_deadline` / `Engine::execute_ast_with_deadline`
via an RAII `DeadlineGuard`, called from the orchestrator client; the
old `execute` / `execute_ast` paths are unchanged so tests, validation,
and the parse-only path keep working.
Invoke re-entry (`sdk/invoke.rs`) inherits the parent's deadline
transparently — the thread-local is set for the entire spawn_blocking
lifetime, and Rhai is single-threaded so the same thread services the
parent + every sub-script.
New tests:
* `deadline_terminates_a_runaway_loop` — `loop {}` body with
`max_operations = u64::MAX` and a 100 ms deadline aborts within 2 s
(typically <200 ms). Maps to `ExecError::Runtime`.
* `no_deadline_set_does_not_abort` — `None` deadline keeps the pre-
audit behavior; the op-budget still bites.
Audit ref: security_audit/05_sandbox_exec.md#f-se-h-01.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stored XSS: the previous get_file handler streamed user-supplied bytes
with Content-Disposition: inline, the user-supplied Content-Type, and
no X-Content-Type-Options / CSP. A Rhai script could store an SVG or
HTML payload whose download URL rendered same-origin under the admin
session cookie.
Closes the response side and the storage side:
* shared::sanitize_stored_content_type: allowlist
(octet-stream/pdf/json/text-plain/text-csv/image-non-svg/audio/video)
with anything else coerced to application/octet-stream. New unit tests
cover the safe/unsafe/case-insensitive/parameter-preserving paths.
* files_service::create/update: sanitize the stored content_type after
the shape checks pass (sanitize-after-validate keeps the existing
MissingField / TooLong errors intact). Two new tests confirm text/html
and image/svg+xml are coerced to application/octet-stream on
create/update respectively.
* files_api::get_file (admin download):
- Content-Disposition: attachment (was inline)
- Content-Type re-sanitized via the shared helper as belt-and-
suspenders for any pre-existing row that pre-dates this change.
- X-Content-Type-Options: nosniff
- Content-Security-Policy: default-src 'none'; sandbox;
frame-ancestors 'none'
- Referrer-Policy: no-referrer
Audit ref: security_audit/07_http_cors_csrf_xss.md#c07-02 (response side)
+ security_audit/06_files_pathtraversal.md#f-fs-001 (storage side).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same-origin CSRF: the platform co-hosts user-route HTML on the same
origin as /api/v1/admin/*; SameSite=Lax did not block a same-origin POST
from a malicious script at /<route>, riding the admin's picloud_session
cookie. The dashboard already uses Bearer (dashboard/src/lib/auth.ts +
api.ts:495), so cookie auth was dead weight on the admin side and
exploitable.
Cuts the cookie path entirely:
- auth_middleware::extract_token is Bearer-only; cookie branch removed.
- auth_api::login no longer sets Set-Cookie.
- auth_api::logout no longer clears the cookie (the bearer token is
still revoked by deleting the session row).
- extract_token_for_logout matches.
- SESSION_COOKIE const + PICLOUD_COOKIE_SECURE env var deleted.
Audit ref: security_audit/07_http_cors_csrf_xss.md#c07-01.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stage 6 review Obs 1b: the 300s default lived as a literal in two
places (DEFAULT_ASYNC_EXEC_TIMEOUT in dispatcher.rs and
DEFAULT_SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS in triggers_api.rs). A
future change to the dispatcher would silently leave the
visibility-timeout warn threshold out of sync.
Extract a single pub const DEFAULT_ASYNC_EXEC_TIMEOUT_SECS: u32 = 300
in dispatcher.rs, derive the existing Duration constant from it, and
re-export it under the triggers_api name so the validator's
self-contained semantics are preserved at the call site. Tests still
inject the value explicitly via TEST_SAFE_LIMIT — no behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stage 6 follow-up Obs 1: the hard-coded
SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS = 300 didn't follow
PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC. A deploy that raised the
executor budget produced false-positive warns; a deploy that lowered
it missed real visibility races.
Refactor validate_queue_visibility_timeout to take safe_limit_secs as
an explicit parameter. The call site reads the live env-overridable
value via safe_visibility_vs_exec_budget_secs() each call; unit tests
inject the boundary directly without touching global env state. New
test safe_limit_threshold_tracks_caller_value asserts both
budget-raised and budget-lowered code paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- F-T-003 actually closed: clients/typescript/src/subscribe.ts now
bounds the 401-refresh loop at 3 consecutive refusals, surfaces an
onError describing the loop, and resets on any successful stream
open. New test covers the cap. The Stage 2 dashboard fix only
addressed adminRequest in dashboard/src/lib/api.ts; the originally-
cited TS client file was untouched.
- users/invitations subtab dropped the redundant api.apps.get fetch
the other 5 subtabs already shed in Stage 6.
- Queue visibility-timeout validator pulled out as
validate_queue_visibility_timeout, with two thresholds: hard-reject
below MIN_QUEUE_VISIBILITY_TIMEOUT_SECS (30s — catches typos), warn-
log between MIN and SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS (300s) so
operators see when their visibility is below the dispatcher's
per-message executor budget. Stage 6 only had the hard floor; the
reviewer caught that a 60s handler still races a 30s visibility
even after the floor. Four new unit tests cover none/above-safe/
between/below-min.
- pic dead-letters count subcommand: cheap headless probe for
unresolved DL totals, parallels the dashboard's badge query.
- pic dead-letters replay now has a happy-path integration test
(replay_against_real_dl_row_succeeds): inserts a synthetic DL row
directly via the rust-postgres sync driver (added as dev-dep),
drives `pic dead-letters replay`, asserts the row is resolved with
reason=replayed and count drops back to 0. Plus a count smoke test.
All 75 CLI integration tests + 16 TS client tests + 4 new
visibility-timeout unit tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes 4 dashboard hardening findings and 5 of the Lows from the audit.
Dashboard hardening:
- Subtabs no longer re-fetch the app via api.apps.get on every page
load. users/files/dead-letters drop the fetch outright (the variable
was set but never read); queues + queues/[name] now consume the
layout's AppContext via getContext for the page title. The layout's
reloadApp() owns the historical-slug redirect — subtab-local redirect
blocks are removed so there's no race.
- The global :global(details > summary::before) chevron is now scoped
to details.chevron. The script editor's "Advanced sandbox" details
and the inbound-email-shape help-text both opt in; the script
exec-list logs no longer inherit a spurious chevron.
- deriveTab now matches the path segment by anchored ===, so a future
/apps/<slug>/queues-archived route wouldn't activate the queues tab.
Lows cherry-pick:
- ExecError gains Serialize/Deserialize derives + a snake_case tag so
RemoteExecutorClient (cluster mode v1.3+) can round-trip the variant.
- triggers_api rejects queue triggers whose visibility_timeout_secs is
below the dispatcher's per-message executor budget; with no minimum
the reclaim task races the handler and the queue silently
double-delivers. Existing test using 5s updated to 30s.
- New migration 0040: execution_logs.script_id cascade switched from
ON DELETE CASCADE to ON DELETE SET NULL so deleting a script no
longer wipes the forensic history that motivated the delete.
- New migration 0041: dead_letters composite index on
(app_id, created_at DESC) so the "list all" dashboard view stops
falling back to seqscan + sort when unresolved=false.
- Schema snapshot re-blessed.
Deferred to v1.2: the ExecRequest principal serde(skip) marker
(documented in-place; the cluster-mode PR will introduce the wire-safe
snapshot at that point) and the `pic --help` mention of
`picloud admin reset-password` (one-line follow-up).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the audit's High-severity CLI coverage gap, raising the count
from ~25 to ~40+ subcommands and bringing the integration test count
from 63 to 73.
- pic triggers {ls, rm, create-kv, create-cron, create-dead-letter,
create-from-json}: three per-kind wrappers cover the most common
trigger shapes; the generic create-from-json is the escape hatch
for docs/files/pubsub/email/queue and any future advanced retry
knobs — body JSON inline, via @<file>, or "-" for stdin.
- pic dead-letters {ls, show, replay, resolve}: full operator
workflow for the dispatcher's dead_letters rows, including the
--unresolved filter and the per-row replay + manual-resolve actions.
- pic secrets {ls, set, rm}: list names + updated_at, set values
via stdin (the only secure channel — inline values would leak
into shell history), and delete by name. --json on set treats
stdin as raw JSON for non-string values.
Ten new integration tests follow the established #[ignore] pattern,
gated on DATABASE_URL. All 73 ignored tests pass against the local
dev stack.
The `pic admin reset-password` server-binary subcommand exists on
the picloud binary side already; the audit's "surface it in pic
--help" note is a one-line addition deferred to Stage 6 with the
other small UX touches.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the audit's Critical "operator can't deploy a serverless endpoint
end-to-end from the CLI" finding. Two new subcommand families bring the
CLI coverage from 14 commands to ~25:
- pic routes {ls, create, rm, check, match}: full route CRUD plus the
dry-run conflict checker and the URL matcher already exposed by the
dashboard. The create form accepts --path-kind, --host-kind, and
--dispatch (sync|async) so async routes can finally be created
headlessly. The ls output adds a dispatch column.
- pic admins {ls, create, show, set, rm}: per-instance admin user
management. Create reads passwords from stdin via --password - so
shell history never sees the cleartext. Set is a JSON-Merge-Patch
shape that lets operators deactivate accounts or rotate roles
without touching the dashboard.
Six new ignored integration tests follow the established #[ignore]
pattern (DATABASE_URL gates them). They cover the happy-path round
trip, the async-dispatch persistence path, the password-required
error path, and the capability gate (a Member sees HTTP 403). All
pass against the local dev stack with PICLOUD_DEV_MODE=true +
PICLOUD_DEV_INSECURE_KEY=i-understand-this-is-insecure.
The `pic members` and `pic domains` subcommands the audit mentioned
are deferred — the apps_api shape may shift in v1.2 with per-app
roles and rebuilding the CLI surface twice would be churn.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the queue DL fan-out gap (audit Medium) and surfaces the
KV/docs/files non-transactional emit gap to operators.
- handle_queue_failure previously called queue.dead_letter to write the
DL row but never invoked fan_out_dead_letter. The comment claimed
"the outbox arm fires registered dead_letter handlers off the new row"
— but queue DL rows are written via a separate path that bypasses the
outbox entirely, so handlers filtered on source="queue" sat idle
forever. Refactor fan_out_dead_letter to take a DeadLetterFanOutCtx
struct so both the outbox arm and the queue arm can call it; the
queue arm constructs a TriggerEvent::Queue from the claimed message
and passes it through.
- New integration test queue_dead_letter_fans_out_to_dead_letter_handler
registers a dead_letter trigger filtered on "queue", forces queue
exhaustion, asserts the handler fires with the correctly-shaped event.
- KV/docs/files services committed the data write then ran events.emit
as a separate operation, logging-and-swallowing on failure. Bump the
six call sites from tracing::warn to tracing::error with an
event_emit_failure=true marker so operators can grep them. The full
single-tx repo refactor (extending ServiceEventEmitter with
emit_in_tx + tx-aware *_repo methods) is documented in kv_service.rs
as a v1.2 follow-up — it's a meaningful redesign that deserves its
own pass (pubsub_service::fan_out_publish is the reference shape).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes 4 audit findings:
- Route TS interface now carries dispatch_mode (sync|async). Backend's
shared::route::Route has had this since 0012_routes_dispatch_mode but
the TS client silently always created sync routes and the list
display dropped the field. Add a Dispatch select to the new-route
form and an "ASYNC" badge in the route list.
- api.files.downloadUrl pointed to a never-registered backend endpoint.
The dashboard's live Download button was hitting 405. Add the GET
handler (AppFilesRead + FilesRepo::head + FilesRepo::get, Content-
Disposition: inline) at the same path that delete already used.
- F-T-003: adminRequest's 401 handler called goto(login) without a
recursion cap. If the login endpoint itself returned 401, the wrapper
looped until browser nav limits. Track consecutive 401s within a 10s
window and hard-reload to /login?reason=auth-loop after the third,
showing the user an explanatory banner. Any 2xx resets the counter.
- Login flow now honors a ?returnTo= query parameter. adminRequest and
the root layout both append the current location when redirecting to
/login; the login page validates the value is a same-origin admin
path (no open-redirect) and goto's there after successful sign-in.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes 4 High-severity audit findings:
- describe_event broken for HTTP-async payloads: was reading source/op
JSON fields that HttpDispatchPayload doesn't carry, producing empty
source on DL rows. from_wire("") then fell back to Kv on replay, the
dispatcher tried to resolve a non-existent trigger, and replay no-op'd
silently. Now branches on OutboxSourceKind: HTTP rows emit
"<method> <path>" + source="http"; Invoke rows emit "invoke_async" +
source="invoke"; TriggerEvent payloads keep top-level field extraction.
- HTTP outbound Authorization leaked across cross-origin redirects.
Manual redirect loop (Policy::none) reused header_map on every hop
and only scrubbed Content-Type for POST->GET. Now compares
url::Url::origin() across hops and strips Authorization,
Proxy-Authorization, Cookie when origin changes — matching reqwest's
default policy.
- F-S-010 inbound email HMAC had no replay protection. Signature was
computed over body only with no timestamp or nonce, so captured POSTs
were replayable indefinitely. Adds X-Picloud-Timestamp header bound
into the HMAC input (signed string is ts || "." || body), 300s
tolerance window, and a process-local LRU keyed on
(ts, sha256(body)) with 600s TTL to catch within-window replays.
Breaking change: webhook senders must include the timestamp header.
- expected_schema.txt re-blessed for migrations 0036-0039 (the previous
snapshot stopped at 0035 so CI would have gone red on first run).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses every finding from the four-agent review of commit aa493b9.
Dashboard — honor redirect_to on subtab loadApp() (closes the silent
historical-slug redirect UX gap):
- queues/+page.svelte
- queues/[name]/+page.svelte
- files/+page.svelte
- dead-letters/+page.svelte
After a rename, the URL bar now reflects the canonical slug instead
of silently rendering the renamed app's data under the stale URL.
Mirrors the established pattern at apps/[slug]/+page.svelte:619-623.
manager-core:
- queues_api.rs IntoResponse now uses the JSON envelope shape
`{"error": "..."}` consistent with every sibling admin api file.
- triggers_api::delete_trigger reordered: cap check fires BEFORE the
trigger load, closing the 404-vs-403 existence side channel an
unauthorized caller could otherwise probe.
- InMemoryAppRepo mocks in topics_api + triggers_api now implement
get_by_slug + get_by_slug_or_history (previously
`unimplemented!()`), unblocking handler-level slug-input tests.
- Added 4 slug-acceptance tests to topics_api and 2 to triggers_api
(slug-resolves, unknown-slug-404, historical-slug-resolves,
create-via-slug). Also added delete-without-cap-is-forbidden test
pinning the new cap-first order.
e2e:
- navigation/tabs.spec.ts split per-tab so a regression on one tab
no longer masks regressions on the others.
- Negative assertion widened: captures every /api/v1/admin/apps/*
response and fails on any 4xx/5xx — not just the literal "Cannot
parse" string. Catches a broader regression shape.
- networkidle replaced with `expect(<main>).toBeVisible()` —
networkidle is officially discouraged for SPAs and was at risk of
timing out behind the queues auto-refresh.
- Cleanup registration moved BEFORE the create-app API call so a
flaky create still gets swept up.
- Queue drilldown route /queues/[name] now covered.
- Stable `data-testid="queues-empty-state"` replaces fragile
UI-copy substring match for the positive assertion.
- Header comment now spells out what this spec does and doesn't
catch.
docs:
- serverless_cloud_blueprint.md: slug-history described as
"200 OK + redirect_to" JSON envelope rather than "301 redirect"
— matches what apps_api actually implements (SPA can't honor a
mid-tree HTTP redirect).
Unit-test gap (acknowledged): queues_api, files_api, secrets_api,
dead_letters_api have zero in-process tests. Adding them properly
needs a shared mock-repo helper crate — the standalone trait surface
(QueueRepo + TriggerRepo + ScriptRepository + AuthzRepo + repo-
specific) is ~30 methods per file. Documented inline in queues_api.rs
near the resolver. Integration coverage via crates/picloud/tests/ and
the new e2e spec cover the same paths end-to-end.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Queues tab at /admin/apps/default/queues was returning
"Cannot parse `app_id` with value `default`: UUID parsing failed".
27 admin endpoints across 6 files used strict `Path<AppId>` instead
of the canonical `Path<String>` + `resolve_app()` pattern from
app_repo.rs:34. Working endpoints (apps, app_members, users_admin)
all use the lenient pattern; this commit brings the remaining 6
files into line:
- queues_api.rs — 2 handlers
- files_api.rs — 2 handlers
- secrets_api.rs — 3 handlers
- topics_api.rs — 4 handlers
- dead_letters_api.rs — 5 handlers
- triggers_api.rs — 10 handlers
The handler bodies (authz, repo calls) are unchanged; only the path
extractor and the per-file `ensure_app_exists` helper (now renamed
`resolve_app`) move. Lib tests updated to pass `.to_string()` at
the call site (Path now takes String, not AppId).
email_inbound_api.rs deliberately stays strict-UUID — it's a public
webhook receiver consumed by external providers, not by the
slug-based dashboard.
Adds Playwright spec `dashboard/tests/e2e/navigation/tabs.spec.ts`
covering every per-app tab (queues, files, dead-letters, users,
invitations, plus the main page hosting triggers/secrets/topics)
with a negative assertion against the "Cannot parse" error text
plus a focused regression test for the original queues report.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cargo fmt regroups; three clippy fixes:
- F-S-006: hoist MAX_API_KEY_CANDIDATES to module scope to satisfy
clippy::items_after_statements.
- F-S-009: use map_or instead of map().unwrap_or() per
clippy::map_unwrap_or.
- F-P-012: replace `|c| c.encode()` closure with the method itself
per clippy::redundant_closure_for_method_calls.
No behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
invoke_service::resolve and enqueue_async performed no authz check —
no AppInvoke capability existed. Same-app isolation was preserved
(cross-app guards work), but within one app an anonymous public-HTTP
script could trigger any other script (e.g. an admin-only worker that
hits secrets/files/external HTTP). Worse: invoke_async runs the
callee with principal: None, so the callee could hold capabilities
the original public caller shouldn't.
- Add Capability::AppInvoke(AppId). app_id() / scope_for_capability
(script:write) / role_satisfies (editor+) are all updated.
- InvokeServiceImpl gains an optional `authz: Option<Arc<dyn AuthzRepo>>`
+ a `with_authz` builder. When set, resolve() runs script_gate on
AppInvoke before doing the cross-app id check.
- picloud/src/lib.rs wires it: `InvokeServiceImpl::new(...).with_authz(...)`.
- Anonymous callers (cx.principal == None) continue to skip the check
via script_gate, preserving the public-HTTP convention.
Existing 5 invoke_service unit tests still pass (the tests use the
authz-less constructor, so the gate is a no-op there).
AUDIT.md anchor: F-S-012.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A 70-line newtype lived in picloud/src/lib.rs wrapping
Arc<PostgresScriptRepository> and re-implementing every ScriptRepository
method as `self.0.method(...).await`. Hand-delegation invited silent
skew when the trait gained a method, and forced 8 call sites to know
about the wrapper.
Add a blanket
impl<T: ScriptRepository + ?Sized> ScriptRepository for Arc<T>
in manager-core::repo. The implementations forward via (**self).method(...)
so any owner of an Arc<dyn ScriptRepository> (or Arc<ConcreteImpl>) can
be passed where the trait is expected.
Migrate the 7 picloud-binary call sites:
Arc::new(PostgresScriptRepoHandle(script_repo.clone())) → script_repo.clone()
Delete the newtype + its hand-delegated impl. Leaves a 6-line comment
documenting why the boilerplate is gone.
AUDIT.md anchor: F-Q-011.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TICK_INTERVAL (100ms) and ASYNC_EXEC_TIMEOUT (300s) were `const`. Every
other timing knob in the file (cron tick, queue reclaim, retry policy)
is env-overridable via TriggerConfig::from_env. Operators on a
constrained Pi or a busier instance could tune retries but not
dispatcher cadence.
Add env knobs:
- PICLOUD_DISPATCHER_TICK_INTERVAL_MS (default 100)
- PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC (default 300)
Read via tick_interval_from_env() / async_exec_timeout_from_env() at
dispatcher startup. Invalid values fall back with a tracing-warn.
AUDIT.md anchor: F-Q-009.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
http_service::run mapped `Method::from_bytes` failure on a user-supplied
HTTP method to HttpError::Backend with format!("invalid method: {}", …).
Same for HeaderName/HeaderValue validation in build_headers. But that's
user input, not a backend problem — misclassifying as Backend corrupts
retry policies (operators may retry "backend" but not "invalid input").
Add a new HttpError::InvalidArgs(String) variant. Reroute the three
validation paths in http_service (method, header name, header value) to
emit InvalidArgs instead of Backend.
The executor-side `map_http_err` uses Display, so the new variant
surfaces to scripts as "http: invalid method: …" — same format, more
structured behind it.
AUDIT.md anchor: F-Q-008.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
InboxRegistry::register returned (Uuid, Receiver) even when the inner
Mutex was poisoned — `if let Ok(mut g) = self.inner.lock()` swallowed
the error, the sender was never inserted, the later deliver(id, …)
saw no entry and returned Abandoned, and the caller's `await rx`
blocked until the orchestrator's outer timeout.
Sister registries (RouteTable, AppDomainTable) already .expect()
panic on poison. Make InboxRegistry::{register, cancel, deliver} loud
in the same way.
A poisoned Mutex means a prior panic inside a critical section — the
process is unrecoverable; silently swallowing it just defers the
crash and corrupts the inbox protocol on the way.
AUDIT.md anchors: F-Q-006 (register), F-Q-010 (consistency policy).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
regex::is_match, find, find_all, replace, replace_all, split, captures
all called Regex::new(pattern) per invocation. A script doing
`regex::is_match("\\d+", x)` in a tight loop paid the compile every
iteration. Regex compile dominates is_match on short strings.
Add a thread-local LruCache<String, Arc<Regex>> (cap 128). The
compile helper now memoises by pattern string, returning Arc<Regex>
on hit. Cap is well above what any sensible script uses but bounded
enough to keep memory predictable on long-running threads.
AUDIT.md anchor: F-P-014.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ORDER BY created_at DESC, id DESC but cursor was `WHERE created_at <
$2` — when two users were created at the same instant, pagination
could skip the second row at a page boundary or return it twice.
- Add ListCursor { created_at, id } with `<rfc3339>_<uuid>` encode /
decode helpers.
- Change WHERE predicate to `(created_at, id) < ($2, $3)` matching the
ORDER BY.
- Surface the opaque cursor string through UsersListOpts /
UsersListPage / users_admin_api ListUsersResponse instead of the raw
DateTime — keeps the wire format stable while the id half stops the
boundary bug.
Same shape as F-P-005 (execution_logs).
AUDIT.md anchor: F-P-012.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
No unique constraint on (app_id, lower(email)) for pending rows meant
calling users::invite("alice@…") N times created N rows with N valid
tokens. Combined with accept_invite returning Ok(None) silently when
the email already exists, an attacker who learned one token could
permanently consume it without effect.
Migration 0040 adds a partial unique index keyed on
(app_id, lower(email)) WHERE accepted_at IS NULL. Re-inviting after a
previous invite was accepted still works — the accepted row falls
outside the index.
The service-layer 409-conflict UX (the audit's secondary suggestion)
is a separate follow-up; this commit closes the data-shape hole.
AUDIT.md anchor: F-S-013.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two (encrypted, nonce) column pairs are each nullable independently in
the current schema:
- email_trigger_details.inbound_secret_encrypted / _nonce
- app_secrets.realtime_signing_key_encrypted / _nonce
A bug or partial write could leave one populated and the other NULL,
silently bypassing signature verification (receivers decrypt only if
the encrypted column is set).
Migration 0038 adds CHECK ((enc IS NULL) = (nonce IS NULL)) on both
tables — defence-in-depth: catches an invariant violation at the DB
boundary even if the writing code regresses.
AUDIT.md anchor: F-M-002.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migration 0017_cron_triggers.sql created idx_cron_triggers_due on
(last_fired_at) with a comment claiming it serves the scheduler. The
actual scheduler query has no last_fired_at predicate — it filters
purely on `t.enabled = TRUE FOR UPDATE OF d SKIP LOCKED`. The index
has been pure write amplification with no read payoff.
Migration 0037 drops it. Reversible by re-running 0017's CREATE INDEX
if the planner story ever changes.
AUDIT.md anchor: F-M-001.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
list_active_queue_consumers fires every 100ms from the dispatcher
queue arm and predicates on `WHERE t.kind='queue' AND t.enabled=TRUE`
with no app_id filter — but the only available index
`idx_triggers_app_kind_enabled` is keyed on `(app_id, kind)` and so
requires an app_id predicate to be useful. Without one, the planner
falls back to a sequential scan every tick.
Add migration 0036 with a partial index on `kind` (WHERE enabled =
TRUE) so the hot dispatcher query becomes an index-only lookup.
AUDIT.md anchor: F-P-010.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PICLOUD_DEV_MODE=true alone fell through to a fully public deterministic
master key — SHA-256("picloud-dev-master-key-v1.1.7"). The warning was
correct but the gate was a single env var. An operator copying a dev
docker-compose file into prod silently encrypted everything with a
world-known key.
Require both:
- PICLOUD_DEV_MODE=true
- PICLOUD_DEV_INSECURE_KEY=i-understand-this-is-insecure
Without the literal-string acknowledgement, MasterKey::from_env returns
the new DevModeUnacknowledged error and the process refuses to start.
Production deployments aren't affected (they set PICLOUD_SECRET_KEY).
AUDIT.md anchor: F-S-009.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
key_cache: Mutex<HashMap<AppId, Vec<u8>>> was populated on first read
and never evicted, bounded, or cleared on app deletion. Two problems:
(1) Once key rotation lands, every running process keeps accepting
tokens signed by the old key until restart.
(2) Dropping and re-creating an app (FK CASCADE removes app_secrets,
fresh row inserted) made the cache hand out the OLD key bytes.
- Wrap the cache value as `(Instant, Vec<u8>)`; entries expire after
KEY_CACHE_TTL = 5min.
- Stale reads fall through to the secrets repo (no error, just slower).
- New `invalidate_signing_key(app_id)` for explicit eviction once the
app-secrets rotation path lands.
AUDIT.md anchor: F-S-008.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
verify_api_key Argon2-verifies every candidate sharing the 8-char
prefix. With unlimited candidates, an attacker who can provision many
keys against one indexed prefix could force the server into per-request
M×Argon2 — a few hundred concurrent connections trivially DoSes the
manager since the verify runs before any concurrency cap.
Cap the candidate set at MAX_API_KEY_CANDIDATES = 16; log a warn when
truncation kicks in so operators can spot it. With 32-byte random key
bodies the natural collision rate is negligible, so truncation under
benign load shouldn't happen — its presence is the alarm.
Switching the verify hash from Argon2 to SHA-256 (the audit's other
suggested fix) is a larger refactor (new hash column, backfill, dual-
hash verify during migration) and is left for a follow-up.
AUDIT.md anchor: F-S-006 (cap; hash swap deferred).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
