Allow `request-ci` on the collaborator's PR without restriction of at least 1 approval

[jakecastelli]

IIRC collaborators are able to manually trigger CI through Jenkins without needing 1 approval on the PR, maybe it makes sense for the collaborators to use request-ci label on their own or other collaborators's PR without at least 1 approval restriction.

node-core-utils/lib/pr_checker.js

Line 560 in a68af4b

return !!this.getApprovedTipOfHead();

just an idea - could we check either the PR author is a current collaborator?

[aduh95]

I've open #920 to let Collaborators request CI for unapproved PRs. Worth noting that whatever change we make will likely decrease the security of our infrastructure, so it's a matter a finding a tradeoff between security and practicality, and since it seems no one has been asking for loosing up the security, I haven't landed that PR yet.

could we check either the PR author is a current collaborator?

FWIW I don't think we discriminate based on who is proposing the change, IMO it only makes sense to discriminate based on who is asking for the CI run.
I guess the question should also be "would it be easier for a malicious actor to trick a collaborator into giving them push access to their repo, or to post comments on their behalf?"; to post a comment, you'd have to steal a token, while push access to a collaborator fork can be given to any account (albeit that's a quite unlikely scenario).

[jakecastelli]

Thanks for explaining it in details 🙏 I thought our previous CI security issue was caused by malicious PR author pushes malicious changes in between request ci label was added and job being picked up.

FWIW I don't think we discriminate based on who is proposing the change

Definitely we shouldn't discriminate anyone who's proposing the change, what I meant was since we allow collaborators to run CI for unapproved commit through Jenkins UI, by using request ci label would be an easier approach - something like

return !!this.getApprovedTipOfHead() || this.isPRAuthorCollaborator()

[aduh95]

And I'm telling you that I don't think the author of the PR is a good metric, a collaborator could open a PR from a fork that some malicious actor has push access.

[jakecastelli]

Yeah that's a fair point 👍 unless we also check if the PR is opened from the same collaborator's fork, but I am seeing this is getting more complicated than what I've thought.

Happy to stick to the current flow.

[aduh95]

unless we also check if the PR is opened from the same collaborator's fork

To drive the point home, that would still not protect against that, the collaborator could have been tricked into giving push access to their fork – if someone asks me access to my fork, I might give them without thinking that would impact the security of Node.js.

[joyeecheung]

I think we can revisit this - for example, if the commit is signed by a collaborator, I don't think there's much risk? Unless the collaborator somehow gives their private key to someone else, which seems unlikely and if that happens, worse thing might already be possible..