Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential security issue with "馃 automerge" #325

Closed
Carreau opened this issue May 22, 2019 · 7 comments
Closed

Potential security issue with "馃 automerge" #325

Carreau opened this issue May 22, 2019 · 7 comments

Comments

@Carreau
Copy link
Contributor

@Carreau Carreau commented May 22, 2019

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 "馃 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.

@Mariatta
Copy link
Member

@Mariatta Mariatta commented 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 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.

@lysnikolaou
Copy link

@lysnikolaou lysnikolaou commented May 22, 2019

I think it makes sense to dismiss the review altogether and apply the awaiting review label again.

@Carreau
Copy link
Contributor Author

@Carreau Carreau commented 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
Copy link
Member

@Mariatta Mariatta commented May 23, 2019

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 "馃 automerge" label).

@lysnikolaou
Copy link

@lysnikolaou lysnikolaou commented May 28, 2019

@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?

Mariatta added a commit to Mariatta/bedevere that referenced this issue May 1, 2020
- Todo: write tests

Closes python/core-workflow#325
Mariatta added a commit to Mariatta/bedevere that referenced this issue May 4, 2020
Add tests:

- new commits in PR that is awaiting merge
- new commits in PR that is in other state
- no new commits in push

Closes python/core-workflow#325
Mariatta added a commit to Mariatta/bedevere that referenced this issue May 4, 2020
Add tests:

- new commits in PR that is awaiting merge
- new commits in PR that is in other state
- no new commits in push

Closes python/core-workflow#325
@Mariatta
Copy link
Member

@Mariatta Mariatta commented 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 review stage. python/bedevere#220

Mariatta added a commit to python/bedevere that referenced this issue May 15, 2020
鈥220)

Add tests:

- new commits in PR that is awaiting merge
- new commits in PR that is in other state
- no new commits in push

Closes python/core-workflow#325
@Mariatta
Copy link
Member

@Mariatta Mariatta commented May 15, 2020

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.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants