From 871fa31bcc0ed30c183c312cc015a2725858b540 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 5 Feb 2025 22:39:44 +0100 Subject: [PATCH] bugfix: fix has_tag lookup (#519) * bugfix: fix has_tag lookup This caused false positives in the `ref-confusion` audit by using the wrong endpoint. The correct endpoint only returns the exact matching tag or 404, rather than a list of one or more partial matches. Signed-off-by: William Woodruff * tests: add repro for #518 This ended up being somewhat involved, since the tests expect to pass `--offline` unconditionally. Signed-off-by: William Woodruff * add test features Signed-off-by: William Woodruff * run online tests Signed-off-by: William Woodruff * release-notes: record changes Signed-off-by: William Woodruff * docs: explain online tests Signed-off-by: William Woodruff --------- Signed-off-by: William Woodruff --- .github/workflows/ci.yml | 4 +- Cargo.toml | 6 +++ docs/development.md | 15 ++++++ docs/release-notes.md | 9 ++++ src/github_api.rs | 2 +- src/main.rs | 11 ++++- tests/snapshot.rs | 47 +++++++++---------- ...napshot__conflicting_online_options-2.snap | 10 ---- ...napshot__conflicting_online_options-3.snap | 10 ---- .../snapshot__conflicting_online_options.snap | 10 ---- .../snapshots/snapshot__ref_confusion-2.snap | 5 ++ tests/snapshots/snapshot__ref_confusion.snap | 13 +++++ .../test-data/artipacked/issue-447-repro.yml | 2 +- tests/test-data/ref-confusion.yml | 11 +++++ .../ref-confusion/issue-518-repro.yml | 12 +++++ 15 files changed, 107 insertions(+), 60 deletions(-) delete mode 100644 tests/snapshots/snapshot__conflicting_online_options-2.snap delete mode 100644 tests/snapshots/snapshot__conflicting_online_options-3.snap delete mode 100644 tests/snapshots/snapshot__conflicting_online_options.snap create mode 100644 tests/snapshots/snapshot__ref_confusion-2.snap create mode 100644 tests/snapshots/snapshot__ref_confusion.snap create mode 100644 tests/test-data/ref-confusion.yml create mode 100644 tests/test-data/ref-confusion/issue-518-repro.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 511cb9a5..dc355adc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,7 +36,9 @@ jobs: - uses: astral-sh/setup-uv@4db96194c378173c656ce18a155ffc14a9fc4355 # v5.2.2 - name: Test - run: cargo test + run: cargo test --features online-tests + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - name: Test snippets run: | diff --git a/Cargo.toml b/Cargo.toml index ee22f85b..2057f35b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,6 +12,12 @@ keywords = ["cli", "github-actions", "static-analysis", "security"] categories = ["command-line-utilities"] rust-version = "1.80.1" +[features] +# Test-only: enable online audits that make use of a GitHub token via GH_TOKEN. +gh-token-tests = [] +# Test-only: enable all online audits. +online-tests = ["gh-token-tests"] + [dependencies] annotate-snippets = "0.11.5" anstream = "0.6.18" diff --git a/docs/development.md b/docs/development.md index fa647216..47b8e105 100644 --- a/docs/development.md +++ b/docs/development.md @@ -90,6 +90,21 @@ cargo test --test snapshot cargo test ``` +### Online tests + +`zizmor` has some online tests that are ignored by default. These +tests are gated behind crate features: + +- `gh-token-tests`: Enable online tests that use the GitHub API. +- `online-tests`: Enable all online tests, including `gh-token-tests`. + +To run these successfully, you'll need to set the `GH_TOKEN` environment +variable and pass the `--features` flag to `cargo test`: + +```bash +GH_TOKEN=$(gh auth token) cargo test --features online-tests +``` + ### Writing snapshot tests `zizmor` uses @mitsuhiko/insta for snapshot testing. diff --git a/docs/release-notes.md b/docs/release-notes.md index bb252617..273cb818 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -9,12 +9,21 @@ of `zizmor`. ## Next (UNRELEASED) +### Improvements 🌱 + +* Passing both `--offline` and a GitHub token (either implicitly with + `GH_TOKEN` or explicitly with `--gh-token`) no longer results in an + error. `--offline` is now given precedence, regardless of + any other flags or environment settings (#519) + ### Bug Fixes 🐛 * Fixed a bug where `zizmor` would fail to parse composite actions with inputs/outputs that are missing descriptions (#502) * Expressions that contain indices with non-semantic whitespace are now parsed correctly (#511) +* Fixed a false positive in [ref-confusion] where partial tag matches were + incorrectly considered confusable (#519) ## v1.3.0 diff --git a/src/github_api.rs b/src/github_api.rs index 3db12dac..1a6526db 100644 --- a/src/github_api.rs +++ b/src/github_api.rs @@ -182,7 +182,7 @@ impl Client { #[tokio::main] pub(crate) async fn has_tag(&self, owner: &str, repo: &str, tag: &str) -> Result { let url = format!( - "{api_base}/repos/{owner}/{repo}/git/refs/tags/{tag}", + "{api_base}/repos/{owner}/{repo}/git/ref/tags/{tag}", api_base = self.api_base ); diff --git a/src/main.rs b/src/main.rs index 7acf8c2c..181f2b2e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -50,8 +50,7 @@ struct App { /// /// This disables all online audit rules, and prevents zizmor from /// auditing remote repositories. - #[arg(short, long, env = "ZIZMOR_OFFLINE", - conflicts_with_all = ["gh_token", "gh_hostname"])] + #[arg(short, long, env = "ZIZMOR_OFFLINE")] offline: bool, /// The GitHub API token to use. @@ -330,6 +329,14 @@ fn run() -> Result { app.persona = Persona::Pedantic; } + // Unset the GitHub token if we're in offline mode. + // We do this manually instead of with clap's `conflicts_with` because + // we want to support explicitly enabling offline mode while still + // having `GH_TOKEN` present in the environment. + if app.offline { + app.gh_token = None; + } + let indicatif_layer = IndicatifLayer::new(); let filter = EnvFilter::builder() diff --git a/tests/snapshot.rs b/tests/snapshot.rs index 37f9d43a..ffd61a87 100644 --- a/tests/snapshot.rs +++ b/tests/snapshot.rs @@ -1,4 +1,4 @@ -use anyhow::Result; +use anyhow::{Context, Result}; use assert_cmd::Command; use common::workflow_under_test; @@ -36,6 +36,7 @@ impl Zizmor { self } + #[allow(dead_code)] fn setenv(mut self, key: &str, value: &str) -> Self { self.cmd.env(key, value); self @@ -65,6 +66,10 @@ impl Zizmor { fn run(mut self) -> Result { if self.offline { self.cmd.arg("--offline"); + } else { + // If we're running in online mode, we pre-assert the + // presence of GH_TOKEN to make configuration failures more obvious. + std::env::var("GH_TOKEN").context("online tests require GH_TOKEN to be set")?; } if let Some(workflow) = &self.workflow { @@ -103,30 +108,6 @@ fn test_cant_retrieve() -> Result<()> { Ok(()) } -#[test] -fn test_conflicting_online_options() -> Result<()> { - insta::assert_snapshot!(zizmor() - .output(OutputMode::Stderr) - .setenv("GH_TOKEN", "phony") - .offline(true) - .run()?); - - insta::assert_snapshot!(zizmor() - .output(OutputMode::Stderr) - .offline(true) - .args(["--gh-token=phony"]) - .run()?); - - insta::assert_snapshot!(zizmor() - .output(OutputMode::Stderr) - .setenv("ZIZMOR_OFFLINE", "true") - .setenv("GH_TOKEN", "phony") - .offline(false) // explicitly disable so that we test ZIZMOR_OFFLINE above - .run()?); - - Ok(()) -} - #[test] fn test_invalid_inputs() -> Result<()> { insta::assert_snapshot!(zizmor() @@ -512,3 +493,19 @@ fn overprovisioned_secrets() -> Result<()> { Ok(()) } + +#[cfg_attr(not(feature = "gh-token-tests"), ignore)] +#[test] +fn ref_confusion() -> Result<()> { + insta::assert_snapshot!(zizmor() + .workflow(workflow_under_test("ref-confusion.yml")) + .offline(false) + .run()?); + + insta::assert_snapshot!(zizmor() + .workflow(workflow_under_test("ref-confusion/issue-518-repro.yml")) + .offline(false) + .run()?); + + Ok(()) +} diff --git a/tests/snapshots/snapshot__conflicting_online_options-2.snap b/tests/snapshots/snapshot__conflicting_online_options-2.snap deleted file mode 100644 index cee9aa21..00000000 --- a/tests/snapshots/snapshot__conflicting_online_options-2.snap +++ /dev/null @@ -1,10 +0,0 @@ ---- -source: tests/snapshot.rs -expression: "zizmor().output(OutputMode::Stderr).offline(true).args([\"--gh-token=phony\"]).run()?" -snapshot_kind: text ---- -error: the argument '--gh-token ' cannot be used with '--offline' - -Usage: zizmor --gh-token ... - -For more information, try '--help'. diff --git a/tests/snapshots/snapshot__conflicting_online_options-3.snap b/tests/snapshots/snapshot__conflicting_online_options-3.snap deleted file mode 100644 index 4680eec7..00000000 --- a/tests/snapshots/snapshot__conflicting_online_options-3.snap +++ /dev/null @@ -1,10 +0,0 @@ ---- -source: tests/snapshot.rs -expression: "zizmor().output(OutputMode::Stderr).setenv(\"ZIZMOR_OFFLINE\",\n\"true\").setenv(\"GH_TOKEN\", \"phony\").run()?" -snapshot_kind: text ---- -error: the argument '--offline' cannot be used with '--gh-token ' - -Usage: zizmor --offline ... - -For more information, try '--help'. diff --git a/tests/snapshots/snapshot__conflicting_online_options.snap b/tests/snapshots/snapshot__conflicting_online_options.snap deleted file mode 100644 index a498dfff..00000000 --- a/tests/snapshots/snapshot__conflicting_online_options.snap +++ /dev/null @@ -1,10 +0,0 @@ ---- -source: tests/snapshot.rs -expression: "zizmor().output(OutputMode::Stderr).setenv(\"GH_TOKEN\",\n\"phony\").offline(true).run()?" -snapshot_kind: text ---- -error: the argument '--offline' cannot be used with '--gh-token ' - -Usage: zizmor --offline ... - -For more information, try '--help'. diff --git a/tests/snapshots/snapshot__ref_confusion-2.snap b/tests/snapshots/snapshot__ref_confusion-2.snap new file mode 100644 index 00000000..28715207 --- /dev/null +++ b/tests/snapshots/snapshot__ref_confusion-2.snap @@ -0,0 +1,5 @@ +--- +source: tests/snapshot.rs +expression: "zizmor().workflow(workflow_under_test(\"ref-confusion/issue-518-repro.yml\")).offline(false).run()?" +--- +No findings to report. Good job! (1 suppressed) diff --git a/tests/snapshots/snapshot__ref_confusion.snap b/tests/snapshots/snapshot__ref_confusion.snap new file mode 100644 index 00000000..d5b98eb4 --- /dev/null +++ b/tests/snapshots/snapshot__ref_confusion.snap @@ -0,0 +1,13 @@ +--- +source: tests/snapshot.rs +expression: "zizmor().workflow(workflow_under_test(\"ref-confusion.yml\")).offline(false).run()?" +--- +warning[ref-confusion]: git ref for action with ambiguous ref type + --> @@INPUT@@:11:9 + | +11 | - uses: woodruffw/gha-hazmat/ref-confusion@confusable + | --------------------------------------------------- uses a ref that's provided by both the branch and tag namespaces + | + = note: audit confidence → High + +2 findings (1 suppressed): 0 unknown, 0 informational, 0 low, 1 medium, 0 high diff --git a/tests/test-data/artipacked/issue-447-repro.yml b/tests/test-data/artipacked/issue-447-repro.yml index 028793d1..be282a80 100644 --- a/tests/test-data/artipacked/issue-447-repro.yml +++ b/tests/test-data/artipacked/issue-447-repro.yml @@ -6,7 +6,7 @@ on: push permissions: {} jobs: - issue-557-repro: + issue-447-repro: runs-on: ubuntu-latest steps: diff --git a/tests/test-data/ref-confusion.yml b/tests/test-data/ref-confusion.yml new file mode 100644 index 00000000..e0227b93 --- /dev/null +++ b/tests/test-data/ref-confusion.yml @@ -0,0 +1,11 @@ +name: example +on: [push] + +permissions: {} + +jobs: + commit: + runs-on: ubuntu-latest + steps: + # NOT OK: `confusable` is both a tag and a branch + - uses: woodruffw/gha-hazmat/ref-confusion@confusable diff --git a/tests/test-data/ref-confusion/issue-518-repro.yml b/tests/test-data/ref-confusion/issue-518-repro.yml new file mode 100644 index 00000000..eeca30a6 --- /dev/null +++ b/tests/test-data/ref-confusion/issue-518-repro.yml @@ -0,0 +1,12 @@ +name: ISSUE-518-REPRO +on: [push] + +permissions: {} + +jobs: + issue-518-repro: + runs-on: ubuntu-latest + + steps: + - name: Install Task + uses: arduino/setup-task@v2