New Injected Test: Require a Stapler verb annotation on web method looking methods

[daniel-beck]

Related PR: jenkins-infra/jenkins.io#2291

This addition to InjectedTest will ensure all explicit or implied Stapler web methods declare an HTTP verb that they're to be used with. Similar to escape-by-default in Jelly, which is also only enforced by InjectedTests (and in a way the recent behavior reversal since 2.138.2).

@RequirePOST has been around forever, while @POST and the other "verb" annotations were available since Jenkins 1.651. I think this 3y 3m old release is a more than reasonable baseline (once there's a non-TypedFilterimplementation for Jenkins before 2.138.4).

The current releases of matrix-auth would behave as follows with this change:

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   PluginAutomaticTestBuilder$OtherTests.testStaplerDispatches:155 There should be no web methods that lack HTTP verb annotations like @RequirePOST, @GET, @POST, etc. -- see https://jenkins.io/redirect/developer/csrf-protection
Expected: is an empty collection
     but: <[hudson.security.GlobalMatrixAuthorizationStrategy$DescriptorImpl#doCheckName, org.jenkinsci.plugins.matrixauth.AuthorizationContainerDescriptor#doCheckName_, org.jenkinsci.plugins.matrixauth.AuthorizationMatrixNodeProperty$DescriptorImpl#doCheckName]>
[INFO] 
[ERROR] Tests run: 54, Failures: 1, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

Unsure whether all the rules are 100% matching what Stapler does, but it should be close enough.

TODO:

• Add implementation for core baselines before 2.138.4 and TypedFilter
• It doesn't seem possible to annotate methods such that they take multiple verbs. Some web methods in core support that (showing a UI on GET, performing an action on POST), so there's no straightforward migration path there. Allow suppressing this check somehow in those cases.

[Wadeck]

there's no straightforward migration path there

We can split the method into two, annotate one with @GET and the second with @POST. One of the difference with @RequirePOST, the two previous ones do not fail the request if the method is incorrect.

[Wadeck]

The code is promising. That will cause lots of failures for most of the plugins, as the @GET is really rarely used. Do you plan to add a blog post explaining the reason of this hardening?

[daniel-beck]

We can split the method into two

Well, yeah, but that's not exactly straightforward IMO, especially if faces with a few dozen of these in some plugins. The code needs to change nontrivially. We probably need @WebMethod too, or achieve different method signatures through creative argument declarations.

Do you plan to add a blog post explaining the reason of this hardening?

Yes, plus a better redirect target. It should take a while for this to get picked up by plugins anyway.

---

A potential negative side effect of this would be that we cannot change form validation request methods at will anymore, without breaking lots of plugins. Looking at you, jenkinsci/jenkins@09d6046.

[jtnord]

I would rather other things did not go into InjectedTest (and that it would die). The reason being is that skipping part of the tests (rather than all of them) is pretty much impossible as it uses old junit Suites rather than newer test classes, and is hard to debug.
This would be much better going forward in a generate-tests mojo in maven-hpi-plugin which then also makes it much easier to debug in an IDE. but otherwise 👍

[jglick]

skipping part of the tests (rather than all of them) is pretty much impossible

Well, you should not be skipping any of them. :-)

I am not sure how creating JUnit 4-style tests would make it easier to ignore some test cases; target/generated-test-sources/whatever/InjectedTest.java would still not be editable.

Note that there is a reason all the tests go into one suite: so that only one Jenkins startup is needed, to minimize overhead.

At any rate, InjectedTest is a well-established part of Jenkins plugin development. Proposals to replace its structure should be kept separate. So long as this PR does not introduce a test which is going to fail widely and spuriously (which I have not yet reviewed), it should be fine IMO.

[jvz]

Use of assume from JUnit might be appropriate here for better integration with error reporting.

[oleg-nenashev]

Needs a merge conflict fix + clarity on TODO items in the PR description

[daniel-beck]

PR build fails because of #167 (comment)

[jglick]

This would be better put on hold until jenkinsci/jenkins#4623 is merged, or exclude doCheckXXX methods, or exclude them unless the Jenkins version indicates that jenkinsci/jenkins#4623 is included.

[daniel-beck]

This would be better put on hold until jenkinsci/jenkins#4623 is merged

Unsure. Would be beneficial, but IMO overall doesn't make a big difference if the docs say:

• Use @POST for any doCheck* and add checkMethod="post" or your plugin will break once Default to POST form validation jenkins#4623 is merged
• Use @POST for any doCheck* and add checkMethod="post" or your plugin will break on Jenkins 2.2xy and newer

The advice needs to favor @POST (or @RequirePOST) for most endpoints anyway; only few, such as doWs, doArtifact, doIndex should be annotated @GET.

[jglick]

Agreed, it is fine so long as the docs guide users to use @POST rather than the HTTP method currently being sent.

[daniel-beck]

I still need to figure some stuff out.