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.