diff --git a/crates/manager-core/src/route_admin.rs b/crates/manager-core/src/route_admin.rs index 1184aac..7adea5e 100644 --- a/crates/manager-core/src/route_admin.rs +++ b/crates/manager-core/src/route_admin.rs @@ -409,13 +409,19 @@ pub async fn rebuild_route_table( table: &RouteTable, ) -> Result<(), ScriptRepositoryError> { let effective = routes.list_effective().await?; - // §11 tail: `(app_id, path)` route suppressions — an inherited route at a - // suppressed path is dropped from the app's slice (404). - let suppressed: std::collections::HashSet<(AppId, String)> = routes - .list_route_suppressions() - .await? - .into_iter() - .collect(); + // §11 tail: route suppressions keyed `(app_id, path)` → the SMALLEST + // suppressor depth seen for that key. An inherited route is dropped only + // when its owner is above the suppressor (`route_depth > suppressor_depth`), + // so a group's decline of an ancestor template never clobbers a nearer + // descendant's own route at the same path. + let mut suppressed: std::collections::HashMap<(AppId, String), i32> = + std::collections::HashMap::new(); + for (app, path, depth) in routes.list_route_suppressions().await? { + suppressed + .entry((app, path)) + .and_modify(|d| *d = (*d).min(depth)) + .or_insert(depth); + } let compiled = compile_effective_routes(&effective, &suppressed); table.replace_all(compiled); Ok(()) @@ -443,10 +449,15 @@ pub async fn rebuild_route_table( /// inherited route, a descendant SUPPRESSES its path (§11 tail, the /// `suppressed_paths` set) — the deliberate per-app opt-out. /// -/// `suppressed_paths` holds `(app_id, path)` route suppressions: an INHERITED -/// row (`depth > 0`) whose `(effective_app_id, path)` is present is skipped -/// entirely (the binding is absent → 404). Gated to `depth > 0` so an app can -/// only decline what it inherits, never its own route. +/// `suppressed_paths` maps `(app_id, path)` → the smallest suppressor depth for +/// that key. An INHERITED route (`depth > 0`) is skipped only when its owner is +/// strictly ABOVE the suppressor (`route.depth > suppressor_depth`) — so an +/// app's own suppression (depth 0) declines anything inherited, a group's +/// suppression declines only what that group itself inherits (owned above it), +/// and a nearer descendant's own re-declaration at the same path SURVIVES an +/// ancestor's suppression. The `route.depth > suppressor_depth` test also +/// subsumes the "inherited only" gate (a suppressor at depth `d` can only touch +/// rows at depth `> d ≥ 0`, never its own depth-0 route). /// /// Requires `rows` ordered by `(effective_app_id, depth ASC)` — which /// [`RouteRepository::list_effective`] guarantees — so first-seen is nearest. @@ -454,7 +465,7 @@ pub async fn rebuild_route_table( #[allow(clippy::implicit_hasher)] // every caller uses the default hasher pub fn compile_effective_routes( rows: &[EffectiveRoute], - suppressed_paths: &std::collections::HashSet<(AppId, String)>, + suppressed_paths: &std::collections::HashMap<(AppId, String), i32>, ) -> Vec { let mut seen: std::collections::HashSet<(AppId, String)> = std::collections::HashSet::new(); let mut out = Vec::new(); @@ -465,12 +476,14 @@ pub fn compile_effective_routes( continue; } // §11 tail: an inherited route whose path this app suppresses is - // dropped — the binding 404s. Gated to inherited rows (`depth > 0`). - // A `sealed` template is non-suppressible: the descendant's opt-out is - // ignored, so it stays in the slice regardless of the suppression. - if er.depth > 0 - && !er.route.sealed - && suppressed_paths.contains(&(er.effective_app_id, er.route.path.clone())) + // dropped — but ONLY if the route's owner is above the suppressor + // (`route.depth > suppressor_depth`), so a descendant group's own + // re-declaration at the same path isn't over-declined by an ancestor's + // suppression. A `sealed` template is non-suppressible. + if !er.route.sealed + && suppressed_paths + .get(&(er.effective_app_id, er.route.path.clone())) + .is_some_and(|&suppressor_depth| er.depth > suppressor_depth) { continue; } @@ -770,8 +783,18 @@ mod tests { } /// No route suppressions — the common case for these compile tests. - fn no_suppress() -> std::collections::HashSet<(AppId, String)> { - std::collections::HashSet::new() + fn no_suppress() -> std::collections::HashMap<(AppId, String), i32> { + std::collections::HashMap::new() + } + + /// Build an `EffectiveRoute` at an explicit chain depth (for suppression + /// tests that need an inherited row). + fn eff_at(route: &Route, effective_app_id: AppId, depth: i32) -> EffectiveRoute { + EffectiveRoute { + effective_app_id, + depth, + route: route.clone(), + } } #[test] @@ -853,4 +876,42 @@ mod tests { "a non-identical /y template must coexist" ); } + + #[test] + fn group_suppression_does_not_over_decline_a_nearer_descendants_own_route() { + // Audit fix #3: a suppression declared at depth 2 (an ancestor group G) + // must decline only routes inherited from ABOVE it (a far ancestor GG's + // template at depth 3), NOT a nearer descendant group H's own + // re-declaration of the same path at depth 1. Both /x routes carry + // DIFFERENT bindings (GET vs POST) so shadowing doesn't hide the bug — + // the suppression, keyed by path, is what would wrongly drop H's route. + let app = AppId::from(Uuid::new_v4()); + let mut gg = route_with_path("/x"); // far ancestor's template + gg.method = Some("GET".into()); + gg.app_id = None; + gg.group_id = Some(picloud_shared::GroupId::from(Uuid::new_v4())); + let mut h = route_with_path("/x"); // nearer descendant group's OWN route + h.method = Some("POST".into()); + h.app_id = None; + h.group_id = Some(picloud_shared::GroupId::from(Uuid::new_v4())); + + // Ordered nearest-first, as list_effective guarantees. + let rows = [eff_at(&h, app, 1), eff_at(&gg, app, 3)]; + // G suppresses /x at depth 2. + let mut suppressed = std::collections::HashMap::new(); + suppressed.insert((app, "/x".to_string()), 2); + + let ids: Vec = compile_effective_routes(&rows, &suppressed) + .iter() + .map(|c| c.route_id) + .collect(); + assert!( + !ids.contains(&gg.id), + "the far-ancestor template (depth 3 > 2) must be declined" + ); + assert!( + ids.contains(&h.id), + "the nearer descendant's own /x (depth 1 < 2) must NOT be over-declined" + ); + } } diff --git a/crates/manager-core/src/route_repo.rs b/crates/manager-core/src/route_repo.rs index 405cd8e..8cb8186 100644 --- a/crates/manager-core/src/route_repo.rs +++ b/crates/manager-core/src/route_repo.rs @@ -64,10 +64,17 @@ pub trait RouteRepository: Send + Sync { ) -> Result, ScriptRepositoryError> { Ok(Vec::new()) } - /// §11 tail per-app opt-out: all `(app_id, path)` ROUTE suppressions. The - /// route-table rebuild drops an inherited route at a suppressed path. + /// §11 tail per-app opt-out: all ROUTE suppressions as + /// `(effective_app_id, path, suppressor_depth)` — the depth is the chain + /// distance from the app to the owner that DECLARED the suppression (0 for + /// the app's own suppression, >0 for an ancestor group's). The route-table + /// rebuild drops an inherited route at a suppressed path only when the + /// route's owner is ABOVE the suppressor (`route_depth > suppressor_depth`), + /// so a nearer descendant's own re-declaration is never over-declined. /// Defaults empty (non-Postgres impls have no suppressions). - async fn list_route_suppressions(&self) -> Result, ScriptRepositoryError> { + async fn list_route_suppressions( + &self, + ) -> Result, ScriptRepositoryError> { Ok(Vec::new()) } /// §11 tail: every route resolved for every app — each app's own routes @@ -166,14 +173,18 @@ impl RouteRepository for PostgresRouteRepository { Ok(rows.into_iter().map(Into::into).collect()) } - async fn list_route_suppressions(&self) -> Result, ScriptRepositoryError> { + async fn list_route_suppressions( + &self, + ) -> Result, ScriptRepositoryError> { // §11 tail M1: a route suppression is owned by an app (declines for // itself) OR a group (declines for its whole subtree). Expand each // group-owned suppression across every descendant app via the all-apps - // `app_chain` CTE (the same one `list_effective` uses): the result is - // `(effective_app_id, path)` pairs the rebuild consumes unchanged — a - // group suppression appears once per descendant app. - let rows: Vec<(Uuid, String)> = sqlx::query_as( + // `app_chain` CTE (the same one `list_effective` uses). We also return + // `ac.depth` — the chain distance from the effective app to the owner + // that DECLARED the suppression — so the rebuild only declines routes + // inherited from ABOVE that owner (`route_depth > suppressor_depth`), + // not a nearer descendant's own re-declaration at the same path. + let rows: Vec<(Uuid, String, i32)> = sqlx::query_as( "WITH RECURSIVE app_chain AS ( \ SELECT a.id AS effective_app_id, a.id AS owner_app, \ NULL::uuid AS owner_group, a.group_id AS next_group, 0 AS depth \ @@ -183,7 +194,7 @@ impl RouteRepository for PostgresRouteRepository { FROM groups g JOIN app_chain ac ON g.id = ac.next_group \ WHERE ac.depth < 64 \ ) \ - SELECT DISTINCT ac.effective_app_id, ts.reference \ + SELECT ac.effective_app_id, ts.reference, ac.depth \ FROM app_chain ac \ JOIN template_suppressions ts \ ON (ts.app_id = ac.owner_app OR ts.group_id = ac.owner_group) \ @@ -191,7 +202,7 @@ impl RouteRepository for PostgresRouteRepository { ) .fetch_all(&self.pool) .await?; - Ok(rows.into_iter().map(|(a, r)| (a.into(), r)).collect()) + Ok(rows.into_iter().map(|(a, r, d)| (a.into(), r, d)).collect()) } async fn list_effective(&self) -> Result, ScriptRepositoryError> { diff --git a/crates/manager-core/src/trigger_repo.rs b/crates/manager-core/src/trigger_repo.rs index bee6435..4fd344d 100644 --- a/crates/manager-core/src/trigger_repo.rs +++ b/crates/manager-core/src/trigger_repo.rs @@ -26,6 +26,12 @@ use crate::trigger_config::BackoffShape; /// The `t.group_id IS NOT NULL` guard keeps an app's OWN trigger unsuppressable /// (an owner can only decline what it inherits); `t.sealed = FALSE` keeps a /// mandatory template non-declinable. +/// +/// `sc.depth < c.depth` is the over-decline guard: a suppressor at depth +/// `sc.depth` may only decline a trigger whose owner is strictly ABOVE it +/// (`c.depth` is the trigger owner's depth in the outer query's `chain c`), so +/// a suppression of a far-ancestor template never clobbers a NEARER descendant +/// group's OWN trigger that happens to bind a same-named handler. pub(crate) const TRIGGER_SUPPRESSION_ANTIJOIN: &str = " \ AND NOT EXISTS ( \ SELECT 1 FROM template_suppressions ts \ @@ -34,7 +40,8 @@ pub(crate) const TRIGGER_SUPPRESSION_ANTIJOIN: &str = " \ WHERE ts.target_kind = 'trigger' \ AND LOWER(ts.reference) = LOWER(s.name) \ AND t.group_id IS NOT NULL \ - AND t.sealed = FALSE)"; + AND t.sealed = FALSE \ + AND sc.depth < c.depth)"; #[derive(Debug, thiserror::Error)] pub enum TriggerRepoError { diff --git a/crates/manager-core/tests/group_route_templates.rs b/crates/manager-core/tests/group_route_templates.rs index 0ac035c..bfc898f 100644 --- a/crates/manager-core/tests/group_route_templates.rs +++ b/crates/manager-core/tests/group_route_templates.rs @@ -63,12 +63,18 @@ async fn app_under(pool: &PgPool, slug: &str, group: Uuid) -> Uuid { /// ancestor-group templates, after nearest-wins shadowing. async fn slice_paths(repo: &PostgresRouteRepository, app: Uuid) -> Vec<(String, Uuid)> { let effective = repo.list_effective().await.expect("list_effective"); - let suppressed = repo + let mut suppressed: std::collections::HashMap<(AppId, String), i32> = + std::collections::HashMap::new(); + for (a, p, d) in repo .list_route_suppressions() .await .expect("list_route_suppressions") - .into_iter() - .collect(); + { + suppressed + .entry((a, p)) + .and_modify(|x| *x = (*x).min(d)) + .or_insert(d); + } compile_effective_routes(&effective, &suppressed) .into_iter() .filter(|c| c.app_id == AppId::from(app)) diff --git a/crates/manager-core/tests/group_suppression.rs b/crates/manager-core/tests/group_suppression.rs index ce916a2..cf2f18b 100644 --- a/crates/manager-core/tests/group_suppression.rs +++ b/crates/manager-core/tests/group_suppression.rs @@ -14,8 +14,6 @@ clippy::too_many_lines )] -use std::collections::HashSet; - use picloud_manager_core::route_admin::compile_effective_routes; use picloud_manager_core::route_repo::{PostgresRouteRepository, RouteRepository}; use picloud_manager_core::trigger_repo::{PostgresTriggerRepo, TriggerRepo}; @@ -77,12 +75,18 @@ async fn app_under(pool: &PgPool, slug: &str, group: Uuid) -> Uuid { /// Whether `app`'s compiled route slice serves `/hello`. async fn serves_hello(repo: &PostgresRouteRepository, app: Uuid) -> bool { let effective = repo.list_effective().await.expect("list_effective"); - let suppressed: HashSet<(AppId, String)> = repo + let mut suppressed: std::collections::HashMap<(AppId, String), i32> = + std::collections::HashMap::new(); + for (a, p, d) in repo .list_route_suppressions() .await .expect("list_route_suppressions") - .into_iter() - .collect(); + { + suppressed + .entry((a, p)) + .and_modify(|x| *x = (*x).min(d)) + .or_insert(d); + } compile_effective_routes(&effective, &suppressed) .into_iter() .any(|c| c.app_id == AppId::from(app) && format!("{:?}", c.path).contains("/hello")) @@ -215,12 +219,18 @@ async fn group_suppression_declines_for_whole_subtree_only() { .await .expect("suppress sealed"); let effective = routes.list_effective().await.expect("list_effective"); - let suppressed: HashSet<(AppId, String)> = routes + let mut suppressed: std::collections::HashMap<(AppId, String), i32> = + std::collections::HashMap::new(); + for (app, path, depth) in routes .list_route_suppressions() .await .expect("suppressions") - .into_iter() - .collect(); + { + suppressed + .entry((app, path)) + .and_modify(|x| *x = (*x).min(depth)) + .or_insert(depth); + } let a_serves_sealed = compile_effective_routes(&effective, &suppressed) .into_iter() .any(|cr| cr.app_id == AppId::from(a) && format!("{:?}", cr.path).contains("/sealed")); @@ -259,3 +269,116 @@ async fn group_suppression_declines_for_whole_subtree_only() { .execute(&pool) .await; } + +/// Audit fix #3 (trigger side): a mid-tree group's suppression of an ancestor +/// template's handler name must NOT over-decline a NEARER descendant group's +/// OWN trigger that binds a same-named handler. `sc.depth < c.depth` in the +/// anti-join is the guard. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn ancestor_suppression_does_not_over_decline_a_descendants_own_trigger() { + let Some(pool) = pool_or_skip().await else { + return; + }; + let sfx = Uuid::new_v4().simple().to_string(); + let admin = id1( + &pool, + "INSERT INTO admin_users (username, password_hash) VALUES ($1, 'x') RETURNING id", + &format!("od-{sfx}"), + ) + .await; + + // Chain: gg (root) → g → h → app a. gg AND h each own a handler of the SAME + // NAME plus a kv trigger bound to it. g suppresses that handler name. + let gg = group_under(&pool, &format!("od-gg-{sfx}"), None).await; + let g = group_under(&pool, &format!("od-g-{sfx}"), Some(gg)).await; + let h = group_under(&pool, &format!("od-h-{sfx}"), Some(g)).await; + let a = app_under(&pool, &format!("od-a-{sfx}"), h).await; + let name = format!("audit-{sfx}"); + + let mk_trigger = |owner: Uuid, name: String| { + let pool = pool.clone(); + async move { + let handler: (Uuid,) = sqlx::query_as( + "INSERT INTO scripts (name, source, group_id) VALUES ($1, 'x', $2) RETURNING id", + ) + .bind(&name) + .bind(owner) + .fetch_one(&pool) + .await + .expect("handler"); + let t: (Uuid,) = sqlx::query_as( + "INSERT INTO triggers \ + (group_id, script_id, kind, enabled, dispatch_mode, \ + retry_max_attempts, retry_backoff, retry_base_ms, \ + registered_by_principal, name) \ + VALUES ($1, $2, 'kv', TRUE, 'async', 3, 'exponential', 1000, $3, $4) \ + RETURNING id", + ) + .bind(owner) + .bind(handler.0) + .bind(admin) + .bind(format!("t-{}", Uuid::new_v4().simple())) + .fetch_one(&pool) + .await + .expect("trigger"); + sqlx::query( + "INSERT INTO kv_trigger_details (trigger_id, collection_glob, ops) \ + VALUES ($1, '*', ARRAY['insert'])", + ) + .bind(t.0) + .execute(&pool) + .await + .expect("kv details"); + (handler.0, t.0) + } + }; + let (gg_handler, gg_trig) = mk_trigger(gg, name.clone()).await; + let (h_handler, h_trig) = mk_trigger(h, name.clone()).await; + + // g suppresses the handler name for its whole subtree. + sqlx::query( + "INSERT INTO template_suppressions (group_id, target_kind, reference) VALUES ($1, 'trigger', $2)", + ) + .bind(g) + .bind(&name) + .execute(&pool) + .await + .expect("suppression"); + + let trig = PostgresTriggerRepo::new(pool.clone()); + let m = trig + .list_matching_kv(AppId::from(a), "users", KvEventOp::Insert) + .await + .expect("match"); + let ids: Vec<_> = m.iter().map(|x| x.trigger_id).collect(); + assert!( + !ids.contains(&gg_trig.into()), + "the far-ancestor template (above the suppressor) must be declined" + ); + assert!( + ids.contains(&h_trig.into()), + "the nearer descendant group's OWN trigger (below the suppressor) must survive" + ); + + // Cleanup (FK order). + let _ = sqlx::query("DELETE FROM triggers WHERE id = ANY($1)") + .bind(vec![gg_trig, h_trig]) + .execute(&pool) + .await; + let _ = sqlx::query("DELETE FROM apps WHERE id = $1") + .bind(a) + .execute(&pool) + .await; + let _ = sqlx::query("DELETE FROM scripts WHERE id = ANY($1)") + .bind(vec![gg_handler, h_handler]) + .execute(&pool) + .await; + let _ = sqlx::query("DELETE FROM groups WHERE id = ANY($1)") + .bind(vec![h, g, gg]) + .execute(&pool) + .await; + let _ = sqlx::query("DELETE FROM admin_users WHERE id = $1") + .bind(admin) + .execute(&pool) + .await; +} diff --git a/crates/manager-core/tests/sealed_templates.rs b/crates/manager-core/tests/sealed_templates.rs index 96116bf..898eba2 100644 --- a/crates/manager-core/tests/sealed_templates.rs +++ b/crates/manager-core/tests/sealed_templates.rs @@ -15,8 +15,6 @@ clippy::too_many_lines )] -use std::collections::HashSet; - use picloud_manager_core::route_admin::compile_effective_routes; use picloud_manager_core::route_repo::{PostgresRouteRepository, RouteRepository}; use picloud_manager_core::trigger_repo::{PostgresTriggerRepo, TriggerRepo}; @@ -111,12 +109,18 @@ async fn route_template(pool: &PgPool, group: Uuid, script: Uuid, path: &str, se /// Whether `app`'s compiled route slice serves `path`. async fn serves(repo: &PostgresRouteRepository, app: Uuid, path: &str) -> bool { let effective = repo.list_effective().await.expect("list_effective"); - let suppressed: HashSet<(AppId, String)> = repo + let mut suppressed: std::collections::HashMap<(AppId, String), i32> = + std::collections::HashMap::new(); + for (a, p, d) in repo .list_route_suppressions() .await .expect("list_route_suppressions") - .into_iter() - .collect(); + { + suppressed + .entry((a, p)) + .and_modify(|x| *x = (*x).min(d)) + .or_insert(d); + } compile_effective_routes(&effective, &suppressed) .into_iter() .any(|c| c.app_id == AppId::from(app) && format!("{:?}", c.path).contains(path)) diff --git a/crates/manager-core/tests/template_suppression.rs b/crates/manager-core/tests/template_suppression.rs index 5ee5ed2..e7ceb87 100644 --- a/crates/manager-core/tests/template_suppression.rs +++ b/crates/manager-core/tests/template_suppression.rs @@ -15,8 +15,6 @@ clippy::too_many_lines )] -use std::collections::HashSet; - use picloud_manager_core::route_admin::compile_effective_routes; use picloud_manager_core::route_repo::{PostgresRouteRepository, RouteRepository}; use picloud_manager_core::trigger_repo::{PostgresTriggerRepo, TriggerRepo}; @@ -65,12 +63,18 @@ async fn app_under(pool: &PgPool, slug: &str, group: Uuid) -> Uuid { /// Whether `app`'s compiled route slice serves `/hello`. async fn serves_hello(repo: &PostgresRouteRepository, app: Uuid) -> bool { let effective = repo.list_effective().await.expect("list_effective"); - let suppressed: HashSet<(AppId, String)> = repo + let mut suppressed: std::collections::HashMap<(AppId, String), i32> = + std::collections::HashMap::new(); + for (a, p, d) in repo .list_route_suppressions() .await .expect("list_route_suppressions") - .into_iter() - .collect(); + { + suppressed + .entry((a, p)) + .and_modify(|x| *x = (*x).min(d)) + .or_insert(d); + } compile_effective_routes(&effective, &suppressed) .into_iter() .any(|c| c.app_id == AppId::from(app) && format!("{:?}", c.path).contains("/hello"))