You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Potential security issue with ":robot: automerge" #325
Opening following private back and forth with @Mariatta
Imagine the following sequence of event:
Evil User: Send Typo Pull-request.
CI Starts
Core-dev: approve and set ":robot: automerge" label
Evil user push new commits with backdoor.
CI Re-Starts
CI Pass
Miss-islington merges with backdoor.
One of the solution is to look at the push event; and remove the label;
You can also activate the "dismiss review on commits push" and have Miss islington merge only if there are positive reviews.
Thanks for the report, @Carreau. I think we need to fix this.
When a PR has been approved, it would have an awaiting merge label. So one way to deal with this is to remove the awaiting merge label and move it back to awaiting core review state. But I think it will make sense to also dismiss review, and request a re-review.
awaiting change review sounds good too. I kinda don't want to create yet another label just for this situation.
I'm thinking perhaps the awaiting change review should be done anyway when there is a new push after last review, (even for the ones without ":robot: automerge" label).
@Mariatta I could work on updating bedevere to apply the awaiting change review label, as soon as a new commit is pushed, if nobody else already works on this. Would that be okay in your opinion?
I've created PR to address this.
If there is new commit pushed to already approved PR, it will be moved back to awaiting core review stage. python/bedevere#220
I've deployed the change where approved PRs will be moved back to the awaiting core review stage if it has a new commit pushed. If there is any issues with it please open a ticket in bedevere repo.
Activity
Mariatta commentedon May 22, 2019
Thanks for the report, @Carreau. I think we need to fix this.
When a PR has been approved, it would have an
awaiting mergelabel. So one way to deal with this is to remove theawaiting mergelabel and move it back toawaiting core reviewstate. But I think it will make sense to also dismiss review, and request a re-review.lysnikolaou commentedon May 22, 2019
I think it makes sense to dismiss the review altogether and apply the
awaiting reviewlabel again.Carreau commentedon May 23, 2019
Or "awaiting changes review"; in the other hand, the push can just cleanup/rebase, so their might not be much to re-review.
Could you push a label "automerged-cancel"; it's really easy to spot and should be pretty rare.
Mariatta commentedon May 23, 2019
awaiting change reviewsounds good too. I kinda don't want to create yet another label just for this situation.I'm thinking perhaps the
awaiting change reviewshould be done anyway when there is a new push after last review, (even for the ones without ":robot: automerge" label).lysnikolaou commentedon May 28, 2019
@Mariatta I could work on updating
bedevereto apply theawaiting change reviewlabel, as soon as a new commit is pushed, if nobody else already works on this. Would that be okay in your opinion?Re-request core review when there is a new commit to the approved PR
Re-request core review when there is a new commit to the approved PR
Re-request core review when there is a new commit to the approved PR
Mariatta commentedon May 4, 2020
I've created PR to address this.
If there is new commit pushed to already approved PR, it will be moved back to
awaiting core reviewstage. python/bedevere#220Re-request core review when there is a new commit to the approved PR (#…
Mariatta commentedon May 15, 2020
I've deployed the change where approved PRs will be moved back to the
awaiting core reviewstage if it has a new commit pushed. If there is any issues with it please open a ticket in bedevere repo.Thanks.