Enable cache storage for stalebot

[avivkeller]

This PR should make the Stalebot more efficient via allowing it to utilize GitHub caching, which needs actions write permissions in order to update the existing cache.

[avivkeller]

Fixes the following warning on all workflow runs:

“ Warning: Error delete _state: [403] Resource not accessible by integration”

[Trott]

@nodejs/actions

[bmuenzenmeyer]

I'm -1 on this - citing actions/stale#1133 (comment) - on it's surface it seems like a HUGE addition of permissions to surpress a warning.

[avivkeller]

@bmuenzenmeyer That may be true, but this action is sending its permissions to actions/cache, without using any arbitrary data, so I don't see a security concern. An attacker would need to compromise Github Actions to compromise this, and even then, I've pushed an SHA pinned commit, so even the action itself's compromise wouldn't affect this workflow.

[Trott]

@bmuenzenmeyer That may be true, but this action is sending its permissions to actions/cache, without using any arbitrary data, so I don't see a security concern. An attacker would need to compromise Github Actions to compromise this, and even then, I've pushed an SHA pinned commit, so even the action itself's compromise wouldn't affect this workflow.

Wouldn't finding an error to compromise in a pinned version of an action be the kind of thing that happens once in a while in the real world and not an unthinkably small risk?

[avivkeller]

Wouldn't finding an error to compromise in a pinned version of an action be the kind of thing that happens once in a while in the real world and not an unthinkably small risk?

It's an unthinkably small risk, because the worst case, an attacker who (A) compromises the GitHub actions repository, and (B) makes a new commit with the same pinned hash, they still won't be able to compromise this, and the only thing that will happen is the stale bot would stop working because of a conflicting head ref. SHA collisions in Git don't overwrite commits, they simply stop working.

So, the attacker would need to compromise GitHub actions (unlikely), run a SHA collision (even more unlikely), and then what do they gain? The stale bot stops working.

(Reference: https://stackoverflow.com/questions/9392365/how-would-git-handle-a-sha-1-collision-on-a-blob)

[bmuenzenmeyer]

appreciate the careful discussion here - a tiny nit - it's nice to have the version number of the SHA at the end of the line as a comment.

dependabot can keep both up to date in the future

suggest we still have someone within @nodejs/actions review

[Trott]

t's an unthinkably small risk, because the worst case, an attacker who (A) compromises the GitHub actions repository

I'm thinking of someone exploiting an existing bug in the version of the action that we're using, not forcing us to check out different code.

[Trott]

t's an unthinkably small risk, because the worst case, an attacker who (A) compromises the GitHub actions repository

I'm thinking of someone exploiting an existing bug in the version of the action that we're using, not forcing us to check out different code.

And yeah, exploiting an existing bug can happen with any version, but the point is we're giving stalebot a lot of extra permissions so it can do a lot more damage.

That said, this isn't blocking. I'm genuinely unsure about the likelihood of exploitable bugs in an action and the extent to which damage could be done with these permissions. If you're comfortable with it and others are comfortable with it, I can be dismissed as someone who needs to stop commenting about things he doesn't know a lot about. :-D

[avivkeller]

t's an unthinkably small risk, because the worst case, an attacker who (A) compromises the GitHub actions repository

I'm thinking of someone exploiting an existing bug in the version of the action that we're using, not forcing us to check out different code.

Oh, well, in my opinion, that is unlikely, as this is used throughout all of github, so I assume they are monitoring the code of vulnerabilities constantly. I also looked over the code, and arbitrary data (such as issue titles) aren't used in unsafe places (or at all really).

[avivkeller]

That said, this isn't blocking. I'm genuinely unsure about the likelihood of exploitable bugs in an action and the extent to which damage could be done with these permissions. If you're comfortable with it and others are comfortable with it, I can be dismissed as someone who needs to stop commenting about things he doesn't know a lot about. :-D

I'm personally fine with it, but @nodejs/actions should probably check it out.

Because this repo has so little actions and so little data to compromise, the worst that can be done is a rerun/run/disable/cancel of the stalebot, or a deletion of its cache.

Keep in mind this also means that the attacker found a vulnerability in the stalebot, which is a difficult task itself.

[Trott]

Sounds good. Thanks. Let's give it another day or two for @nodejs/actions folks (or other knowledgable folks--maybe some people on @nodejs/build?) to comment.

[avivkeller]

@marco-ippolito, seeing as you, a member of @nodejs/actions approved, do you think this is safe enough to merge?

[H4ad]

LGTM!

@redyetidev Can you create an issue to track the changes on actions stale to see if they will create a version that requires less permission? You can link this PR and the issues on actions/stale to revert this change as soon we have a version that requires less permission.

[avivkeller]

actions/stale#1159