Request: Prevent (or recover from) malicious last-minute changes to PR's prior to merge.

[patricknelson]

PR owners could easily change their code at the last minute with the intent to deceive voters into thinking that the code is completely harmless. This would be very easy to do by simply calculating their vote threshold (which you can predict algorithmically referencing current bot code) and then waiting before the last vote to quickly push their intended changes.

Aside from manual review, how do you automate the process to ensure that these sorts of potentially malicious changes don't take place and cause the bot to crash in an unrecoverable manner (thereby preventing subsequent fixes to revert the change)? Should this be done manually by the bot owner or can this be solved in code?

One proposed architectural solution broken up into two parts (have mercy, I'm not a python dev):

• Have a separate daemon run from code stored in a discrete/known sub-directory. This daemon is responsible for working as a watchdog, watching chatbot for unexpected crashes which happen only after a commit/change. Eventually, it could also perform other security related tasks, such as ensuring the file system in certain spots are not tampered with unexpectedly.
• To ensure it can continue to properly monitor itself and chatbot, this new daemon can ensure that code modifications to itself cannot occur when modifications to chatbot have also been made. Also, it doesn't reload/restart unless its code has been modified and, if it is unable to restart, the previous instance of this daemon (if possible) can revert the code base again.

This could be a horrible idea. Anyway, let's have a vote...

• 👍 Solve manually: Bot owner should recover/revert these changes, this sort of thing isn't a problem (yet).
• 👎 Solve automatically: Find automated solution to preventing potentially malicious changes late in the game.

[rudehn]

Is this not already solved in code? Specifically in the code there's a check for

if some clever person attempts to submit more commits while we're
aggregating votes, this sha check will fail and no merge will occur

I have no idea if it works, just pointing it out.

[JordanReiter]

In addition to just malicious code, the real problem would be code that specifically breaks the system for tallying and processing votes. For instance, allowing changes to deploy after just 1-2 votes, or blocking all but a small selection of users from having their votes actually count.

[patricknelson]

@rudehn I see, didn't notice that in the code. But based on what I can read (again not a python guy) it is basing that SHA1 hash on results from the list of PR's immediately returned from the API, so unfortunately that doesn't apply.

i.e. since code is changing so quickly:

        prs = gh.prs.get_ready_prs(api, settings.URN, voting_window)

        # ... lower down...

                try:
                    gh.prs.merge_pr(api, settings.URN, pr, votes, vote_total,
                            threshold)

       # In merge_pr(), it only includes the SHA1 it just retrieved.

        # if some clever person attempts to submit more commits while we're
        # aggregating votes, this sha check will fail and no merge will occur
        "sha": pr["head"]["sha"],

So, currently it's only:

1. Querying the PR's in the API
2. Immediately aggregating votes on each PR
3. If it passes the threshold, merges (but includes the SHA1 hash it just retrieved).

So for you to fail, you'd have to push your code in that minuscule window between the initial API request fetching PR's and when it merges. Not to mention that, even if you were to track the SHA1 hash from the initial submission of the PR (or any point in between submit -> merge) you could prevent legitimate changes (e.g. updating documentation on changes).

[rudehn]

Fair point. I didn't look too hard into the code to see exactly what it was doing. I do like your idea of having a watchdog daemon to monitor chaosbot for failure. Perhaps even reverting the pull request that broke the bot.

[patricknelson]

I like the idea of a revert commit + a 'Tsk!' comment on the PR that broke it from the bot so you'd know you hurt it's code 😄

[rudehn]

Something definitely needs to be done to tighten up the security risk.

PR #48 got through

PR passed with a vote of 2 for and 0 against, with a
weighted total of 1.2 and a threshold of 1.0.

due to the current decision rules

[mmhobi7]

👎

[amoffat]

Just to throw some additional thoughts in there: it seems like comment and reaction votes are confusing. Maybe we should only rely on review votes. Here's why:

I believe the reason #48 was merged in was partially due to it appearing as if there were plenty of downvotes to stop it, when in reality the votes had reset because the head repo had a new commit. The threshold for getting merge approval was then easy to hit.

I believe (needs confirmation) if only PR reviews were used, people wouldn't be able to see reviews on older versions of the PR, which would help people have a little better sense of where voting stands.

Another option, suggested by my buddy @hoelzro, is "freezing" PRs, and possibly only ever merging them IFF the head sha the PR was created with matches the head sha at merge time. This would prevent all changes though.

[hongaar]

Also, it's kind of counter-intuitive to cast a reaction vote after a PR has changed. In the current system, if you approve the changes you'd have to undo and redo your reaction.

However it would be a waste to disregard the reaction system: it's a perfect tool for voting given it's visibility and low threshold for people to cast votes.

[davidak]

Why not reset the voting window at every new commit? (havn't read all text, maybe already suggested)

[reddraggone9]

It currently resets on push. That's been the behavior since the beginning. That's also what caused the problem with #48.

[hongaar]

@reddraggone9 The voting window (some time needs to pass before PR gets accepted/rejected) is not reset afaik. When new commits are added only the vote counts are reset.

[reddraggone9]

The length of the voting window is defined here. That's picked up and passed into another function used to check PRs where it's compared to a per-PR duration. That duration is since the PR was lasted updated. "Last updated" is defined in terms of Github API stuff that seems to me to check when the PR was lasted changed. This is also the same value used to reset votes, so the duration and votes reset at the same time.

[patricknelson]

@reddraggone9 Thanks for the breakdown. Does this mean that 👍's on the initial comment in the PR don't count after subsequent commits/pushes (even if they're registered afterwards)?

[reddraggone9]

You people... They count if they're redone. I'll source that in a minute. Source

[geekyi]

I think this is a huge ux bug. Even if something got tons of negative votes, someone could still add a commit and change the votes; no matter what the interval threshold, they have to simply get a few "friends" to vote to counter the negatives. And, if they don't have that many votes, keep updating the commit.

If they somehow get their pr closed, simply have to open a new and try again and again, until it gets done.

Most people wouldn't care or understand that their negative votes may not count..
The feedback isn't obvious enough even if @chaosbot comments. IMHO
Only way would probably be to not allow further commits in a merge

[patricknelson]

Yeah -- while the resetting window idea isn't a bad idea, maybe @chaosbot should comment on the PR to flag a new post that you can then 👍/👎 to start voting on again? The comment basically can inform people that voting has reset and only comments on/after the current comment count.

Edit: I think it should be on that specific comment for which reactions are then counted as +/- votes. That or further comments containing 👍/👎 in the comment itself.

[Ajedi32]

@patricknelson Yeah, I like that solution the best. Reviews wouldn't be very good as the primary way to vote, since they make it really hard to see at a glance what the vote totals are and they clutter the issue with unnecessary comments.

[hongaar]

@patricknelson I think this solution would work pretty well. Both reactions to that @chaosbot comment and comments further below containing emojis should count as votes.

[patricknelson]

Maybe someday when the web server has matured, we can have a page/area dedicated to seeing the status of PR's and how many votes are needed and etc 🎉 one can dream.

[chaosbot]

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

[chaosbot]

/vote close

[patricknelson]

No longer relevant.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[Ajedi32]

Uh... I think ChaosBot is broken. 😕

[andrewda]

Yes, very. It's being addressed in #523

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[chaosbot]

/vote close

Command Ran

[chaosbot]

⛔ The issue has been closed after a vote.

[patricknelson]

I... I think it's broken. 😂

Muted.