From 3f272daf04322145476652a6d8694aa3910c0891 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Tue, 7 Jul 2026 21:48:09 +0200 Subject: [PATCH] fix(cli): make the per-env approval gate fail closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review #2 (MEDIUM). The M3 `[project.environments]` approval gate silently failed open in three shapes: - `EnvPolicy.confirm` was `#[serde(default)]`, so `production = {}` parsed as un-gated. `confirm` is now a REQUIRED field — an env listed with an empty policy is a load error (`missing field 'confirm'`), not a silent no-gate. - `pic apply --file ` where the leaf carried no `[project]` skipped the gate even when the repo root gated the env. `run` now discovers the governing `[project]` by walking up from the manifest to the nearest ancestor `picloud.toml` that declares one (`find_governing_project`, loaded without the env overlay — only the block matters). - A `[project].environments` in a non-root manifest under `--dir` was dropped with a generic "ignored" note; `build_tree` now warns explicitly that its approval gating is not enforced. Pinned by `tests/env_approval.rs` (empty-policy load error; leaf `--file` apply honoring the root gate). Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/picloud-cli/src/cmds/apply.rs | 24 ++++- crates/picloud-cli/src/discover.rs | 9 ++ crates/picloud-cli/src/manifest.rs | 7 +- crates/picloud-cli/tests/env_approval.rs | 114 +++++++++++++++++++++++ 4 files changed, 151 insertions(+), 3 deletions(-) diff --git a/crates/picloud-cli/src/cmds/apply.rs b/crates/picloud-cli/src/cmds/apply.rs index 6ff216f..d51ece5 100644 --- a/crates/picloud-cli/src/cmds/apply.rs +++ b/crates/picloud-cli/src/cmds/apply.rs @@ -29,7 +29,14 @@ pub async fn run( let client = Client::from_creds(&creds)?; let manifest = Manifest::load_with_env(manifest_path, env)?; - require_env_approval(manifest.project.as_ref(), env, approve)?; + // §3 M3: the env-approval gate is a property of the PROJECT, which lives at + // the repo root. A leaf manifest carries no `[project]`, so find the + // governing one (own block, else the nearest ancestor's) before gating. + let governing = manifest + .project + .clone() + .or_else(|| find_governing_project(manifest_path)); + require_env_approval(governing.as_ref(), env, approve)?; let base_dir = manifest_path.parent().unwrap_or_else(|| Path::new(".")); let bundle = build_bundle(&manifest, base_dir)?; let kind = if manifest.is_group() { @@ -233,6 +240,21 @@ pub async fn run_tree( /// (`[project.environments]` with `confirm = true`) unless it was explicitly /// `--approve `d. A blanket `--yes` does NOT cover a gated env (§4.2 "CI must /// opt in per environment"). An unlisted or `confirm = false` env applies freely. +/// §3 M3: find the `[project]` that governs a single-file apply. The applied +/// manifest's own block wins (the caller checks that first); otherwise walk up +/// from its directory to the nearest ancestor `picloud.toml` that declares one +/// (the repo root), so `pic apply --file services/api/picloud.toml` still +/// honors the repo's env-approval gate. Loaded WITHOUT the env overlay — only +/// the `[project]` block matters here, and `load_with_env` would require an +/// overlay next to every ancestor and spuriously skip the governing manifest. +fn find_governing_project(manifest_path: &Path) -> Option { + let dir = manifest_path.parent()?; + dir.ancestors().skip(1).find_map(|anc| { + let candidate = anc.join(crate::manifest::MANIFEST_FILE); + Manifest::load(&candidate).ok().and_then(|m| m.project) + }) +} + fn require_env_approval( project: Option<&ManifestProject>, env: Option<&str>, diff --git a/crates/picloud-cli/src/discover.rs b/crates/picloud-cli/src/discover.rs index 7d4f569..17ce102 100644 --- a/crates/picloud-cli/src/discover.rs +++ b/crates/picloud-cli/src/discover.rs @@ -83,6 +83,15 @@ pub fn build_tree( path.display(), root_manifest.display() ); + // §3 M3: a non-root [project] that carries env-approval policy is + // a silent-safety trap — the gate is dropped with the block. Call + // it out explicitly so a misplaced gate isn't mistaken for active. + if !p.environments.is_empty() { + eprintln!( + "warning: its [project.environments] approval gating is NOT enforced — \ + only the tree root's [project] governs env approval" + ); + } } } loaded.push((path.clone(), manifest)); diff --git a/crates/picloud-cli/src/manifest.rs b/crates/picloud-cli/src/manifest.rs index eb00353..b979668 100644 --- a/crates/picloud-cli/src/manifest.rs +++ b/crates/picloud-cli/src/manifest.rs @@ -296,8 +296,11 @@ pub struct ManifestProject { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct EnvPolicy { - /// Require an explicit `--approve ` to apply to this env. - #[serde(default)] + /// Require an explicit `--approve ` to apply to this env. NOT + /// `#[serde(default)]`: an env listed with an empty policy (`prod = {}`) + /// must fail to load (`missing field 'confirm'`) rather than silently + /// parse as un-gated — listing an env to protect it, then having the gate + /// quietly absent, is the failure mode to avoid. pub confirm: bool, } diff --git a/crates/picloud-cli/tests/env_approval.rs b/crates/picloud-cli/tests/env_approval.rs index bda5767..c988f46 100644 --- a/crates/picloud-cli/tests/env_approval.rs +++ b/crates/picloud-cli/tests/env_approval.rs @@ -90,3 +90,117 @@ fn confirm_required_env_needs_explicit_approve() { "an un-gated env must apply without --approve" ); } + +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn an_env_listed_with_an_empty_policy_is_a_hard_error() { + // `production = {}` (no `confirm`) must FAIL to load, not silently parse as + // un-gated — you can't half-declare a gate. `confirm` is a required field. + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let group = common::unique_slug("eae-grp"); + let project = common::unique_slug("eae-proj"); + let _g = GroupGuard::new(&env.url, &env.token, &group); + + let dir = TempDir::new().unwrap(); + let manifest = dir.path().join("picloud.toml"); + fs::write( + &manifest, + format!( + "[project]\nslug = \"{project}\"\n\n\ + [project.environments]\nproduction = {{}}\n\n\ + [group]\nslug = \"{group}\"\nname = \"Empty Policy\"\n" + ), + ) + .unwrap(); + fs::write(dir.path().join("picloud.production.toml"), "# overlay\n").unwrap(); + + let out = common::pic_as(&env) + .args(["apply", "--file"]) + .arg(&manifest) + .args(["--env", "production"]) + .output() + .expect("apply"); + assert!( + !out.status.success(), + "an env with an empty policy must be a load error, not a silent no-gate" + ); + let err = String::from_utf8_lossy(&out.stderr).to_lowercase(); + assert!( + err.contains("confirm"), + "the error must name the missing `confirm` field:\n{err}" + ); +} + +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn leaf_file_apply_honors_the_root_env_gate() { + // `pic apply --file ` where the leaf carries no `[project]` still + // honors the gate declared in the repo root (found by walking up). + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let base = common::unique_slug("eal-base"); + let leaf = common::unique_slug("eal-leaf"); + let project = common::unique_slug("eal-proj"); + let _b = GroupGuard::new(&env.url, &env.token, &base); + let _l = GroupGuard::new(&env.url, &env.token, &leaf); + common::pic_as(&env) + .args(["groups", "create", &base]) + .assert() + .success(); + common::pic_as(&env) + .args(["groups", "create", &leaf, "--parent", &base]) + .assert() + .success(); + + // Root manifest carries the gating `[project]`; the leaf (in a subdir) has + // no `[project]` of its own and is applied on its own with `--file`. + let dir = TempDir::new().unwrap(); + fs::write( + dir.path().join("picloud.toml"), + format!( + "[project]\nslug = \"{project}\"\n\n\ + [project.environments]\nproduction = {{ confirm = true }}\n\n\ + [group]\nslug = \"{base}\"\nname = \"Base\"\n" + ), + ) + .unwrap(); + let sub = dir.path().join("sub"); + fs::create_dir_all(&sub).unwrap(); + let leaf_manifest = sub.join("picloud.toml"); + fs::write( + &leaf_manifest, + format!("[group]\nslug = \"{leaf}\"\nname = \"Leaf\"\n"), + ) + .unwrap(); + fs::write(sub.join("picloud.production.toml"), "# overlay\n").unwrap(); + + // Without --approve: refused, because the gate is discovered up-tree. + let out = common::pic_as(&env) + .args(["apply", "--file"]) + .arg(&leaf_manifest) + .args(["--env", "production"]) + .output() + .expect("apply"); + assert!( + !out.status.success(), + "a leaf --file apply must honor the root's env gate" + ); + let err = String::from_utf8_lossy(&out.stderr).to_lowercase(); + assert!( + err.contains("approve") && err.contains("production"), + "the refusal must point at --approve production:\n{err}" + ); + + // With --approve production: passes the gate and applies (leaf pre-exists). + common::pic_as(&env) + .args(["apply", "--file"]) + .arg(&leaf_manifest) + .args(["--env", "production", "--approve", "production"]) + .assert() + .success(); +}