From a91b13428538735d813dee34c087d1be2f39e35f Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 13 Jun 2026 09:59:24 +0200 Subject: [PATCH] fix(review): harden the E2E-gap fixes after security/regression review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Cargo.lock | 1 + crates/executor-core/src/sdk/users.rs | 3 + crates/manager-core/src/users_service.rs | 14 ++++- crates/orchestrator-core/src/api.rs | 7 ++- crates/picloud-cli/Cargo.toml | 1 + crates/picloud-cli/src/client.rs | 71 ++++++++++++++++++++++++ crates/shared/src/outbox_writer.rs | 31 +++++++++++ crates/shared/src/users.rs | 7 +++ docs/stdlib-reference.md | 25 +++++++++ 9 files changed, 156 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6734c52..ce51ffb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1962,6 +1962,7 @@ dependencies = [ "clap", "directories", "libc", + "percent-encoding", "picloud-shared", "postgres", "predicates", diff --git a/crates/executor-core/src/sdk/users.rs b/crates/executor-core/src/sdk/users.rs index 5f59d39..a0bdccc 100644 --- a/crates/executor-core/src/sdk/users.rs +++ b/crates/executor-core/src/sdk/users.rs @@ -10,6 +10,9 @@ //! // for anonymous public scripts. //! let free = users::email_available("a@b"); // bool — anonymous-safe pre-check //! // for self-serve registration. +//! // Leaks only "exists/doesn't" but +//! // is cheap + unthrottled: throttle +//! // the route if enumeration matters. //! users::update(id, #{ display_name: "Alicia" }); //! let removed = users::delete(id); // bool //! let page = users::list(#{ "$limit": 50, cursor: () }); diff --git a/crates/manager-core/src/users_service.rs b/crates/manager-core/src/users_service.rs index 06b21a9..6916f25 100644 --- a/crates/manager-core/src/users_service.rs +++ b/crates/manager-core/src/users_service.rs @@ -520,9 +520,17 @@ impl UsersService for UsersServiceImpl { // Gated like `create` (the registration write path) so the probe // is available exactly where self-serve registration is — and, // crucially, WITHOUT find_by_email's anonymous-principal rejection. - // It returns only a boolean, so it leaks no more than `create`'s - // own uniqueness error already does (F-S-003: enumeration risk is - // bounded to "exists / doesn't"). + // + // SECURITY (F-S-003, enumeration): a single probe reveals only + // "exists / doesn't" — the same bit a register form leaks via its + // "email already taken" path. But unlike `create`, this probe has + // no Argon2 cost and no side effect (a `create` miss writes a junk + // row), so it is a *cheaper, quieter* enumeration oracle when a + // script exposes it on an anonymous public route. It is NOT rate + // limited here, and there is no built-in per-route throttle/CAPTCHA + // primitive. Scripts that put this behind a public endpoint and + // care about enumeration must add their own `kv`-based counter. + // See docs/stdlib-reference.md. self.require(cx.principal.as_ref(), Capability::AppUsersWrite(cx.app_id)) .await?; let normalized = validate_email(email)?; diff --git a/crates/orchestrator-core/src/api.rs b/crates/orchestrator-core/src/api.rs index 852affe..8abd84a 100644 --- a/crates/orchestrator-core/src/api.rs +++ b/crates/orchestrator-core/src/api.rs @@ -170,7 +170,12 @@ where E: ExecutorClient + 'static, R: ScriptResolver + 'static, { - let method = request.method().as_str().to_string(); + // Uppercased so `ctx.request.method` honors its documented contract + // (extension methods arrive in their original case). Route matching is + // case-insensitive (see `routing::matcher::method_matches`), so this + // only normalizes the value surfaced to scripts / persisted in the + // async-dispatch payload. + let method = request.method().as_str().to_ascii_uppercase(); let uri = request.uri().clone(); let path = uri.path().to_string(); let query_str = uri.query().unwrap_or("").to_string(); diff --git a/crates/picloud-cli/Cargo.toml b/crates/picloud-cli/Cargo.toml index 57dcfec..caa5d71 100644 --- a/crates/picloud-cli/Cargo.toml +++ b/crates/picloud-cli/Cargo.toml @@ -24,6 +24,7 @@ path = "tests/cli.rs" [dependencies] picloud-shared.workspace = true reqwest = { workspace = true, features = ["json"] } +percent-encoding.workspace = true serde.workspace = true serde_json.workspace = true tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } diff --git a/crates/picloud-cli/src/client.rs b/crates/picloud-cli/src/client.rs index 9343c75..c95da4b 100644 --- a/crates/picloud-cli/src/client.rs +++ b/crates/picloud-cli/src/client.rs @@ -9,6 +9,7 @@ use std::collections::BTreeMap; use anyhow::{anyhow, Context, Result}; use chrono::{DateTime, Utc}; +use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS}; use picloud_shared::{ AdminUserId, ApiKeyId, App, AppDomain, AppId, AppRole, AppUser, DispatchMode, ExecutionLog, HostKind, InstanceRole, PathKind, Route, Scope, Script, ScriptId, @@ -19,6 +20,24 @@ use serde_json::Value; use crate::config::Credentials; +/// Characters that must be escaped inside a single URL **path segment** so +/// a free-form value (an app slug, a topic name) can't break out of its +/// segment — `/` would start a new segment, `?`/`#` would start the query +/// or fragment. Encodes controls, space, and the structural delimiters. +const PATH_SEGMENT: &AsciiSet = &CONTROLS + .add(b' ') + .add(b'/') + .add(b'?') + .add(b'#') + .add(b'%') + .add(b'&') + .add(b'+'); + +/// Percent-encode a value for safe interpolation into one path segment. +fn seg(s: &str) -> String { + utf8_percent_encode(s, PATH_SEGMENT).to_string() +} + pub struct Client { http: reqwest::Client, url: String, @@ -73,6 +92,7 @@ impl Client { /// `GET /api/v1/admin/apps/{id_or_slug}` — slug or UUID accepted. pub async fn apps_get(&self, ident: &str) -> Result { + let ident = seg(ident); let resp = self .request(Method::GET, &format!("/api/v1/admin/apps/{ident}")) .send() @@ -119,6 +139,7 @@ impl Client { /// Server requires `AppAdmin` capability; without `force`, returns /// 409 if the app still has scripts. pub async fn apps_delete(&self, ident: &str, force: bool) -> Result<()> { + let ident = seg(ident); let path = if force { format!("/api/v1/admin/apps/{ident}?force=true") } else { @@ -131,6 +152,7 @@ impl Client { /// `DELETE /api/v1/admin/scripts/{id}` — requires `AppAdmin` on the /// owning app (stricter than the edit endpoints, by design). pub async fn scripts_delete(&self, id: &str) -> Result<()> { + let id = seg(id); let resp = self .request(Method::DELETE, &format!("/api/v1/admin/scripts/{id}")) .send() @@ -151,6 +173,7 @@ impl Client { /// `PUT /api/v1/admin/scripts/{id}` — matches the dashboard, which /// uses PUT despite the field-level update semantics. pub async fn scripts_update_source(&self, id: &str, source: &str) -> Result