Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add security update support for Github Actions #6071

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

deivid-rodriguez
Copy link
Contributor

It currently sits on top of #6052, because they were updating the same part of the code and I didn't want conflicts.

Still needs tests but just open a WIP PR for now.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/security-updates-for-actions branch 2 times, most recently from da1c9b6 to 2eb35c6 Compare November 11, 2022 15:30
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review November 11, 2022 15:55
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner November 11, 2022 15:55
@deivid-rodriguez
Copy link
Contributor Author

This one is ready for a review now! :)

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/security-updates-for-actions branch from 2eb35c6 to cceaa21 Compare November 14, 2022 11:06
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/security-updates-for-actions branch 6 times, most recently from ab46138 to 557f5f4 Compare November 17, 2022 17:23
@deivid-rodriguez
Copy link
Contributor Author

Alright, so I addressed review comments, let me update the status of this PR.

The PR currently implements the standard "lowest fixed version" logic of other ecosystems with the following particularities:

  • It tries to update to the lowest fixed version that keeps the existing version precision. So a vulnerable v2 would go to v3 if possible. This seems ✅ to me.

  • If a fixed version keeping precision does not exist, it changes precision in favor of fixing the vulnerability. So say v2 is vulnerable, because it points to v2.7.4 and the lowest fixed version is 2.7.5. And say there's no fixed v3 yet. In that case we propose an update from v2 to v2.7.5. Maybe this one is too smart, not fully sure if users will like this change in precision or if it's best to not create an update at all ❓.

  • If pinned to a major version tag which belongs to the vulnerable series but points to a non vulnerable tag, it does not propose an update. So in the case above, if pinned to v2 pointing to v2.7.5 (the latest patch, most common case), then everything is ok and we don't create an update. This one also seems ✅, just noting that this is currently a bug in DG, so although we may receive false positive alerts due to it, we won't create alerts for them.

Finally, I went a bit overboard an added a few cross-ecosystem refactoring that are not directly related to this PR. May I split them to a separate PR?

@mattt
Copy link
Contributor

mattt commented Nov 18, 2022

  • If a fixed version keeping precision does not exist, it changes precision in favor of fixing the vulnerability. So say v2 is vulnerable, because it points to v2.7.4 and the lowest fixed version is 2.7.5. And say there's no fixed v3 yet. In that case we propose an update from v2 to v2.7.5. Maybe this one is too smart, not fully sure if users will like this change in precision or if it's best to not create an update at all ❓.

Yeah, that one's a head-scratcher.

If a v2 points to a vulnerable (old) version, it's most likely to be an error on the part of the maintainer. If the major tag is moved by automation, that should happen immediately; an outdated ref indicates that the maintainer forgot to perform this step in their release process. In this case, an update to a precise version (e.g. 2.7.5) makes sense to me — it mitigates a vulnerability and hedges against human error to maintain the major version alias.

The main problem, I think, is in communicating why Dependabot is suggesting a particular change. I don't expect most users to understand the idiosyncrasies of Actions versioning, but I'm not sure what we can really do about that. In the interest of shipping to learn, I'd be happy going with this behavior and seeing what kind of reception it gets from users.

@mattt
Copy link
Contributor

mattt commented Nov 18, 2022

Finally, I went a bit overboard an added a few cross-ecosystem refactoring that are not directly related to this PR. May I split them to a separate PR?

Yeah, let's evaluate those separately.

@deivid-rodriguez
Copy link
Contributor Author

I totally agree with your points!

The main problem, I think, is in communicating why Dependabot is suggesting a particular change. I don't expect most users to understand the idiosyncrasies of Actions versioning, but I'm not sure what we can really do about that. In the interest of shipping to learn, I'd be happy going with this behavior and seeing what kind of reception it gets from users.

Yes, that was my biggest hesitation here, that users may not understand why we're proposing this update changing precision. I guess we could add a note about this to the PR, but I'm not fully sure how to go about implementing that.

Yeah, let's evaluate those separately.

Sure, let me split them!

It works like this:

* It tries to update to the lowest fixed version that keeps the existing
  version precision.
* If a fixed version keeping precision does not exist, it changes
  precision in favor of fixing the vulnerability.
* If pinned to a major version tag which belongs to the vulnerable
  series but points to a non vulnerable tag, it does not propose an
  update.
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/security-updates-for-actions branch from 557f5f4 to 7196bb0 Compare November 21, 2022 10:03
@deivid-rodriguez deivid-rodriguez merged commit 02d3a00 into main Nov 21, 2022
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/security-updates-for-actions branch November 21, 2022 12:24
@pavera pavera mentioned this pull request Nov 30, 2022
@deivid-rodriguez deivid-rodriguez mentioned this pull request Jun 14, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants