[SECURITY-3033] Submit POST requests as needed

[daniel-beck]

This is an attempt to resolve SECURITY-3033.

It's a lot more complex than the usual solution of adding @POST or @RequirePOST to a web method and calling it a day.

First, the sidebar link directly intiates the action, so this needed a task.jelly for that that sets the <l:task post="true"/> attribute.

The next problem is that RebuildAction#doIndex does everything (meaning, triggering a build or redirecting to the form to set parameters), depending on the configuration. Adding @RequirePOST or @POST would secure the part that triggers a build, but <l:task post="true" …> doesn't redirect to the Location returned from the response, so if a form needs to be filled, that would never happen if post="true" is set unconditionally.

Additionally, #getUrlName cannot end in a slash, but an <l:task href="${action.urlName}" post="true" … /> doesn't work, as the Stapler-internal forward from …/rebuild to …/rebuild/ loses the HTTP POST method. So introduce #getTaskUrl and link there instead.

So the task.jelly needed a way to determine whether to send a POST request or not, beforehand. This was straightforward enough for the regular RebuildAction (for builds), but the job-scoped RebuildLastCompletedBuildAction could not call into RebuildAction after looking it up on the build, since everything was operating on Stapler ancestors -- and there's no build ancestor when on the project page.

So, some cleanup: RebuildAction is no longer a superclass of RebuildLastCompletedBuildAction, both now share the common ancestor AbstractRebuildAction, which provides the new task.jelly shared between the two. Make RebuildAction a RunAction2 (for backwards compatibility when RebuildAction was persisted with builds, newly removed here) and provide the Run to it. Remove all code from Rebuilder since everything is transient now; instead have RebuildActionFactory (no longer a TransientBuildActionFactory) check whether a persisted action already exists before attaching a transient one.

This PR also deletes a bunch of code that appears long obsolete. I urge maintainers tempted to merge this to double-check; I am unaware why this code has been kept around for many years and there may be a good reason to have done this.

Jobs without builds now no longer allow "Rebuild Last"), because that seemed weird to work in the first place.

Testing done

Configured three freestyle projects (no params, params, and params with auto-rebuild) and clicked the actions. They seemed about correct.

