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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spam PRs can override the original PR's IPR status #172

Open
domenic opened this issue Feb 10, 2021 · 3 comments
Open

Spam PRs can override the original PR's IPR status #172

domenic opened this issue Feb 10, 2021 · 3 comments

Comments

@domenic
Copy link
Member

domenic commented Feb 10, 2021

whatwg/xhr#303 is a spam PR where someone forked whatwg/xhr, and then PRed from the annevk/response-fragments branch on their fork to the master/main branch on whatwg/xhr.

This overwrote the IPR status on the actual PR, whatwg/xhr#200.

This kind of makes sense in that IPR status is reported on a commit, so if it's the same commit hash, I can see how a conflict would arise.

This is similar to #33 but not quite the same.

Unfortunately the "Recent Deliveries" tab for the webhook does not have that spam PR (it's been too long). So I can't inspect the body to figure out a good way of detecting this. If something similar happens, post here and we can capture it. Or, we could reproduce the situation ourselves I suppose.

@domenic
Copy link
Member Author

domenic commented Feb 10, 2021

I investigated whether it's possible to report statuses on pull requests, instead of on commits. That does not seem possible; GitHub ties statuses very closely to commits.

I wonder if the IPR check should have succeeded anyway on the spam PR, since the commits were all authored by @annevk, not by the spammer? That is, we currently check body.pull_request.user.login, which might not be a good idea anyway: in theory it allows participant-agreement-signing people to "smuggle in" commits authored by those who have not signed the agreement.

@annevk
Copy link
Member

annevk commented Feb 10, 2021

Well, unless the commit is verified it can say whatever, right? So the PR author seems like a better source of truth as to who is making the contribution (I think squash will also attribute it to them, git-wise). And even with verified commits we wouldn't want to take a commit if that person didn't actually contribute it to us, I think, even if they signed the agreement.

@domenic
Copy link
Member Author

domenic commented Feb 10, 2021

Good point.

I guess this might be wontfix/unfixable, then. (Although I admit I haven't thought too hard about it.) After all, if you'd rebased and force-pushed to your branch it would have generated a new commit, which would pass the IPR checker.

Hmm, another interesting note: I think using /agreement-status would also let you fix the IPR check, even without rebasing. Which would have made that spam PR also pass the IPR check? Eek.

Maybe the IPR checker should just fail loudly if there are multiple PRs containing the commit? I dunno, probably not worth it.

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

No branches or pull requests

2 participants