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

fix(interceptors): harden the §9.4 slice — seal, fail-closed, re-entrancy, shared coverage

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-07-13 21:20:20 +02:00
parent 2fc9476f9e
commit d08df88df5
7 changed files with 504 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files

168
crates/executor-core/src/sdk/interceptor.rs
View File

@@ -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<u32> = 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<dyn InterceptorService>,
invoke: &Arc<dyn InvokeService>,
self_engine: Option<&Arc<Engine>>,
cx: &Arc<SdkCallCx>,
limits: Limits,
@@ -39,45 +79,57 @@ pub(super) fn run_before(
key: &str,
value: Option<&Json>,
) -> Result<(), Box<EvalAltResult>> {
// 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
)))
}

48
crates/executor-core/src/sdk/kv.rs
View File

@@ -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<dyn KvService>,
cx: Arc<SdkCallCx>,
interceptors: Arc<dyn InterceptorService>,
invoke: Arc<dyn InvokeService>,
self_engine: Option<Arc<Engine>>,
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<dyn GroupKvService>,
cx: Arc<SdkCallCx>,
interceptors: Arc<dyn InterceptorService>,
self_engine: Option<Arc<Engine>>,
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<GroupKvHandle, Box<EvalAltResult>> {
@@ -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<bool, Box<EvalAltResult>> {
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<EvalAltResult>> {
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<EvalAltResult>> {
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<bool, Box<EvalAltResult>> {
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