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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sometimes a PR is immediately accepted/rejected #199

Closed
hongaar opened this issue May 24, 2017 · 85 comments
Closed

Sometimes a PR is immediately accepted/rejected #199

hongaar opened this issue May 24, 2017 · 85 comments

Comments

@hongaar
Copy link
Member

hongaar commented May 24, 2017

i.e.

@amoffat:

[...] It merged due to a bug that hasn't been patched yet, where if a downstream branch sits for longer than the voting window, before a PR is opened, then a PR is opened, chaos thinks its ready to be approved. Needs to be patched ASAP

Agreed. Needs to be patched ASAP!

Or we'll risk another autocracy.

Or worse...
@rudehn
Copy link
Contributor

rudehn commented May 24, 2017

Yeah it's bad but we do have a larger voting threshold now.

@hongaar
Copy link
Member Author

hongaar commented May 24, 2017

That's true, will offset the risk a bit atm as the PR will need to gather enough votes in under 30 seconds or whatever the PULL_REQUEST_POLLING_INTERVAL_SECONDS is.

@ECrownofFire
Copy link
Contributor

Checking against the more recent of pushed_at and the PR's creation time solves this pretty easily.

@PlasmaPower
Copy link
Contributor

That was my original strategy for gaining power! Details: #162 (comment)

@PlasmaPower
Copy link
Contributor

@ECrownofFire does creation time reflect opened time?

@PlasmaPower
Copy link
Contributor

I think it'd be best to do max(updated_at, pushed_at).

@ECrownofFire
Copy link
Contributor

updated_at also includes people adding comments and such, so that's a no-go. And the creation time is whenever the PR itself was created.

And to solve that issue in #162 I'd suggest that chaosbot close previously merged/rejected PRs. It'd also be good to reject (non-WIP) PRs that have sat for over 24 hours without being merged, for the same reasons.

@PlasmaPower
Copy link
Contributor

@ECrownofFire an insta-merge can still happen with that if the author closes the PR just before the voting period ends.

@ECrownofFire
Copy link
Contributor

Closed PRs aren't considered for merging, or am I misunderstanding you?

@PlasmaPower
Copy link
Contributor

PlasmaPower commented May 25, 2017 •
edited

  1. Attacker creates legit PR, and gathers votes
  2. Attacker closes PR just before voting period ends (to collect votes but not merge it)
  3. Attacker waits for attention to shift off of their PR
  4. Attacker amends malicious code to their PR
  5. Attacker waits the length of the voting period
  6. Attacker re-opens PR
  7. Attacker's PR is instantly merged

@amoffat amoffat mentioned this issue May 25, 2017
Closed
@amoffat
Copy link
Contributor

amoffat commented May 25, 2017

Similar to what @PlasmaPower said, would max(pr_created_at, repo_pushed_at) work?

@PlasmaPower
Copy link
Contributor

I don't think created_at changes when a PR is closed or opened though.

@PlasmaPower
Copy link
Contributor

Oh, you're talking about the original bug. Yeah that'd work.

@amoffat
Copy link
Contributor

amoffat commented May 25, 2017

For your attack vector @PlasmaPower, I ran some tests:

  • You are able to re-open a PR that you yourself closed
  • You are not able to re-open a PR that the owner (chaosbot) closed
  • There is no field on the PR api object that indicates if the PR was ever previously closed

So it looks grim. However, I did find out about the issue locking api, which should work for PRs too. The progression would be:

  1. User closes their PR
  2. Chaosbot scans closed, unmerged PRs and locks them

Thoughts?

@reddraggone9
Copy link
Contributor

That sounds good to me, but you could still pull off the attack using WIP instead of closing the PR yourself.

@amoffat
Copy link
Contributor

amoffat commented May 25, 2017

Ah, true. Maybe we should do away with WIP PRs? They don't seem to be used very much anyways

@reddraggone9 reddraggone9 mentioned this issue May 25, 2017
Merged
@amoffat amoffat mentioned this issue May 26, 2017
Merged
chaosbot added a commit that referenced this issue May 26, 2017
@chaosbot
merging PR #271: get true PR pushed_at datetime
ef69d70 
#271: get true PR pushed_at datetime

Description:
This should fix a number of issues, but it needs sanity checking.  Underlying issue mentioned #199 and #147 and #239 (comment)

🙆‍♀️ PR passed with a vote of 11 for and 0 against, with a weighted total of 11.0 and a threshold of 6.1.

Vote record:
@akilegaspi: 1
@amoffat: 1
@andrewda: 1
@bengjerstad: 1
@droogmic: 1
@eukaryote31: 1
@kumardeepakr3: 1
@ozyx: 1
@qgustavor: 1
@reddraggone9: 1
@rhengles: 1
@chaosbot
Copy link
Collaborator

This issue hasn't been active for a while.To keep it open, react with 👎 on the vote close post.

@chaosbot
Copy link
Collaborator

/vote close

@chaosbot
Copy link
Collaborator

chaosbot commented May 31, 2017 •
edited

/vote close

Vote failed

@mark-i-m
Copy link
Contributor

mark-i-m commented Jun 1, 2017

This should be fixed by #420

@amoffat
Copy link
Contributor

amoffat commented Jun 1, 2017

@mark-i-m aha, events! very creative!

@mark-i-m
Copy link
Contributor

mark-i-m commented Jun 2, 2017

/vote close

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 3, 2017 •
edited

/vote close

Command Ran

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 3, 2017

⛔ The issue has been closed after a vote.

@chaosbot chaosbot closed this as completed Jun 3, 2017
@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

8 similar comments
@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@rudehn rudehn mentioned this issue Jun 5, 2017
Closed
@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

17 similar comments
@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Vote Failed

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017 •
edited

/vote close

Command Ran

@chaosbot
Copy link
Collaborator

chaosbot commented Jun 5, 2017

⛔ The issue has been closed after a vote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@hongaar @amoffat @ECrownofFire @reddraggone9 @rudehn @PlasmaPower @mark-i-m @chaosbot