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

fix(project-tool): close M5+M3 review findings (authz + data-loss + None-path)

Browse Source 
A fresh two-lens end-to-end review (security + correctness) of the committed
M5/M3 diff surfaced four real defects; all fixed here with regressions.

- HIGH (M5 authz bypass) — `enforce_env_approval` resolved a group node with a
  bare `get_by_slug` and SKIPPED the GroupAdmin gate on miss. A node addressed
  by the group's UUID (which the apply still resolves) let a group EDITOR apply
  to a confirm-required env without admin. Now resolves UUID-or-slug and fails
  closed, mirroring authz_tree / prepare_tree. Raw-wire regression added.

- FIX-FIRST (M3 correctness) — the legacy `project_key=None` path was not inert:
  the conflict loop pushed a conflict for any owned group (`Some(oid) != None`),
  so a keyless/direct-API tree apply touching an already-claimed group was
  spuriously 409'd. Ownership classification is now gated on a key being present
  (the discriminator is key-present, not project-persisted, so a first-ever
  keyed apply still sees an already-owned group as a conflict). Regression added.

- FIX-FIRST (M3 data-loss) — the structural-prune guard checked collection
  MARKERS (`group_collections`) but data outlives its marker: un-declaring a
  `collections=[...]` entry drops the marker while the kv/docs/files/queue rows
  survive until group delete. A dropped-then-pruned group would silently CASCADE
  that orphaned data. The guard now EXISTS-checks the actual data tables
  (group_kv_entries/docs/files/queue_messages) plus secrets + markers.

- MEDIUM (audit) — group ownership takeover (and claim) now emit a
  tracing::info! audit line with the actor + ousted owner, matching the M5
  gated-env audit. Previously only a report counter.

Also: documented WHY `pic plan --dir` mints the project key (a rare write for a
read-only command) — plan and apply must present the same key or the ownership
token folds diverge and trip a spurious StateMoved.

Re-verified: 411 manager-core lib tests; 28 CLI journeys (incl. 2 new
regressions: uuid-slug admin gate, keyless legacy apply); workspace clippy
-D warnings + fmt clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-07-05 20:45:23 +02:00
parent 5c33b7490b
commit 4c68d2fa33
6 changed files with 222 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
crates/manager-core/src/apply_api.rs
View File

