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

fix(suppress): don't over-decline a nearer descendant's own route/trigger

Browse Source 
A group's template suppression is "coarse by reference" (path for routes,
handler-name for triggers), and both resolution points dropped ANY inherited
row matching that reference across the whole subtree — including one a NEARER
descendant group deliberately re-declared. So an ancestor group G that declines
a far-ancestor's `/x` (or `audit` handler) would also silently kill a child
group H's OWN `/x` / `audit`-bound trigger at the same reference, violating the
documented "an owner can only decline what it inherits, never a descendant's
own rows" invariant.

Fix: gate each decline on the chain DEPTH of the suppressor vs the target's
owner — a suppressor at depth `d_s` may only decline a row whose owner is
strictly ABOVE it (`target_depth > d_s`):
- Routes: `list_route_suppressions` now returns `(app, path, suppressor_depth)`;
  the rebuild folds it to the min depth per `(app, path)` and
  `compile_effective_routes` skips an inherited route only when
  `route.depth > suppressor_depth`. (This also subsumes the old `depth > 0`
  inherited-only gate.)
- Triggers: the dispatch anti-join gains `AND sc.depth < c.depth` (the
  suppressor's chain depth below the trigger owner's), correlating on the outer
  `chain c` that every kv/docs/files/pubsub match query already binds.

An app's own suppression is depth 0 → still declines anything it inherits; a
group's suppression declines only what that group itself inherits. `sealed`
still overrides. No schema change.

Pinned by a new `compile_effective_routes` unit test (a depth-1 descendant
route survives a depth-2 suppression that declines a depth-3 template) and a
new `group_suppression` DB test (same for triggers); all existing suppression /
sealed / template journeys stay green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-07-13 22:17:01 +02:00
parent 31e6fc964d
commit 590b98b60f
7 changed files with 268 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

101
crates/manager-core/src/route_admin.rs
View File

@@ -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<CompiledRoute> {
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<Uuid> = 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"
);
}
}