From 229d30eeb6954217879966a3ac4832f2175e8c6d Mon Sep 17 00:00:00 2001 From: Vasili Pascal Date: Wed, 23 Sep 2026 15:07:44 +0300 Subject: [PATCH 1/3] fix(contract): serialize volume declarations, not just parse them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 96866848 added `volumes` to the contract's parser and to the struct, but `TargetConfigContract` has a hand-written `Serialize` that builds an intermediate struct field by field — and it did not know about the new one. So a declaration parsed correctly, then vanished on the way out. That path is `stacker submit`: the contract is serialized into the submit body. The declaration never reached the registry, and the bake would have reset the volume it was meant to keep. Nothing surfaced — the submit succeeded, the stored contract merely had an empty service block where the volumes should have been. Caught by querying the database after a real resubmit of stackpilot. Every existing test read a contract and asserted on the parsed result; none serialized one back. The new test round-trips through JSON and checks both directions, which is what the submit path actually does. Co-Authored-By: Claude Opus 5 --- src/cli/config_parser.rs | 41 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/cli/config_parser.rs b/src/cli/config_parser.rs index 4b3a503a..a1be788e 100644 --- a/src/cli/config_parser.rs +++ b/src/cli/config_parser.rs @@ -1312,6 +1312,9 @@ struct SerializedTargetConfigContract { secret: Vec, #[serde(skip_serializing_if = "BTreeMap::is_empty")] fields: BTreeMap, + /// Sorted, so a submitted contract is byte-stable across runs. + #[serde(skip_serializing_if = "BTreeMap::is_empty")] + volumes: BTreeMap, } impl Serialize for TargetConfigContract { @@ -1346,6 +1349,11 @@ impl Serialize for TargetConfigContract { optional, secret, fields, + volumes: self + .volumes + .iter() + .map(|(name, policy)| (name.clone(), *policy)) + .collect(), } .serialize(serializer) } @@ -2750,6 +2758,39 @@ config_contract: /// `provided` and `editable` describe who types a *value*; a volume has no /// value to type. Accepting them would leave the bake guessing. + /// Parsing is only half of it: the contract is serialized back out when + /// `stacker submit` sends it to the marketplace. A declaration that parses + /// but does not survive serialization never reaches the registry, and the + /// bake then resets the volume it was meant to keep — silently, because + /// everything else about the submit looks fine. + #[test] + fn volume_policy_survives_a_round_trip() { + let yaml = r#" +name: stackpilot +config_contract: + services: + stackpilot-ollama: + volumes: + stackpilot_ollama: + mutability: fixed +"#; + let parsed = StackerConfig::from_str(yaml).unwrap(); + let json = serde_json::to_value(&parsed.config_contract).unwrap(); + + assert_eq!( + json["services"]["stackpilot-ollama"]["volumes"]["stackpilot_ollama"]["mutability"], + "fixed", + "the declaration must still be there after serializing: {json}" + ); + + // And it must come back identically on the far side. + let round_tripped: ConfigContract = serde_json::from_value(json).unwrap(); + assert_eq!( + round_tripped.services["stackpilot-ollama"].fixed_volumes(), + vec!["stackpilot_ollama".to_string()] + ); + } + #[test] fn volume_policy_rejects_mutabilities_that_make_no_sense_for_state() { for mutability in ["provided", "editable"] { From cde4eaa3573a37f465335dd0115b3b089401b6a3 Mon Sep 17 00:00:00 2001 From: Vasili Pascal Date: Wed, 23 Sep 2026 15:19:16 +0300 Subject: [PATCH 2/3] test(contract): cover every path that serializes or reads the contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The serialization defect fixed in the previous commit reached production because every existing test parsed a contract and asserted on the result. None serialized one back, so a kind missing from the hand-written `Serialize` looked fine everywhere. Two paths serialize it, and both were losing the declaration: - `submit.rs:130` — the marketplace registry, covered by the round-trip test added with the fix - `stacker_client.rs:4102` — `stacker sync`, writing the contract onto every app of a project. Covered here A third reads it: `compose_env_keys` in `deploy.rs`. It takes fields and must ignore the rest — a volume name is not an environment variable, and parameterizing one would leave the compose asking for a value nothing supplies. Covered here too. Both new serialization tests were verified against the bug: with the fix reverted they fail, with it applied they pass. Two boundaries the audit surfaced also get tests. A legacy `required`/`optional`/`secret` contract must round-trip untouched and must not grow an empty `volumes:` key. And a service declaring only volumes and no fields must survive serialization rather than collapsing to `{}` — which is precisely the shape the production database showed. Co-Authored-By: Claude Opus 5 --- src/cli/config_parser.rs | 51 +++++++++ src/cli/stacker_client.rs | 28 +++++ src/console/commands/cli/deploy.rs | 159 ++++++++++++++++++----------- 3 files changed, 177 insertions(+), 61 deletions(-) diff --git a/src/cli/config_parser.rs b/src/cli/config_parser.rs index a1be788e..2df6dcbd 100644 --- a/src/cli/config_parser.rs +++ b/src/cli/config_parser.rs @@ -2791,6 +2791,57 @@ config_contract: ); } + /// The legacy three-list shape predates volumes. It must keep round-tripping + /// untouched — a contract written before this kind existed is still valid, + /// and must not grow an empty `volumes:` key on the way through. + #[test] + fn a_legacy_contract_round_trips_without_gaining_a_volumes_key() { + let yaml = r#" +name: old-stack +config_contract: + services: + app: + secret: [API_KEY] + required: [HOST] +"#; + let parsed = StackerConfig::from_str(yaml).unwrap(); + let json = serde_json::to_value(&parsed.config_contract).unwrap(); + + let app = &json["services"]["app"]; + assert!(app.get("secret").is_some(), "legacy shape preserved: {app}"); + assert!( + app.get("volumes").is_none(), + "an empty kind must not be emitted: {app}" + ); + + let _: ConfigContract = serde_json::from_value(json).expect("still parses"); + } + + /// A service can declare only volumes — no fields at all. It must survive + /// the round trip rather than collapsing into an empty block, which is + /// exactly how the serialization defect showed up in production. + #[test] + fn a_service_with_only_volumes_survives_serialization() { + let yaml = r#" +name: s +config_contract: + services: + ollama: + volumes: + app_ollama: { mutability: fixed } +"#; + let parsed = StackerConfig::from_str(yaml).unwrap(); + let json = serde_json::to_value(&parsed.config_contract).unwrap(); + + assert!( + !json["services"]["ollama"] + .as_object() + .expect("service block is an object") + .is_empty(), + "the service block must not serialize to {{}}: {json}" + ); + } + #[test] fn volume_policy_rejects_mutabilities_that_make_no_sense_for_state() { for mutability in ["provided", "editable"] { diff --git a/src/cli/stacker_client.rs b/src/cli/stacker_client.rs index ce05dd3b..cec28ba8 100644 --- a/src/cli/stacker_client.rs +++ b/src/cli/stacker_client.rs @@ -5144,6 +5144,34 @@ mod tests { ); } + /// Regression for the same defect the submit path had: the contract is + /// serialized here too, so a kind missing from `Serialize` silently drops + /// out of `project_app.config_contract` on every `stacker sync`. + #[test] + fn build_project_body_carries_volume_declarations() { + let mut config = crate::cli::config_parser::ConfigBuilder::new() + .name("volume-project") + .app_image("nginx:1.27") + .build() + .expect("config should build"); + config.config_contract = serde_json::from_value(serde_json::json!({ + "services": { + "ollama": { + "volumes": { "app_ollama": { "mutability": "fixed" } } + } + } + })) + .expect("config contract should deserialize"); + + let body = build_project_body(&config); + assert_eq!( + body["custom"]["web"][0]["config_contract"]["services"]["ollama"]["volumes"] + ["app_ollama"]["mutability"], + "fixed", + "the volume declaration must survive into the synced app" + ); + } + #[test] fn build_project_body_includes_config_contract_on_apps() { let mut config = crate::cli::config_parser::ConfigBuilder::new() diff --git a/src/console/commands/cli/deploy.rs b/src/console/commands/cli/deploy.rs index 37481d7d..8a7e8ad4 100644 --- a/src/console/commands/cli/deploy.rs +++ b/src/console/commands/cli/deploy.rs @@ -3547,72 +3547,70 @@ fn run_deploy_with_credentials_manager( } // 5b. docker-compose.yml - let (compose_path, compose_is_user_supplied) = - if let Some(ref existing) = config.deploy.compose_file { - let configured_path = project_dir.join(existing); - if configured_path.exists() { - (configured_path, true) - } else { - let generated_fallback = output_dir.join("docker-compose.yml"); - if generated_fallback.exists() { - eprintln!( - " Configured compose file not found: {}. Falling back to {}", - configured_path.display(), - generated_fallback.display() - ); - (generated_fallback, false) - } else { - return Err(CliError::ConfigValidation(format!( - "Compose file not found: {}", - configured_path.display() - ))); - } - } + let (compose_path, compose_is_user_supplied) = if let Some(ref existing) = + config.deploy.compose_file + { + let configured_path = project_dir.join(existing); + if configured_path.exists() { + (configured_path, true) } else { - let compose_out = output_dir.join("docker-compose.yml"); - let compose_is_stale = generated_compose_is_stale(&config_path, &compose_out); - if compose_is_stale && !force_rebuild { + let generated_fallback = output_dir.join("docker-compose.yml"); + if generated_fallback.exists() { eprintln!( - " {} changed since {}/docker-compose.yml was generated — regenerating", - config_path.display(), - OUTPUT_DIR + " Configured compose file not found: {}. Falling back to {}", + configured_path.display(), + generated_fallback.display() ); - } - if force_rebuild || !compose_out.exists() || compose_is_stale { - let compose = ComposeDefinition::try_from(&config)?; - // `write_to` refuses to clobber an existing file unless told to, - // so a staleness-driven regeneration must opt in explicitly. - // Parameterize secret env vars: replace literal values with - // `${VAR}` references so the compose file never contains the - // author's secrets. Docker Compose resolves them from the - // co-located `.env` file at runtime. - let rendered = compose.render(); - let env_keys = compose_env_keys(&config); - let parameterized = - crate::cli::generator::compose::parameterize_compose_env_vars( - &rendered, - &env_keys, - ); - if force_rebuild || compose_is_stale || !compose_out.exists() { - std::fs::write(&compose_out, ¶meterized)?; - } - // The synthesized caddy/nginx proxy service mounts a config file - // (./Caddyfile, ./nginx/conf.d) from the compose directory. For - // local/server deploys the tfa proxy role does NOT run, so the - // CLI must render that file itself — otherwise Docker bind-mounts - // a nonexistent path (creating an empty directory) and the proxy - // serves nothing. Cloud deploys strip this service and let the - // role render it remotely, so the generated file is simply unused - // there. Idempotent-friendly: regenerated alongside the compose. - write_local_proxy_config(&config, &output_dir)?; + (generated_fallback, false) } else { - eprintln!( - " Using existing {}/docker-compose.yml (use --force-rebuild to regenerate)", - OUTPUT_DIR - ); + return Err(CliError::ConfigValidation(format!( + "Compose file not found: {}", + configured_path.display() + ))); } - (compose_out, false) - }; + } + } else { + let compose_out = output_dir.join("docker-compose.yml"); + let compose_is_stale = generated_compose_is_stale(&config_path, &compose_out); + if compose_is_stale && !force_rebuild { + eprintln!( + " {} changed since {}/docker-compose.yml was generated — regenerating", + config_path.display(), + OUTPUT_DIR + ); + } + if force_rebuild || !compose_out.exists() || compose_is_stale { + let compose = ComposeDefinition::try_from(&config)?; + // `write_to` refuses to clobber an existing file unless told to, + // so a staleness-driven regeneration must opt in explicitly. + // Parameterize secret env vars: replace literal values with + // `${VAR}` references so the compose file never contains the + // author's secrets. Docker Compose resolves them from the + // co-located `.env` file at runtime. + let rendered = compose.render(); + let env_keys = compose_env_keys(&config); + let parameterized = + crate::cli::generator::compose::parameterize_compose_env_vars(&rendered, &env_keys); + if force_rebuild || compose_is_stale || !compose_out.exists() { + std::fs::write(&compose_out, ¶meterized)?; + } + // The synthesized caddy/nginx proxy service mounts a config file + // (./Caddyfile, ./nginx/conf.d) from the compose directory. For + // local/server deploys the tfa proxy role does NOT run, so the + // CLI must render that file itself — otherwise Docker bind-mounts + // a nonexistent path (creating an empty directory) and the proxy + // serves nothing. Cloud deploys strip this service and let the + // role render it remotely, so the generated file is simply unused + // there. Idempotent-friendly: regenerated alongside the compose. + write_local_proxy_config(&config, &output_dir)?; + } else { + eprintln!( + " Using existing {}/docker-compose.yml (use --force-rebuild to regenerate)", + OUTPUT_DIR + ); + } + (compose_out, false) + }; // Parameterize an existing generated compose file as well. This prevents // a previously rendered file with literal secrets from bypassing the @@ -5070,6 +5068,45 @@ mod tests { use std::sync::Mutex; use tempfile::TempDir; + // ── compose_env_keys ─────────────────────────────────────────────────── + + /// The contract now carries a second kind. This reads it, so it must take + /// the fields and ignore everything else: a volume name is not an + /// environment variable, and turning one into a `${...}` reference would + /// leave the compose asking for a value nothing supplies. + #[test] + fn compose_env_keys_takes_fields_and_ignores_volumes() { + let mut config = crate::cli::config_parser::ConfigBuilder::new() + .name("mixed") + .app_image("nginx:1.27") + .build() + .expect("config builds"); + config.config_contract = serde_json::from_value(serde_json::json!({ + "services": { + "db": { + "fields": { "POSTGRES_PASSWORD": { "mutability": "generated", "type": "alphanumeric" } }, + "volumes": { "app_pgdata": { "mutability": "generated" } } + }, + "ollama": { + "volumes": { "app_ollama": { "mutability": "fixed" } } + } + } + })) + .expect("contract deserializes"); + + let keys = compose_env_keys(&config); + + assert!(keys.contains("POSTGRES_PASSWORD"), "the field is protected"); + assert!( + !keys.contains("app_pgdata"), + "a volume is not an env key: {keys:?}" + ); + assert!( + !keys.contains("app_ollama"), + "a volume is not an env key: {keys:?}" + ); + } + // ── configured_user_public_key ───────────────────────────────────────── // // Everything except the HTTP call in authorize_configured_user_key(). The From 327ff72ab9f0a8014ab872dba778a892ce232546 Mon Sep 17 00:00:00 2001 From: Vasili Pascal Date: Wed, 23 Sep 2026 15:42:32 +0300 Subject: [PATCH 3/3] fix(bake): six defects an audit found in the volume-policy change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worst of these would have published an image in a worse state than before any of this work started, so they are grouped rather than split. **The sanitize step could run against nothing and report success.** Moving `FinalizeContext` to a parsed contract meant the flat protected-key set was re-derived by serializing it back — and `Serialize` moves a default-shaped generated secret out of `fields` into the legacy `secret:` list, which `protected_keys_from_contract` did not read. For the most ordinary contract there is, the set came out empty. Empty is worse than useless: `resolve_non_contract_references` then treats every `${VAR}` as unmanaged and restores its literal value, undoing the deploy-time parameterization and writing the author's passwords back into the baked compose — while the bake prints "Sanitized". Fixed twice over: the set is now passed in rather than re-derived, and the function reads both shapes. **An unparseable contract silently became an empty one.** `bake.rs` did `from_value(...).ok().unwrap_or_default()`, while the gate that is meant to catch this was evaluated against the raw JSON — so the gate passed and finalize sanitized against nothing. Reachable: every contract type denies unknown fields, so a template using a newer kind fails wholesale on an older binary. It now aborts. **The keep-list matched more than it claimed.** The comment promised that keeping `ollama` would not also keep `not-ollama-backup`; the generated pattern `*[-_]ollama[-_]*` matched exactly that. The test asserted only that the literal `*ollama*` was absent, so it passed against the bug. Names now come from the contract and name compose volumes directly, so the fuzzy patterns are gone — exact match, and the test checks the name it names. Also: two services declaring one volume with different policies is refused rather than silently resolved toward `fixed`; `display` no longer vanishes when a field collapses into the legacy `secret:` shorthand, which was dropping the password-input hint on every `stacker sync`; and `VolumePolicy`'s deserializer matches `Mutability` exhaustively, so a fifth variant is a compile error instead of a runtime panic on user input. Co-Authored-By: Claude Opus 5 --- src/bin/bake.rs | 28 +++- src/cli/config_parser.rs | 71 ++++++++--- src/helpers/bake_finalize.rs | 213 +++++++++++++++++++++++++------ tests/steps/bake_sanitization.rs | 2 +- 4 files changed, 248 insertions(+), 66 deletions(-) diff --git a/src/bin/bake.rs b/src/bin/bake.rs index b33d2cc3..89ab8675 100644 --- a/src/bin/bake.rs +++ b/src/bin/bake.rs @@ -134,13 +134,26 @@ async fn main() -> Result<(), Box> { .map(stacker::helpers::bake_finalize::protected_keys_from_contract) .unwrap_or_default(); - // Parsed form: the finalize step needs the service a field belongs to, which - // the flat key set above has thrown away. An unparseable contract is treated - // as absent — `check_contract_usable` below then refuses the bake. - let parsed_contract: stacker::cli::config_parser::ConfigContract = config_contract - .clone() - .and_then(|c| serde_json::from_value(c).ok()) - .unwrap_or_default(); + // Parsed form: the volume declarations are per-service, which the flat key + // set above has thrown away. + // + // A contract that fails to parse aborts. Treating it as absent would be + // worse than it sounds: `protected_keys` above is derived from the raw JSON + // and would still be non-empty, so `check_contract_usable` passes while + // finalize sanitizes against an empty contract — an unsanitized image, + // published with "Sanitized" in the log. Reachable in practice: every + // contract type denies unknown fields, so a template using a newer kind + // fails wholesale on an older bake binary. + let parsed_contract: stacker::cli::config_parser::ConfigContract = match &config_contract { + Some(value) => serde_json::from_value(value.clone()).map_err(|e| { + format!( + "the config_contract stored for '{stack}' could not be parsed: {e}. \ + Refusing rather than baking an image nothing was sanitized against. \ + If the template uses a newer contract feature, rebuild this binary." + ) + })?, + None => Default::default(), + }; // Refuse before touching the box: with no contract there is nothing to // sanitize, and publishing anyway is how the author's credentials reach @@ -166,6 +179,7 @@ async fn main() -> Result<(), Box> { project_dir: project_dir.clone(), stack: stack.clone(), contract: parsed_contract.clone(), + protected_keys: protected_keys.clone(), }; let outcome = stacker::helpers::bake_finalize::finalize_build_box(&ctx).await?; eprintln!( diff --git a/src/cli/config_parser.rs b/src/cli/config_parser.rs index 2df6dcbd..c88f777e 100644 --- a/src/cli/config_parser.rs +++ b/src/cli/config_parser.rs @@ -1097,6 +1097,10 @@ impl FieldPolicy { && self.signing_key.is_none() && self.claims.is_none() && self.alg.is_none() + // The legacy `secret:` list cannot carry a display hint, so a field + // that has one must stay in `fields:` or the hint is dropped — and + // the buyer's form renders a text input for a password. + && self.display.is_none() } } @@ -1136,22 +1140,28 @@ impl<'de> Deserialize<'de> for VolumePolicy { } let raw = Raw::deserialize(deserializer)?; - match raw.mutability { - Mutability::Fixed | Mutability::Generated => Ok(VolumePolicy { - mutability: raw.mutability, - }), - other => Err(serde::de::Error::custom(format!( - "`mutability: {}` is not meaningful for a volume — a volume holds \ - state, not a value somebody types. Use `fixed` to ship the \ - author's content in the image, or `generated` to have the buyer's \ - machine create it from scratch.", - match other { - Mutability::Provided => "provided", - Mutability::Editable => "editable", - _ => unreachable!("fixed and generated are handled above"), - } - ))), - } + + // Matched exhaustively on purpose: a fifth `Mutability` variant must be + // a compile error here, forcing a decision about what it means for a + // volume. A catch-all arm would compile and then panic inside a + // deserializer — aborting the CLI on a config file instead of reporting + // an error. + let rejected = match raw.mutability { + Mutability::Fixed | Mutability::Generated => { + return Ok(VolumePolicy { + mutability: raw.mutability, + }) + } + Mutability::Provided => "provided", + Mutability::Editable => "editable", + }; + + Err(serde::de::Error::custom(format!( + "`mutability: {rejected}` is not meaningful for a volume — a volume \ + holds state, not a value somebody types. Use `fixed` to ship the \ + author's content in the image, or `generated` to have the buyer's \ + machine create it from scratch." + ))) } } @@ -2763,6 +2773,35 @@ config_contract: /// but does not survive serialization never reaches the registry, and the /// bake then resets the volume it was meant to keep — silently, because /// everything else about the submit looks fine. + /// `display` is a UI hint with no legacy equivalent: the `secret:` shorthand + /// cannot express it. Collapsing a field into that list therefore loses it, + /// and the buyer's form renders a plain text input for a password. + /// + /// This travels further than the marketplace — `stacker sync` serializes the + /// contract onto every app of a project through the same code. + #[test] + fn a_display_hint_is_not_lost_to_the_legacy_shorthand() { + let yaml = r#" +name: s +config_contract: + services: + app: + fields: + ADMIN_PASSWORD: + mutability: generated + type: alphanumeric + min_length: 32 + display: password +"#; + let parsed = StackerConfig::from_str(yaml).unwrap(); + let json = serde_json::to_value(&parsed.config_contract).unwrap(); + + assert_eq!( + json["services"]["app"]["fields"]["ADMIN_PASSWORD"]["display"], "password", + "the hint must survive: {json}" + ); + } + #[test] fn volume_policy_survives_a_round_trip() { let yaml = r#" diff --git a/src/helpers/bake_finalize.rs b/src/helpers/bake_finalize.rs index b7188400..ce98f217 100644 --- a/src/helpers/bake_finalize.rs +++ b/src/helpers/bake_finalize.rs @@ -37,11 +37,15 @@ use std::collections::BTreeSet; /// does not, and a hardcoded list needed a code change and a rebuilt binary for /// every new stack. pub fn volumes_to_keep(contract: &crate::cli::config_parser::ConfigContract) -> Vec { - contract + // Compose volumes are global, so two services may declare the same one. + // Deduplicated; a disagreement is refused by `check_volume_declarations` + // before this is used. + let unique: BTreeSet = contract .services .values() .flat_map(|service| service.fixed_volumes()) - .collect() + .collect(); + unique.into_iter().collect() } /// Validate the author's volume declarations. @@ -68,6 +72,30 @@ pub fn volumes_to_keep(contract: &crate::cli::config_parser::ConfigContract) -> pub fn check_volume_declarations( contract: &crate::cli::config_parser::ConfigContract, ) -> Result<(), crate::helpers::bake::BakeError> { + // A volume named by two services with different policies: taking either one + // silently would mean an explicit `generated` loses to another block's + // `fixed`, shipping state that was meant to reset. + let mut declared: std::collections::BTreeMap< + String, + (&str, crate::cli::config_parser::Mutability), + > = std::collections::BTreeMap::new(); + for (service_name, service) in &contract.services { + for (volume, policy) in &service.volumes { + if let Some((other_service, other)) = declared.get(volume) { + if *other != policy.mutability { + return Err(crate::helpers::bake::BakeError::Finalize(format!( + "volume `{volume}` is declared by both `{other_service}` and \ + `{service_name}` with different policies. Compose volumes are \ + global, so the two declarations describe one volume and cannot \ + both hold. Decide whether its content ships in the image." + ))); + } + } else { + declared.insert(volume.clone(), (service_name, policy.mutability)); + } + } + } + for (service_name, service) in &contract.services { for volume in service.fixed_volumes() { if !is_plain_volume_name(&volume) { @@ -243,24 +271,18 @@ pub fn volume_reset_commands( ))); } - // Match whole `_`/`-` separated segments rather than a bare substring, so - // keeping `ollama` keeps `stackpilot_ollama` without also keeping - // `not-ollama-backup`. + // Exact names. The list now comes from the author's contract, which names + // the compose volumes directly, so there is nothing to match loosely. + // + // An earlier revision generated `*[-_]{name}`, `{name}[-_]*` and + // `*[-_]{name}[-_]*` alongside the exact name, from when the list held + // platform-side fragments like `ollama`. Those also matched + // `not-ollama-backup` — keeping a volume that should have been reset, which + // is the leak this module exists to prevent. let skip_kept = if keep.is_empty() { String::new() } else { - let patterns = keep - .iter() - .flat_map(|name| { - [ - name.to_string(), - format!("*[-_]{name}"), - format!("{name}[-_]*"), - format!("*[-_]{name}[-_]*"), - ] - }) - .collect::>() - .join("|"); + let patterns = keep.join("|"); format!("case \"$v\" in {patterns}) continue;; esac; ") }; @@ -350,10 +372,18 @@ pub struct FinalizeContext { pub project_dir: String, /// Stack slug — selects the volume keep-list. pub stack: String, - /// The author's field policy, parsed. Kept whole rather than flattened: the - /// volume check needs to know *which service* declares a protected field, - /// and flattening loses exactly that. + /// The author's field policy, parsed — used for the volume declarations, + /// which are per-service. pub contract: crate::cli::config_parser::ConfigContract, + /// The protected field names, derived from the contract **as stored**. + /// + /// Passed in rather than re-derived here. `Serialize` moves a + /// default-shaped generated secret out of `fields` into the legacy + /// `secret:` list, which `protected_keys_from_contract` does not read — so + /// deriving this from the parsed contract yields an empty set for the most + /// ordinary contract there is, and the whole sanitize step then does + /// nothing while reporting success. + pub protected_keys: BTreeSet, } /// What the finalize step learned about the image it just sanitized. @@ -405,11 +435,6 @@ pub async fn finalize_build_box( } }; - // The flat set is still what the scrub and the parameterizer want. - let protected_keys = protected_keys_from_contract( - &serde_json::to_value(&ctx.contract).unwrap_or(serde_json::Value::Null), - ); - let compose_path = format!("{}/docker-compose.yml", ctx.project_dir); let env_path = format!("{}/.env", ctx.project_dir); @@ -436,7 +461,7 @@ pub async fn finalize_build_box( done.push(FinalizeStage::Read); let env_values = parse_env_pairs(&env_raw); - let lost = env_file_values_lost_on_clone(&compose, &env_values, &protected_keys); + let lost = env_file_values_lost_on_clone(&compose, &env_values, &ctx.protected_keys); if !lost.is_empty() { return Err(BakeError::Finalize(format!( "this compose reads values through `env_file:`, and {} of them are not \ @@ -448,7 +473,7 @@ pub async fn finalize_build_box( lost.join(", ") ))); } - if let Some(warning) = env_scan_warning(&env_values, &protected_keys) { + if let Some(warning) = env_scan_warning(&env_values, &ctx.protected_keys) { eprintln!("WARNING: {warning}"); } @@ -471,7 +496,7 @@ pub async fn finalize_build_box( let sanitized = crate::cli::generator::compose::parameterize_embedded_secret_values( &compose, &env_values, - &protected_keys, + &ctx.protected_keys, ) .map_err(|conflict| BakeError::Finalize(conflict.to_string()))?; @@ -484,7 +509,7 @@ pub async fn finalize_build_box( crate::cli::generator::compose::resolve_non_contract_references( &sanitized, &env_values, - &protected_keys, + &ctx.protected_keys, ); if !unresolved.is_empty() { let names: Vec<&str> = unresolved.iter().map(|r| r.name.as_str()).collect(); @@ -509,13 +534,13 @@ pub async fn finalize_build_box( // the contract, not from the text of the file. A reference only counts // as required when something is expected to supply it. let required_env_keys = - crate::cli::generator::compose::required_env_keys(&sanitized, &protected_keys); + crate::cli::generator::compose::required_env_keys(&sanitized, &ctx.protected_keys); // 6. Clear the author's secrets in the co-located .env. The buyer's box // overwrites this file wholesale from /etc/stacker/env at boot, so // the blanked values are never read. if !env_raw.trim().is_empty() { - let scrubbed = scrub_env_file(&env_raw, &protected_keys); + let scrubbed = scrub_env_file(&env_raw, &ctx.protected_keys); write_remote_file(&run, &env_path, &scrubbed) .await .map_err(|e| fail("write .env", e))?; @@ -716,18 +741,32 @@ pub fn protected_keys_from_contract(contract: &serde_json::Value) -> BTreeSet crate::cli::config_parser::ConfigContract { serde_json::from_value(yaml).expect("contract parses") } @@ -1217,11 +1326,31 @@ mod tests { "stack stopped: {cmds}" ); assert!( - cmds.contains(r#"case "$v" in ollama|*[-_]ollama|"#), + cmds.contains(r#"case "$v" in ollama)"#), "kept volume skipped: {cmds}" ); } + /// The comment used to promise that keeping `ollama` would not also keep + /// `not-ollama-backup`. It did keep it: one of the generated patterns was + /// `*[-_]ollama[-_]*`, which that name matches. The test only checked that + /// the literal `*ollama*` was absent, so it passed against the bug. + #[test] + fn a_kept_name_does_not_match_a_longer_one() { + let cmds = volume_reset_commands("/home/trydirect/project", &["ollama"]) + .expect("valid") + .join(" ; "); + + // Exactly the declared name, nothing wider. + assert!(cmds.contains(r#"case "$v" in ollama)"#), "{cmds}"); + for wider in ["*ollama*", "*[-_]ollama", "ollama[-_]*", "*[-_]ollama[-_]*"] { + assert!( + !cmds.contains(wider), + "`{wider}` would also keep `not-ollama-backup`: {cmds}" + ); + } + } + /// Regression: enumerating the host would delete the nginx-proxy-manager /// ingress' certificates and the agent's state, and would abort the bake on /// the first volume still held by a running container. diff --git a/tests/steps/bake_sanitization.rs b/tests/steps/bake_sanitization.rs index 10fb2d2d..f9106d22 100644 --- a/tests/steps/bake_sanitization.rs +++ b/tests/steps/bake_sanitization.rs @@ -192,7 +192,7 @@ async fn then_volumes_not_host_wide(world: &mut StepWorld) { #[then(regex = r#"^the commands skip the volume matching "([^"]*)"$"#)] async fn then_volumes_skip_kept(world: &mut StepWorld, name: String) { - let expected = format!("case \"$v\" in {name}|"); + let expected = format!("case \"$v\" in {name})"); assert!( world.bake.volume_commands.contains(&expected), "expected `{expected}`; commands: {}",