bugfix: sanitize gh_token & avoid panic (#1027)

acidghost · woodruffw · web-flow · commit b87e2d386261 · 2025-07-15T22:19:25.000Z
Co-authored-by: William Woodruff &lt;william@yossarian.net&gt;
diff --git a/crates/zizmor/src/audit/artipacked.rs b/crates/zizmor/src/audit/artipacked.rs
@@ -208,8 +208,7 @@ mod tests {
             let audit_state = AuditState {
                 config: &Default::default(),
                 no_online_audits: false,
-                cache_dir: "/tmp/zizmor".into(),
-                gh_token: None,
+                gh_client: None,
                 gh_hostname: GitHubHost::Standard("github.com".into()),
             };
             let audit = <$audit_type>::new(&audit_state).unwrap();
diff --git a/crates/zizmor/src/audit/bot_conditions.rs b/crates/zizmor/src/audit/bot_conditions.rs
@@ -415,8 +415,7 @@ mod tests {
             let audit_state = AuditState {
                 config: &Default::default(),
                 no_online_audits: false,
-                cache_dir: "/tmp/zizmor".into(),
-                gh_token: None,
+                gh_client: None,
                 gh_hostname: GitHubHost::Standard("github.com".into()),
             };
             let audit = <$audit_type>::new(&audit_state).unwrap();
diff --git a/crates/zizmor/src/audit/cache_poisoning.rs b/crates/zizmor/src/audit/cache_poisoning.rs
@@ -467,8 +467,7 @@ mod tests {
             let audit_state = AuditState {
                 config: &Default::default(),
                 no_online_audits: false,
-                cache_dir: "/tmp/zizmor".into(),
-                gh_token: None,
+                gh_client: None,
                 gh_hostname: GitHubHost::Standard("github.com".into()),
             };
             let audit = <$audit_type>::new(&audit_state).unwrap();
diff --git a/crates/zizmor/src/audit/github_env.rs b/crates/zizmor/src/audit/github_env.rs
@@ -523,8 +523,7 @@ mod tests {
             let audit_state = AuditState {
                 config: &Default::default(),
                 no_online_audits: false,
-                cache_dir: "/tmp/zizmor".into(),
-                gh_token: None,
+                gh_client: None,
                 gh_hostname: GitHubHost::Standard("github.com".into()),
             };
 
@@ -640,8 +639,7 @@ mod tests {
             let audit_state = AuditState {
                 config: &Default::default(),
                 no_online_audits: false,
-                cache_dir: "/tmp/zizmor".into(),
-                gh_token: None,
+                gh_client: None,
                 gh_hostname: GitHubHost::Standard("github.com".into()),
             };
 
diff --git a/crates/zizmor/src/audit/impostor_commit.rs b/crates/zizmor/src/audit/impostor_commit.rs
@@ -120,13 +120,11 @@ impl Audit for ImpostorCommit {
             )));
         }
 
-        let Some(client) = state.github_client() else {
-            return Err(AuditLoadError::Skip(anyhow!(
-                "can't run without a GitHub API token"
-            )));
-        };
-
-        Ok(ImpostorCommit { client })
+        state
+            .gh_client
+            .clone()
+            .ok_or_else(|| AuditLoadError::Skip(anyhow!("can't run without a GitHub API token")))
+            .map(|client| ImpostorCommit { client })
     }
 
     fn audit_workflow<'doc>(&self, workflow: &'doc Workflow) -> Result<Vec<Finding<'doc>>> {
diff --git a/crates/zizmor/src/audit/known_vulnerable_actions.rs b/crates/zizmor/src/audit/known_vulnerable_actions.rs
@@ -154,13 +154,11 @@ impl Audit for KnownVulnerableActions {
             )));
         }
 
-        let Some(client) = state.github_client() else {
-            return Err(AuditLoadError::Skip(anyhow!(
-                "can't run without a GitHub API token"
-            )));
-        };
-
-        Ok(Self { client })
+        state
+            .gh_client
+            .clone()
+            .ok_or_else(|| AuditLoadError::Skip(anyhow!("can't run without a GitHub API token")))
+            .map(|client| KnownVulnerableActions { client })
     }
 
     fn audit_step<'doc>(&self, step: &Step<'doc>) -> Result<Vec<Finding<'doc>>> {
diff --git a/crates/zizmor/src/audit/ref_confusion.rs b/crates/zizmor/src/audit/ref_confusion.rs
@@ -59,13 +59,11 @@ impl Audit for RefConfusion {
             )));
         }
 
-        let Some(client) = state.github_client() else {
-            return Err(AuditLoadError::Skip(anyhow!(
-                "can't run without a GitHub API token"
-            )));
-        };
-
-        Ok(Self { client })
+        state
+            .gh_client
+            .clone()
+            .ok_or_else(|| AuditLoadError::Skip(anyhow!("can't run without a GitHub API token")))
+            .map(|client| RefConfusion { client })
     }
 
     fn audit_workflow<'doc>(
