From b3f05dfe2ad002b11242bd49e2564e585d9e0814 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Tue, 23 Jun 2026 21:59:50 +0200 Subject: [PATCH] fix(enabled): close disabled-script execution on the async paths (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Holistic Phase-1 review found the "a disabled script can't execute via any path" guarantee (§4.3) held only for the sync user-route, the execute-by-id bypass, and the trigger outbox arm — three paths still ran disabled scripts: - Queue arm: `list_active_queue_consumers` filtered the trigger's `enabled` but not the bound script's. Add `JOIN scripts … AND s.enabled = TRUE` so a disabled script's queue trigger stops consuming. - Async-HTTP (202) + queued invoke() arms: `build_http_request` / `build_invoke_request` hardcoded `active: true`. Set it to `script.enabled`, and MOVE the dispatcher's fire-time `active` drop to after the source-kind match so it covers all three arms uniformly (previously trigger-only). - `invoke()` (script-to-script): `resolve_id`/`resolve_name` checked cross-app isolation but not `enabled`. A disabled target now resolves to NotFound (indistinguishable from absent), matching the data plane. Also (review LOW/§4.7): - The route-on/script-off 404 returned `NotFound(script_id)`, leaking the internal id and distinguishable from absent. Return the same flat "no route matches" 404 as the unmatched case. - Add the §4.7 "enabled endpoint with no route and no trigger" reachability warning (was unimplemented; only disabled-target shipped). Tested: manager-core lib 368 + orchestrator 75 + 22 project-tool journeys (incl. a new enabled-route→disabled-script flat-404 e2e) green; clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/manager-core/src/apply_service.rs | 59 +++++++++++++++++++++++ crates/manager-core/src/dispatcher.rs | 45 +++++++++-------- crates/manager-core/src/invoke_service.rs | 8 +++ crates/manager-core/src/trigger_repo.rs | 3 +- crates/orchestrator-core/src/api.rs | 15 ++++-- crates/picloud-cli/tests/enabled.rs | 49 +++++++++++++++++++ 6 files changed, 155 insertions(+), 24 deletions(-) diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 95cbbe0..dc8bdae 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -473,6 +473,9 @@ impl ApplyService { // deployed-but-unreachable. Not an error (it's valid desired state), // but surfaced so the operator isn't surprised by a silent 404. report.warnings.extend(disabled_target_warnings(bundle)); + report + .warnings + .extend(unreachable_endpoint_warnings(bundle)); let bundle_scripts: HashMap = bundle .scripts @@ -1426,6 +1429,31 @@ fn reject_reserved_path(path: &str) -> Result<(), ApplyError> { Ok(()) } +/// §4.7 reachability warning: an *enabled* endpoint script with no route and +/// no trigger has no event surface — it's only reachable via the +/// execute-by-id bypass / `invoke()`. Modules are exempt (never invoked +/// directly); disabled endpoints are intentionally inert, so skip them. +fn unreachable_endpoint_warnings(bundle: &Bundle) -> Vec { + let bound: HashSet<&str> = bundle + .routes + .iter() + .map(|r| r.script.as_str()) + .chain(bundle.triggers.iter().map(BundleTrigger::script)) + .collect(); + bundle + .scripts + .iter() + .filter(|s| s.kind == ScriptKind::Endpoint && s.enabled && !bound.contains(s.name.as_str())) + .map(|s| { + format!( + "endpoint `{}` has no route or trigger — only reachable via the \ + execute-by-id bypass / invoke()", + s.name + ) + }) + .collect() +} + /// §4.7 reachability warnings: an *enabled* route or trigger bound to a /// script the manifest marks *disabled* is deployed but unreachable (the /// route 404s, the trigger won't fire). Valid desired state, so a warning — @@ -2179,6 +2207,37 @@ mod tests { assert!(disabled_target_warnings(&b).is_empty()); } + #[test] + fn warns_on_unreachable_endpoint() { + // §4.7: an enabled endpoint with no route and no trigger is flagged. + let mut b = empty_bundle(); + b.scripts = vec![bundle_script("orphan", "x")]; + let w = unreachable_endpoint_warnings(&b); + assert!( + w.iter().any(|m| m.contains("no route or trigger")), + "expected an unreachable-endpoint warning: {w:?}" + ); + // Bound by a route → no warning. + b.routes = vec![BundleRoute { + script: "orphan".into(), + method: None, + host_kind: HostKind::Any, + host: String::new(), + host_param_name: None, + path_kind: PathKind::Exact, + path: "/o".into(), + dispatch_mode: DispatchMode::Sync, + enabled: true, + }]; + assert!(unreachable_endpoint_warnings(&b).is_empty()); + // A module is exempt even with no binding. + let mut m = empty_bundle(); + let mut lib = bundle_script("lib", "x"); + lib.kind = ScriptKind::Module; + m.scripts = vec![lib]; + assert!(unreachable_endpoint_warnings(&m).is_empty()); + } + #[test] fn trigger_diff_create_noop_delete() { let s = script("h", "x"); diff --git a/crates/manager-core/src/dispatcher.rs b/crates/manager-core/src/dispatcher.rs index 99da1fe..053b3b5 100644 --- a/crates/manager-core/src/dispatcher.rs +++ b/crates/manager-core/src/dispatcher.rs @@ -611,23 +611,6 @@ impl Dispatcher { | OutboxSourceKind::Pubsub | OutboxSourceKind::Email => { let resolved = self.resolve_trigger(&row).await?; - // §4.3 fire-time re-check: if the trigger or its target script - // was disabled after this row was enqueued, drop the row rather - // than fire a stale event. Closes the outbox gap where the - // match-time `enabled` check can't see a later toggle. - if !resolved.active { - tracing::debug!( - outbox_id = %row.id, - app_id = %row.app_id, - "trigger or target script disabled since enqueue; dropping row" - ); - self.outbox - .delete(row.id) - .await - .map_err(|e| DispatcherError::Outbox(e.to_string()))?; - drop(permit); - return Ok(()); - } let req = match self.build_exec_request(&row, &resolved).await { Ok(req) => req, Err(err) => { @@ -644,6 +627,26 @@ impl Dispatcher { } }; + // §4.3 fire-time re-check, for EVERY outbox source (trigger, async + // HTTP/202, and invoke): if the target script (or, for triggers, the + // trigger) was disabled after this row was enqueued, drop it rather + // than fire a stale event. The match-time `enabled` check can't see a + // later toggle, so this is the gate that makes a disabled script + // genuinely non-invocable on the async paths too. + if !resolved.active { + tracing::debug!( + outbox_id = %row.id, + app_id = %row.app_id, + "target script/trigger disabled since enqueue; dropping outbox row" + ); + self.outbox + .delete(row.id) + .await + .map_err(|e| DispatcherError::Outbox(e.to_string()))?; + drop(permit); + return Ok(()); + } + // The gate permit auto-releases when this scope ends or when // the executor finishes. We hand control to the executor and // wait synchronously here — sync HTTP and dispatcher share the @@ -863,7 +866,9 @@ impl Dispatcher { let resolved = ResolvedTrigger { trigger_kind: TriggerKind::Kv, // placeholder; HTTP doesn't have a kind is_dead_letter_handler: false, - active: true, + // §4.3: an async-HTTP (202) row whose script was disabled after + // enqueue is dropped at fire time by the post-match active check. + active: script.enabled, script_id, script_source: script.source, script_name: payload.script_name, @@ -961,7 +966,9 @@ impl Dispatcher { let resolved = ResolvedTrigger { trigger_kind: TriggerKind::Cron, // placeholder; not used downstream is_dead_letter_handler: false, - active: true, + // §4.3: a queued invoke() whose target script was disabled after + // enqueue is dropped at fire time by the post-match active check. + active: script.enabled, script_id: script.id, script_source: script.source, script_name: script.name, diff --git a/crates/manager-core/src/invoke_service.rs b/crates/manager-core/src/invoke_service.rs index b0fe4ba..68ee612 100644 --- a/crates/manager-core/src/invoke_service.rs +++ b/crates/manager-core/src/invoke_service.rs @@ -83,6 +83,11 @@ impl InvokeServiceImpl { if script.app_id != cx.app_id { return Err(InvokeError::CrossApp); } + if !script.enabled { + // §4.3: a disabled script is not invocable via any path. Surface as + // NotFound (indistinguishable from absent), like the data plane. + return Err(InvokeError::NotFound(format!("id {script_id}"))); + } Ok(ResolvedScript { script_id: script.id, app_id: script.app_id, @@ -103,6 +108,9 @@ impl InvokeServiceImpl { .await .map_err(|e| InvokeError::Backend(e.to_string()))? .ok_or_else(|| InvokeError::NotFound(format!("name {name:?}")))?; + if !script.enabled { + return Err(InvokeError::NotFound(format!("name {name:?}"))); + } Ok(ResolvedScript { script_id: script.id, app_id: script.app_id, diff --git a/crates/manager-core/src/trigger_repo.rs b/crates/manager-core/src/trigger_repo.rs index 244af2a..584a828 100644 --- a/crates/manager-core/src/trigger_repo.rs +++ b/crates/manager-core/src/trigger_repo.rs @@ -1532,7 +1532,8 @@ impl TriggerRepo for PostgresTriggerRepo { t.registered_by_principal \ FROM triggers t \ JOIN queue_trigger_details d ON d.trigger_id = t.id \ - WHERE t.kind = 'queue' AND t.enabled = TRUE", + JOIN scripts s ON s.id = t.script_id \ + WHERE t.kind = 'queue' AND t.enabled = TRUE AND s.enabled = TRUE", ) .fetch_all(&self.pool) .await?; diff --git a/crates/orchestrator-core/src/api.rs b/crates/orchestrator-core/src/api.rs index 4e49588..4ae94a4 100644 --- a/crates/orchestrator-core/src/api.rs +++ b/crates/orchestrator-core/src/api.rs @@ -227,11 +227,18 @@ where .resolve(matched.matched.script_id) .await? .ok_or(ApiError::NotFound(matched.matched.script_id))?; - // An enabled route bound to a disabled script (§4.3, §4.7) is - // unreachable: 404 as if no route matched (the disabled route is already - // dropped from the match table; this covers the route-on/script-off case). + // An enabled route bound to a disabled script (§4.3, §4.7) is unreachable. + // Return the SAME flat "no route matches" 404 as the unmatched case — not + // `NotFound(script_id)`, which would both leak the internal script id to an + // anonymous caller and be distinguishable from absent. if !script.enabled { - return Err(ApiError::NotFound(matched.matched.script_id)); + return Ok(( + StatusCode::NOT_FOUND, + Json(serde_json::json!({ + "error": format!("no route matches {method} {path}") + })), + ) + .into_response()); } // Drain the body now that we know we'll execute. 10 MiB cap matches diff --git a/crates/picloud-cli/tests/enabled.rs b/crates/picloud-cli/tests/enabled.rs index a0041f1..c824627 100644 --- a/crates/picloud-cli/tests/enabled.rs +++ b/crates/picloud-cli/tests/enabled.rs @@ -125,6 +125,55 @@ fn disabling_a_route_makes_it_404_then_reenable() { ); } +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn enabled_route_to_disabled_script_404s_flatly() { + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let slug = common::unique_slug("enbl-os"); + common::pic_as(&env) + .args(["apps", "create", &slug]) + .assert() + .success(); + let _guard = AppGuard::new(&env.url, &env.token, &slug); + let host = format!("{slug}.test"); + common::pic_as(&env) + .args(["apps", "domains", "add", &slug, &host]) + .assert() + .success(); + + let dir = TempDir::new().unwrap(); + fs::create_dir_all(dir.path().join("scripts")).unwrap(); + fs::write(dir.path().join("scripts/hello.rhai"), "\"hi\"").unwrap(); + let manifest_path = dir.path().join("picloud.toml"); + // Route stays enabled; the SCRIPT it binds is disabled (route-on/script-off). + fs::write( + &manifest_path, + format!( + "[app]\nslug = \"{slug}\"\nname = \"OS\"\n\n\ + [[scripts]]\nname = \"hello\"\nfile = \"scripts/hello.rhai\"\nenabled = false\n\n\ + [[routes]]\nscript = \"hello\"\nmethod = \"GET\"\n\ + host_kind = \"any\"\npath_kind = \"exact\"\npath = \"/hello\"\n" + ), + ) + .unwrap(); + apply(&env, &manifest_path); + + // 404, and the body must be the flat "no route matches" form — never the + // internal script id (no info leak; indistinguishable from absent). + let client = reqwest::blocking::Client::new(); + let resp = client + .get(format!("{}/hello", env.url)) + .header(reqwest::header::HOST, &host) + .send() + .unwrap(); + assert_eq!(resp.status().as_u16(), 404); + let body = resp.text().unwrap(); + assert!(body.contains("no route matches"), "flat 404 body: {body}"); +} + fn apply(env: &common::TestEnv, manifest_path: &Path) { common::pic_as(env) .args(["apply", "--file"])