From d08df88df53168d0e56193491c5f2ee7f054f9ea Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Mon, 13 Jul 2026 21:20:20 +0200 Subject: [PATCH] =?UTF-8?q?fix(interceptors):=20harden=20the=20=C2=A79.4?= =?UTF-8?q?=20slice=20=E2=80=94=20seal,=20fail-closed,=20re-entrancy,=20sh?= =?UTF-8?q?ared=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An end-to-end audit of the (unmerged) interceptor slice surfaced seven ways a KV allow/deny guard could be silently defeated or misbehave. Close all of them: 1. Shared-collection bypass — `kv::shared_collection(...).set/delete` skipped the hook entirely, so any `(kv, set/delete)` guard was circumvented by choosing the shared handle. `GroupKvHandle` now runs the same before-op hook. 2. Seal to the declaring owner — the marker resolved nearest-owner-wins but its script name was then re-resolved on the CALLER's chain, so a descendant app could shadow a group's mandatory guard with a same-named local script. `resolve_before` now returns the script sealed to the owner that declared the marker (one LEFT JOIN in manager-core), so the executor never re-resolves by name. A descendant can only override with its OWN explicit marker. 3. Re-entrancy — an "allow + audit-log" guard that itself wrote KV re-triggered itself to the depth cap and then DENIED the original write (plus ~8x resolve/compile/execute amplification). A thread-local guard makes a write performed by an interceptor bypass interception. 4. Fail-closed verdict — only `#{ allowed: false }` denied; a bare bool, a typo'd key, a non-bool, or a bare unit all ALLOWED. Now allow ONLY on an explicit `#{ allowed: true }`; every other shape denies. 5/6. Fail-closed edges — a dangling (missing/disabled) interceptor script and a missing engine back-reference now DENY instead of allowing. 7. AppInvoke coupling — resolution no longer routes through `invoke.resolve`, so installing a guard no longer denies writes to principals who hold KV-write but not AppInvoke. The seam (`InterceptorService::resolve_before`) now returns an `InterceptorResolution { None | Dangling | Run(ResolvedScript) }`; the executor runs the sealed script straight through the shared `run_resolved_blocking` core and drops its `InvokeService` dependency. executor-core stays Postgres-free. No migration change (0073 unchanged). Pinned by three new journeys in tests/interceptors.rs: the seal (a same-named app script does NOT shadow the group's guard), shared-collection coverage, and the fail-closed verdict. Full journey suite 157/157. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/executor-core/src/sdk/interceptor.rs | 168 +++++++++----- crates/executor-core/src/sdk/kv.rs | 48 +++- crates/manager-core/src/interceptor_repo.rs | 92 +++++++- .../manager-core/src/interceptor_service.rs | 40 +++- crates/picloud-cli/tests/interceptors.rs | 205 ++++++++++++++++++ crates/shared/src/interceptor.rs | 51 +++-- crates/shared/src/lib.rs | 2 +- 7 files changed, 504 insertions(+), 102 deletions(-) diff --git a/crates/executor-core/src/sdk/interceptor.rs b/crates/executor-core/src/sdk/interceptor.rs index 4f819cb..e6adad7 100644 --- a/crates/executor-core/src/sdk/interceptor.rs +++ b/crates/executor-core/src/sdk/interceptor.rs @@ -1,18 +1,31 @@ //! §9.4 Service Interceptors — the executor-side before-op hook. //! -//! MVP: `kv::set` / `kv::delete` run an allow/deny interceptor first. The hook -//! (1) resolves the nearest interceptor script name for `(service, op)` on the -//! calling app's chain via the injected `InterceptorService` (a cheap indexed -//! query; `None` = un-hooked → allow, no further work), then (2) resolves that -//! name to a script and runs it through the SAME `invoke()` re-entry path -//! (`run_resolved_blocking`) — no second dispatch mechanism. The interceptor -//! receives the operation context as its request body and returns a map; the -//! op is DENIED iff that map has `allowed == false` (fail-open on any other -//! shape, documented — allow/deny only, no data transform). +//! MVP: `kv::set` / `kv::delete` (private AND group-shared collections) run an +//! allow/deny interceptor first. The hook resolves the nearest interceptor for +//! `(service, op)` on the calling app's chain via the injected +//! `InterceptorService` (a cheap indexed query; `None` = un-hooked → allow, no +//! further work). The resolver returns a script already **sealed to the owner +//! that declared the marker** and fully materialized, so we run it straight +//! through the SAME `invoke()` re-entry core (`run_resolved_blocking`) — no +//! second dispatch mechanism, no re-resolution by name (which would let a +//! descendant app shadow a group's guard). +//! +//! **Fail closed.** Everything but an explicit allow denies: +//! - a registered marker whose script is missing/disabled → deny; +//! - a registered marker with no runnable engine back-reference → deny; +//! - a return value that is not `#{ allowed: true }` → deny. +//! +//! Only a total absence of any marker allows without running anything. +//! +//! **Re-entrancy.** An interceptor that itself performs the guarded op (e.g. +//! an "allow + audit-log" hook that writes KV) must not re-trigger itself. A +//! thread-local guard, set for the duration of the interceptor's synchronous +//! execution, makes any nested op the interceptor performs bypass interception. +use std::cell::Cell; use std::sync::Arc; -use picloud_shared::{InterceptorService, InvokeService, InvokeTarget, SdkCallCx}; +use picloud_shared::{InterceptorResolution, InterceptorService, SdkCallCx}; use rhai::EvalAltResult; use serde_json::{json, Value as Json}; use tokio::runtime::Handle as TokioHandle; @@ -22,14 +35,41 @@ use crate::sandbox::Limits; use crate::sdk::bridge::runtime_err; use crate::sdk::invoke::run_resolved_blocking; +thread_local! { + /// Re-entrancy depth: `> 0` while an interceptor script (and anything it + /// synchronously invokes) is executing on this thread. The whole re-entry + /// runs synchronously in the caller's `spawn_blocking` thread (same model + /// as `invoke()`), so a thread-local is sufficient and correct. + static IN_INTERCEPTOR: Cell = const { Cell::new(0) }; +} + +fn currently_in_interceptor() -> bool { + IN_INTERCEPTOR.with(|c| c.get() > 0) +} + +/// RAII: increment the re-entrancy counter while an interceptor runs, decrement +/// on drop (including on the `?`/panic-unwind paths). +struct ReentryGuard; +impl ReentryGuard { + fn enter() -> Self { + IN_INTERCEPTOR.with(|c| c.set(c.get().saturating_add(1))); + Self + } +} +impl Drop for ReentryGuard { + fn drop(&mut self) { + IN_INTERCEPTOR.with(|c| c.set(c.get().saturating_sub(1))); + } +} + /// Run the before-op interceptor for `(service, op)` if one is registered. /// Returns `Ok(())` to allow the operation (un-hooked, or the interceptor -/// allowed it) or an `Err` runtime error to deny it (the caller must NOT then -/// perform the write). `value` is the payload being written (`None` for delete). +/// explicitly allowed it) or an `Err` runtime error to deny it (the caller must +/// NOT then perform the write). `value` is the payload being written (`None` +/// for delete). #[allow(clippy::too_many_arguments)] pub(super) fn run_before( interceptors: &Arc, - invoke: &Arc, self_engine: Option<&Arc>, cx: &Arc, limits: Limits, @@ -39,45 +79,57 @@ pub(super) fn run_before( key: &str, value: Option<&Json>, ) -> Result<(), Box> { + // Re-entrancy break: writes performed BY an interceptor (or anything it + // invokes) are not themselves intercepted — otherwise an "allow + write" + // hook would recurse to the depth cap and deny the original op. + if currently_in_interceptor() { + return Ok(()); + } + let handle = TokioHandle::try_current() .map_err(|e| runtime_err(&format!("{service} interceptor: no tokio runtime: {e}")))?; - // (1) Resolve the nearest interceptor script name. Un-hooked → allow. - let name = { + // (1) Resolve the nearest interceptor, sealed to its declaring owner. + let resolution = { let interceptors = interceptors.clone(); let cx = cx.clone(); handle .block_on(async move { interceptors.resolve_before(&cx, service, op).await }) .map_err(|e| runtime_err(&format!("{service}::{op} interceptor resolve: {e}")))? }; - let Some(name) = name else { return Ok(()) }; - - // An interceptor is registered but the engine back-reference isn't installed - // (a bare test engine without `set_self_weak`): there is no way to run it, so - // skip rather than block a write. Production always installs the back-ref. - let Some(self_engine) = self_engine else { - return Ok(()); + let resolved = match resolution { + // Un-hooked: the ONLY path that allows without running anything. + InterceptorResolution::None => return Ok(()), + // A guard is declared but its script is missing/disabled — fail closed. + InterceptorResolution::Dangling(name) => { + return Err(runtime_err(&format!( + "{service}::{op} denied: interceptor script `{name}` is missing or disabled" + ))); + } + InterceptorResolution::Run(r) => r, }; - // Depth bound (shared with invoke / trigger fan-out): an interceptor that - // itself writes and re-enters can't recurse past the ceiling. + // A guard is registered but the engine back-reference isn't installed (a + // bare test engine without `set_self_weak`): we cannot run it, so DENY — + // a resolvable guard we can't execute must not fail open. Production always + // installs the back-ref. + let Some(self_engine) = self_engine else { + return Err(runtime_err(&format!( + "{service}::{op} denied: interceptor `{}` cannot run (engine back-reference not installed)", + resolved.name + ))); + }; + + // Depth bound (shared with invoke / trigger fan-out). if cx.trigger_depth + 1 > limits.trigger_depth_max { return Err(runtime_err(&format!( - "{service}::{op} interceptor `{name}`: depth limit exceeded (max {})", - limits.trigger_depth_max + "{service}::{op} interceptor `{}`: depth limit exceeded (max {})", + resolved.name, limits.trigger_depth_max ))); } - // (2) Resolve the interceptor script by name on the caller's chain and run - // it with the operation context as its request body. - let resolved = { - let invoke = invoke.clone(); - let cx = cx.clone(); - let target = InvokeTarget::Name(name.clone()); - handle - .block_on(async move { invoke.resolve(&cx, target).await }) - .map_err(|e| runtime_err(&format!("{service}::{op} interceptor `{name}`: {e}")))? - }; + // (2) Run the sealed script with the operation context as its request body, + // under the re-entrancy guard so its own writes bypass interception. let payload = json!({ "service": service, "action": op, @@ -87,25 +139,35 @@ pub(super) fn run_before( "caller_script_id": cx.script_id.to_string(), "caller_execution_id": cx.execution_id.to_string(), }); - let ret = run_resolved_blocking( - self_engine, - cx, - &resolved, - payload, - &format!("{service}::{op} interceptor `{name}`"), - )?; + let ret = { + let _guard = ReentryGuard::enter(); + run_resolved_blocking( + self_engine, + cx, + &resolved, + payload, + &format!("{service}::{op} interceptor `{}`", resolved.name), + )? + }; - // Deny iff the interceptor returned a map with `allowed == false`. + // Fail-closed verdict: allow ONLY on an explicit `#{ allowed: true }`. A + // bare bool, a typo'd key, a non-bool `allowed`, a bare unit, or any other + // shape DENIES — a control whose job is to deny must not allow by omission. if let Json::Object(m) = &ret { - if m.get("allowed") == Some(&Json::Bool(false)) { - let reason = m - .get("reason") - .and_then(Json::as_str) - .unwrap_or("denied by interceptor"); - return Err(runtime_err(&format!( - "{service}::{op} denied by interceptor `{name}`: {reason}" - ))); + if m.get("allowed") == Some(&Json::Bool(true)) { + return Ok(()); } + let reason = m + .get("reason") + .and_then(Json::as_str) + .unwrap_or("denied by interceptor"); + return Err(runtime_err(&format!( + "{service}::{op} denied by interceptor `{}`: {reason}", + resolved.name + ))); } - Ok(()) + Err(runtime_err(&format!( + "{service}::{op} denied by interceptor `{}`: no allow verdict returned", + resolved.name + ))) } diff --git a/crates/executor-core/src/sdk/kv.rs b/crates/executor-core/src/sdk/kv.rs index 42d4f3e..94fc1b8 100644 --- a/crates/executor-core/src/sdk/kv.rs +++ b/crates/executor-core/src/sdk/kv.rs @@ -30,9 +30,7 @@ use std::sync::Arc; -use picloud_shared::{ - GroupKvService, InterceptorService, InvokeService, KvService, SdkCallCx, Services, -}; +use picloud_shared::{GroupKvService, InterceptorService, KvService, SdkCallCx, Services}; use rhai::{Array, Dynamic, Engine as RhaiEngine, EvalAltResult, Map, Module}; use super::bridge::{block_on, dynamic_to_json, json_to_dynamic}; @@ -48,7 +46,6 @@ pub struct KvHandle { service: Arc, cx: Arc, interceptors: Arc, - invoke: Arc, self_engine: Option>, limits: Limits, } @@ -57,12 +54,17 @@ pub struct KvHandle { /// Rhai type from `KvHandle` so the method set can diverge (e.g. shared /// collections never grow triggers) and a script's choice of private-vs-shared /// scope is explicit. Routes through the `GroupKvService`, which resolves the -/// owning group from `cx.app_id` (the script never names a group). +/// owning group from `cx.app_id` (the script never names a group). Carries the +/// §9.4 interceptor deps too so a `(kv, set/delete)` guard covers shared- +/// collection writes — otherwise the shared handle would be a silent bypass. #[derive(Clone)] pub struct GroupKvHandle { collection: String, service: Arc, cx: Arc, + interceptors: Arc, + self_engine: Option>, + limits: Limits, } pub(super) fn register( @@ -75,7 +77,6 @@ pub(super) fn register( let kv_service = services.kv.clone(); let group_kv_service = services.group_kv.clone(); let interceptors = services.interceptors.clone(); - let invoke = services.invoke.clone(); // `kv::collection(name)` / `kv::shared_collection(name)` — both constructors live in // the `kv` static module so the script-visible calls are `kv::collection` @@ -85,7 +86,6 @@ pub(super) fn register( let kv_service = kv_service.clone(); let cx = cx.clone(); let interceptors = interceptors.clone(); - let invoke = invoke.clone(); let self_engine = self_engine.clone(); module.set_native_fn( "collection", @@ -98,7 +98,6 @@ pub(super) fn register( service: kv_service.clone(), cx: cx.clone(), interceptors: interceptors.clone(), - invoke: invoke.clone(), self_engine: self_engine.clone(), limits, }) @@ -108,6 +107,8 @@ pub(super) fn register( { let group_kv_service = group_kv_service.clone(); let cx = cx.clone(); + let interceptors = interceptors.clone(); + let self_engine = self_engine.clone(); module.set_native_fn( "shared_collection", move |name: &str| -> Result> { @@ -118,6 +119,9 @@ pub(super) fn register( collection: name.to_string(), service: group_kv_service.clone(), cx: cx.clone(), + interceptors: interceptors.clone(), + self_engine: self_engine.clone(), + limits, }) }, ); @@ -180,7 +184,6 @@ fn register_set(engine: &mut RhaiEngine) { // the write below never runs. super::interceptor::run_before( &handle.interceptors, - &handle.invoke, handle.self_engine.as_ref(), &handle.cx, handle.limits, @@ -237,7 +240,6 @@ fn register_delete(engine: &mut RhaiEngine) { |handle: &mut KvHandle, key: &str| -> Result> { super::interceptor::run_before( &handle.interceptors, - &handle.invoke, handle.self_engine.as_ref(), &handle.cx, handle.limits, @@ -255,6 +257,27 @@ fn register_delete(engine: &mut RhaiEngine) { ); } +/// §9.4: run the before-op interceptor for a shared-collection write, so a +/// `(kv, set/delete)` guard covers the shared handle too (not just private KV). +fn group_run_before( + handle: &GroupKvHandle, + op: &'static str, + key: &str, + value: Option<&serde_json::Value>, +) -> Result<(), Box> { + super::interceptor::run_before( + &handle.interceptors, + handle.self_engine.as_ref(), + &handle.cx, + handle.limits, + "kv", + op, + &handle.collection, + key, + value, + ) +} + fn register_list(engine: &mut RhaiEngine) { // Zero-arg form — full page, no cursor. engine.register_fn( @@ -320,8 +343,10 @@ fn register_group_set(engine: &mut RhaiEngine) { engine.register_fn( "set", |handle: &mut GroupKvHandle, key: &str, value: Dynamic| -> Result<(), Box> { - let h = handle.clone(); let json = dynamic_to_json(&value); + // §9.4 before-op interceptor also guards shared-collection writes. + group_run_before(handle, "set", key, Some(&json))?; + let h = handle.clone(); block_on("kv", async move { h.service.set(&h.cx, &h.collection, key, json).await }) @@ -363,6 +388,7 @@ fn register_group_delete(engine: &mut RhaiEngine) { engine.register_fn( "delete", |handle: &mut GroupKvHandle, key: &str| -> Result> { + group_run_before(handle, "delete", key, None)?; let h = handle.clone(); block_on("kv", async move { h.service.delete(&h.cx, &h.collection, key).await diff --git a/crates/manager-core/src/interceptor_repo.rs b/crates/manager-core/src/interceptor_repo.rs index d53b76d..407c92a 100644 --- a/crates/manager-core/src/interceptor_repo.rs +++ b/crates/manager-core/src/interceptor_repo.rs @@ -7,8 +7,10 @@ //! functions over `&PgPool` / `&mut Transaction`, keyed by [`ScriptOwner`]), //! plus the runtime [`resolve_before`] chain walk (nearest-owner-wins). +use chrono::{DateTime, Utc}; use picloud_shared::{AppId, ScriptOwner}; use sqlx::{PgPool, Postgres, Transaction}; +use uuid::Uuid; use crate::config_resolver::CHAIN_LEVELS_CTE; @@ -84,29 +86,95 @@ pub async fn list_on_app_chain( .collect()) } -/// Resolve the interceptor script guarding `(service, op)` for a calling app — -/// the nearest declaration on the app's chain (app, then nearest ancestor -/// group). `None` when un-hooked. **This chain walk is the resolution boundary** -/// — a sibling-subtree app never sees another subtree's interceptor. +/// The nearest interceptor marker on an app's chain, with its script resolved +/// **at the declaring owner** (the seal). `script` is `None` when the marker +/// names a script that is missing or disabled at that owner — a dangling hook +/// the caller must fail closed on. +#[derive(Debug, Clone)] +pub struct SealedInterceptor { + pub marker_app: Option, + pub marker_group: Option, + pub script_name: String, + /// `(script_id, source, updated_at)` of the sealed script, or `None` if it + /// is missing/disabled at the declaring owner. + pub script: Option<(Uuid, String, DateTime)>, +} + +impl SealedInterceptor { + /// The owner that DECLARED the marker (and therefore owns the resolved + /// script) — the lexical origin for the interceptor's own `import`s (§5.5). + #[must_use] + pub fn sealing_owner(&self) -> ScriptOwner { + if let Some(g) = self.marker_group { + ScriptOwner::Group(g.into()) + } else { + ScriptOwner::App( + self.marker_app + .expect("marker XOR: app when not group") + .into(), + ) + } + } +} + +/// Resolve the interceptor guarding `(service, op)` for a calling app — the +/// nearest MARKER on the app's chain (app, then nearest ancestor group), with +/// its script **sealed to the owner that declared the marker** (a descendant +/// app cannot shadow it with a same-named local script). `None` when un-hooked. +/// **This chain walk is the resolution boundary** — a sibling-subtree app never +/// sees another subtree's interceptor. The script join is `LEFT` so a marker +/// whose script is missing/disabled still returns a row (with `script: None`) +/// rather than silently falling through to a farther, weaker marker. pub async fn resolve_before( pool: &PgPool, app_id: AppId, service: &str, op: &str, -) -> Result, sqlx::Error> { - let row: Option<(String,)> = sqlx::query_as(&format!( - "{CHAIN_LEVELS_CTE} \ - SELECT i.interceptor_script FROM chain c \ - JOIN interceptors i ON (i.app_id = c.app_owner OR i.group_id = c.group_owner) \ - WHERE i.service = $2 AND i.op = $3 \ - ORDER BY c.depth ASC LIMIT 1", +) -> Result, sqlx::Error> { + #[allow(clippy::type_complexity)] + let row: Option<( + Option, + Option, + String, + Option, + Option, + Option>, + )> = sqlx::query_as(&format!( + "{CHAIN_LEVELS_CTE}, \ + nearest AS ( \ + SELECT i.interceptor_script AS script_name, \ + i.app_id AS marker_app, i.group_id AS marker_group, c.depth AS depth \ + FROM chain c \ + JOIN interceptors i ON (i.app_id = c.app_owner OR i.group_id = c.group_owner) \ + WHERE i.service = $2 AND i.op = $3 \ + ORDER BY c.depth ASC LIMIT 1 \ + ) \ + SELECT n.marker_app, n.marker_group, n.script_name, \ + s.id, s.source, s.updated_at \ + FROM nearest n \ + LEFT JOIN scripts s ON ( \ + (n.marker_app IS NOT NULL AND s.app_id = n.marker_app \ + AND s.name = n.script_name AND s.enabled) \ + OR (n.marker_group IS NOT NULL AND s.group_id = n.marker_group \ + AND s.name = n.script_name AND s.enabled) \ + )", )) .bind(app_id.into_inner()) .bind(service) .bind(op) .fetch_optional(pool) .await?; - Ok(row.map(|(s,)| s)) + Ok(row.map( + |(marker_app, marker_group, script_name, sid, src, upd)| SealedInterceptor { + marker_app, + marker_group, + script_name, + script: match (sid, src, upd) { + (Some(id), Some(source), Some(updated_at)) => Some((id, source, updated_at)), + _ => None, + }, + }, + )) } /// Upsert a marker at `owner` in the apply transaction. A re-apply that changes diff --git a/crates/manager-core/src/interceptor_service.rs b/crates/manager-core/src/interceptor_service.rs index fde9997..ac0394c 100644 --- a/crates/manager-core/src/interceptor_service.rs +++ b/crates/manager-core/src/interceptor_service.rs @@ -1,12 +1,14 @@ //! `InterceptorServiceImpl` — the Postgres-backed §9.4 interceptor resolver -//! injected into `Services`. Resolve-only: it maps `(cx.app_id, service, op)` -//! to the nearest interceptor script name on the calling app's chain (via -//! [`crate::interceptor_repo::resolve_before`]). Running that script is the -//! executor's job (the `invoke()` re-entry path), which keeps `executor-core` -//! Postgres-free. +//! injected into `Services`. It maps `(cx.app_id, service, op)` to the nearest +//! interceptor marker on the calling app's chain and materializes its script +//! **sealed to the declaring owner** (via [`crate::interceptor_repo`]). Running +//! that script is the executor's job (the `invoke()` re-entry core), which keeps +//! `executor-core` Postgres-free. use async_trait::async_trait; -use picloud_shared::{InterceptorService, SdkCallCx}; +use picloud_shared::{ + InterceptorResolution, InterceptorService, ResolvedScript, ScriptId, SdkCallCx, +}; use sqlx::PgPool; pub struct InterceptorServiceImpl { @@ -27,11 +29,31 @@ impl InterceptorService for InterceptorServiceImpl { cx: &SdkCallCx, service: &str, op: &str, - ) -> Result, String> { + ) -> Result { // `app_id` derives from `cx` (never a script arg) — the isolation // boundary; the chain walk then scopes resolution to this app's subtree. - crate::interceptor_repo::resolve_before(&self.pool, cx.app_id, service, op) + let sealed = crate::interceptor_repo::resolve_before(&self.pool, cx.app_id, service, op) .await - .map_err(|e| e.to_string()) + .map_err(|e| e.to_string())?; + let Some(sealed) = sealed else { + return Ok(InterceptorResolution::None); + }; + // A marker exists but its sealed script is missing/disabled → dangling, + // the executor fails closed (denies) rather than silently allowing. + let Some((sid, source, updated_at)) = sealed.script.clone() else { + return Ok(InterceptorResolution::Dangling(sealed.script_name)); + }; + // The execution boundary is always the CALLER's app; the `owner` is the + // sealing (declaring) owner, the lexical origin for the interceptor's + // own `import`s (§5.5) — a group marker's script runs its imports from + // the group. + Ok(InterceptorResolution::Run(Box::new(ResolvedScript { + script_id: ScriptId::from(sid), + app_id: cx.app_id, + owner: Some(sealed.sealing_owner()), + source, + updated_at, + name: sealed.script_name, + }))) } } diff --git a/crates/picloud-cli/tests/interceptors.rs b/crates/picloud-cli/tests/interceptors.rs index e0ce498..ea4ed18 100644 --- a/crates/picloud-cli/tests/interceptors.rs +++ b/crates/picloud-cli/tests/interceptors.rs @@ -66,6 +66,30 @@ kv::collection("c").set("ok", 2); #{ denied: denied } "#; +/// A permissive guard an app might define to try to SHADOW a group's guard by +/// re-using its script name. The seal must make the group's guard win anyway. +const PERMISSIVE_GUARD: &str = r#"#{ allowed: true }"#; + +/// A guard that returns a map WITHOUT an `allowed` key — a malformed verdict. +/// The fail-closed contract must treat this as a deny. +const NO_VERDICT_GUARD: &str = r#"#{ note: "forgot to return allowed" }"#; + +/// A writer over a group SHARED collection (finding 1: the shared handle must +/// be intercepted too, not just `kv::collection`). +const SHARED_WRITER: &str = r#" +let denied = false; +try { kv::shared_collection("catalog").set("secret", 1); } catch(e) { denied = true; } +kv::shared_collection("catalog").set("ok", 2); +#{ denied: denied } +"#; + +/// A writer that only tries a single (guarded) set. +const ANY_WRITER: &str = r#" +let denied = false; +try { kv::collection("c").set("anything", 1); } catch(e) { denied = true; } +#{ denied: denied } +"#; + #[ignore = "needs DATABASE_URL pointing at a running Postgres"] #[test] fn app_interceptor_denies_a_guarded_kv_write_and_allows_others() { @@ -191,3 +215,184 @@ fn a_group_interceptor_is_inherited_by_a_descendant_app() { "a descendant app must inherit the group's interceptor" ); } + +/// The seal (finding 2): a group's interceptor script is resolved AT THE GROUP, +/// so a descendant app cannot defeat a mandatory guard by defining a permissive +/// script of the same name. The app declares no interceptor of its own — it just +/// shadows the script name — and the group's strict guard must still win. +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn a_descendant_cannot_shadow_a_group_interceptor_by_reusing_the_script_name() { + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let group = common::unique_slug("seal-grp"); + let app = common::unique_slug("seal-app"); + let _g = GroupGuard::new(&env.url, &env.token, &group); + common::pic_as(&env) + .args(["groups", "create", &group]) + .assert() + .success(); + + // The group owns the strict guard + the interceptor marker. + let dir = manifest_dir(); + fs::write(dir.path().join("scripts/guard.rhai"), GUARD).unwrap(); + fs::write( + dir.path().join("group.toml"), + format!( + "[group]\nslug = \"{group}\"\nname = \"Seal\"\n\n\ + [[scripts]]\nname = \"guard\"\nfile = \"scripts/guard.rhai\"\n\n\ + [[interceptors]]\nscript = \"guard\"\nops = [\"set\"]\n" + ), + ) + .unwrap(); + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(dir.path().join("group.toml")) + .assert() + .success(); + + // The app under the group defines its OWN script named `guard` (permissive) + // but declares NO interceptor. The seal must keep the group's guard in force. + let _a = AppGuard::new(&env.url, &env.token, &app); + common::pic_as(&env) + .args(["apps", "create", &app, "--group", &group]) + .assert() + .success(); + fs::write( + dir.path().join("scripts/guard_permissive.rhai"), + PERMISSIVE_GUARD, + ) + .unwrap(); + fs::write(dir.path().join("scripts/writer.rhai"), WRITER).unwrap(); + fs::write( + dir.path().join("app.toml"), + format!( + "[app]\nslug = \"{app}\"\nname = \"SealApp\"\n\n\ + [[scripts]]\nname = \"guard\"\nfile = \"scripts/guard_permissive.rhai\"\n\n\ + [[scripts]]\nname = \"writer\"\nfile = \"scripts/writer.rhai\"\n" + ), + ) + .unwrap(); + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(dir.path().join("app.toml")) + .assert() + .success(); + + let body = invoke_body(&env, &app_script_id(&env, &app, "writer")); + assert_eq!( + body, + serde_json::json!({ "denied": true }), + "the group's guard is sealed — a same-named app script must NOT shadow it" + ); +} + +/// Finding 1: a `(kv, set)` interceptor covers group SHARED-collection writes, +/// not just `kv::collection`. Otherwise `kv::shared_collection(...).set(...)` +/// would be a silent bypass of the guard. +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn an_interceptor_guards_shared_collection_writes() { + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let group = common::unique_slug("shc-grp"); + let app = common::unique_slug("shc-app"); + let _g = GroupGuard::new(&env.url, &env.token, &group); + common::pic_as(&env) + .args(["groups", "create", &group]) + .assert() + .success(); + + // The group owns a shared `catalog` collection, the guard, and the marker. + let dir = manifest_dir(); + fs::write(dir.path().join("scripts/guard.rhai"), GUARD).unwrap(); + fs::write( + dir.path().join("group.toml"), + format!( + "[group]\nslug = \"{group}\"\nname = \"Shc\"\n\ncollections = [\"catalog\"]\n\n\ + [[scripts]]\nname = \"guard\"\nfile = \"scripts/guard.rhai\"\n\n\ + [[interceptors]]\nscript = \"guard\"\nops = [\"set\"]\n" + ), + ) + .unwrap(); + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(dir.path().join("group.toml")) + .assert() + .success(); + + let _a = AppGuard::new(&env.url, &env.token, &app); + common::pic_as(&env) + .args(["apps", "create", &app, "--group", &group]) + .assert() + .success(); + fs::write(dir.path().join("scripts/shared_writer.rhai"), SHARED_WRITER).unwrap(); + fs::write( + dir.path().join("app.toml"), + format!( + "[app]\nslug = \"{app}\"\nname = \"ShcApp\"\n\n\ + [[scripts]]\nname = \"shared_writer\"\nfile = \"scripts/shared_writer.rhai\"\n" + ), + ) + .unwrap(); + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(dir.path().join("app.toml")) + .assert() + .success(); + + let body = invoke_body(&env, &app_script_id(&env, &app, "shared_writer")); + assert_eq!( + body, + serde_json::json!({ "denied": true }), + "the shared-collection write of `secret` must be denied by the interceptor" + ); +} + +/// Finding 4: the verdict is fail-closed. An interceptor that returns a map with +/// no `allowed` key (a malformed verdict) DENIES the op — a guard must never +/// allow by omission. +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn a_malformed_verdict_fails_closed() { + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let app = common::unique_slug("fc-app"); + let _a = AppGuard::new(&env.url, &env.token, &app); + common::pic_as(&env) + .args(["apps", "create", &app]) + .assert() + .success(); + + let dir = manifest_dir(); + fs::write(dir.path().join("scripts/guard.rhai"), NO_VERDICT_GUARD).unwrap(); + fs::write(dir.path().join("scripts/writer.rhai"), ANY_WRITER).unwrap(); + fs::write( + dir.path().join("picloud.toml"), + format!( + "[app]\nslug = \"{app}\"\nname = \"FC\"\n\n\ + [[scripts]]\nname = \"guard\"\nfile = \"scripts/guard.rhai\"\n\n\ + [[scripts]]\nname = \"writer\"\nfile = \"scripts/writer.rhai\"\n\n\ + [[interceptors]]\nscript = \"guard\"\nops = [\"set\"]\n" + ), + ) + .unwrap(); + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(dir.path().join("picloud.toml")) + .assert() + .success(); + + let body = invoke_body(&env, &app_script_id(&env, &app, "writer")); + assert_eq!( + body, + serde_json::json!({ "denied": true }), + "a verdict map with no `allowed` key must fail closed (deny)" + ); +} diff --git a/crates/shared/src/interceptor.rs b/crates/shared/src/interceptor.rs index 3486332..d4811c0 100644 --- a/crates/shared/src/interceptor.rs +++ b/crates/shared/src/interceptor.rs @@ -2,34 +2,53 @@ //! //! An interceptor is a script registered (declaratively, per app/group) to run //! **before** a data-plane operation and allow or deny it. This trait is the -//! narrow, executor-facing seam: it only RESOLVES which interceptor script (by -//! name) applies to a `(service, op)` on the calling app's chain — nearest-owner -//! wins, like extension points. Running the resolved script reuses the existing -//! `invoke()` re-entry path in `executor-core` (resolve the name → compile → -//! execute), so `executor-core` stays Postgres-free and there is no second -//! script-dispatch mechanism. +//! narrow, executor-facing seam: it RESOLVES the interceptor for a +//! `(service, op)` on the calling app's chain — the nearest MARKER wins +//! (nearest-owner, like extension points), and the marker's script is then +//! resolved **at the owner that declared it**. That seal is the compliance +//! guarantee: a descendant app cannot shadow a group's interceptor with a +//! same-named local script (contrast a bare `invoke()` name, which +//! nearest-owner-wins and *would* let the app win). Resolution returns a fully +//! materialized [`ResolvedScript`] so the executor runs it straight through the +//! existing `invoke()` re-entry core — no second dispatch mechanism, no re- +//! resolution by name, and `executor-core` stays Postgres-free. //! //! MVP scope (v1.2): `service = "kv"`, `op ∈ {set, delete}`, allow/deny only — //! no data transform, no chaining, no `after_*` hooks. See blueprint §9.4. use async_trait::async_trait; -use crate::SdkCallCx; +use crate::{ResolvedScript, SdkCallCx}; + +/// Outcome of resolving the before-op interceptor for a `(service, op)`. +/// +/// The three arms encode a **fail-closed** contract: a registered-but-broken +/// hook DENIES rather than silently allowing (only [`None`](Self::None) — no +/// marker at all — allows without running anything). +#[derive(Debug, Clone)] +pub enum InterceptorResolution { + /// No marker registered for `(service, op)` on the chain — allow, no work. + None, + /// A marker IS registered but its sealed script is missing or disabled. + /// The caller must DENY (a mis-declared guard must never fail open). + /// Carries the script name for the error message. + Dangling(String), + /// Run this script (sealed to the declaring owner) as the before-op hook. + Run(Box), +} #[async_trait] pub trait InterceptorService: Send + Sync { - /// The NAME of the interceptor script registered for `(service, op)` at the - /// nearest owner on `cx.app_id`'s chain (app, else nearest ancestor group), - /// or `None` when the operation is un-hooked. The executor resolves that - /// name to a script and runs it. `Err` is a backend failure (fail-closed: - /// the caller turns it into an operation error rather than silently - /// allowing). + /// Resolve the interceptor guarding `(service, op)` for the calling app — + /// the nearest marker on `cx.app_id`'s chain, its script sealed to the + /// declaring owner. `Err` is a backend failure (the caller fails closed: + /// turns it into an operation error rather than silently allowing). async fn resolve_before( &self, cx: &SdkCallCx, service: &str, op: &str, - ) -> Result, String>; + ) -> Result; } /// Default: nothing is ever intercepted. The shape every non-picloud `Services` @@ -45,7 +64,7 @@ impl InterceptorService for NoopInterceptorService { _cx: &SdkCallCx, _service: &str, _op: &str, - ) -> Result, String> { - Ok(None) + ) -> Result { + Ok(InterceptorResolution::None) } } diff --git a/crates/shared/src/lib.rs b/crates/shared/src/lib.rs index ba9b967..1b600e5 100644 --- a/crates/shared/src/lib.rs +++ b/crates/shared/src/lib.rs @@ -87,7 +87,7 @@ pub use ids::{ pub use inbox::{ InboxDeliveryOutcome, InboxFailureKind, InboxResolver, InboxResult, NoopInboxResolver, }; -pub use interceptor::{InterceptorService, NoopInterceptorService}; +pub use interceptor::{InterceptorResolution, InterceptorService, NoopInterceptorService}; pub use invoke::{InvokeError, InvokeService, InvokeTarget, NoopInvokeService, ResolvedScript}; pub use kv::{KvError, KvListPage, KvService, NoopKvService}; pub use log_sink::{ExecutionLogSink, LogSinkError};