diff --git a/crates/zizmor/src/audit/stale_action_refs.rs b/crates/zizmor/src/audit/stale_action_refs.rs
@@ -67,13 +67,11 @@ impl Audit for StaleActionRefs {
             )));
         }
 
-        let Some(client) = state.github_client() else {
-            return Err(AuditLoadError::Skip(anyhow!(
-                "can't run without a GitHub API token"
-            )));
-        };
-
-        Ok(Self { client })
+        state
+            .gh_client
+            .clone()
+            .ok_or_else(|| AuditLoadError::Skip(anyhow!("can't run without a GitHub API token")))
+            .map(|client| StaleActionRefs { client })
     }
 
     fn audit_step<'w>(&self, step: &Step<'w>) -> Result<Vec<Finding<'w>>> {
diff --git a/crates/zizmor/src/audit/template_injection.rs b/crates/zizmor/src/audit/template_injection.rs
@@ -605,8 +605,7 @@ mod tests {
             let audit_state = AuditState {
                 config: &Default::default(),
                 no_online_audits: false,
-                cache_dir: "/tmp/zizmor".into(),
-                gh_token: None,
+                gh_client: None,
                 gh_hostname: GitHubHost::Standard("github.com".into()),
             };
             let audit = <$audit_type>::new(&audit_state).unwrap();
diff --git a/crates/zizmor/src/github_api.rs b/crates/zizmor/src/github_api.rs
@@ -15,7 +15,7 @@ use http_cache_reqwest::{
 use owo_colors::OwoColorize;
 use reqwest::{
     Response, StatusCode,
-    header::{ACCEPT, AUTHORIZATION, HeaderMap, USER_AGENT},
+    header::{ACCEPT, AUTHORIZATION, HeaderMap, HeaderValue, InvalidHeaderValue, USER_AGENT},
 };
 use reqwest_middleware::{ClientBuilder, ClientWithMiddleware};
 use serde::{Deserialize, de::DeserializeOwned};
@@ -61,20 +61,43 @@ impl GitHubHost {
     }
 }
 
+/// A sanitized GitHub access token.
+#[derive(Clone)]
+pub(crate) struct GitHubToken(String);
+
+impl GitHubToken {
+    pub(crate) fn from_clap(token: &str) -> Result<Self, String> {
+        let token = token.trim();
+        if token.is_empty() {
+            return Err("GitHub token cannot be empty".into());
+        }
+        Ok(Self(token.to_owned()))
+    }
+
+    fn to_header_value(&self) -> Result<HeaderValue, InvalidHeaderValue> {
+        HeaderValue::from_str(&format!("Bearer {}", self.0))
+    }
+}
+
+#[derive(Clone)]
 pub(crate) struct Client {
     api_base: String,
     http: ClientWithMiddleware,
 }
 
 impl Client {
-    pub(crate) fn new(hostname: &GitHubHost, token: &str, cache_dir: &Path) -> Self {
+    pub(crate) fn new(
+        hostname: &GitHubHost,
+        token: &GitHubToken,
+        cache_dir: &Path,
+    ) -> anyhow::Result<Self> {
         let mut headers = HeaderMap::new();
         headers.insert(USER_AGENT, "zizmor".parse().unwrap());
         headers.insert(
             AUTHORIZATION,
-            format!("Bearer {token}")
-                .parse()
-                .expect("couldn't build authorization header for GitHub client?"),
+            token
+                .to_header_value()
+                .context("couldn't build authorization header for GitHub client")?,
         );
         headers.insert("X-GitHub-Api-Version", "2022-11-28".parse().unwrap());
         headers.insert(ACCEPT, "application/vnd.github+json".parse().unwrap());
@@ -105,10 +128,10 @@ impl Client {
         }))
         .build();
 
-        Self {
+        Ok(Self {
             api_base: hostname.to_api_url(),
             http,
-        }
+        })
     }
 
     async fn paginate<T: DeserializeOwned>(
@@ -525,7 +548,7 @@ pub(crate) struct File {
 
 #[cfg(test)]
 mod tests {
-    use crate::github_api::GitHubHost;
+    use crate::github_api::{GitHubHost, GitHubToken};
 
     #[test]
     fn test_github_host() {
@@ -540,4 +563,23 @@ mod tests {
             assert_eq!(GitHubHost::from_clap(host).unwrap().to_api_url(), expected);
         }
     }
+
+    #[test]
+    fn test_github_token() {
+        for (token, expected) in [
+            ("gha_testtest\n", "gha_testtest"),
+            ("  gha_testtest  ", "gha_testtest"),
+            ("gho_testtest", "gho_testtest"),
+            ("gho_test\ntest", "gho_test\ntest"),
+        ] {
+            assert_eq!(GitHubToken::from_clap(token).unwrap().0, expected);
+        }
+    }
+
+    #[test]
+    fn test_github_token_err() {
+        for token in ["", " ", "\r", "\n", "\t", "     "] {
+            assert!(GitHubToken::from_clap(token).is_err());
+        }
+    }
 }
diff --git a/crates/zizmor/src/lsp.rs b/crates/zizmor/src/lsp.rs
@@ -261,8 +261,7 @@ pub(crate) async fn run() -> anyhow::Result<()> {
     let audit_state = AuditState {
         config: &config,
         no_online_audits: false,
-        cache_dir: std::env::temp_dir(),
-        gh_token: None,
+        gh_client: None,
         gh_hostname: crate::GitHubHost::Standard("github.com".into()),
     };
 
diff --git a/crates/zizmor/src/main.rs b/crates/zizmor/src/main.rs
@@ -16,7 +16,7 @@ use clap_verbosity_flag::InfoLevel;
 use config::Config;
 use finding::{Confidence, Persona, Severity};
 use github_actions_models::common::Uses;
-use github_api::GitHubHost;
+use github_api::{GitHubHost, GitHubToken};
 use ignore::WalkBuilder;
 use indicatif::ProgressStyle;
 use owo_colors::OwoColorize;
@@ -69,8 +69,8 @@ struct App {
     offline: bool,
 
     /// The GitHub API token to use.
-    #[arg(long, env, value_parser = NonEmptyStringValueParser::new())]
-    gh_token: Option<String>,
+    #[arg(long, env, value_parser = GitHubToken::from_clap)]
+    gh_token: Option<GitHubToken>,
 
     /// The GitHub Server Hostname. Defaults to github.com
     #[arg(long, env = "GH_HOST", default_value = "github.com", value_parser = GitHubHost::from_clap)]
@@ -441,7 +441,7 @@ fn collect_from_repo_slug(
         )));
     }
 
-    let client = state.github_client().ok_or_else(|| {
+    let client = state.gh_client.as_ref().ok_or_else(|| {
         anyhow!(tips(
             format!("can't retrieve repository: {input}", input = input.green()),
             &[format!(
@@ -646,7 +646,7 @@ fn run() -> Result<ExitCode> {
         ))
     })?;
 
-    let audit_state = AuditState::new(&app, &config);
+    let audit_state = AuditState::new(&app, &config)?;
     let registry = collect_inputs(
         &app.inputs,
         &app.collect,
diff --git a/crates/zizmor/src/state.rs b/crates/zizmor/src/state.rs
@@ -14,13 +14,13 @@ use crate::{
 pub(crate) struct AuditState<'a> {
     pub(crate) config: &'a Config,
     pub(crate) no_online_audits: bool,
-    pub(crate) cache_dir: PathBuf,
-    pub(crate) gh_token: Option<String>,
+    /// A cache-configured GitHub API client, if a GitHub API token is given.
+    pub(crate) gh_client: Option<Client>,
     pub(crate) gh_hostname: GitHubHost,
 }
 
 impl<'a> AuditState<'a> {
-    pub(crate) fn new(app: &App, config: &'a Config) -> Self {
+    pub(crate) fn new(app: &App, config: &'a Config) -> anyhow::Result<Self> {
         let cache_dir = match &app.cache_dir {
             Some(cache_dir) => PathBuf::from(cache_dir),
             None => choose_app_strategy(AppStrategyArgs {
@@ -35,21 +35,17 @@ impl<'a> AuditState<'a> {
 
         tracing::debug!("using cache directory: {cache_dir:?}");
 
-        Self {
+        let gh_client = app
+            .gh_token
+            .as_ref()
+            .map(|token| Client::new(&app.gh_hostname, token, &cache_dir))
+            .transpose()?;
+
+        Ok(Self {
             config,
             no_online_audits: app.no_online_audits,
-            cache_dir,
-            gh_token: app.gh_token.clone(),
+            gh_client,
             gh_hostname: app.gh_hostname.clone(),
-        }
-    }
-
-    /// Return a cache-configured GitHub API client, if
-    /// a GitHub API token is present.
-    /// If gh_hostname is also present, set it as api_base for client.
-    pub(crate) fn github_client(&self) -> Option<Client> {
-        self.gh_token
-            .as_ref()
-            .map(|token| Client::new(&self.gh_hostname, token, &self.cache_dir))
+        })
     }
 }
diff --git a/docs/release-notes.md b/docs/release-notes.md
@@ -17,6 +17,8 @@ of `zizmor`.
 
 * Fixed a bug where `--fix` would fail to preserve comments when modifying
   block-style YAML mappings (#995)
+* Fixed a bug where `zizmor` would crash when given a GitHub API token
+  with leading or trailing whitespace (#1027)
 
 ## 1.11.0
 
