Don't allow someone with commits in a PR to approve #23134
-
We have our repo set up so all PRs must be approved by another engineer before they can be merged to main/master. You can not approve a PR that you have created. We rely on this to stop a single engineer getting code into production without someone else checking it. But there is a (possible) security hole. A bad actor (eg disgruntled employee) could find a PR that is already open, revert the original changes, commit malicious code, then approve it and merge it. Is there any way to stop someone from approving a PR if they have any commits in that PR? Thanks |
Beta Was this translation helpful? Give feedback.
Replies: 5 comments
-
The issue is, the malicious person can commit code as someone else. You can try this yourself by changing your In theory if you had everyone GPG sign their commits and somehow checked for this too, it wouldn’t be an issue, but most people don’t GPG sign commits. What I’d suggest is requiring approval from 2 seperate people with write+ access to the repository if you have enough people to enforce this. |
Beta Was this translation helpful? Give feedback.
-
Thanks for your reply! That’s interesting. I hadn’t considered that github has no way to verify whether the listed committer was actually the committer without people signing commits. I wonder if you could say a user who has pushed to the PR can’t approve it. Surely Github knows who has pushed to a branch (you need to auth for that) |
Beta Was this translation helpful? Give feedback.
-
I don’t think it’s true that GitHub can’t verify the committer… here’s a cut-n-paste from the UI:
To me, this seems like a tragic gap in the pull request process. Does anyone know something we don’t? |
Beta Was this translation helpful? Give feedback.
-
GitLab has this feature:
|
Beta Was this translation helpful? Give feedback.
-
Hi all! Tagging @peckjon on this 👋t2:Jon just shared:
@davich this post is originally from Jan 2021 so hoping you see this. Please let us know if this solves your issue. |
Beta Was this translation helpful? Give feedback.
Hi all! Tagging @peckjon on this 👋t2:Jon just shared:
@davich this post is originally from Jan 2021 so hoping you see this. Please let us know if this solves your issue.