nodejs / help

:sparkles: Need help with Node.js? File an Issue here. :rocket:
1.44k stars 276 forks source link

Enable cache storage for stalebot #4396

Closed RedYetiDev closed 1 month ago

RedYetiDev commented 1 month ago

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.

RedYetiDev commented 1 month ago

Fixes the following warning on all workflow runs:

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

Trott commented 1 month ago

@nodejs/actions

bmuenzenmeyer commented 1 month ago

I'm -1 on this - citing https://github.com/actions/stale/issues/1133#issuecomment-2133029572 - on it's surface it seems like a HUGE addition of permissions to surpress a warning.

RedYetiDev commented 1 month ago

@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 commented 1 month ago

@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?

RedYetiDev commented 1 month ago

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)

Trott commented 1 month ago

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 commented 1 month ago

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

RedYetiDev commented 1 month ago

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).

RedYetiDev commented 1 month ago

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 commented 1 month ago

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.

RedYetiDev commented 1 month ago

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

RedYetiDev commented 1 month ago

https://github.com/actions/stale/issues/1159