I have tested with the persisted action a bit (rebuild of historical builds both with and without parameters, checking there's no second action), but not extensively.

Additionally, if someone else wants to take this on, I strongly recommend adding tests with historical build records with persisted RebuildAction, as well as adding tests confirming the effectiveness of the security fix (basically, what the tests kind of did until they were adapted, except with assertThrows so they pass). Any obscure undocumented use cases (e.g., "expect reasonable outcome when someone navigates to …/rebuild directly and a parameters page should show up") will need to have tests added as well.

Submitter checklist

Preview Give feedback

1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
2. Ensure that the pull request title represents the desired changelog entry
3. Please describe what you did
4. Link to relevant issues in GitHub or Jira
5. Link to relevant pull requests, esp. upstream and downstream changes
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

[asafnahshon]

Maintainers are gone, eh? what should we do then?

Do you need more manual testing?

[morganchristiansson]

Last PR that was merged has this comment

CC @GLundh

[daniel-beck]

Note that in jenkins-infra/update-center2@fcb1ae7#r121610706 the maintainer informed us they have no time to maintain the plugin anymore.

As a user, you could download and install the CI build ("incremental") to address the issue (note that the warning shown on the UI needs to be dismissed manually).

As a Java developer, consider becoming a maintainer of this plugin. I am not a maintainer of this plugin, nor do I want to become one.

[asafnahshon]

Downloaded and installed the "incremental" build as Daniel suggested, works fine in production, thank you.

[Evan-Bluhm]

We've also been using this for a while now (thank you @daniel-beck for the SECURITY-3033 fix!!), and I think the new AbstractRebuildAction action.jelly introduces a minor bug that isn't present in rebuilder 320.v5a_0933a_e7d61. The RebuildAction works great if clicked from the same pane as the AbstractBuild, but it gets the URL wrong if clicked from within a different action, such as hudson.model.ParametersAction.

For example, if I have a completed parameterized build /job/TestJob/1 and navigate to /job/TestJob/1/parameters (or just click the Parameters action in the sidebar /job/TestJob/1), then ParametersAction will render the RebuildAction with a taskUrl of ".../job/TestJob/1/parameters/rebuild/parameterized", which is not a valid URL and should be ".../job/TestJob/1/rebuild/parameterized"

I'm not very familiar with modern Jenkins code, so it's not clear to me whether this is a problem with the href provided by the RebuildAction, or if that is the expected behavior when the jelly for one action gets rendered by a different one (as in the st:include page="sidepanel.jelly" in ParametersAction). In any case, the end result is that the Rebuild button doesn't work as intended, so I'll see if I can construct a representative testcase.

[daniel-beck]

Oversight when I wrote the custom action.jelly, should be fixed now. (FTR I'm unable to do what t:actions does and use Functions#getActionUrl, because not using Action#getUrlName is the entire point of the custom action.jelly.)

[lmgray]

do we want to link https://issues.jenkins.io/browse/JENKINS-71932 and assign to you @daniel-beck ?

[daniel-beck]

I'm not sure filing an issue for an open PR makes sense. Who's the audience of that? As I wrote further up, I'm not interested in maintaining this plugin, so this PR should be considered unsupported, and I'm certainly not keeping an eye on newly reported issues; and any maintainers aren't responsible for my mistakes (at least until they merge them).

[lmgray]

I'm not sure filing an issue for an open PR makes sense. Who's the audience of that? As I wrote further up, I'm not interested in maintaining this plugin, so this PR should be considered unsupported, and I'm certainly not keeping an eye on newly reported issues; and any maintainers aren't responsible for my mistakes (at least until they merge them).

just thinking...  I created https://issues.jenkins.io/browse/JENKINS-71932 not realizing it was related to this change -- and now it's fixed so just wondering best way to mark https://issues.jenkins.io/browse/JENKINS-71932 resolved?

[fengerzh]

@GLundh Could you please take a look at this PR? I know it's rude to ask someone to maintain an open source PR, I am very sorry. But this PR is really very important, as it blocks all further upgrade and keeps warn about security issue. If this PR could be approved and merged into master, it will solve many issues.

[Sudarshan-TN]

@GLundh Could you please take a look at this PR? I know it's rude to ask someone to maintain an open source PR, I am very sorry. But this PR is really very important, as it blocks all further upgrade and keeps warn about security issue. If this PR could be approved and merged into master, it will solve many issues.

Can someone please review this PR...

[GLundh]

LGTM.

[morganchristiansson]

Now we need to push out a new release..

[emericcolombe]

Hi guys, huge thanks for this awesome work ! @GLundh do you know when a new release will be available so the plugin can get rid of its CSRF vulnerability tag ?
I'd be happy to help if necessary

[GLundh]

Not sure how automated releases work. I did not carry out the last release: #136
If you want to maintain this plugin, I'll be happy to add you to the maintainers list.

[emericcolombe]

I'm afraid I'm not really familiar with the automated release system either.. @timja could you enlighten us with your knowledge on what to do to create a release ? Thanks a lot

[an-nikolaev]

Seems that there should be label on PR, from interesting categories

[kutzi]

Apparently, the current version of the plugin is still being listed as being affected by the issue fixed in this PR

[lemeurherve]

Apparently, the current version of the plugin is still being listed as being affected by the issue fixed in this PR

The warning comes from https://github.com/jenkins-infra/update-center2/blob/ed6bf6344f5b172864b4cceea788fc169bdb12f8/resources/warnings.json#L16396-L16408

To remove it, a pull request like https://github.com/jenkins-infra/update-center2/pulls?q=is%3Apr+%22remove+warning%22 has to be opened, approved and merged.

[emericcolombe]

I created the pull request
jenkins-infra/update-center2#752

[sghill]

Hi folks, I opened #158 that adds a null check after getting a NPE when upgrading to this version. In our case we needed to pin the plugin back to v320 to move forward.