PiCloud/security_audit/04_injection.md

Injection & Query Safety

Audit date: 2026-06-11 Scope: SQL injection (sqlx dynamic queries, ORDER BY / LIMIT / OFFSET dynamic clauses, JSONB operators, LIKE patterns, migrations, advisory locks), command injection (std::process::Command / tokio::process), log injection (newline-in-tracing-fields), HTTP response-header injection, SMTP-header injection on outbound mail, route matcher path normalisation, regex DoS, JSON body limits, and the Rhai → JSONB serialisation path.

Methodology: enumerated every sqlx::query, query_as, format! near SQL keywords, QueryBuilder, bind(), push_str near SQL, EXECUTE, search_path across the workspace; traced three dynamic builders end-to-end (docs_repo::build_find_query, repo::list_for_user, topic_repo::create); read every process::Command site; read every tracing::{info,warn,error}! site in manager-core for newline-in-value risk; read email_service::build_message and lettre's SMTP-injection posture; read Response::builder().header(...) sites in manager-core and orchestrator-core; read crates/orchestrator-core/src/routing/{pattern,matcher}.rs for path parsing; read crates/executor-core/src/sdk/stdlib/regex.rs; read caddy/Caddyfile{,.prod}. Cross-checked AUDIT.md (no overlap — Q-001 and friends sit at the architectural-arrow boundary, not the SQL injection surface).

Headline: the SQL layer is parameterized cleanly end-to-end

The codebase has 75 dynamic sqlx::query(…)/query_as(…) callsites and zero uses of the compile-time-checked sqlx::query! macro. Despite the lack of macro coverage, every dynamic call I read uses positional $1..$N parameters and .bind(…); no callsite interpolates a user-controlled value into the SQL text. The format! interpolations that survive into the SQL string only inject a hardcoded &str constant (SCRIPT_SELECT_COLS / SELECT_COLS) so the column list isn't duplicated across queries — see repo.rs:211, topic_repo.rs:144, etc. Two dynamic builders that take user input — docs_repo::build_find_query (the v1.1.2 docs find-DSL) and kv_repo::list keyset cursor — both run user-controlled path segments and comparison values through QueryBuilder::push_bind, not push. The structural risk of an SQL-injection bug landing in a future patch is real because the macro form isn't enforced, but no live SQL-injection finding emerged from this pass.

High

(None.)

Medium

Response-header injection / handler panic via attacker-controlled files.content_type

• Where: crates/manager-core/src/files_api.rs:158-164 (build response), crates/shared/src/files.rs:190-211 (NewFile::validate — accepts any content_type that is non-empty and ≤ MAX_CONTENT_TYPE_BYTES).
• What: files::create is reachable from any Rhai script (incl. anonymous public HTTP routes). NewFile::validate enforces no upper bound on character set: empty/trim check + length check + nothing else. The stored content_type is then plumbed directly into Response::builder().header(CONTENT_TYPE, meta.content_type) on GET /api/v1/admin/apps/:id/files/:col/:fid. axum::http::Response::builder().header(...) defers the HeaderValue parse until .body(), and the file API uses .expect("build files download response") — so a stored content_type of "text/plain\r\nX-Injected: yes" makes every subsequent admin-side download for that row panic the handler. Axum catches the panic per-task (no process crash), but the dashboard's download page 500s and that single file is effectively poisoned for download until manually edited. A determined script can fill an app's files collection with poisoned content_types and break admin file listings. Additionally, if a future code path streams the file to a non-admin context that has weaker error handling, the CRLF would smuggle a forged header through.
• Recommendation: extend NewFile::validate / FileUpdate::validate to reject any content_type that fails HeaderValue::from_str (or, cheaper, reject any byte in \x00..=\x1f and any non-ASCII). Mirror the same check on name (already sanitised at the response site in files_api.rs:155 but only there — a future route that materialises name into a header without that scrub would inherit the bug). Belt: change files_api.rs:164 from .expect(...) to a 500-with-JSON fallback so a single poisoned row can't keep panicking the request task.

tracing fields embed script-controlled strings — log forging under text-formatter

