Add Fix for known-vulnerable-actions audit rule - #1019
Conversation
woodruffw
added
enhancement
| // Get the first patched version from the first vulnerability in the advisory | ||
| let first_patched_version = vuln | ||
| .vulnerabilities | ||
| .first() | ||
| .and_then(|v| v.first_patched_version.clone()); |
There was a problem hiding this comment.
Noting: this looks great to me for now, but we might want to additionally filter by semver compatibility between versions. But that can be a future problem 🙂
|
Thanks @mostafa, this looks great! One comment about the patch we apply, but otherwise this LGTM. |
Still not correct, but closer.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Commit pinned action | ||
| uses: actions/download-artifact@87c55149d96e628cc2ef7e6fc2aab372015aec85 # v4.1.3 # v4.0.0 |
There was a problem hiding this comment.
(Whoops, invalidated my own earlier comment.)
Removing some of the mocking revealed this bug -- our current RewriteFragment is probably too brittle with respect to whitespace. I think what we need to do here is either extend the RewriteFragment API to allow regexes or use two different fragments here (one for the commit hash, and one for the version itself).
There was a problem hiding this comment.
Thinking about it some more, we'll probably need to do it with a regex -- there will be a lot of small variations in syntax and whitespace that we'll need to match consistently for the replacement.
Specifically, we should probably match Dependabot's own behavior here:
There was a problem hiding this comment.
(More random thoughts)
I think what we can do to make this clean here is move the Fragment and Subfeature APIs (along with Span) into a common crate, which the location APIs (in zizmor) and the RewriteFragment op (in yamlpatch) can then both consume. That'll give us regex-based fragment rewriting using the primitives that are already well-tested in the codebase.
There was a problem hiding this comment.
I see you've already implemeneted it, so I'll leave it up to you.
There was a problem hiding this comment.
(I guess we should release a new version of yamlpatch as well.)
There was a problem hiding this comment.
Yep -- I have yamlpatch set up with trusted publishing on crates.io, so I'll cut a tag after this is merged and it'll auto-release 🙂
2 participants
xref #876