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 <william@yossarian.net>

* tests: add repro for #518

This ended up being somewhat involved, since
the tests expect to pass `--offline` unconditionally.

Signed-off-by: William Woodruff <william@yossarian.net>

* add test features

Signed-off-by: William Woodruff <william@yossarian.net>

* run online tests

Signed-off-by: William Woodruff <william@yossarian.net>

* release-notes: record changes

Signed-off-by: William Woodruff <william@yossarian.net>

* docs: explain online tests

Signed-off-by: William Woodruff <william@yossarian.net>

---------

Signed-off-by: William Woodruff <william@yossarian.net>

.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: |

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"

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.

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
 

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<bool> {
         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
         );
 

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<ExitCode> {
         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()

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<String> {
         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(())
+}

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)

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

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:

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

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