Closed WebFreak001 closed 1 year ago
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:
pull_request_target
we just open an intro comment, so it's at the very top of the comment chain (this is trusted and we need to be extra careful here, but as we only make a comment and not do anything else, it should be fine) - we should NEVER checkout the repository / what the user made here or try to build dub.pull_request
action running in context of forking user generates the comment text (may be malicious, but it's only content inside their own PR comment, so that's fine I think) - that comment is uploaded as github artifact, along with the pull request id, so we later know where to make the commentpull_request
actions for PR Info
, when one occurs, it downloads the pr.zip artifact that was created, extracts it and comments the content of pr/comment.txt into PR/issue with the id that's specified in the ID inside the zipwill 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.