Test-infra: ok-to-test should be reverted for new commits

[suleymanakbas91]

Currently, once someone from the trusted org comments ok-to-test to a PR, any upcoming commit will trigger the jobs. This could lead to security issues if the commits after ok-to-test comment introduce malicious code.

[nikhita]

/cc @matthyx

[matthyx]

Ok, sorry I was confused reading that on my phone.
This is not a bug, by design /ok-to-test is a one time command needed to allow tests for non-org members.
I don't see the benefit of blocking all tests after each new commit, this will lead to a lot of slow down, but maybe @cjwagner has another opinion?

[suleymanakbas91]

I know it'd be awfully inconvenient, but otherwise this would stay as a security vulnerability. I think this is a trade-off Prow admins should make between security and convenience, so how about making it configurable?

[cjwagner]

This is not a security vulnerability this is the intended behavior. /ok-to-test is designed to mark a PR as trusted and "ok to test" at any point in the future. We have a separate mechanism for triggering tests once, the /test all command. For convenience /lgtm also behaves like /test all on non-collaborator PRs.

Providing a configuration option to completely disable the /ok-to-test command for some or all repos might make sense though. That would restrict testing to collaborator PRs or one off test runs triggered by collaborators on non-collaborator PRs with /test *.

[BenTheElder]

Edit: somehow I skimmed right past this in @cjwagner's comment, sorry.

~if you only want to test the current commit you can comment /test all as a trusted commentor to test things once, instead of commenting /ok-to-test.~ (see above, this was already mentioned)

[BenTheElder]

It is definitely a trade-off, and I think we'd welcome a PR to make it configurable. As Cole mentioned a work-around for now is to use /test all instead.

[BenTheElder]

/help

[matthyx]

/assign

[matthyx]

/remove-help