• Where: crates/manager-core/src/dispatcher.rs:242 (queue = %consumer.queue_name), crates/manager-core/src/files_repo.rs:469 (path = %path.display()), crates/manager-core/src/files_sweep.rs:50, 119 (env value + sweep path), crates/manager-core/src/users_service.rs:726, 815 (rate-limit warnings carry no user-controlled strings, but dispatcher.rs:242 does — queue_name comes from a script's queue::enqueue(name, …) call).
• What: With the default tracing_subscriber text formatter, log lines are emitted as field=value with no escaping of newlines in the value. A script that creates a queue named "prod\n2026-06-11T00:00:00 INFO admin login succeeded user_id=00000000-0000-0000-0000-000000000000" writes a forged login line into every operator's log stream the next time the dispatch loop logs an error against that queue. Even without text-formatter, downstream log aggregators that split on \n are confused. The JSON formatter escapes; the text formatter (which is what picloud/src/lib.rs likely installs in dev/MVP) does not. None of the other fields I sampled carry script-input values, so the surface is narrow — but a queue-name log line is the one that lands during steady-state operation.
• Recommendation: (a) standardise on the JSON formatter for non-dev deployments, documenting that tracing_subscriber::fmt::format::Json escapes control chars; (b) add a small log_safe(s: &str) -> Cow<'_, str> helper that replaces \n/\r/control chars and apply at the script-input boundary (queue names, topic patterns, file collection names, KV collection names) at the tracing callsite; (c) sanitise queue/topic/collection identifiers at registration to a [a-zA-Z0-9_.-] allowlist — this also tightens the input domain for the parsers further downstream. Linked to F-S-001 (the broader "scripts can DoS the platform via unbounded inputs") in AUDIT.md.

SMTP outbound: subject / from / reply_to pass to lettre without explicit CRLF scrub

• Where: crates/manager-core/src/email_service.rs:276-321 (build_message), crates/manager-core/src/email_service.rs:330-353 (parse_address).
• What: subject is builder.subject(email.subject.clone()) — lettre's header writer rejects CR/LF in the resulting HeaderValue (returning lettre::error::Error::EmailMissingAt-family at singlepart/multipart), but the error surfaces as EmailError::Transport(e.to_string()), which is mapped to a string in the OutboundEmail flow. Functionally lettre saves us from the CRLF-into-SMTP-headers attack; structurally the safety lives in a third-party crate's behaviour, with no in-tree test pinning it. An upgrade that relaxes lettre's header sanitiser (or a switch to a different transport later) silently regresses to SMTP header injection. parse_address rejects bad shapes but accepts CRLF inside a quoted display-name. Defence-in-depth.
• Recommendation: add a reject_header_unsafe(s: &str) -> Result<(), EmailError> that fails on any byte < 0x20 (and 0x7f) and call it on subject, from, reply_to, and every entry of to/cc/bcc before they reach lettre. Cheap, future-proof, and pins the boundary against transport-library churn.

Rhai regex::* compiles arbitrary user patterns with no length cap and no compile-time-budget enforcement

• Where: crates/executor-core/src/sdk/stdlib/regex.rs:39-51.
• What: PiCloud uses the Rust regex crate (non-backtracking, linear-time RE2 family) — so the classic catastrophic-backtracking ReDoS doesn't apply. But the regex crate still defends against exponential-NFA-size attacks with a size_limit (default 10 MB). A script can pass a pattern that hits exactly that ceiling per-compile, and the compile itself is expensive. The thread-local LRU caches by string key (pattern.to_string()), so a script that loops over 129 distinct patterns evicts the cache and pays full compile every iteration. Compounded by the existing F-P-014 thread-local design, a single script can burn CPU compiling 50-MB-of-NFA-state patterns serially until the execution budget runs out. Today the executor's wall-clock + operations budget caps this, so the worst-case is "the script's own request burns its own budget" — but the per-app and per-process budgets aren't tightened against regex compile, and a public route that compiles a script-supplied user-input regex is one script-side bug away from the full-process DoS.
• Recommendation: enforce a per-call hard limit on the pattern length (e.g., 4 KiB) before calling Regex::new; consider regex::RegexBuilder::size_limit(usize) capped lower (e.g., 256 KB) than the crate default; and document the pattern-cache semantics so script authors know the compile cost is not amortised across distinct strings. Cheap belt against the future "public route validates an attacker-supplied regex".

email_inbound_api parses untrusted JSON body without an explicit size cap before deserialisation

• Where: crates/manager-core/src/email_inbound_api.rs:160.
• What: serde_json::from_slice(&body).map_err(...) — body arrives via axum's default body extractor and there's no DefaultBodyLimit::max(...) configured for the email-inbound route I could find. By contrast, the user-route handler in orchestrator-core/src/api.rs:219 reads body bytes with an explicit 10 MiB cap (to_bytes(...,10 * 1024 * 1024)). The email-inbound webhook is public (HMAC-authenticated but discoverable), so an attacker can send a 1 GB JSON body and force a slow parse + large allocation per request, possibly bypassing the HMAC check (since the body is read before the HMAC verify in the handler — verify this end-to-end). Likely capped at the axum default of 2 MiB anyway, but pinning it explicitly is cheap and avoids the "future maintainer adds .layer(DefaultBodyLimit::disable())" regression.
• Recommendation: apply axum::extract::DefaultBodyLimit::max(256 * 1024) to the email-inbound route, matching PICLOUD_EMAIL_MAX_MESSAGE_BYTES's logical scale. While here, do the same on every other ingress route that takes a JSON body — they should all opt in to an explicit cap rather than rely on the default.

Low

auth_api.rs:228 builds Set-Cookie via format! with an internally-generated token

• Where: crates/manager-core/src/auth_api.rs:228-232.
• What: The Set-Cookie value format!("{SESSION_COOKIE}={raw_token}; HttpOnly{secure_attr}; …") interpolates raw_token which is generated server-side (generate_session_token). No untrusted input today. Flagging because the format! shape is the canonical CRLF-smuggling footgun — if a future refactor lets a caller-supplied token reach this path (e.g., a "remember-me" import flow), the lack of HeaderValue::from_str validation at the boundary becomes a header-injection bug.
• Recommendation: build via HeaderValue::from_str(...)? (the caller already does at auth_api.rs:145) and surface the parse error as 500 rather than silently emitting an empty cookie. The existing .unwrap_or_else(|_| HeaderValue::from_static("")) (line 148) hides a tamper case.

route_admin's reserved-path check is prefix-based; .. and percent-encoded / in literal segments aren't normalised by the matcher

• Where: crates/orchestrator-core/src/routing/pattern.rs:110-117 (reserved check), crates/orchestrator-core/src/routing/matcher.rs:279-302 (match_param, request_path.trim_start_matches('/').split('/')), crates/orchestrator-core/src/api.rs:175 (request.uri().path().to_string()).
• What: The matcher uses Axum's uri().path() which does NOT decode percent-encoded characters (e.g., %2F, %2E). So a user route bound to /admin-foo is safely never reachable by a request to /api/v1/admin/.../?next=%2Fadmin-foo. But: parse_param calls split('/') directly on the raw path, and a request to /users/%2Fid segments to ["users", "%2Fid"], capturing id = "%2Fid". The capture is then handed to the script as-is. If a script naively re-emits the capture into another HTTP request or into a filesystem operation (files API), the encoded / decodes downstream and is treated as a path separator. Out-of-scope here (the route matcher itself is internally consistent) but adjacent to the path-traversal questions agent 6 is handling. Same for ..: match_param will happily capture .. as a param value; no normalisation, so a script doing files::get(collection, id_from_capture) is on its own. The collection validator in files.rs:259 does check for .. and /, but the file_id capture does not.
• Recommendation: in the matcher, percent-decode each captured segment and reject (or normalise) any segment that decodes to /, .., or \0. This aligns the matcher's per-segment semantics with the file-collection validator and removes a class of "the SDK trusted what the matcher gave me" surprises.

Email-trigger HMAC verify reads x-picloud-timestamp and parses without strict bounds

• Where: crates/picloud/tests/email_inbound.rs:161-212 shows the surface; the inbound handler reads timestamp via header and includes it in the HMAC payload.
• What: Log forging via x-picloud-timestamp is theoretically possible if the value is logged unsanitised. Quick scan of email_inbound_api.rs did not surface a tracing line that emits the header value, so this is preventive — calling out that any future tracing::warn!(timestamp = %ts, …) line on a malformed signature would re-introduce the log-injection surface from the Medium above.
• Recommendation: add explicit parse::<u64>() on the header value at the boundary, store the parsed integer in the local, and never emit the raw header string into a log line.

format!-based dynamic SQL builders interpolate compile-time constants only, but the macro form (sqlx::query!) is not used anywhere — defence-in-depth gap

• Where: crates/manager-core/src/repo.rs:211, 225, 236, 245, 263, 279, 333, 423 etc. (the &format!("SELECT {SCRIPT_SELECT_COLS} …") pattern), crates/manager-core/src/topic_repo.rs:144, 163, 173, 191 (same with SELECT_COLS).
• What: Every interpolation expands a const &str — there is no live SQL-injection here. But because the codebase has chosen the sqlx::query(...) (string) form over the sqlx::query!(...) (macro, compile-time-checked) form everywhere, an injection bug becomes a cargo clippy-invisible regression. Today's discipline is "every format!-into-SQL only interpolates from a hardcoded constant" — that invariant lives in the heads of reviewers, not the type system. A drive-by refactor that replaces SCRIPT_SELECT_COLS with &format!("{prefix}, id, …") and lets prefix come from input is the canonical bug-shaped change.
• Recommendation: write a project-rule lint (or a CI grep) that fails on format!\(.*"(SELECT|INSERT|UPDATE|DELETE|WITH) outside an allowlist of files. Alternative: migrate the hottest sqlx::query(...) sites to sqlx::query!(...) so the macro errors at compile time when the columns change shape — pays for itself the next time a migration adds a column.

Info

docs_repo::build_find_query is the most load-bearing dynamic SQL builder and is parameterized correctly

• Where: crates/manager-core/src/docs_repo.rs:308-420.
• What: Every value goes through qb.push_bind(...); the only literal qb.push("…") strings are SQL keywords / operators selected from a closed enum (ComparisonOp, SortDir). Path segments (FieldPath::segments) are also bound (push_bind(seg.as_str())) into jsonb_extract_path_text(data, $N1, $N2, …). The DSL parser (docs_filter.rs:85-115) rejects depth > 5, .., $-prefixed segments, and .-prefixed/-suffixed segments. The $limit clause goes through qb.push_bind(i64::from(limit)) and is hard-capped at DOCS_LIST_MAX_LIMIT. ORDER BY direction comes from a closed enum. This is the right shape.

kv_repo::list keyset cursor is parameter-bound; cursor decode is total

• Where: crates/manager-core/src/kv_repo.rs:188-200, 218-223.
• What: $3::text IS NULL OR key > $3 with last_key (Option) bound. Cursor decoded as base64-URL-safe-no-pad → UTF-8 → bound text. Decode errors return InvalidCursor cleanly. No injection surface; cursor tampering at worst skips rows.

dead_letter_repo::resolve validates reason against a fixed allowlist

• Where: crates/manager-core/src/dead_letter_repo.rs:104-105, 183-200.
• What: ALLOWED_RESOLUTIONS = &["replayed", "ignored", "handled_by_script", "handler_failed"]; checked before the SQL UPDATE. Even though the reason then goes through .bind($2) (so injection wasn't possible anyway), the allowlist gives a clean error early. Right pattern; consider replicating wherever a free-form "reason" or "status" field could appear in the future.

process::Command usage is confined to test code

• Where: crates/picloud-cli/tests/common/server.rs:58.
• What: The only std::process::Command callsite anywhere in the workspace spawns the picloud binary with the test's own controlled env. No sh -c, no shell escape, no user input. The production CLI (picloud-cli) is pure HTTP — no shell-out for git/curl/etc. Hardened by absence.

Caddy reverse-proxy config doesn't construct headers from input

• Where: caddy/Caddyfile, caddy/Caddyfile.prod.
• What: Static handle directives + static error bodies. No header directive interpolates a captured request value into a response header. Caddy's own request normalisation drops CR/LF from headers before reaching the upstream.

No LIKE / ILIKE patterns in the repo layer; no dynamic ORDER BY / LIMIT / OFFSET from input

• Where: workspace-wide rg 'LIKE \|ILIKE' → 0 hits in crates/manager-core/src/. ORDER BY is either a static string or a closed-enum (SortDir) selection in docs_repo. LIMIT/OFFSET are bound parameters with hard upper-caps. execution_logs::list_for_script (F-P-005 in AUDIT.md) uses OFFSET but with a bound integer — performance issue, not injection.

Postgres advisory-lock keys are derived from a stable DefaultHasher, not user input directly

• Where: crates/manager-core/src/trigger_repo.rs:1346-1359.
• What: pg_advisory_xact_lock($1) with a bind(lock_key) where lock_key is DefaultHasher.finish() as i64 of (app_id, queue_name). Untrusted queue_name only feeds the hash, not the SQL text. Good.

JSONB value serialisation: scripts → serde_json::Value → sqlx → JSONB — no string interpolation in the chain

• Where: crates/manager-core/src/kv_repo.rs:111-130, crates/manager-core/src/docs_repo.rs (write paths), crates/manager-core/src/log_sink.rs:24-54.
• What: Every JSONB write column binds a serde_json::Value directly (sqlx's Json/JsonValue ↔ JSONB codec is binary-safe). No path constructs a JSON string by hand and feeds it to INSERT … VALUES ('{...}'::jsonb). The blueprint's promise that JSONB is the storage shape is upheld at the boundary.

serde_json::from_slice on untrusted bodies is gated by axum body-size limits where it matters

• Where: crates/orchestrator-core/src/api.rs:219-228 (10 MiB cap, then parse). crates/manager-core/src/email_inbound_api.rs:160 lacks an explicit cap — see Medium above.

Migrations are static SQL; no EXECUTE format(…) or search_path manipulation

• Where: crates/manager-core/migrations/*.sql.
• What: All 41 migrations are static text. No DO $$ … EXECUTE format('…') $$; block. The schema-snapshot test (tests/schema_snapshot.rs) pins the final shape so a future dynamic-SQL migration would have to update the snapshot, surfacing the change in review.