diff --git a/.github/scripts/next-version.sh b/.github/scripts/next-version.sh index 27fce0e..a0c3758 100755 --- a/.github/scripts/next-version.sh +++ b/.github/scripts/next-version.sh @@ -2,7 +2,7 @@ # next-version.sh — compute THIS repo's next release tag for release-on-upstream.yml. # # Single source of truth for the version math, exercised in CI by release-selftest.yml so the -# release automation can't silently rot (guard #135.8). Prints "v.." to stdout. +# release automation can't silently rot. Prints "v.." to stdout. # # Inputs (env, all optional): # INPUT_VERSION explicit version to cut (leading "v" tolerated) -> used verbatim. diff --git a/.github/workflows/release-selftest.yml b/.github/workflows/release-selftest.yml index a6aafb6..9cfa502 100644 --- a/.github/workflows/release-selftest.yml +++ b/.github/workflows/release-selftest.yml @@ -1,4 +1,4 @@ -# CI self-test for the release-on-upstream version-compute logic (guard #135.8). +# CI self-test for the release-on-upstream version-compute logic. # Runs the REAL .github/scripts/next-version.sh against synthetic repos and asserts it produces a # valid next version for BOTH the has-prior-tag and no-prior-tag cases — WITHOUT publishing anything. # This is what keeps the release automation from silently rotting before the fleet fan-out is armed. diff --git a/store-postgres-plugin/tests/admin_api_e2e.rs b/store-postgres-plugin/tests/admin_api_e2e.rs index ad44053..785d43c 100644 --- a/store-postgres-plugin/tests/admin_api_e2e.rs +++ b/store-postgres-plugin/tests/admin_api_e2e.rs @@ -40,6 +40,8 @@ //! Together: real admin-API install -> real (admin-API-driven) plugin load -> real admin-API write //! -> real, independently-verified Postgres persistence. +mod common; + use base64::Engine as _; use std::path::PathBuf; use std::process::{Child, Command, Stdio}; @@ -80,9 +82,8 @@ fn postgres_url() -> Option { /// Checks BOTH the "uplifted" `/` copy (only refreshed when `[lib]` is a ROOT /// build target of the invocation, e.g. `cargo build --all-targets`) and the raw /// `/deps/` compiler output (refreshed on every build that recompiles the lib, -/// uplifted or not). A bare `cargo test --release` (what `release-check.sh`'s Phase 2 runs, and what -/// cargo-mutants runs) does NOT uplift the cdylib to the top-level profile dir, only to -/// `target/deps` — checking only `profile_dir` silently finds nothing and this test's CI hard-panic +/// uplifted or not). A bare `cargo test --release` (what `release-check.sh`'s Phase 2 runs) does +/// NOT uplift the cdylib to the top-level profile dir, only to `target/deps` — checking only `profile_dir` silently finds nothing and this test's CI hard-panic /// fires even though the cdylib really was built (confirmed against `release-check.sh`'s CI run: /// "the store-postgres-plugin cdylib is not built under CI" despite the prior `cargo test /// --workspace --release` step compiling it). Same fix already applied to auth-oidc-plugin's and @@ -287,7 +288,9 @@ fn install_over_admin_api_then_mint_a_key_and_verify_postgres_directly() { let config1 = work.join("config1.yaml"); std::fs::write(&config1, &providers_and_common).unwrap(); - let child1 = Command::new(&busbar_bin) + let mut boot1 = Command::new(&busbar_bin); + common::apply_placeholder_secrets_from_files(&mut boot1, &[&config1, &providers]); + let child1 = boot1 .env("BUSBAR_CONFIG", &config1) .env("BUSBAR_PROVIDERS", &providers) .env("BUSBAR_ADMIN_TOKEN", admin_token) @@ -381,7 +384,9 @@ fn install_over_admin_api_then_mint_a_key_and_verify_postgres_directly() { ) .unwrap(); - let child2 = Command::new(&busbar_bin) + let mut boot2 = Command::new(&busbar_bin); + common::apply_placeholder_secrets_from_files(&mut boot2, &[&config2, &providers]); + let child2 = boot2 .env("BUSBAR_CONFIG", &config2) .env("BUSBAR_PROVIDERS", &providers) .env("BUSBAR_ADMIN_TOKEN", admin_token) diff --git a/store-postgres-plugin/tests/common/mod.rs b/store-postgres-plugin/tests/common/mod.rs new file mode 100644 index 0000000..4691343 --- /dev/null +++ b/store-postgres-plugin/tests/common/mod.rs @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright (C) 2026 Busbar Inc and contributors + +//! Support code shared by this crate's integration tests: supplying the secrets a config fixture +//! references so the real `busbar` binary can start. +//! +//! busbar resolves every `env:`/`file:` secret reference during `--validate` AND at boot, and exits +//! non-zero when one of them cannot resolve. A fixture that names a variable nobody exported is +//! therefore a gateway that cannot come up, and the test reports that as an opaque timeout or +//! early-exit a long way from the YAML that caused it. +//! +//! The variable names are derived FROM the fixture text rather than listed here on purpose: the +//! next person to add a secret reference to a config in these tests must not have to also remember +//! to export it, because forgetting produces a failure that does not name the variable. +//! +//! This lives under `tests/common/` rather than `tests/` so cargo does not compile it as a test +//! binary of its own; its own coverage lives in `tests/config_secrets.rs`. + +#![allow(dead_code)] +// Each integration test binary compiles this module separately and uses a different subset of it, +// so anything only one of them needs is genuinely dead code from the other's point of view. + +use std::path::Path; +use std::process::Command; + +/// Stands in for every secret a fixture references but the test does not otherwise set. The content +/// is irrelevant — what matters is only that resolution SUCCEEDS. +pub const SECRET_PLACEHOLDER: &str = "e2e-placeholder"; + +/// Every environment variable `config` names, whether through a `{ env: NAME }` secret reference or +/// through a legacy `*_env: NAME` field, in first-seen order and without duplicates. +pub fn referenced_env_vars(config: &str) -> Vec { + let mut names = Vec::new(); + for (idx, _) in config.match_indices("env:") { + let name: String = config[idx + "env:".len()..] + .trim_start_matches([' ', '\t']) + .chars() + .take_while(|c| c.is_ascii_alphanumeric() || *c == '_') + .collect(); + // A bare `env:` with nothing usable after it (a line break, or the key of a nested mapping) + // names no variable at all; skip it rather than exporting an empty-named one. + if !name.is_empty() && !names.contains(&name) { + names.push(name); + } + } + names +} + +/// Sets [`SECRET_PLACEHOLDER`] for every environment variable `config` references. +/// +/// Call this BEFORE any `.env()` carrying a value the test actually asserts on (an admin token, a +/// signing key): the later `.env()` wins, so the placeholder only ever fills in the secrets whose +/// value the test does not care about. +pub fn apply_placeholder_secrets(cmd: &mut Command, config: &str) { + for name in referenced_env_vars(config) { + cmd.env(name, SECRET_PLACEHOLDER); + } +} + +/// [`apply_placeholder_secrets`] over the files busbar will actually read. +/// +/// Reading the files back, rather than the strings the test built them from, is what makes this +/// impossible to get out of step: whatever landed on disk is exactly what busbar resolves. +pub fn apply_placeholder_secrets_from_files(cmd: &mut Command, paths: &[&Path]) { + for path in paths { + let text = std::fs::read_to_string(path).unwrap_or_else(|e| { + panic!( + "read config fixture {} for its secrets: {e}", + path.display() + ) + }); + apply_placeholder_secrets(cmd, &text); + } +} diff --git a/store-postgres-plugin/tests/config_secrets.rs b/store-postgres-plugin/tests/config_secrets.rs new file mode 100644 index 0000000..85a2075 --- /dev/null +++ b/store-postgres-plugin/tests/config_secrets.rs @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright (C) 2026 Busbar Inc and contributors + +//! Coverage for `tests/common`'s secret-reference scan. +//! +//! That scan is what keeps the real-binary e2e tests bootable, and when it misses a reference the +//! symptom surfaces a long way from here — as `busbar` exiting 1 before its listener comes up — +//! so it is worth testing directly rather than only through the e2e tests it supports. + +mod common; + +use common::referenced_env_vars; + +#[test] +fn finds_secret_references_and_legacy_env_fields() { + let config = "\ +providers:\n mock:\n api_key: { env: MOCK_KEY }\n\ +identity-providers:\n admin-tokens: { module: admin-tokens, token: { env: BUSBAR_ADMIN_TOKEN } }\n\ +auth:\n signing_key: { env: BUSBAR_SIGNING_KEY }\n"; + assert_eq!( + referenced_env_vars(config), + vec!["MOCK_KEY", "BUSBAR_ADMIN_TOKEN", "BUSBAR_SIGNING_KEY"], + ); + + // The flat providers catalog still names variables through the legacy `*_env:` field rather + // than a secret reference, and busbar resolves those too, so the same scan has to see them. + assert_eq!( + referenced_env_vars("mock:\n api_key_env: MOCK_KEY\n"), + vec!["MOCK_KEY"], + ); +} + +#[test] +fn deduplicates_and_ignores_references_that_name_nothing() { + assert_eq!( + referenced_env_vars("a: { env: SAME }\nb: { env: SAME }\n"), + vec!["SAME"], + "a variable referenced twice must be exported once, not twice" + ); + + // A bare `env:` opening a nested mapping, or ending a line, names no variable; exporting an + // empty-named one would be nonsense and on some platforms an outright error. + assert!(referenced_env_vars("a: { env:\n b: c }\n").is_empty()); + assert!(referenced_env_vars("listen: \"127.0.0.1:0\"\nauth:\n chain: []\n").is_empty()); +} diff --git a/store-postgres-plugin/tests/e2e.rs b/store-postgres-plugin/tests/e2e.rs index 6c75983..de79ce8 100644 --- a/store-postgres-plugin/tests/e2e.rs +++ b/store-postgres-plugin/tests/e2e.rs @@ -38,6 +38,8 @@ //! install work," and converting them to a full process-boot-and-capture-stderr harness for each //! error shape is a much larger, lower-value lift than the persistence test's conversion. +mod common; + use busbar_store_postgres::PostgresStore; use std::path::PathBuf; use std::process::{Child, Command, Stdio}; @@ -74,9 +76,8 @@ fn postgres_url() -> Option { /// Checks BOTH the "uplifted" `/` copy (only refreshed when `[lib]` is a ROOT /// build target of the invocation, e.g. `cargo build --all-targets`) and the raw /// `/deps/` compiler output (refreshed on every build that recompiles the lib, -/// uplifted or not). A bare `cargo test --release` (what `release-check.sh`'s Phase 2 runs, and what -/// cargo-mutants runs) does NOT uplift the cdylib to the top-level profile dir, only to -/// `target/deps` — checking only `profile_dir` silently finds nothing. Same fix already applied to +/// uplifted or not). A bare `cargo test --release` (what `release-check.sh`'s Phase 2 runs) does +/// NOT uplift the cdylib to the top-level profile dir, only to `target/deps` — checking only `profile_dir` silently finds nothing. Same fix already applied to /// this crate's sibling `admin_api_e2e.rs` and to auth-oidc-plugin's/webrequest-hook's equivalent /// helpers. fn plugin_path() -> Option { @@ -245,7 +246,9 @@ fn load_and_exercise_postgres_plugin_via_file_drop() { // file-dropped plugin passes the trust/manifest gate; then a REAL BOOT (no `--validate` flag, // below) is the only thing that actually `dlopen`s the plugin and runs // `Store::connect`/migration. - let out = Command::new(&busbar_bin) + let mut validate = Command::new(&busbar_bin); + common::apply_placeholder_secrets_from_files(&mut validate, &[&config, &providers]); + let out = validate .arg("--validate") .env("BUSBAR_CONFIG", &config) .env("BUSBAR_PROVIDERS", &providers) @@ -263,7 +266,9 @@ fn load_and_exercise_postgres_plugin_via_file_drop() { // PostgresStore::connect, so this check can't accidentally create the schema itself -- for the // `keys` table to appear. This is the only genuine proof that boot actually dlopened the plugin // and called Store::connect (which runs migrate()) before ever handling a request. - let child = Command::new(&busbar_bin) + let mut boot = Command::new(&busbar_bin); + common::apply_placeholder_secrets_from_files(&mut boot, &[&config, &providers]); + let child = boot .env("BUSBAR_CONFIG", &config) .env("BUSBAR_PROVIDERS", &providers) .env("BUSBAR_STATE_FILE", "") // disable the state-snapshot file; not under test here diff --git a/store-postgres/src/lib.rs b/store-postgres/src/lib.rs index 0584f4f..1b80267 100644 --- a/store-postgres/src/lib.rs +++ b/store-postgres/src/lib.rs @@ -61,7 +61,7 @@ fn is_undefined_table(e: &postgres::Error) -> bool { e.code() == Some(&postgres::error::SqlState::UNDEFINED_TABLE) } -/// Extract the PASSWORD from a Postgres DSN (L2). Supports both the URL form +/// Extract the PASSWORD from a Postgres DSN. Supports both the URL form /// (`postgres://user:pass@host:5432/db`) and the libpq keyword form (`... password=secret ...`), so /// a connect-error string can be scrubbed of the secret regardless of which shape the operator used. fn dsn_password(dsn: &str) -> Option { diff --git a/store-postgres/src/tests.rs b/store-postgres/src/tests.rs index 4114ba2..36deec2 100644 --- a/store-postgres/src/tests.rs +++ b/store-postgres/src/tests.rs @@ -115,8 +115,8 @@ fn connect_store_with_retry(url: &str) -> StoreResult { /// TRUE reset for test isolation -- unlike `delete_key` (a deliberate tombstone that can never fully /// reset a row by design), this raw-SQL wipe gives each test a genuinely clean slate for its id, so -/// re-running the suite (or running it twice, as red-before-green proofs do) never sees stale state -/// from a prior run leaking into the CHECK constraints (e.g. re-minting into a row still marked +/// re-running the suite (including running it twice in a row) never sees stale state from a prior +/// run leaking into the CHECK constraints (e.g. re-minting into a row still marked /// `deleted_at` from a previous run's tombstone would violate `keys_tombstone_disabled`). fn hard_reset(store: &PostgresStore, id: &str) { let mut client = store.lock(); @@ -214,10 +214,9 @@ fn delete_key_is_a_tombstone_not_a_hard_delete() { store.delete_key(id).unwrap(); - // RED-BEFORE-GREEN evidence lives in the assertions below: this test only passes if delete_key - // genuinely tombstones rather than hard-deletes. Confirmed by temporarily reverting delete_key - // to `DELETE FROM keys WHERE id=$1` during development: get_key(id) then returned None and this - // test failed at the very first assertion, exactly as expected. + // The assertions below are non-vacuous: they pass only if delete_key genuinely tombstones + // rather than hard-deletes. Were delete_key a plain `DELETE FROM keys WHERE id=$1`, get_key(id) + // would return None and the very first assertion below would fail. let after = store.get_key(id).unwrap(); assert!( after.is_some(), @@ -248,11 +247,10 @@ fn delete_key_is_a_tombstone_not_a_hard_delete() { /// that was previously deleted (deleted_at set, enabled=false by delete_key) must produce a fully /// LIVE key, not one that is enabled=true while still marked deleted_at. /// -/// RED-BEFORE-GREEN: reverting the ON CONFLICT UPDATE SET clause to omit `deleted_at=NULL` (its -/// state before this fix) makes this test fail at the `deleted_at` assertion below: `enabled` flips -/// to `true` as the caller intended, but `deleted_at` is left at whatever the tombstone set it to, -/// so the row is simultaneously "enabled" and "deleted" -- exactly the corrupt state this test -/// guards against. Confirmed by temporarily reverting the fix and re-running: this assertion failed. +/// The `deleted_at` assertion below is the load-bearing one: an ON CONFLICT UPDATE SET clause that +/// omits `deleted_at=NULL` flips `enabled` to `true` as the caller intended but leaves `deleted_at` +/// at whatever the tombstone set it to, so the row is simultaneously "enabled" and "deleted" -- +/// exactly the corrupt state this test rejects. #[test] fn put_key_with_credential_on_conflict_clears_a_stale_tombstone() { let Some(url) = live_url() else { return }; @@ -366,11 +364,10 @@ fn credential_slot_guard_rejects_clobbering_a_live_credential() { /// put_credential_tx must bind CredentialMeta::updated_at to its own column, not silently reuse /// created_at's parameter for both. /// -/// RED-BEFORE-GREEN: reverting the fix (VALUES ...,$8,$8,$9,... binding created_at's placeholder -/// twice, with `updated_at` never bound at all) makes this test fail: the round-tripped -/// `updated_at` comes back equal to `created_at` (100) instead of the distinct value (200) this -/// test mints with. Confirmed by temporarily reverting and re-running: assertion failed with -/// `left: 100, right: 200`. +/// The test is non-vacuous: were the INSERT to bind created_at's placeholder twice +/// (VALUES ...,$8,$8,$9,...) and never bind `updated_at` at all, the round-tripped `updated_at` +/// would come back equal to `created_at` (100) instead of the distinct value (200) this test mints +/// with, and the assertion below would fail with `left: 100, right: 200`. #[test] fn put_credential_binds_updated_at_to_its_own_column_not_created_at() { let Some(url) = live_url() else { return }; @@ -574,11 +571,10 @@ fn get_usage_transaction_is_actually_repeatable_read() { /// between the two steps on a SEPARATE connection, then asserts REPEATABLE READ's snapshot held: the /// second read still sees the pre-interleave state, not the concurrent writer's new model row. /// -/// RED-BEFORE-GREEN: this test is a genuine regression guard rather than a fresh finding (get_usage -/// already opens REPEATABLE READ via snapshot_consistent_tx) -- confirmed non-vacuous by temporarily -/// downgrading snapshot_consistent_tx's isolation level to READ COMMITTED and re-running: the -/// `model_count` assertion below failed (it observed the interleaved writer's new model row), then -/// passed again after restoring REPEATABLE READ. +/// This is a regression guard on an isolation level get_usage already opens (REPEATABLE READ, via +/// snapshot_consistent_tx). It is non-vacuous: at READ COMMITTED the `model_count` assertion below +/// observes the interleaved writer's new model row and fails; only REPEATABLE READ holds the +/// snapshot across both reads. #[test] fn get_usage_snapshot_does_not_observe_a_concurrent_add_usage_between_its_two_reads() { let Some(url) = live_url() else { return }; @@ -719,9 +715,9 @@ fn metering_roundtrip_new_fields() { } // --------------------------------------------------------------------------------------------- -// Mutation-testing gap fixes (cargo-mutants round 1 against this file): each test below is named -// for, and directly targets, one or more MISSED mutants -- confirmed red against the mutant before -// being folded in green here. +// Boundary and arithmetic guards for this file: each test below is named for, and directly +// targets, one specific predicate or binding whose behaviour was not otherwise pinned. Each fails +// if that predicate is perturbed in the way its name describes. // --------------------------------------------------------------------------------------------- /// `percent_decode`'s length guard and hi/lo-nibble arithmetic, pinned with cases the existing @@ -1265,10 +1261,9 @@ fn migrate_v6_does_not_rerun_the_backfill_on_an_already_migrated_database() { /// unchanged in both cases -- `migrate_locked` unconditionally runs `CREATE TABLE IF NOT EXISTS /// busbar_schema` as its very first statement, so by the time the guarded `SELECT` runs, the table /// either already exists (guard never fires) or the preceding `CREATE TABLE` itself already -/// propagated the error several lines earlier (guard never reached). This is a confirmed equivalent -/// mutant / dead branch given the current code structure, not a test-coverage gap; left unfixed -/// per policy (no test written to "kill" it, since none can, without changing the source itself -- -/// out of scope for a mutation-testing coverage pass). +/// propagated the error several lines earlier (guard never reached). Given the current statement +/// order the guarded branch is therefore unreachable, so no test can drive it without changing the +/// source itself; the test below pins the reachable half of the behaviour instead. #[test] fn migrate_propagates_a_non_undefined_table_error_and_never_silently_succeeds() { let Some(url) = live_url() else { return };