@@ -395,19 +395,23 @@ async fn enforce_env_approval(
.map_err(map_authz)?;
}
NodeKind::Group => {
if let Some(g) = svc
.groups
.get_by_slug(&node.slug)
.await
.map_err(|e| ApplyError::Backend(e.to_string()))?
{
require(svc.authz.as_ref(), principal, Capability::GroupAdmin(g.id))
// Resolve UUID-or-slug and FAIL CLOSED on miss — mirror
// `authz_tree`/`prepare_tree`. A bare `get_by_slug` skips this
// admin gate when the node addresses the group by its UUID (which
// `resolve_group`/`resolve_group_id` still resolve for the apply),
// letting a group editor apply to a confirm-required env without
// the admin authority M5 requires.
let group_id = resolve_group_id(svc.groups.as_ref(), &node.slug).await?;
require(
svc.authz.as_ref(),
principal,
Capability::GroupAdmin(group_id),
)
.await
.map_err(map_authz)?;
}
}
}
}
tracing::info!(
user_id = ?principal.user_id,
env = %env,

38
crates/manager-core/src/apply_service.rs
View File

@@ -1658,12 +1658,26 @@ impl ApplyService {
crate::group_repo::set_group_owner_tx(&mut tx, *gid, pid)
.await
.map_err(map_group_err)?;
// Audit (§7.4): seizing another project's group is at
// least as sensitive as the M5 gated-env override — record
// the actor + the ousted owner.
tracing::info!(
user_id = ?actor,
group = %slug,
prev_owner_key = %okey,
"apply: group ownership TAKEN OVER (audit)"
);
report.groups_taken_over += 1;
}
None => {
crate::group_repo::set_group_owner_tx(&mut tx, *gid, pid)
.await
.map_err(map_group_err)?;
tracing::info!(
user_id = ?actor,
group = %slug,
"apply: group ownership CLAIMED (audit)"
);
report.groups_claimed += 1;
}
}
@@ -2005,21 +2019,27 @@ impl ApplyService {
token_parts.push(format!("proj|{}", envs.join(",")));
}
// Ownership (§7, M3): resolve this repo's project, then classify each
// declared group node as ours / unclaimed / owned-by-another. Folding
// the owner key into the token means a claim or takeover by a different
// repo between plan and apply trips StateMoved. (Every group node on the
// tree-apply path pre-exists — `resolve_group` errors otherwise — so
// there is no to-create node to skip, unlike a create-capable engine.)
// Ownership (§7, M3): classify each declared group node as ours /
// unclaimed / owned-by-another, and fold the owner key into the token so
// a claim or takeover by a different repo between plan and apply trips
// StateMoved. Gated on `project_key.is_some()` — a legacy caller with NO
// project key skips ALL ownership (no conflicts, no prune, no owner token
// fold), matching the "optional for legacy callers" contract. Note the
// discriminator is a *key present*, NOT a *persisted project*: a
// first-ever apply has a key but no `projects` row yet (our_project_id ==
// None), and MUST still see an already-owned group as a conflict. (Every
// group node pre-exists — `resolve_group` errors otherwise.)
let mut existing_group_nodes: Vec<(GroupId, String)> = Vec::new();
let mut conflicts: Vec<OwnershipConflict> = Vec::new();
let mut prune_candidates: Vec<(GroupId, String)> = Vec::new();
if project_key.is_some() {
let our_project_id = match project_key {
Some(k) => crate::group_repo::get_project_id_by_key(&self.pool, k)
.await
.map_err(|e| ApplyError::Backend(e.to_string()))?,
None => None,
};
let mut existing_group_nodes: Vec<(GroupId, String)> = Vec::new();
let mut declared_group_slugs: HashSet<String> = HashSet::new();
let mut conflicts: Vec<OwnershipConflict> = Vec::new();
for n in &bundle.nodes {
if n.kind != NodeKind::Group {
continue;
@@ -2048,7 +2068,6 @@ impl ApplyService {
// Structural-prune candidates: groups THIS project owns that the
// manifest omits. Computed read-only here (so `plan` can list them);
// `apply --prune` re-derives the set in-tx and deletes leaf-first.
let mut prune_candidates: Vec<(GroupId, String)> = Vec::new();
if let Some(pid) = our_project_id {
for (gid, slug) in crate::group_repo::list_owned_groups(&self.pool, pid)
.await
@@ -2059,6 +2078,7 @@ impl ApplyService {
}
}
}
}
token_parts.sort_unstable();
Ok(PreparedTree {

26
crates/manager-core/src/group_repo.rs
View File

@@ -469,21 +469,29 @@ pub(crate) async fn list_owned_groups_tx(
}
/// Does this group hold data a structural prune would SILENTLY cascade away
/// (§7 M3 safety guard)? The `apps`/child-group FKs are RESTRICT (delete aborts
/// loudly), but the §11.6 group-owned surfaces are `ON DELETE CASCADE`: shared
/// collections (`group_collections`, covering kv/docs/files/topic/queue) and
/// group secrets (owner-polymorphic `secrets.group_id`). Rather than let
/// `apply --prune` vaporize a tenant's shared data + secrets because a manifest
/// node was dropped, the prune skips such a group with a warning — the operator
/// must delete it explicitly. Returns true if the group has any shared-collection
/// marker or any secret.
/// (§7 M3 safety guard)? The `apps`/child-group/group-script FKs are RESTRICT
/// (delete aborts loudly), but the §11.6 group-owned data surfaces are
/// `ON DELETE CASCADE`. Rather than let `apply --prune` vaporize a tenant's
/// shared data + secrets because a manifest node was dropped, the prune skips
/// such a group with a warning — the operator must delete it explicitly.
///
/// We check the actual DATA rows, not just the collection MARKER: un-declaring a
/// `collections = [...]` entry deletes only the `group_collections` marker while
/// the `group_kv_entries`/`group_docs`/`group_files`/`group_queue_messages` rows
/// survive (they're reaped only on group delete). Checking markers alone would
/// miss that orphaned-but-live data and cascade it away. Returns true if the
/// group has any shared-collection marker, any secret, or any shared data row.
pub(crate) async fn group_has_protected_data_tx(
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
group_id: GroupId,
) -> Result<bool, GroupRepositoryError> {
let row: (bool,) = sqlx::query_as(
"SELECT EXISTS (SELECT 1 FROM group_collections WHERE group_id = $1) \
OR EXISTS (SELECT 1 FROM secrets WHERE group_id = $1)",
OR EXISTS (SELECT 1 FROM secrets WHERE group_id = $1) \
OR EXISTS (SELECT 1 FROM group_kv_entries WHERE group_id = $1) \
OR EXISTS (SELECT 1 FROM group_docs WHERE group_id = $1) \
OR EXISTS (SELECT 1 FROM group_files WHERE group_id = $1) \
OR EXISTS (SELECT 1 FROM group_queue_messages WHERE group_id = $1)",
)
.bind(group_id.into_inner())
.fetch_one(&mut **tx)

9
crates/picloud-cli/src/cmds/plan.rs
View File

@@ -50,8 +50,13 @@ pub async fn run_tree(dir: &Path, env: Option<&str>, mode: OutputMode) -> Result
let creds = config::resolve()?;
let client = Client::from_creds(&creds)?;
let (bundle, _count, _envs) = crate::discover::build_tree(dir, env)?;
// Mint/read this repo's stable project key (§7, M3) so the plan can surface
// ownership conflicts + structural-prune candidates for us.
// Mint/read this repo's project key (§7, M3) so the plan can surface
// ownership conflicts + prune candidates. `plan` DOES mint it (a rare
// write for an otherwise read-only command) rather than read-only-peek,
// because the bound-plan token folds the ownership state gated on a key
// being present: a keyless plan and a keyed apply would fold DIFFERENT
// token parts and trip a spurious `StateMoved`. Minting here keeps plan and
// apply keyed identically. The key is local + gitignored + idempotent.
let project_key = crate::linkstate::ensure_project_key(dir)?;
let plan = client.plan_tree(&bundle, Some(&project_key)).await?;
if !plan.state_token.is_empty() {

73
crates/picloud-cli/tests/approval.rs
View File

@@ -165,3 +165,76 @@ fn approving_a_gated_apply_requires_admin() {
"approval denial should be an authz error:\n{err}"
);
}
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
#[test]
fn gated_group_node_admin_gate_is_not_bypassable_by_uuid_slug() {
// Regression (§4.2): `enforce_env_approval` must resolve a group node by
// UUID-or-slug and FAIL CLOSED — a bare `get_by_slug` skipped the GroupAdmin
// gate when the node addressed the group by its UUID (which the apply still
// resolves), letting a group EDITOR apply to a confirm-required env without
// admin. We drive the raw `/tree/apply` wire directly (the CLI only ever
// sends slugs) with the group node's UUID as its slug.
let Some(fx) = common::fixture_or_skip() else {
return;
};
let env = common::admin_env(fx);
let group = common::unique_slug("appr3-g");
let _g = GroupGuard::new(&env.url, &env.token, &group);
common::pic_as(&env)
.args(["groups", "create", &group])
.assert()
.success();
// Resolve the group's UUID.
let http = reqwest::blocking::Client::new();
let detail: serde_json::Value = http
.get(format!("{}/api/v1/admin/groups/{}", env.url, group))
.bearer_auth(&env.token)
.send()
.expect("get group")
.json()
.expect("group json");
let group_uuid = detail["id"].as_str().expect("group id").to_string();
// A group `editor` — write caps, but NOT GroupAdmin.
let m = member::member_user(fx, &common::unique_username("appr3"));
common::pic_as(&env)
.args([
"groups", "members", "add", &group, &m.id, "--role", "editor",
])
.assert()
.success();
// A gated bundle whose single group node is addressed by UUID. A `vars`
// entry makes the node non-empty (and exercises the editor's write cap).
let body = serde_json::json!({
"bundle": {
"nodes": [{
"kind": "group",
"slug": group_uuid,
"bundle": { "vars": { "region": "eu" } }
}],
"project": { "environments": [{ "name": "production", "confirm": true }] }
},
"prune": false,
"env": "production",
"approved_envs": ["production"],
"allow_takeover": false,
});
let resp = http
.post(format!("{}/api/v1/admin/tree/apply", env.url))
.bearer_auth(&m.token)
.json(&body)
.send()
.expect("tree apply");
assert_eq!(
resp.status().as_u16(),
403,
"a group editor must be 403'd approving a gated apply even when the node \
is addressed by UUID (got {}: {})",
resp.status(),
resp.text().unwrap_or_default()
);
}

44
crates/picloud-cli/tests/ownership.rs
View File

@@ -286,3 +286,47 @@ fn prune_refuses_to_cascade_a_groups_shared_data() {
"a group owning shared collections must NOT be pruned"
);
}
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
#[test]
fn legacy_apply_without_project_key_ignores_ownership() {
// Regression (§7): a tree apply with NO project key (legacy / direct-API
// caller) must skip ALL ownership — no conflicts, no claims. A bug gated the
// conflict classification on `our_project_id` (None for a keyless caller),
// so ANY already-owned declared group was spuriously rejected. Drive the raw
// wire without `project_key` (the CLI always sends one).
let Some(fx) = common::fixture_or_skip() else {
return;
};
let env = common::admin_env(fx);
let slug = common::unique_slug("own-legacy");
let _g = GroupGuard::new(&env.url, &env.token, &slug);
create_group(&env, &slug, None);
// Repo A claims the group (a normal CLI apply — sends a project key).
let repo_a = group_dir(&slug, "");
assert!(
apply(&env, repo_a.path(), &[]).status.success(),
"claim apply should succeed"
);
// A keyless raw apply of the same (now-owned) group must NOT 409 — ownership
// is simply not evaluated without a project key.
let http = reqwest::blocking::Client::new();
let body = serde_json::json!({
"bundle": { "nodes": [{ "kind": "group", "slug": slug, "bundle": {} }] },
"prune": false,
});
let resp = http
.post(format!("{}/api/v1/admin/tree/apply", env.url))
.bearer_auth(&env.token)
.json(&body)
.send()
.expect("tree apply");
assert!(
resp.status().is_success(),
"a keyless (legacy) apply must ignore ownership, not conflict (got {}: {})",
resp.status(),
resp.text().unwrap_or_default()
);
}