Default to POST form validation

[daniel-beck]

I wonder whether this is worth an escape hatch? I don't think so given how frequent POST requests for these already are. Thoughts?

Proposed changelog entries

• Switch to sending POST requests by default for form validation URLs

Proposed upgrade guidelines

N/A

Submitter checklist

• (If applicable) Jira issue is well described
• Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
  • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
• Appropriate autotests or explanation to why this change has no tests
• For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

• There are at least 2 approvals for the pull request and no outstanding requests for change
• Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
• Changelog entries in the PR title and/or Proposed changelog entries are correct
• Proper changelog labels are set so that the changelog can be generated automatically
• If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[fqueiruga]

One question, is there any risk to break plugins with this one? For example, if plugins perform a request to their own defined endpoints and depend on the default HTTP method.

[jglick]

In principle some plugin could have added a @GET annotation to a doCheckXXX method and thus be broken by this. I am not sure why anyone would have bothered, though, as this would not have been solving any bug; normally people add @POST to defend against CSRF attacks, but there is little reason to add @GET except as a matter of documentation.

[sladyn98]

LGTM 👍

[varyvol]

@daniel-beck this has several approvals but it's marked as WiP and on hold? Does it need further work?

[oleg-nenashev]

Pull request template should be followed. This security hardening (IIUC) should definitely be present in the changelog and, if there is a risk of regressions, in the upgrade guidelines

[daniel-beck]

security hardening

Nope, as the endpoints would still handle GET requests.

[uhafner]

combobox.jelly is missing a checkMethod attribute, is this intended?

Otherwise the PR looks good!

[daniel-beck]

@uhafner If it's supported internally, it's just a case of missing Jelly doc. Wouldn't be the first time. Will check when I have some time to work on this.

[daniel-beck]

@uhafner Thanks for the suggestion. It was in fact just missing Jelly doc, which I added. (Tested via UI Samples Plugin)

[daniel-beck]

@oleg-nenashev

Pull request template should be followed.

Done, thanks for the explicit reminder. I removed it as this was really just for a PR build, as the comment stated. Now I think I'm feeling better about merging this even without all the pieces in place.

addressed

[timja]

@daniel-beck if you want this in then can you remove on-hold please?

[daniel-beck]

@timja Thanks for the reminder. I don't want this in 2.282, perhaps 2.283 just in case I forgot something.

[timja]

@daniel-beck do you want this in soon?

[daniel-beck]

@timja Thanks for the reminder. I removed the label.

Thanks for the reviews everyone!

[timja]

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

[miglen]

In principle some plugin could have added a @GET annotation to a doCheckXXX method and thus be broken by this. I am not sure why anyone would have bothered, though, as this would not have been solving any bug; normally people add @POST to defend against CSRF attacks, but there is little reason to add @GET except as a matter of documentation.

You had actually predicted the future. I personally don't see much value here as those checks should not contain any sensitive information or configurable settings and only be used to validate things.

There is the related issue: https://issues.jenkins.io/browse/JENKINS-65790

[sabberworm]

You had actually predicted the future. I personally don't see much value here as those checks should not contain any sensitive information or configurable settings and only be used to validate things.

There is the related issue: https://issues.jenkins.io/browse/JENKINS-65790

Maybe the solution isn’t to roll back this change but to look at the status code of the response and handle non-200 responses differently (e.g. by not blindly inserting the response into the DOM as HTML)?

[daniel-beck]

look at the status code of the response and handle non-200 responses differently (e.g. by not blindly inserting the response into the DOM as HTML

I attempted to do that in #5333 but based on the bug it isn't effective here.

[sabberworm]

Debugging a bit, it seems here’s the part where the HTML is inserted:

jenkins/war/src/main/webapp/scripts/hudson-behavior.js

Line 2929 in 2f612fc

+ id + '" style="display:none">' + rsp.responseText + '</div>';

Interestingly, the code already checks the response status but then decides to parse and add the HTML anyway, just hidden (with a toggle to let the user see it). Which won’t prevent the style tags from being evaluated…

[daniel-beck]

@sabberworm Nice find, could you submit a PR to fix this?

[sabberworm]

@sabberworm Nice find, could you submit a PR to fix this?

Done, see #5601