From 5aa014db59413bbcd1e27ad83b98c5346c1c222e Mon Sep 17 00:00:00 2001 From: Glenn Gore Date: Sat, 12 Sep 2026 22:50:43 +0200 Subject: [PATCH 1/2] test(deps): add yaml-rust2 for the CI-workflow structural tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workflow-injection regression test asks a question about the YAML tree — "does any `run:` scalar contain a `${{ }}` expression" — so it needs a parser. Matching the text instead finds the same marker in a comment or an input description and reports it as a finding; the verify-trust action carries exactly such a comment. Added as a workspace dev-dependency and taken by verify-trust, which is the crate the composite action wraps. Signed-off-by: Glenn Gore --- Cargo.lock | 27 +++++++++++++++++++++++++++ Cargo.toml | 5 +++++ crates/verify-trust/Cargo.toml | 2 ++ 3 files changed, 34 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 212805b..a0568ba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -677,6 +677,12 @@ dependencies = [ "zeroize", ] +[[package]] +name = "arraydeque" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d902e3d592a523def97af8f317b08ce16b7ab854c1985a0c671e6f15cebc236" + [[package]] name = "async-lock" version = "3.4.2" @@ -2455,6 +2461,15 @@ version = "0.17.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a" +[[package]] +name = "hashlink" +version = "0.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "824e001ac4f3012dd16a264bec811403a67ca9deb6c102fc5049b32c4574b35f" +dependencies = [ + "hashbrown 0.16.1", +] + [[package]] name = "heck" version = "0.5.0" @@ -5662,6 +5677,7 @@ dependencies = [ "trql-client", "vgi-core", "vta-sdk", + "yaml-rust2", ] [[package]] @@ -6190,6 +6206,17 @@ dependencies = [ "zeroize", ] +[[package]] +name = "yaml-rust2" +version = "0.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b36710ce3a279cfce8465dbab826f161675a262950b922cb2c3663852dfe9eb0" +dependencies = [ + "arraydeque", + "encoding_rs", + "hashlink", +] + [[package]] name = "yoke" version = "0.8.3" diff --git a/Cargo.toml b/Cargo.toml index 0fab7d8..4af2b9a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -210,3 +210,8 @@ tempfile = "3" serial_test = "4" # Property-based testing: used by vgi-core's trailer differential against git. proptest = "1" +# YAML parser for the CI-workflow structural tests. A parser, not a regex: the +# check is "no `${{ }}` inside a `run:` scalar", which is a statement about the +# YAML tree. Matching the text instead finds the same marker in a comment or a +# description and reports it as a finding. +yaml-rust2 = "0.11" diff --git a/crates/verify-trust/Cargo.toml b/crates/verify-trust/Cargo.toml index cd01c9d..01955e3 100644 --- a/crates/verify-trust/Cargo.toml +++ b/crates/verify-trust/Cargo.toml @@ -41,3 +41,5 @@ hex = { workspace = true } ed25519-dalek = { workspace = true } tempfile = { workspace = true } rand = { workspace = true } +# Parses `.github/` for the workflow-injection regression test. +yaml-rust2 = { workspace = true } From f6f6de5bef5f43a2871646c9fd34dc74576616c8 Mon Sep 17 00:00:00 2001 From: Glenn Gore Date: Sat, 12 Sep 2026 22:55:04 +0200 Subject: [PATCH 2/2] test(ci): gate the workflow injection and action pinning structurally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SEC-4045 reproduced two CI findings: the verify-trust composite action pasted `${{ inputs.* }}` into its `run:` script, where an input holding `$(…)` became a command, and the workflows named actions by mutable tag in jobs holding `contents: write` or an OIDC token. Both were fixed by changing the shape of the YAML — values moved into `env:`, refs replaced by commit SHAs — so the gate asserts that shape. `action-selftest.yml` already catches the injection at runtime, by calling the action with shell syntax in its inputs. This is the other half: a structural check over every definition file, including the ones no runtime test exercises, that fails at review time rather than needing a runner. The new test parses each file under `.github/workflows/` and `.github/actions/` and walks the resulting tree: - no `${{ }}` expression appears inside any `run:` scalar; - every `uses:` names a 40-hex commit SHA, local `./…` paths aside. It parses rather than greps because the injection fix left a comment in action.yml explaining that `${{ }}` must not appear in a script body, and several input descriptions mention expressions too. A text search reports all of those. A YAML comment belongs to no scalar, so the tree-walk ignores prose while still seeing a `${{ }}` hidden in a shell comment inside a `run:` block, which GitHub does substitute. Failures name the file, the position in the tree (`jobs.test.steps[4]`) and the step's `name:`. Two further tests keep the gate honest: one asserts the walk actually reached `run:` and `uses:` scalars in both directories, so a rename cannot turn the checks green by leaving them nothing to read; the other runs both checks over an inline fixture that carries the marker four times outside a script and once inside one, and asserts exactly one finding. Signed-off-by: Glenn Gore --- .../tests/ci_workflow_hardening.rs | 366 ++++++++++++++++++ 1 file changed, 366 insertions(+) create mode 100644 crates/verify-trust/tests/ci_workflow_hardening.rs diff --git a/crates/verify-trust/tests/ci_workflow_hardening.rs b/crates/verify-trust/tests/ci_workflow_hardening.rs new file mode 100644 index 0000000..1da5b75 --- /dev/null +++ b/crates/verify-trust/tests/ci_workflow_hardening.rs @@ -0,0 +1,366 @@ +//! Structural regression gate for the two CI findings of SEC-4045: the +//! composite action interpolated `${{ inputs.* }}` straight into its `run:` +//! script, and the workflows referenced actions by mutable tag. +//! +//! Both were fixed by changing the *shape* of the YAML — values moved into +//! `env:`, refs replaced by commit SHAs — so the gate asks questions about the +//! YAML tree rather than about the file's text: +//! +//! 1. no `${{ }}` expression appears inside any `run:` scalar; +//! 2. every `uses:` names a 40-hex commit SHA. +//! +//! It is a parser, not a grep, for a concrete reason: the fix for the injection +//! left a comment in `.github/actions/verify-trust/action.yml` explaining that +//! `${{ }}` must not appear in a script body. A text search reports that +//! comment — and every input description that mentions an expression — as a +//! finding. A YAML comment is not part of any scalar, so anchoring on the tree +//! ignores prose and sees only what GitHub will actually substitute. The +//! reverse also holds: a `${{ }}` inside a *shell* comment in a `run:` block is +//! substituted before bash ever reads it, and the tree-walk catches it. +//! +//! [`the_checks_flag_a_reintroduced_injection_and_ignore_prose`] pins both +//! halves of that down against an inline fixture, so the gate cannot rot into +//! a check that passes because it looks at nothing. + +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use std::fmt::Write as _; +use std::path::{Path, PathBuf}; + +use yaml_rust2::{Yaml, YamlLoader}; + +/// What the audit found, over every definition file it read. +#[derive(Default)] +struct Audit { + /// Files parsed, by repo-relative path. + files: Vec, + /// `run:` scalars examined. Zero means the walk stopped working. + run_scalars: usize, + /// `uses:` scalars examined. Zero means the walk stopped working. + uses_scalars: usize, + /// One entry per `${{ }}` found inside a `run:` scalar. + injections: Vec, + /// One entry per `uses:` that is not pinned to a commit SHA. + unpinned: Vec, +} + +impl Audit { + fn report(findings: &[String]) -> String { + let mut out = String::new(); + for finding in findings { + let _ = write!(out, "\n - {finding}"); + } + out + } +} + +// --- locating the definitions ------------------------------------------------ + +/// The repository root: the first ancestor of this crate holding `.github`. +fn repo_root() -> PathBuf { + let mut dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + loop { + if dir.join(".github").is_dir() { + return dir; + } + assert!( + dir.pop(), + "no ancestor of {} contains a .github directory", + env!("CARGO_MANIFEST_DIR") + ); + } +} + +/// Every YAML file under `dir`, recursively, sorted for a stable report. +fn yaml_files_under(dir: &Path) -> Vec { + let mut found = Vec::new(); + if !dir.is_dir() { + return found; + } + let mut entries: Vec = std::fs::read_dir(dir) + .unwrap_or_else(|e| panic!("read {}: {e}", dir.display())) + .map(|e| e.expect("directory entry").path()) + .collect(); + entries.sort(); + for path in entries { + if path.is_dir() { + found.extend(yaml_files_under(&path)); + } else if matches!( + path.extension().and_then(|e| e.to_str()), + Some("yml" | "yaml") + ) { + found.push(path); + } + } + found +} + +/// Read and audit every workflow and every composite action in the repository. +/// +/// `.github/actions/` is covered as well as `.github/workflows/`: the injection +/// was in a composite action, which is where a `run:` script is most exposed +/// because its inputs come from whoever calls it. +fn audit_repository() -> Audit { + let root = repo_root(); + let mut audit = Audit::default(); + let mut paths = yaml_files_under(&root.join(".github/workflows")); + paths.extend(yaml_files_under(&root.join(".github/actions"))); + + for path in paths { + let label = path + .strip_prefix(&root) + .unwrap_or(&path) + .display() + .to_string(); + let text = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("read {}: {e}", path.display())); + audit_document(&label, &text, &mut audit); + } + audit +} + +/// Parse one definition file and walk its tree into `audit`. +fn audit_document(label: &str, text: &str, audit: &mut Audit) { + let docs = YamlLoader::load_from_str(text) + .unwrap_or_else(|e| panic!("{label} is not parseable YAML: {e}")); + audit.files.push(label.to_string()); + for doc in &docs { + walk(doc, label, "", audit); + } +} + +// --- the tree walk ----------------------------------------------------------- + +/// Visit `node`, checking every `run:` and `uses:` scalar beneath it. +/// +/// `trail` is the position in the tree, rendered the way a reader would address +/// it (`jobs.test.steps[5]`), so a failure names the file, the job and the step +/// without the walk needing to know what a job or a step is. That also means +/// the walk covers shapes the workflow schema grows later — reusable-workflow +/// `jobs..uses`, composite-action steps, anything nested — rather than only +/// the two it was written against. +fn walk(node: &Yaml, file: &str, trail: &str, audit: &mut Audit) { + match node { + Yaml::Hash(hash) => { + // A step's `name:` is how a person refers to it in the run log, so + // it goes in the message alongside the structural position. + let step_name = hash + .get(&Yaml::String("name".to_string())) + .and_then(Yaml::as_str) + .map(|n| format!(" (\"{n}\")")) + .unwrap_or_default(); + + for (key, value) in hash { + let Some(key) = key.as_str() else { + continue; + }; + let here = if trail.is_empty() { + key.to_string() + } else { + format!("{trail}.{key}") + }; + if let Some(scalar) = value.as_str() { + let at = format!("{file}: {here}{step_name}"); + match key { + "run" => { + audit.run_scalars += 1; + check_run(scalar, &at, audit); + } + "uses" => { + audit.uses_scalars += 1; + check_uses(scalar, &at, audit); + } + _ => {} + } + } + walk(value, file, &here, audit); + } + } + Yaml::Array(items) => { + for (index, item) in items.iter().enumerate() { + walk(item, file, &format!("{trail}[{index}]"), audit); + } + } + _ => {} + } +} + +/// A `run:` script must contain no `${{ }}`. +/// +/// GitHub substitutes an expression into the script as text before any shell +/// parses it, so an input holding `$(…)` or a backtick becomes a command. The +/// fix is to bind the value in `env:` and reference it as a quoted variable, +/// which keeps it data whatever it contains. +fn check_run(script: &str, at: &str, audit: &mut Audit) { + let mut rest = script; + while let Some(start) = rest.find("${{") { + let expression = match rest[start..].find("}}") { + Some(end) => &rest[start..start + end + 2], + // Unterminated: report what there is rather than dropping it. + None => &rest[start..], + }; + audit.injections.push(format!( + "{at}: `run:` interpolates {expression} — bind it in `env:` and \ + reference it as a quoted shell variable instead" + )); + rest = &rest[start + 3..]; + } +} + +/// A `uses:` must name a 40-hex commit SHA. +/// +/// A tag or branch is whatever the action's owner points it at today, so a +/// workflow holding `contents: write` or an OIDC token would run code chosen +/// after review. Only a local path is exempt: `./…` resolves inside this +/// repository, at the commit being tested. Anything else — including a +/// `docker://` image without a digest — has to fail and be looked at. +fn check_uses(reference: &str, at: &str, audit: &mut Audit) { + if reference.starts_with("./") || reference.starts_with("../") { + return; + } + let pinned = reference.rsplit_once('@').is_some_and(|(_, git_ref)| { + git_ref.len() == 40 && git_ref.bytes().all(|b| b.is_ascii_hexdigit()) + }); + if !pinned { + audit.unpinned.push(format!( + "{at}: `uses: {reference}` is not pinned — give it the 40-hex commit \ + SHA of the release, with the version in a trailing comment" + )); + } +} + +// --- the gates --------------------------------------------------------------- + +#[test] +fn no_expression_is_interpolated_into_a_run_script() { + let audit = audit_repository(); + assert!( + audit.injections.is_empty(), + "{} `run:` script(s) interpolate a GitHub expression:{}", + audit.injections.len(), + Audit::report(&audit.injections) + ); +} + +#[test] +fn every_action_reference_is_pinned_to_a_commit_sha() { + let audit = audit_repository(); + assert!( + audit.unpinned.is_empty(), + "{} action reference(s) are not pinned to a commit SHA:{}", + audit.unpinned.len(), + Audit::report(&audit.unpinned) + ); +} + +/// Both gates above pass when they find nothing, so this asserts they found +/// something: the definition files exist, both directories contribute, and the +/// walk reached real `run:` and `uses:` scalars. Without it, renaming +/// `.github/workflows` or breaking the walk would turn the gates green. +#[test] +fn the_audit_reads_every_workflow_and_action_definition() { + let audit = audit_repository(); + + let workflows = audit + .files + .iter() + .filter(|f| f.starts_with(".github/workflows/")) + .count(); + let actions = audit + .files + .iter() + .filter(|f| f.starts_with(".github/actions/")) + .count(); + + assert!( + workflows > 0, + "no workflow was read; files seen: {:?}", + audit.files + ); + assert!( + actions > 0, + "no composite action was read; files seen: {:?}", + audit.files + ); + assert!( + audit.run_scalars > 0, + "the walk found no `run:` scalar in {} file(s), so the injection check \ + examined nothing", + audit.files.len() + ); + assert!( + audit.uses_scalars > 0, + "the walk found no `uses:` scalar in {} file(s), so the pinning check \ + examined nothing", + audit.files.len() + ); +} + +/// The gate must fire on a reintroduced injection and stay quiet on prose about +/// one. An earlier string-matching check in this programme reported the +/// explanatory comment below as a finding; the marker appears four times here +/// outside a script, and once inside one. +#[test] +fn the_checks_flag_a_reintroduced_injection_and_ignore_prose() { + let fixture = r#" +name: fixture +inputs: + range: + # Never paste ${{ inputs.range }} into a script body. + description: "A range. Do not interpolate ${{ inputs.range }} into run:." + default: ${{ github.repository }} +runs: + using: composite + steps: + - name: Safe + env: + VT_RANGE: ${{ inputs.range }} + run: | + # A shell comment, and a shell variable, are both fine. + verify-trust --range "$VT_RANGE" + - name: Unsafe + run: verify-trust --range "${{ inputs.range }}" + - name: Pinned + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 + - name: Local + uses: ./.github/actions/verify-trust + - name: Floating + uses: actions/checkout@v7 +"#; + + let mut audit = Audit::default(); + audit_document("fixture.yml", fixture, &mut audit); + + assert_eq!( + audit.injections.len(), + 1, + "exactly the one script-body expression must be reported:{}", + Audit::report(&audit.injections) + ); + let injection = &audit.injections[0]; + assert!( + injection.contains("\"Unsafe\""), + "the finding must name the offending step: {injection}" + ); + assert!( + injection.contains("${{ inputs.range }}"), + "the finding must quote the expression: {injection}" + ); + + assert_eq!( + audit.unpinned.len(), + 1, + "exactly the floating tag must be reported:{}", + Audit::report(&audit.unpinned) + ); + assert!( + audit.unpinned[0].contains("\"Floating\""), + "the finding must name the offending step: {}", + audit.unpinned[0] + ); + + // Two `run:` and three `uses:` were examined, so neither check passed by + // looking at nothing. + assert_eq!(audit.run_scalars, 2); + assert_eq!(audit.uses_scalars, 3); +}