Skip to content

bugfix: sanitize gh_token & avoid panic - #1027

Merged
woodruffw merged 6 commits into
zizmorcore:mainfrom
acidghost:fix/gh-token-panic
Jul 15, 2025
Merged

bugfix: sanitize gh_token & avoid panic#1027
woodruffw merged 6 commits into
zizmorcore:mainfrom
acidghost:fix/gh-token-panic

Conversation

@acidghost

@acidghost acidghost commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

Fixes #1021.

✨ Copilot summary This pull request refactors the handling of GitHub API tokens and clients across the codebase, replacing the previous token-based approach with a new `GitHubToken` struct and updating related logic to improve encapsulation and error handling. Additionally, it modifies the `AuditState` structure to directly store an optional GitHub client instead of a token, simplifying client initialization and usage.

Refactoring GitHub API Token Handling:

  • Introduced the GitHubToken struct in crates/zizmor/src/github_api.rs to encapsulate and validate GitHub tokens. Added methods for sanitizing token input and converting tokens to header values.
  • Updated the Client::new method to accept a GitHubToken instance instead of a raw string, improving error handling during header creation. (F889a5b4L89R128)

Changes to AuditState Structure:

  • Replaced gh_token and cache_dir fields in AuditState with an optional gh_client field, directly storing a pre-configured GitHub client. Updated the AuditState::new method to initialize the client based on the presence of a token. [1] [2]

Updates to Audit Implementations:

  • Refactored audit implementations (ImpostorCommit, KnownVulnerableActions, RefConfusion, StaleActionRefs) to use the new gh_client field directly, simplifying error handling and client initialization logic. [1] [2] [3] [4]

Test Updates:

  • Updated tests across multiple files to reflect the removal of gh_token and the addition of gh_client. Added new tests for GitHubToken validation and sanitization. [1] [2] [3] [4] [5]

Other Changes:

  • Updated command-line argument parsing in crates/zizmor/src/main.rs to use GitHubToken::from_clap for validating tokens.

Comment thread crates/zizmor/src/github_api.rs
@acidghost
acidghost requested a review from woodruffw July 14, 2025 14:58

Ok(ImpostorCommit { client })
state
.github_client()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging: maybe not worth fixing with this PR, but it seems non-ideal that we now have to re-check the error state on GitHubClient on each audit initialization. Maybe we should create a single Option<GitHubClient> on the audit state instead.

(Feel free to look into that if you feel like it; otherwise that can wait until another refactor 🙂)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to play around with lifetimes to avoid cloning the Client and keep a reference in the Audit implementations, but I've not been using much Rust in the past year and it was starting to get like a bigger rabbit hole than I can dig through at the moment 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries at all, thanks for poking at it!

@funnelfiasco funnelfiasco added the bugfix Fixes a known bug label Jul 14, 2025
Move the client as a property on `AuditState` to check token validity
(if given) early on. This triggers the check even if audit rules making
use of GH API are not used.

Additionally, instead of creating a new `Client` for audit rules needing
it, we create it once and clone it.
@acidghost
acidghost requested a review from woodruffw July 14, 2025 20:18

@woodruffw woodruffw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- I wish we didn't have to clone the GitHub client in so many places, but it's a pretty small cost given that we reuse each audit many times.

@woodruffw woodruffw added the cli label Jul 15, 2025
@woodruffw
woodruffw enabled auto-merge (squash) July 15, 2025 22:18
@woodruffw
woodruffw merged commit b87e2d3 into zizmorcore:main Jul 15, 2025
8 checks passed
@woodruffw

Copy link
Copy Markdown
Member

Thanks @acidghost!

@acidghost
acidghost deleted the fix/gh-token-panic branch July 16, 2025 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes a known bug cli

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Panic if GH_TOKEN contains unexpected characters

3 participants