diff --git a/crates/manager-core/src/dispatcher.rs b/crates/manager-core/src/dispatcher.rs index e6975a1..ab3a2cb 100644 --- a/crates/manager-core/src/dispatcher.rs +++ b/crates/manager-core/src/dispatcher.rs @@ -29,9 +29,9 @@ use picloud_executor_core::{ }; use picloud_orchestrator_core::{ExecutionGate, ExecutorClient}; use picloud_shared::{ - DeadLetterId, ExecResponseSummary, ExecutionId, ExecutionLogSink, ExecutionSource, + AppId, DeadLetterId, ExecResponseSummary, ExecutionId, ExecutionLogSink, ExecutionSource, HttpDispatchPayload, InboxDeliveryOutcome, InboxFailureKind, InboxResolver, InboxResult, - RequestId, ScriptId, ScriptSandbox, TriggerEvent, + RequestId, Script, ScriptId, ScriptSandbox, TriggerEvent, }; use rand::Rng; use uuid::Uuid; @@ -191,6 +191,32 @@ impl ExecLogContext { } impl Dispatcher { + /// Phase 4 isolation backstop: may `script` run under `app_id`'s context? + /// True when the script is owned by `app_id` directly, or by a group on + /// its ancestor chain (an inherited group script). App-owned is the + /// in-memory fast path — only a group-owned script pays the chain query, + /// and a query error fails closed (not invocable). This generalizes the + /// pre-Phase-4 same-app guard (`script.app_id == row.app_id`) to the + /// hierarchy without changing behavior for app-owned scripts. + async fn script_invocable(&self, script: &Script, app_id: AppId) -> bool { + if script.is_owned_by_app(app_id) { + return true; + } + if script.group_id.is_none() { + return false; + } + match self.scripts.is_invocable_by_app(script.id, app_id).await { + Ok(ok) => ok, + Err(e) => { + tracing::error!( + error = %e, script_id = %script.id, %app_id, + "chain-membership check failed; treating script as not invocable" + ); + false + } + } + } + /// Spawn the dispatcher loop as a detached `tokio::task`. Also /// spawns the v1.1.9 queue visibility-timeout reclaim task. Both /// run for the process lifetime; returned `JoinHandle`s are @@ -359,12 +385,12 @@ impl Dispatcher { // existing consumers. Dead-letter the message so the misfire is // observable rather than executing one app's script under // another app's SdkCallCx. - // Phase 4: a group-owned script (`app_id: None`) is not yet - // dispatchable here — the chain-membership check that authorizes - // inherited scripts lands with group-script binding. Until then - // `is_owned_by_app` is the exact app-owned guard as before and a - // group script fails closed (dead-lettered, not silently run). - if !script.is_owned_by_app(claimed.app_id) { + // Phase 4: the target must be invocable in the claimed message's app + // context — owned by that app or an inherited group script on its + // chain. App-owned scripts take the in-memory fast path; only a group + // script pays the chain query, and a non-member fails closed + // (dead-lettered, not silently run under another app's SdkCallCx). + if !self.script_invocable(&script, claimed.app_id).await { tracing::error!( script_app = ?script.app_id, claim_app = %claimed.app_id, @@ -791,9 +817,9 @@ impl Dispatcher { // boundary. Not reachable via the trigger-create or `apply` paths // (both resolve the script within the app's own scope), so this is // the runtime backstop the other arms already carry. - // Phase 4: group scripts (`app_id: None`) fail closed here until the - // chain-membership check lands with group-script binding. - if !script.is_owned_by_app(row.app_id) { + // Phase 4: invocable in the outbox row's app context (app-owned or an + // inherited group script on the chain); a non-member fails closed. + if !self.script_invocable(&script, row.app_id).await { return Err(DispatcherError::ResolveTrigger( "trigger outbox target belongs to a different app".into(), )); @@ -883,8 +909,8 @@ impl Dispatcher { // build_invoke_request and dispatch_one_queue. A hand-edited // outbox row could otherwise execute one app's script under // another app's SdkCallCx. - // Phase 4: group scripts fail closed here (see queue/trigger arms). - if !script.is_owned_by_app(row.app_id) { + // Phase 4: invocable in this app's context (see queue/trigger arms). + if !self.script_invocable(&script, row.app_id).await { return Err(DispatcherError::ResolveTrigger( "http outbox target belongs to a different app".into(), )); @@ -977,8 +1003,8 @@ impl Dispatcher { })?; // Same-app guard — the script could have been re-assigned or // the row could have been hand-edited. Reject mismatches. - // Phase 4: group scripts fail closed here (see queue/trigger arms). - if !script.is_owned_by_app(row.app_id) { + // Phase 4: invocable in this app's context (app-owned or inherited). + if !self.script_invocable(&script, row.app_id).await { return Err(DispatcherError::ResolveTrigger( "invoke target belongs to a different app".into(), )); @@ -1649,6 +1675,20 @@ mod tests { ) -> Result, ScriptRepositoryError> { unimplemented!("not used by this test") } + async fn get_by_name_inherited( + &self, + _app_id: AppId, + _name: &str, + ) -> Result, ScriptRepositoryError> { + unimplemented!("not used by this test") + } + async fn is_invocable_by_app( + &self, + _script_id: ScriptId, + app_id: AppId, + ) -> Result { + Ok(self.script.is_owned_by_app(app_id)) + } async fn list(&self) -> Result, ScriptRepositoryError> { unimplemented!("not used by this test") } diff --git a/crates/manager-core/src/invoke_service.rs b/crates/manager-core/src/invoke_service.rs index a324fd1..5b9af81 100644 --- a/crates/manager-core/src/invoke_service.rs +++ b/crates/manager-core/src/invoke_service.rs @@ -106,9 +106,13 @@ impl InvokeServiceImpl { cx: &SdkCallCx, name: &str, ) -> Result { + // Phase 4: resolve down the app's chain — an inherited group script + // of this name is reachable, nearest-owner-wins (an app's own script + // shadows it). The chain only contains the app + its ancestors, so the + // resolved script is invocable by `cx.app_id` by construction. let script = self .scripts - .get_by_name(cx.app_id, name) + .get_by_name_inherited(cx.app_id, name) .await .map_err(|e| InvokeError::Backend(e.to_string()))? .ok_or_else(|| InvokeError::NotFound(format!("name {name:?}")))?; @@ -117,8 +121,8 @@ impl InvokeServiceImpl { } Ok(ResolvedScript { script_id: script.id, - // Resolved within `cx.app_id`'s own scope (get_by_name filters by - // it), so the execution-context app is the caller's app. + // The execution-context app is always the caller's app — a group + // script runs under the inheriting app, never its owner. app_id: cx.app_id, source: script.source, updated_at: script.updated_at, @@ -240,6 +244,21 @@ mod tests { }, ) } + async fn get_by_name_inherited( + &self, + app_id: AppId, + name: &str, + ) -> Result, ScriptRepositoryError> { + // This one-script repo has no hierarchy; inherited == own-scope. + self.get_by_name(app_id, name).await + } + async fn is_invocable_by_app( + &self, + script_id: ScriptId, + _app_id: AppId, + ) -> Result { + Ok(script_id == self.script.id) + } async fn list(&self) -> Result, ScriptRepositoryError> { Ok(vec![self.script.clone()]) } diff --git a/crates/manager-core/src/repo.rs b/crates/manager-core/src/repo.rs index cb818ab..4d37517 100644 --- a/crates/manager-core/src/repo.rs +++ b/crates/manager-core/src/repo.rs @@ -34,6 +34,26 @@ pub trait ScriptRepository: Send + Sync { app_id: AppId, name: &str, ) -> Result, ScriptRepositoryError>; + /// Phase 4: resolve `name → Script` down `app_id`'s ownership chain — + /// the app itself, then its ancestor groups nearest-first. Nearest owner + /// wins (an app's own script shadows an inherited group one of the same + /// name: CoW). Returns `None` if no script in the chain has that name. + /// Backs inherited `invoke("name")` and declarative name→id binding. + async fn get_by_name_inherited( + &self, + app_id: AppId, + name: &str, + ) -> Result, ScriptRepositoryError>; + /// Phase 4: is `script_id` invocable in `app_id`'s context? True iff the + /// script is owned by `app_id` directly, OR owned by a group on `app_id`'s + /// ancestor chain. The cross-app isolation predicate generalized to the + /// hierarchy — the runtime backstop for trigger/queue/invoke dispatch and + /// trigger-bind validation. A missing script returns `false` (fail closed). + async fn is_invocable_by_app( + &self, + script_id: ScriptId, + app_id: AppId, + ) -> Result; /// Every script across all apps. Mostly for tests and admin /// "global" views; the dashboard reaches scripts via `list_for_app`. async fn list(&self) -> Result, ScriptRepositoryError>; @@ -97,6 +117,20 @@ impl ScriptRepository for std::sync::Arc { ) -> Result, ScriptRepositoryError> { (**self).get_by_name(app_id, name).await } + async fn get_by_name_inherited( + &self, + app_id: AppId, + name: &str, + ) -> Result, ScriptRepositoryError> { + (**self).get_by_name_inherited(app_id, name).await + } + async fn is_invocable_by_app( + &self, + script_id: ScriptId, + app_id: AppId, + ) -> Result { + (**self).is_invocable_by_app(script_id, app_id).await + } async fn list(&self) -> Result, ScriptRepositoryError> { (**self).list().await } @@ -253,6 +287,58 @@ impl ScriptRepository for PostgresScriptRepository { Ok(row.map(Into::into)) } + async fn get_by_name_inherited( + &self, + app_id: AppId, + name: &str, + ) -> Result, ScriptRepositoryError> { + // Walk app → ancestor groups (CHAIN_LEVELS_CTE binds $1 = app_id), + // join scripts owned at each level, and take the nearest (lowest + // depth) row named `name`. Case-insensitive to match the per-owner + // `LOWER(name)` unique indexes. Mirrors the vars/secrets resolvers. + let cols = SCRIPT_SELECT_COLS + .split(", ") + .map(|c| format!("s.{c}")) + .collect::>() + .join(", "); + let row = sqlx::query_as::<_, ScriptRow>(&format!( + "{cte} SELECT {cols} FROM scripts s \ + JOIN chain c ON (s.app_id = c.app_owner OR s.group_id = c.group_owner) \ + WHERE LOWER(s.name) = LOWER($2) \ + ORDER BY c.depth ASC \ + LIMIT 1", + cte = crate::config_resolver::CHAIN_LEVELS_CTE, + )) + .bind(app_id.into_inner()) + .bind(name) + .fetch_optional(&self.pool) + .await?; + Ok(row.map(Into::into)) + } + + async fn is_invocable_by_app( + &self, + script_id: ScriptId, + app_id: AppId, + ) -> Result { + // EXISTS over the same chain: the script's owner (app or group) must + // appear on `app_id`'s app→ancestor-group chain. CHAIN_LEVELS_CTE + // binds $1 = app_id; $2 = script_id. + let exists: (bool,) = sqlx::query_as(&format!( + "{cte} SELECT EXISTS ( \ + SELECT 1 FROM scripts s \ + JOIN chain c ON (s.app_id = c.app_owner OR s.group_id = c.group_owner) \ + WHERE s.id = $2 \ + )", + cte = crate::config_resolver::CHAIN_LEVELS_CTE, + )) + .bind(app_id.into_inner()) + .bind(script_id.into_inner()) + .fetch_one(&self.pool) + .await?; + Ok(exists.0) + } + async fn list(&self) -> Result, ScriptRepositoryError> { let rows = sqlx::query_as::<_, ScriptRow>(&format!( "SELECT {SCRIPT_SELECT_COLS} FROM scripts ORDER BY name" diff --git a/crates/manager-core/src/triggers_api.rs b/crates/manager-core/src/triggers_api.rs index 375c049..a93654d 100644 --- a/crates/manager-core/src/triggers_api.rs +++ b/crates/manager-core/src/triggers_api.rs @@ -289,10 +289,17 @@ async fn validate_trigger_target( .ok_or_else(|| { TriggersApiError::Invalid(format!("script {script_id} not found in this app")) })?; - // Phase 4: trigger targets are app-owned scripts; a group-owned script - // (`app_id: None`) fails closed here until group-script trigger binding - // lands in C3. - if !script.is_owned_by_app(app_id) { + // Phase 4: a trigger target must be invocable in this app's context — + // owned by the app, or by a group on the app's ancestor chain (inherited). + // App-owned is the in-memory fast path (no query); only a group-owned + // candidate pays the chain lookup. + let invocable = script.is_owned_by_app(app_id) + || (script.group_id.is_some() + && scripts + .is_invocable_by_app(script_id, app_id) + .await + .map_err(map_script_repo_err)?); + if !invocable { return Err(TriggersApiError::Invalid(format!( "script {script_id} does not belong to this app" ))); @@ -1400,6 +1407,26 @@ mod tests { .find(|s| s.app_id == Some(app_id) && s.name == name) .cloned()) } + async fn get_by_name_inherited( + &self, + app_id: AppId, + name: &str, + ) -> Result, crate::repo::ScriptRepositoryError> { + // No hierarchy in this in-memory repo; inherited == own-scope. + self.get_by_name(app_id, name).await + } + async fn is_invocable_by_app( + &self, + script_id: ScriptId, + app_id: AppId, + ) -> Result { + Ok(self + .existing + .lock() + .await + .get(&script_id) + .is_some_and(|s| s.is_owned_by_app(app_id))) + } async fn list( &self, ) -> Result, crate::repo::ScriptRepositoryError> {