feat: new audit: ref-version-mismatch #972
Conversation
| Ok(Self { client }) | ||
| } | ||
|
|
||
| fn audit_step<'doc>(&self, step: &Step<'doc>) -> anyhow::Result<Vec<Finding<'doc>>> { |
There was a problem hiding this comment.
NB: We'll probably also need audit_composite_step (for uses: in composite actions) and audit_reusable_job (for reusable workflows) here.
The process_step pattern (e.g. in forbidden-uses) might be a good reference for deduping these 🙂
crates/zizmor/src/main.rs
Outdated
| @@ -627,6 +627,7 @@ fn run() -> Result<ExitCode> { | |||
| register_audit!(audit::dangerous_triggers::DangerousTriggers); | |||
| register_audit!(audit::impostor_commit::ImpostorCommit); | |||
| register_audit!(audit::ref_confusion::RefConfusion); | |||
| register_audit!(audit::ref_version_mismatch::RefVersionMismatch); | |||
There was a problem hiding this comment.
Nitpick: new audits are added at the bottom!
(Elsewhere it's alphabetic, here the registry records them in order of addition.)
| // For now, we'll search the entire document source for version patterns | ||
| // that might correspond to this uses line. This is a simplified approach. |
There was a problem hiding this comment.
Q: is this comment accurate still? It looks like the implementation below doesn't scan the whole doc source, only the comment next to the uses: clause (which is the right way to do it!)
| } | ||
| findings.push( | ||
| Self::finding() | ||
| .severity(Severity::Low) |
There was a problem hiding this comment.
I could see an argument for this being medium -- it breaks tools like Dependabot and makes people think they're up to date/using a specific version, but actually aren't. OTOH I don't feel super strong about it; thoughts?
| impl AsRef<str> for Comment<'_> { | ||
| fn as_ref(&self) -> &str { | ||
| self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Flagging: not something that needs doing in this PR, but I'd be nice to avoid puncturing the newtype here and instead add the VERSION_COMMENT_PATTERNS matching within Comment's own interface. But that can be a follow-on refactor.
Adds a new audit that detects when commit SHAs don't match their corresponding version comment tags in GitHub Actions workflows. This addresses a case where Dependabot will not pick up on potential updates when the ref and version comment does not match. Fixes zizmorcore#643 Signed-off-by: Samuel Giddins <[email protected]>
segiddins
force-pushed
the
segiddins/feat-new-audit-ref-version-mismatch
branch
from
ef76d1d to
270314a
Compare
| return Ok(None); | ||
| }; | ||
|
|
||
| while !matches!(tag_ref.object.r#type, Some(ObjectType::Commit)) { |
There was a problem hiding this comment.
isnt git fun...
should this maybe have a limit to avoid tag cycles?
There was a problem hiding this comment.
Hmm, I think we could maybe avoid this loop entirely by switching endpoints here -- instead of using the "get a reference" endpoint we could use the "get a commit" endpoint, which should always produce a commit object.
This specifically: https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#get-a-commit
There was a problem hiding this comment.
(The docs don't say it super clearly, but gh api /repos/Swatinem/rust-cache/commits/refs/tags/v2.7.8 works and similar for refs/heads/...)
There was a problem hiding this comment.
So the issue with that endpoint is it doesn't return 404 when the ref doesnt exist, and instead it returns a 422, which is indistinguishable from e.g. running out of rate limit usage
There was a problem hiding this comment.
Huh, that seems 100% wrong from GitHub. Let me see if I can get some attention from them on that.
(I'd definitely prefer to have this use a single request without a loop if possible, for similar rate limit reasons. So maybe they can fix this somewhat quickly.)
There was a problem hiding this comment.
Has this issue been raised to GitHub? If not, I'm happy to file and raise awareness.
(Also, than you so much for working on this new feature!!)
There was a problem hiding this comment.
Yep, I’ve flagged it with some people I know, who confirmed that it’s an issue. But certainly having more people raise awareness of it would probably increase its relative priority internally 🙂
|
(Marking as blocked until GitHub fixes the REST API bug here.) |
Adds a new audit that detects when commit SHAs don't match their corresponding version comment tags in GitHub Actions workflows.
This addresses a case where Dependabot will not pick up on potential updates when the ref and version comment does not match.
Fixes #643