fabi/PiCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(review): harden the E2E-gap fixes after security/regression review

Browse Source 
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>
This commit is contained in:
MechaCat02
2026-06-13 09:59:24 +02:00
parent 30bad27711
commit a91b134285
9 changed files with 156 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
Cargo.lock generated
View File

@@ -1962,6 +1962,7 @@ dependencies = [
"clap",
"directories",
"libc",
"percent-encoding",
"picloud-shared",
"postgres",
"predicates",

3
crates/executor-core/src/sdk/users.rs
View File

@@ -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: () });

14
crates/manager-core/src/users_service.rs
View File

@@ -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)?;

7
crates/orchestrator-core/src/api.rs
View File

@@ -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();

1
crates/picloud-cli/Cargo.toml
View File

@@ -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"] }

71
crates/picloud-cli/src/client.rs
View File

@@ -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<AppLookupDto> {
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<Script> {
let id = seg(id);
let body = UpdateScriptBody { source };
let resp = self
.request(Method::PUT, &format!("/api/v1/admin/scripts/{id}"))
@@ -169,6 +192,7 @@ impl Client {
body: Value,
headers: &[(String, String)],
) -> Result<ExecuteResponse> {
let id = seg(id);
let mut req = self
.request(Method::POST, &format!("/api/v1/execute/{id}"))
.json(&body);
@@ -199,6 +223,7 @@ impl Client {
/// `GET /api/v1/admin/scripts/{id}/logs?limit=N`
pub async fn logs_list(&self, script_id: &str, limit: u32) -> Result<Vec<ExecutionLog>> {
let script_id = seg(script_id);
let resp = self
.request(
Method::GET,
@@ -244,6 +269,7 @@ impl Client {
/// `DELETE /api/v1/admin/api-keys/{id}` — 404 covers both "doesn't
/// exist" and "not yours" (server flattens to avoid enumeration).
pub async fn apikeys_delete(&self, id: &str) -> Result<()> {
let id = seg(id);
let resp = self
.request(Method::DELETE, &format!("/api/v1/admin/api-keys/{id}"))
.send()
@@ -253,6 +279,7 @@ impl Client {
/// `GET /api/v1/admin/scripts/{id}/routes`
pub async fn routes_list_for_script(&self, script_id: &str) -> Result<Vec<Route>> {
let script_id = seg(script_id);
let resp = self
.request(
Method::GET,
@@ -269,6 +296,7 @@ impl Client {
script_id: &str,
body: &CreateRouteBody<'_>,
) -> Result<Route> {
let script_id = seg(script_id);
let resp = self
.request(
Method::POST,
@@ -282,6 +310,7 @@ impl Client {
/// `DELETE /api/v1/admin/routes/{route_id}`
pub async fn routes_delete(&self, route_id: &str) -> Result<()> {
let route_id = seg(route_id);
let resp = self
.request(Method::DELETE, &format!("/api/v1/admin/routes/{route_id}"))
.send()
@@ -322,6 +351,7 @@ impl Client {
/// `GET /api/v1/admin/admins/{id}`
pub async fn admins_get(&self, id: &str) -> Result<AdminDto> {
let id = seg(id);
let resp = self
.request(Method::GET, &format!("/api/v1/admin/admins/{id}"))
.send()
@@ -341,6 +371,7 @@ impl Client {
/// `PATCH /api/v1/admin/admins/{id}`
pub async fn admins_patch(&self, id: &str, body: &PatchAdminBody<'_>) -> Result<AdminDto> {
let id = seg(id);
let resp = self
.request(Method::PATCH, &format!("/api/v1/admin/admins/{id}"))
.json(body)
@@ -351,6 +382,7 @@ impl Client {
/// `DELETE /api/v1/admin/admins/{id}`
pub async fn admins_delete(&self, id: &str) -> Result<()> {
let id = seg(id);
let resp = self
.request(Method::DELETE, &format!("/api/v1/admin/admins/{id}"))
.send()
@@ -360,6 +392,7 @@ impl Client {
/// `GET /api/v1/admin/apps/{id}/triggers`
pub async fn triggers_list(&self, app: &str) -> Result<TriggerListDto> {
let app = seg(app);
let resp = self
.request(Method::GET, &format!("/api/v1/admin/apps/{app}/triggers"))
.send()
@@ -369,6 +402,7 @@ impl Client {
/// `DELETE /api/v1/admin/apps/{id}/triggers/{trigger_id}`
pub async fn triggers_delete(&self, app: &str, trigger_id: &str) -> Result<()> {
let (app, trigger_id) = (seg(app), seg(trigger_id));
let resp = self
.request(
Method::DELETE,
@@ -388,6 +422,7 @@ impl Client {
kind: &str,
body: &serde_json::Value,
) -> Result<TriggerDto> {
let (app, kind) = (seg(app), seg(kind));
let resp = self
.request(
Method::POST,
@@ -401,6 +436,7 @@ impl Client {
/// `GET /api/v1/admin/apps/{id}/dead_letters/count`
pub async fn dead_letters_count(&self, app: &str) -> Result<DeadLetterCountDto> {
let app = seg(app);
let resp = self
.request(
Method::GET,
@@ -418,6 +454,7 @@ impl Client {
unresolved: bool,
limit: u32,
) -> Result<DeadLetterListDto> {
let app = seg(app);
let path =
format!("/api/v1/admin/apps/{app}/dead_letters?unresolved={unresolved}&limit={limit}");
let resp = self.request(Method::GET, &path).send().await?;
@@ -426,6 +463,7 @@ impl Client {
/// `GET /api/v1/admin/apps/{id}/dead_letters/{dl_id}`
pub async fn dead_letters_get(&self, app: &str, dl_id: &str) -> Result<DeadLetterDto> {
let (app, dl_id) = (seg(app), seg(dl_id));
let resp = self
.request(
Method::GET,
@@ -438,6 +476,7 @@ impl Client {
/// `POST /api/v1/admin/apps/{id}/dead_letters/{dl_id}/replay`
pub async fn dead_letters_replay(&self, app: &str, dl_id: &str) -> Result<()> {
let (app, dl_id) = (seg(app), seg(dl_id));
let resp = self
.request(
Method::POST,
@@ -450,6 +489,7 @@ impl Client {
/// `POST /api/v1/admin/apps/{id}/dead_letters/{dl_id}/resolve`
pub async fn dead_letters_resolve(&self, app: &str, dl_id: &str, reason: &str) -> Result<()> {
let (app, dl_id) = (seg(app), seg(dl_id));
let resp = self
.request(
Method::POST,
@@ -463,6 +503,7 @@ impl Client {
/// `GET /api/v1/admin/apps/{id}/secrets`
pub async fn secrets_list(&self, app: &str) -> Result<SecretListDto> {
let app = seg(app);
let resp = self
.request(Method::GET, &format!("/api/v1/admin/apps/{app}/secrets"))
.send()
@@ -472,6 +513,8 @@ impl Client {
/// `POST /api/v1/admin/apps/{id}/secrets`
pub async fn secrets_set(&self, app: &str, name: &str, value: serde_json::Value) -> Result<()> {
// `name` travels in the JSON body, not the path — only `app` needs encoding.
let app = seg(app);
let resp = self
.request(Method::POST, &format!("/api/v1/admin/apps/{app}/secrets"))
.json(&serde_json::json!({ "name": name, "value": value }))
@@ -482,6 +525,7 @@ impl Client {
/// `DELETE /api/v1/admin/apps/{id}/secrets/{name}`
pub async fn secrets_delete(&self, app: &str, name: &str) -> Result<()> {
let (app, name) = (seg(app), seg(name));
let resp = self
.request(
Method::DELETE,
@@ -496,6 +540,7 @@ impl Client {
/// `GET /api/v1/admin/apps/{id_or_slug}/domains`
pub async fn domains_list(&self, app: &str) -> Result<Vec<AppDomain>> {
let app = seg(app);
let resp = self
.request(Method::GET, &format!("/api/v1/admin/apps/{app}/domains"))
.send()
@@ -505,6 +550,7 @@ impl Client {
/// `POST /api/v1/admin/apps/{id_or_slug}/domains`
pub async fn domains_create(&self, app: &str, pattern: &str) -> Result<AppDomain> {
let app = seg(app);
let resp = self
.request(Method::POST, &format!("/api/v1/admin/apps/{app}/domains"))
.json(&serde_json::json!({ "pattern": pattern }))
@@ -515,6 +561,7 @@ impl Client {
/// `DELETE /api/v1/admin/apps/{id_or_slug}/domains/{domain_id}`
pub async fn domains_delete(&self, app: &str, domain_id: &str) -> Result<()> {
let (app, domain_id) = (seg(app), seg(domain_id));
let resp = self
.request(
Method::DELETE,
@@ -529,6 +576,7 @@ impl Client {
/// `GET /api/v1/admin/apps/{id_or_slug}/topics`
pub async fn topics_list(&self, app: &str) -> Result<Vec<TopicDto>> {
let app = seg(app);
let resp = self
.request(Method::GET, &format!("/api/v1/admin/apps/{app}/topics"))
.send()
@@ -545,6 +593,7 @@ impl Client {
external_subscribable: bool,
auth_mode: &str,
) -> Result<TopicDto> {
let app = seg(app);
let resp = self
.request(Method::POST, &format!("/api/v1/admin/apps/{app}/topics"))
.json(&serde_json::json!({
@@ -573,6 +622,7 @@ impl Client {
if let Some(m) = auth_mode {
body.insert("auth_mode".into(), Value::String(m.to_string()));
}
let (app, name) = (seg(app), seg(name));
let resp = self
.request(
Method::PATCH,
@@ -586,6 +636,7 @@ impl Client {
/// `DELETE /api/v1/admin/apps/{id_or_slug}/topics/{name}`
pub async fn topics_delete(&self, app: &str, name: &str) -> Result<()> {
let (app, name) = (seg(app), seg(name));
let resp = self
.request(
Method::DELETE,
@@ -600,6 +651,7 @@ impl Client {
/// `GET /api/v1/admin/apps/{id_or_slug}/users?limit={n}`
pub async fn app_users_list(&self, app: &str, limit: u32) -> Result<Vec<AppUser>> {
let app = seg(app);
let resp = self
.request(
Method::GET,
@@ -613,6 +665,7 @@ impl Client {
/// `GET /api/v1/admin/apps/{id_or_slug}/users/{user_id}`
pub async fn app_user_get(&self, app: &str, user_id: &str) -> Result<AppUser> {
let (app, user_id) = (seg(app), seg(user_id));
let resp = self
.request(
Method::GET,
@@ -629,6 +682,7 @@ impl Client {
app: &str,
user_id: &str,
) -> Result<ResetPasswordResponseDto> {
let (app, user_id) = (seg(app), seg(user_id));
let resp = self
.request(
Method::POST,
@@ -645,6 +699,7 @@ impl Client {
app: &str,
user_id: &str,
) -> Result<RevokeSessionsResponseDto> {
let (app, user_id) = (seg(app), seg(user_id));
let resp = self
.request(
Method::POST,
@@ -1096,4 +1151,20 @@ mod tests {
let c = Client::new("http://localhost:8000/", "pic_x").unwrap();
assert_eq!(c.url(), "http://localhost:8000");
}
#[test]
fn seg_escapes_path_delimiters() {
// A free-form segment can't break out into a new path segment,
// the query, or the fragment.
assert_eq!(seg("a/b"), "a%2Fb");
assert_eq!(seg("a?b"), "a%3Fb");
assert_eq!(seg("a#b"), "a%23b");
assert_eq!(seg("a b"), "a%20b");
// Ordinary slugs / UUIDs pass through unchanged.
assert_eq!(seg("my-app"), "my-app");
assert_eq!(
seg("4c8b8775-2a9e-4212-9187-a8f56a1b60e7"),
"4c8b8775-2a9e-4212-9187-a8f56a1b60e7"
);
}
}

31
crates/shared/src/outbox_writer.rs
View File

@@ -47,6 +47,10 @@ pub struct NewHttpOutbox {
pub struct HttpDispatchPayload {
pub script_name: String,
pub path: String,
/// HTTP method (uppercased). `#[serde(default)]` so async-HTTP outbox
/// rows enqueued before this field existed still decode (as `""`,
/// matching non-HTTP triggers) instead of dead-lettering on upgrade.
#[serde(default)]
pub method: String,
pub headers: std::collections::BTreeMap<String, String>,
pub body: serde_json::Value,
@@ -70,3 +74,30 @@ pub enum OutboxWriterError {
#[error("outbox write failed: {0}")]
Backend(String),
}
#[cfg(test)]
mod tests {
use super::*;
/// Async-HTTP outbox rows enqueued before `method` existed must still
/// decode after upgrade (it's `#[serde(default)]`), not dead-letter on
/// a "missing field `method`" error. Regression guard for that field.
#[test]
fn http_payload_decodes_without_method_field() {
let legacy = serde_json::json!({
"script_name": "todos",
"path": "/todos",
// no "method" key — pre-upgrade row
"headers": {},
"body": null,
"params": {},
"query": {},
"rest": "",
"timeout_seconds": 30
});
let payload: HttpDispatchPayload =
serde_json::from_value(legacy).expect("legacy payload must decode");
assert_eq!(payload.method, "");
assert_eq!(payload.path, "/todos");
}
}

7
crates/shared/src/users.rs
View File

@@ -207,6 +207,13 @@ pub trait UsersService: Send + Sync {
/// this leaks only a single boolean — no id, profile, or timing
/// distinction beyond "exists / doesn't" — so a public registration
/// script can pre-check before calling `create`.
///
/// Enumeration note: that bit is the same one a register form already
/// leaks ("email taken"), but this probe is cheaper (no password hash),
/// has no side effect (a `create` miss writes a junk row), and is not
/// rate limited (there is no built-in throttle/CAPTCHA primitive). A
/// script exposing it on an anonymous route inherits account-enumeration
/// risk and must add its own `kv`-based throttle if that matters.
async fn email_available(&self, cx: &SdkCallCx, email: &str) -> Result<bool, UsersError>;
async fn update(

25
docs/stdlib-reference.md
View File

@@ -255,6 +255,31 @@ Always include `statusCode` when you mean to set headers or a specific
body shape. Returning a bare value (`42`, `#{ ok: true }`) is fine — it
becomes a `200` body as-is.
### Security note — `users::email_available` and account enumeration
`users::email_available(email) -> bool` is the anonymous-safe way to
pre-check an email during self-serve registration (`users::find_by_email`
is forbidden for anonymous public scripts to prevent enumeration). The
probe returns only `true`/`false` — the same bit a register form already
leaks via "email already taken" (and via `create`'s own uniqueness error),
so it adds no *new* enumeration vector — but it is **cheap (no password
hash), has no side effect, and is not rate limited**, which makes bulk
probing faster than going through `create`.
There is no built-in per-route throttle or CAPTCHA primitive today (only
the global `PICLOUD_MAX_CONCURRENT_EXECUTIONS` cap). If account enumeration
matters for your app, the available mitigation is a `kv`-based counter
keyed on the client (e.g. an IP/email bucket you increment and check) —
the same pattern you'd use for any custom rate limit:
```rhai
// Pre-check, then create. Gate behind your own kv counter if enumeration
// is a concern — the platform does not rate-limit this probe for you.
if !users::email_available(email) {
return #{ statusCode: 409, body: #{ error: "email already registered" } };
}
```
## What's not here
- **Crypto** (sha256/hmac/argon2/encryption) — deferred to a focused