fix(cli): make the per-env approval gate fail closed
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 <leaf>` 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) <noreply@anthropic.com>
This commit is contained in:
@@ -29,7 +29,14 @@ pub async fn run(
|
|||||||
let client = Client::from_creds(&creds)?;
|
let client = Client::from_creds(&creds)?;
|
||||||
|
|
||||||
let manifest = Manifest::load_with_env(manifest_path, env)?;
|
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 base_dir = manifest_path.parent().unwrap_or_else(|| Path::new("."));
|
||||||
let bundle = build_bundle(&manifest, base_dir)?;
|
let bundle = build_bundle(&manifest, base_dir)?;
|
||||||
let kind = if manifest.is_group() {
|
let kind = if manifest.is_group() {
|
||||||
@@ -233,6 +240,21 @@ pub async fn run_tree(
|
|||||||
/// (`[project.environments]` with `confirm = true`) unless it was explicitly
|
/// (`[project.environments]` with `confirm = true`) unless it was explicitly
|
||||||
/// `--approve <e>`d. A blanket `--yes` does NOT cover a gated env (§4.2 "CI must
|
/// `--approve <e>`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.
|
/// 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<ManifestProject> {
|
||||||
|
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(
|
fn require_env_approval(
|
||||||
project: Option<&ManifestProject>,
|
project: Option<&ManifestProject>,
|
||||||
env: Option<&str>,
|
env: Option<&str>,
|
||||||
|
|||||||
@@ -83,6 +83,15 @@ pub fn build_tree(
|
|||||||
path.display(),
|
path.display(),
|
||||||
root_manifest.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));
|
loaded.push((path.clone(), manifest));
|
||||||
|
|||||||
@@ -296,8 +296,11 @@ pub struct ManifestProject {
|
|||||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||||
#[serde(deny_unknown_fields)]
|
#[serde(deny_unknown_fields)]
|
||||||
pub struct EnvPolicy {
|
pub struct EnvPolicy {
|
||||||
/// Require an explicit `--approve <env>` to apply to this env.
|
/// Require an explicit `--approve <env>` to apply to this env. NOT
|
||||||
#[serde(default)]
|
/// `#[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,
|
pub confirm: bool,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -90,3 +90,117 @@ fn confirm_required_env_needs_explicit_approve() {
|
|||||||
"an un-gated env must apply without --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 <leaf>` 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();
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user