Reopening PR with "test this please" comment triggers build (even if new/malicious commits are added)

[chenxiaolong]

I'm running into an issue where reopening a closed pull request causes a build to trigger if there's a previous comment from an admin that contains "test this please".

This is the particular test case I'm looking at: https://github.com/chenxiaolong/DualBootPatcher/pull/67

This is what happened with that test PR:

1. Somebody makes a (good) commit and creates a PR
2. I say "test this please"
3. The other person closes the PR and makes an additional (malicious) commit
4. The other person reopens the PR
5. Jenkins comments "Can one of the admins verify this patch?" but runs a new build anyway, which contains the malicious commit

If I delete the "test this please" comment, I can close and reopen the PR as much as I want and no build is triggered.

I am using GitHub hooks for the triggering and do not have the Build every pull request automatically without asking (Dangerous!). option enabled.

Any idea why this happens? This can cause malicious code to run on my Jenkins slaves. Thanks in advance!

---

EDIT: Jenkins build when the PR was created: https://jenkins.noobdev.io/job/DualBootPatcher_PR/15/ Jenkins build when the PR was reopened https://jenkins.noobdev.io/job/DualBootPatcher_PR/16/

My config:

[chenxiaolong]

I also created an issue here https://github.com/janinko/ghprb/issues/394 as I'm not sure which repo is the "official" one (both are very active).

[chenxiaolong]

Any ideas on what might be causing this? Is there any more information I can provide to help?

I had the same issue occur with a real pull request today: https://github.com/chenxiaolong/DualBootPatcher/pull/73

I approved the PR for testing via a "test this please" comment, which triggered this build: https://jenkins.noobdev.io/job/DualBootPatcher_PR/17/ It failed and the user made a second commit in attempt to fix it and also happened to close and reopen the PR. The GHPRB plugin made a new build https://jenkins.noobdev.io/job/DualBootPatcher_PR/19/ without my approval.

[DavidTanner]

I don't have any ideas at the moment how to tell the plugin not to treat the reopened pull request like anything other than a new pull request.

[chenxiaolong]

I don't have any ideas at the moment how to tell the plugin not to treat the reopened pull request like anything other than a new pull request.

Thanks for the reply! I don't have a problem with treating reopened pull requests like a new pull request. The problem is that it's triggering a new build for reopened pull requests that contain a "test this please" comment.

[DavidTanner]

That is what I mean. When it checks creates a new pull request internally then it checks all of the comments since the PR was created. One of those comments being "test this please" tells the plugin to schedule a build. Hence the problem of building PRs that are closed and re-opened.

[chenxiaolong]

That is what I mean. When it checks creates a new pull request internally then it checks all of the comments since the PR was created. One of those comments being "test this please" tells the plugin to schedule a build. Hence the problem of building PRs that are closed and re-opened.

Thanks for the explanation. Would it be possible to only look for "test this please" after the last "Can one of the admins verify this patch?" comment? When a PR is reopened, "Can one of the admins verify this patch?" is posted by the bot user and there would be no "test this please" comment after it.

[ghost]

Could do this: Get the PR's events, e.g. https://api.github.com/repos/chenxiaolong/DualBootPatcher/issues/67/events. Ignore any comments older than the last reopened event.

[benpatterson]

I think solving this would involve looking over message, reading time stamps, etc. I think it's viable but I don't think we'll do it any time soon. Marking this as deferred for now.

[AdamBrousseau]

I think this is a dup of #518 so I'll copy my comment here since this issue is still open

We've experienced this as well. Reopening a PR that was previously tested re-triggers the build. The problem for us is that the new build has parameter ghprbCommentBody equal to the last comment in the PR and we parse the trigger comment for build inputs. Since the last comment is not necessarily the trigger comment, our build errors out because the comment is not what our build is expecting. If there could be an option added like "re-trigger on reopen" that would solve this for us. This is mostly an annoying issue of a random build being triggered and erroring out but it could be malicious if bad commits or PR comments were added by the PR author who doesn't have trigger permissions.