-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
try to fix pr_info permissions problem #2621
Conversation
wait this is broken since people can inject scripts into the build chain, e.g. dumping the token see https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ |
ok so the new architecture I made now:
|
will just merge and see if it fixes the issues we are having on PRs right now. We may need to throw away the whole thing if it doesn't work, but I will just try and push it myself into master for github actions to pick it up. Feel free to still review afterwards though, we can always change this, as it's only part of the build and nothing ending up in the dub binary or library. |
Perhaps we should regenerate that token. |
why? |
If I understand the situation correctly, there was a period of time when it could have been leaked and could lead to future surprises. |
no, as we only had the pull_request target, which didn't leak it. If this PR would have been merged before I realized that issue (the first code version I pushed) it would have become that situation though. It hasn't occurred though, so the token never left us here. (also it would be noticable with a suspicious PR on the dub repo) |
All good, had to mention it. Better off being safe about it. |
cc @rikkimax
turns out when people make PRs from other forks, the action also runs in their fork context there!
Using pull_request_target means we need to specially check that everything is correct to avoid that the token gets pwnd.
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target
I have also found a vulnerability that was in there before for testing, where it would load an updated script from the fork. This is now fixed and explicitly guarded against.