feat(apply): server-side enforcement of per-env approval gating (§3 M3 / §4.2)
The per-env approval gate (`[project.environments]`, `confirm = true`) was
client-side only — the policy was `#[serde(skip_serializing)]`, so a patched or
older CLI applying to a confirm-required env bypassed it entirely. This makes
the gate server-enforced, admin-gated, and audited (the deferred §4.2 piece).
- Wire: `ProjectDecl` gains `environments` (the CLI stops skip_serializing it and
sends the selected `env` + the `--approve` set on the apply request). The
server RE-DERIVES the gate from the request (`env_gate_check`): a
confirm-required env not in `approved_envs` → 409 `ApprovalRequired`.
- Admin gate: an approved override additionally requires `AppAdmin`/`GroupAdmin`
on EVERY declared node — a step up from the editor write caps an ordinary
apply needs (reusing the admin caps per §4.2) — and emits a `tracing::info!`
audit line. Enforced on all three handlers (single app, single group, tree);
the check runs before any tx, so a denial (403) precedes any mutation.
- Token: the env policy folds into the TREE bound-plan token (a policy edit
between plan and apply trips StateMoved). The single-node token is left
unchanged (a bare live-state hash), so plan/apply still match without threading
a project into it.
- CLI: single-node `apply`/`plan` now resolve the GOVERNING project (own block
else nearest ancestor's) so a leaf apply carries the root's policy; `plan`
surfaces `approvals_required`. Client-side `require_env_approval` fast-fail
kept as UX.
Adapts the earlier feat/ownership-and-approval M5 (654e387) to main's DTOs; main
addresses tree group nodes by slug only, so the admin loop mirrors authz_tree
(app: resolve_app_id fail-closed; group: get_by_slug, skip to-create).
Threat-model note (honest scope, in the doc + code comments): this closes the
patched/older-CLI bypass and admin-gates + audits the override, but is NOT a
hermetic boundary against a hand-crafted request that OMITS the policy — the
policy is applier-supplied, not persisted server state. Full closure (persist a
`project_environments` source of truth + a server-determined env) is deferred.
Review (adversarial pass): fixed a MEDIUM (single-node `plan` didn't resolve the
governing project, so a leaf plan under-reported the gate `apply` enforces) and a
LOW (duplicate clippy attr); corrected overstated "can't bypass" wording.
Tests: ProjectDecl gate unit test; env_approval journeys
`server_enforces_the_gate_against_a_non_stock_client` (raw request → 409 without
approval, admin-approved → 200) and `approving_a_gated_apply_requires_admin`
(editor + --approve → 403). 417 lib tests + 33 CLI journeys + workspace clippy
-D warnings + fmt clean. No migration.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -1276,6 +1276,8 @@ impl Client {
|
||||
project: Option<&crate::manifest::ManifestProject>,
|
||||
takeover: bool,
|
||||
expected_token: Option<&str>,
|
||||
env: Option<&str>,
|
||||
approved_envs: &[String],
|
||||
) -> Result<ApplyReportDto> {
|
||||
let slug = seg(slug);
|
||||
let body = serde_json::json!({
|
||||
@@ -1284,6 +1286,8 @@ impl Client {
|
||||
"expected_token": expected_token,
|
||||
"project": project,
|
||||
"takeover": takeover,
|
||||
"env": env,
|
||||
"approved_envs": approved_envs,
|
||||
});
|
||||
let resp = self
|
||||
.request(
|
||||
@@ -1330,6 +1334,8 @@ impl Client {
|
||||
takeover: bool,
|
||||
expected_token: Option<&str>,
|
||||
structure_mode: &str,
|
||||
env: Option<&str>,
|
||||
approved_envs: &[String],
|
||||
) -> Result<ApplyReportDto> {
|
||||
let body = serde_json::json!({
|
||||
"bundle": bundle,
|
||||
@@ -1338,6 +1344,8 @@ impl Client {
|
||||
"project": project,
|
||||
"takeover": takeover,
|
||||
"structure_mode": structure_mode,
|
||||
"env": env,
|
||||
"approved_envs": approved_envs,
|
||||
});
|
||||
let resp = self
|
||||
.request(Method::POST, "/api/v1/admin/tree/apply")
|
||||
@@ -1574,6 +1582,9 @@ pub struct PlanDto {
|
||||
/// §7 M3: the ownership outcome this apply would produce.
|
||||
#[serde(default)]
|
||||
pub ownership: Option<OwnershipPreviewDto>,
|
||||
/// §3 M3: envs the governing project marks confirm-required (need `--approve`).
|
||||
#[serde(default)]
|
||||
pub approvals_required: Vec<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
@@ -1610,6 +1621,9 @@ pub struct TreePlanDto {
|
||||
pub nodes: Vec<NodePlanDto>,
|
||||
#[serde(default)]
|
||||
pub state_token: String,
|
||||
/// §3 M3: envs the project marks confirm-required (need `--approve <env>`).
|
||||
#[serde(default)]
|
||||
pub approvals_required: Vec<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
|
||||
@@ -68,9 +68,15 @@ pub async fn run(
|
||||
&slug,
|
||||
&bundle,
|
||||
prune,
|
||||
manifest.project.as_ref(),
|
||||
// §3 M3: send the GOVERNING project (own block, else nearest ancestor's)
|
||||
// so the server receives the env-approval policy and can enforce the
|
||||
// gate authoritatively. It is also the correct ownership project for
|
||||
// this node (a leaf applying under its repo's project).
|
||||
governing.as_ref(),
|
||||
takeover,
|
||||
expected_token.as_deref(),
|
||||
env,
|
||||
approve,
|
||||
)
|
||||
.await?;
|
||||
crate::linkstate::clear_plan(base_dir, &slug);
|
||||
@@ -172,6 +178,8 @@ pub async fn run_tree(
|
||||
takeover,
|
||||
expected_token.as_deref(),
|
||||
structure_mode,
|
||||
env,
|
||||
approve,
|
||||
)
|
||||
.await?;
|
||||
crate::linkstate::clear_plan(dir, token_key);
|
||||
@@ -247,7 +255,7 @@ pub async fn run_tree(
|
||||
/// 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> {
|
||||
pub(crate) 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);
|
||||
|
||||
@@ -31,8 +31,15 @@ pub async fn run(manifest_path: &Path, env: Option<&str>, mode: OutputMode) -> R
|
||||
NodeKind::App
|
||||
};
|
||||
|
||||
// §3 M3: preview the gate against the GOVERNING project (own block, else the
|
||||
// nearest ancestor's), symmetric with `apply::run` — so `pic plan --file` on
|
||||
// a leaf still surfaces `approvals_required` for the gate the apply enforces.
|
||||
let governing = manifest
|
||||
.project
|
||||
.clone()
|
||||
.or_else(|| crate::cmds::apply::find_governing_project(manifest_path));
|
||||
let plan = client
|
||||
.plan_node(kind, manifest.slug(), &bundle, manifest.project.as_ref())
|
||||
.plan_node(kind, manifest.slug(), &bundle, governing.as_ref())
|
||||
.await?;
|
||||
// Record the bound-plan token so a subsequent `pic apply` can detect the
|
||||
// node changing underneath the reviewed plan (best-effort — a read-only
|
||||
@@ -122,6 +129,18 @@ fn render_tree(plan: &TreePlanDto, mode: OutputMode) {
|
||||
}
|
||||
}
|
||||
table.print(mode);
|
||||
render_approvals(&plan.approvals_required, mode);
|
||||
}
|
||||
|
||||
/// §3 M3: surface the confirm-required environments so CI sees the gate at plan
|
||||
/// time, before an `apply --approve <env>` is needed.
|
||||
fn render_approvals(approvals_required: &[String], mode: OutputMode) {
|
||||
if mode != OutputMode::Json && !approvals_required.is_empty() {
|
||||
eprintln!("\nenvironments requiring explicit approval (apply with --approve <env>):");
|
||||
for e in approvals_required {
|
||||
eprintln!(" - {e}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Assemble the wire bundle: scripts carry inlined source (read from
|
||||
@@ -257,4 +276,5 @@ fn render(plan: &PlanDto, mode: OutputMode) {
|
||||
}
|
||||
}
|
||||
table.print(mode);
|
||||
render_approvals(&plan.approvals_required, mode);
|
||||
}
|
||||
|
||||
@@ -286,9 +286,15 @@ pub struct ManifestProject {
|
||||
/// §3 M3 per-env approval policy: `[project.environments]` maps an env name
|
||||
/// to its policy. A `confirm = true` env requires an explicit `--approve
|
||||
/// <env>` on `pic apply --env <env>` (a blanket `--yes` does NOT cover it —
|
||||
/// §4.2 "CI must opt in per environment"). Client-side gating only; never
|
||||
/// sent to the server.
|
||||
#[serde(default, skip_serializing)]
|
||||
/// §4.2 "CI must opt in per environment"). The CLI gates client-side (fast
|
||||
/// fail) AND sends the policy to the server, which re-derives the gate from
|
||||
/// the request and enforces it (admin-gated on every declared node + audited).
|
||||
/// This closes the "patched/older CLI that still declares the project but
|
||||
/// skips the local check" bypass; it is NOT a hermetic boundary against a
|
||||
/// request that omits the policy entirely (the policy lives in the applier's
|
||||
/// manifest, not persisted server state — full closure would persist it).
|
||||
/// Sent only when non-empty (pre-M3 wire otherwise).
|
||||
#[serde(default, skip_serializing_if = "std::collections::BTreeMap::is_empty")]
|
||||
pub environments: std::collections::BTreeMap<String, EnvPolicy>,
|
||||
}
|
||||
|
||||
|
||||
@@ -4,7 +4,13 @@
|
||||
//! to such an env with `pic apply --env <e>` is refused unless it is explicitly
|
||||
//! `--approve <e>`d — and a blanket `--yes` does NOT cover it (§4.2, "CI must
|
||||
//! opt in per environment"). An unlisted or `confirm = false` env applies
|
||||
//! freely. The gate is client-side, so a refused apply never reaches the server.
|
||||
//! freely. The CLI gates client-side (fast fail) AND the SERVER re-derives the
|
||||
//! policy from the request and enforces it — a confirm-required apply is refused
|
||||
//! (409) unless approved, and an approved override additionally requires ADMIN on
|
||||
//! every declared node (a step up from editor write caps) and is audited. This
|
||||
//! closes the patched/older-CLI bypass (a client that still declares the project
|
||||
//! but skips the local check); a request that omits the policy is inherently
|
||||
//! ungated (the policy is applier-supplied, not persisted server state).
|
||||
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
@@ -13,6 +19,7 @@ use tempfile::TempDir;
|
||||
|
||||
use crate::common;
|
||||
use crate::common::cleanup::GroupGuard;
|
||||
use crate::common::member;
|
||||
|
||||
/// A repo whose `[project]` gates `production` (confirm-required) but not
|
||||
/// `staging`, managing one pre-existing `[group]`. Minimal per-env overlay files
|
||||
@@ -204,3 +211,129 @@ fn leaf_file_apply_honors_the_root_env_gate() {
|
||||
.assert()
|
||||
.success();
|
||||
}
|
||||
|
||||
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
||||
#[test]
|
||||
fn server_enforces_the_gate_against_a_non_stock_client() {
|
||||
// The client-side gate is a convenience; a patched CLI or a raw request can
|
||||
// skip it. The server re-derives the policy from the wire and enforces it:
|
||||
// a raw tree apply to a confirm-required env WITHOUT the env in
|
||||
// `approved_envs` is refused (409), regardless of the client.
|
||||
let Some(fx) = common::fixture_or_skip() else {
|
||||
return;
|
||||
};
|
||||
let env = common::admin_env(fx);
|
||||
let group = common::unique_slug("eas-grp");
|
||||
let project = common::unique_slug("eas-proj");
|
||||
let _g = GroupGuard::new(&env.url, &env.token, &group);
|
||||
common::pic_as(&env)
|
||||
.args(["groups", "create", &group])
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
let http = reqwest::blocking::Client::new();
|
||||
let body = |approved: serde_json::Value| {
|
||||
serde_json::json!({
|
||||
"bundle": { "nodes": [{ "kind": "group", "slug": group, "bundle": {} }] },
|
||||
"project": {
|
||||
"slug": project,
|
||||
"environments": { "production": { "confirm": true } }
|
||||
},
|
||||
"env": "production",
|
||||
"approved_envs": approved,
|
||||
"prune": false,
|
||||
})
|
||||
};
|
||||
|
||||
// No approval on the wire → 409 ApprovalRequired (server-enforced, the CLI's
|
||||
// client-side gate never ran).
|
||||
let resp = http
|
||||
.post(format!("{}/api/v1/admin/tree/apply", env.url))
|
||||
.bearer_auth(&env.token)
|
||||
.json(&body(serde_json::json!([])))
|
||||
.send()
|
||||
.expect("tree apply");
|
||||
assert_eq!(
|
||||
resp.status().as_u16(),
|
||||
409,
|
||||
"a raw gated apply without approval must be refused server-side (got {}: {})",
|
||||
resp.status(),
|
||||
resp.text().unwrap_or_default()
|
||||
);
|
||||
|
||||
// Approved on the wire + an admin token (holds GroupAdmin) → succeeds.
|
||||
let ok = http
|
||||
.post(format!("{}/api/v1/admin/tree/apply", env.url))
|
||||
.bearer_auth(&env.token)
|
||||
.json(&body(serde_json::json!(["production"])))
|
||||
.send()
|
||||
.expect("tree apply");
|
||||
assert!(
|
||||
ok.status().is_success(),
|
||||
"an approved gated apply by an admin must succeed (got {}: {})",
|
||||
ok.status(),
|
||||
ok.text().unwrap_or_default()
|
||||
);
|
||||
}
|
||||
|
||||
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
||||
#[test]
|
||||
fn approving_a_gated_apply_requires_admin() {
|
||||
// §4.2: overriding a gate is ADMIN authority — a step up from the editor
|
||||
// write caps an ordinary apply needs. A group `editor` who CAN write the
|
||||
// group still cannot approve a gated apply of it.
|
||||
let Some(fx) = common::fixture_or_skip() else {
|
||||
return;
|
||||
};
|
||||
let env = common::admin_env(fx);
|
||||
let group = common::unique_slug("eaa-grp");
|
||||
let project = common::unique_slug("eaa-proj");
|
||||
let _g = GroupGuard::new(&env.url, &env.token, &group);
|
||||
common::pic_as(&env)
|
||||
.args(["groups", "create", &group])
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
// A member granted `editor` (write, not admin) on the group.
|
||||
let m = member::member_user(fx, &common::unique_username("eaa"));
|
||||
common::pic_as(&env)
|
||||
.args([
|
||||
"groups", "members", "add", &group, &m.id, "--role", "editor",
|
||||
])
|
||||
.assert()
|
||||
.success();
|
||||
let member_env = common::custom_env(&fx.url, &m.token);
|
||||
common::seed_credentials(&member_env, &m.username);
|
||||
|
||||
// A gated repo managing that group (with a var, so the editor's write cap is
|
||||
// exercised — proving the admin gate is ABOVE it). Applied by the editor.
|
||||
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 = \"{group}\"\nname = \"Gated\"\n\n\
|
||||
[vars]\nregion = \"eu\"\n"
|
||||
),
|
||||
)
|
||||
.unwrap();
|
||||
fs::write(dir.path().join("picloud.production.toml"), "# overlay\n").unwrap();
|
||||
let manifest = dir.path().join("picloud.toml");
|
||||
|
||||
let out = common::pic_as(&member_env)
|
||||
.args(["apply", "--file"])
|
||||
.arg(&manifest)
|
||||
.args(["--env", "production", "--approve", "production"])
|
||||
.output()
|
||||
.expect("apply");
|
||||
assert!(
|
||||
!out.status.success(),
|
||||
"a non-admin editor must not be able to approve a gated apply"
|
||||
);
|
||||
let err = String::from_utf8_lossy(&out.stderr).to_lowercase();
|
||||
assert!(
|
||||
err.contains("forbidden") || err.contains("403"),
|
||||
"the approval denial should be an authz error:\n{err}"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user