[JENKINS-70931] Remove Prototype `Ajax.Request` usages from `hudson-behavior.js` #7951

timja · 2023-05-06T18:39:27Z

Testing done

FormChecker has been tested with the timezone field on the user configuration page.
On page load a doCheck with no attributes is done and works, and then when selecting other timezones it also works

Help fetching has been tested by putting a breakpoint on the code and then clicking help buttons, breakpoint was hit and help areas were loaded.

Lazy map was tested by going to the build time trend and seeing the breakpoint hit on page load and the page looked correct.

Stop button by adding a breakpoint on the code, starting a freestyle build, ensuring the breakpoint was hit and that it behaved as expected.

descriptionForm by editing a freestyle project description, putting a breakpoint ensuring the code was executed behaving as expected

refreshPart was tested by adding a breakpoint and making sure it was hit when executors updated.

validateButton was tested by clicking test connection on the slack plugins global configuration, ensuring breakpoint was hit and it behaved as expected

Proposed changelog entries

(skip)

Proposed upgrade guidelines

N/A

Submitter checklist

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
- Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Maintainer checklist

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

There are at least two (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 is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see 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).

basil

Fine with me pending the one issue I opened.

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

basil

Fine but there are still a lot of checkMethod="post" usages in plugins (including proprietary plugins) so maybe we should add a developer changelog entry and/or log a warning instructing the plugin developer to remove the dead code.

…or.js Co-authored-by: Basil Crow <me@basilcrow.com>

yaroslavafenkin

Looks fine to me security wise

yaroslavafenkin · 2023-05-08T08:57:29Z

test/src/test/java/hudson/tools/ZipExtractionInstallerTest.java

@@ -94,89 +77,4 @@ public void onlyAdminCanReachTheDoCheck() throws Exception {
        userWc.login(USER);
        assertEquals(HttpURLConnection.HTTP_FORBIDDEN, userWc.getPage(request).getWebResponse().getStatusCode());
    }
-
-    @Test
-    @Issue("SECURITY-794")


Why is this removed? Not relevant anymore?

It was a very hacky test relying on internals of htmlunit which changed when switching to fetch I wasn’t able to replicate it and it didn’t seem like a critical test

timja · 2023-05-08T13:00:23Z

/label ready-for-merge

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

Thanks!

Wadeck · 2023-05-08T13:18:40Z

log a warning instructing the plugin developer to remove the dead code

Yeah that could be pretty useful.

Also, have you looked for plugins expecting to have checkMethod="get" being supported? It's a breaking change AFAICT.
Quick search seems to provide 0 results.

timja · 2023-05-08T13:52:00Z

I have checked, it’s in a comment here and like you I found no results

timja · 2023-05-09T21:48:21Z

Breaks plugins.MatrixAuthPluginTest and plugins.ScriptSecurityPluginTest in ATH.

Fixed

basil · 2023-05-09T21:50:37Z

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

@@ -228,50 +228,73 @@ var FormChecker = {
   * @param url
   *      Remote doXYZ URL that performs the check. Query string should include the field value.
   * @param method
-   *      HTTP method. GET or POST. I haven't confirmed specifics, but some browsers seem to cache GET requests.
+   *     Unused, kept to maintain compatibility with the old signature.


matrix-auth and azure-ad call this with "GET". This changes the existing behavior. Not sure there is a valid justification for that.

If there is a valid justification for changing the existing behavior, then this calling convention is pretty ugly. Better to define a new two-argument method and deprecate the three-argument one, logging a warning if anyone is still calling the three-argument version and filing PRs to update consumers.

JavaScript doesn't have function overloading.
https://stackoverflow.com/questions/456177/function-overloading-in-javascript-best-practices

We're stuck with that function with that name.
We could create a new function with a new name and use an object for parameters to make it more flexible in future

Still not sure there is a valid justification for changing the existing behavior.

core 2.285 changed the default to post, these 2 would have been missed by accident as they will be the only consumers calling it directly.

anyway I've added some compat so they will be able to set GET for now and will file separate PRs to switch them to post if that's okay?

basil

This PR contains two unrelated changes and is too difficult for me to review. Requesting any changes related to the deprecation of checkMethod to be split into a separate PR.

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

basil · 2023-05-10T06:44:24Z

Still failing ATH as far as I can tell.

changes have been made

basil · 2023-05-10T21:50:58Z

Code looks good. Once this gets a passing ATH run (or at least one that is no worse than trunk builds) I will be ready to approve this.

basil · 2023-05-11T03:58:30Z

ATH passed so this is good to go from my perspective.

basil

Thanks for your hard work cleaning up this patch and getting ATH to pass!

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

daniel-beck · 2023-05-12T08:53:28Z

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

      var idx = url.indexOf("?");
      params.parameters = url.substring(idx + 1);
      url = url.substring(0, idx);
    }
-    new Ajax.Request(url, params);
+
+    const parsedUrl = method === "get" ? `${url}?${params.parameters}` : url;


This is a breaking change when the request method is GET, as params.parameters is undefined.

matrix-auth:

(The ] is from the internal format of form validation requested in the plugin. The original url looks like /jenkins/manage/descriptorByName/hudson.security.ProjectMatrixAuthorizationStrategy/checkName?value=%5BUSER%3AtheUsername%5D)

balakine · 2023-07-31T07:32:27Z

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

@@ -242,36 +242,65 @@ var FormChecker = {
  },

  sendRequest: function (url, params) {


I realize I'm late to the party, but are we cool with breaking this function? It's used in plugins and the response parameter in onComplete having no responseText is a bit of a surprise to all the scripts.

Could you clarify what's being broken? What scripts?

Here is where I noticed it
https://github.com/jenkinsci/global-build-stats-plugin/blob/master/src/main/webapp/scripts/global-build-stats/standardFunctions.js#L39

JENKINS-71741 FTR

timja force-pushed the JENKINS-70931-hudson-behavior branch 2 times, most recently from df81a05 to 8eae594 Compare May 6, 2023 18:50

timja added the skip-changelog Should not be shown in the changelog label May 6, 2023

timja requested review from basil and a team May 6, 2023 19:01

timja marked this pull request as ready for review May 6, 2023 19:01

timja added the needs-security-review Awaiting review by a security team member label May 7, 2023

timja requested a review from a team May 7, 2023 07:38

basil self-assigned this May 7, 2023

basil changed the title ~~JENKINS-70931 Remove Prototype Ajax.Request usages from hudson-behavior.js~~ [JENKINS-70931] Remove Prototype Ajax.Request usages from hudson-behavior.js May 7, 2023

basil approved these changes May 7, 2023

View reviewed changes

war/src/main/webapp/scripts/hudson-behavior.js Outdated Show resolved Hide resolved

timja requested a review from basil May 7, 2023 18:38

basil previously approved these changes May 7, 2023

View reviewed changes

timja added developer Changes which impact plugin developers and removed skip-changelog Should not be shown in the changelog labels May 7, 2023

timja and others added 3 commits May 7, 2023 21:54

JENKINS-70931 Remove Prototype Ajax.Request usages from hudson-behavi…

b3384bb

…or.js Co-authored-by: Basil Crow <me@basilcrow.com>

Remove hacky test that doesn't work with fetch

d8fd7f9

Remove unused checkMethod

3c890bd

timja force-pushed the JENKINS-70931-hudson-behavior branch from ed2df1c to 3c890bd Compare May 7, 2023 21:31

NotMyFault approved these changes May 8, 2023

View reviewed changes

yaroslavafenkin reviewed May 8, 2023

View reviewed changes

yaroslavafenkin added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels May 8, 2023

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 8, 2023

Merge branch 'master' into JENKINS-70931-hudson-behavior

0708202

basil added a commit to basil/acceptance-test-harness that referenced this pull request May 8, 2023

Testing jenkinsci/jenkins#7951

5e6c924

basil added a commit to basil/bom that referenced this pull request May 8, 2023

Testing jenkinsci/jenkins#7951

8a61d85

timja added 2 commits May 9, 2023 22:29

debug

2236c4e

Fix compat with matrix auth plugin

4d0e7a7

timja requested a review from basil May 9, 2023 21:48

basil reviewed May 9, 2023

View reviewed changes

Restore support for setting checkMethod

fe4c9bd

basil previously requested changes May 9, 2023

View reviewed changes

Revert checkMethod changes

008c3d2

basil reviewed May 9, 2023

View reviewed changes

war/src/main/webapp/scripts/hudson-behavior.js Show resolved Hide resolved

Pass through method

f6f1438

timja added skip-changelog Should not be shown in the changelog and removed developer Changes which impact plugin developers labels May 10, 2023

basil added 2 commits May 9, 2023 21:39

fix lint

b11e908

Merge branch 'master' into JENKINS-70931-hudson-behavior

c0cc3ca

basil added a commit to basil/acceptance-test-harness that referenced this pull request May 10, 2023

Testing jenkinsci/jenkins#7951

a6dcc7e

Fix matrix-auth plugin

597aeb0

Merge branch 'master' into JENKINS-70931-hudson-behavior

4ab1b14

basil added a commit to basil/acceptance-test-harness that referenced this pull request May 11, 2023

Testing jenkinsci/jenkins#7951

98733ef

basil approved these changes May 11, 2023

View reviewed changes

basil added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 11, 2023

basil merged commit df1ef51 into jenkinsci:master May 11, 2023
16 checks passed

daniel-beck reviewed May 12, 2023

View reviewed changes

timja deleted the JENKINS-70931-hudson-behavior branch May 12, 2023 08:54

daniel-beck mentioned this pull request May 12, 2023

[JENKINS-70906] Compatibility with Prototype removal jenkinsci/matrix-auth-plugin#140

Merged

basil mentioned this pull request Jun 28, 2023

refactor: Do not rely on prototype.js jenkinsci/git-parameter-plugin#98

Merged

balakine reviewed Jul 31, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-70931] Remove Prototype `Ajax.Request` usages from `hudson-behavior.js` #7951

[JENKINS-70931] Remove Prototype `Ajax.Request` usages from `hudson-behavior.js` #7951

timja commented May 6, 2023 •

edited

basil left a comment

basil left a comment

yaroslavafenkin left a comment

yaroslavafenkin May 8, 2023

timja May 8, 2023

timja commented May 8, 2023

Wadeck commented May 8, 2023

timja commented May 8, 2023

timja commented May 9, 2023

basil May 9, 2023

timja May 9, 2023

basil May 9, 2023

timja May 9, 2023

basil left a comment

basil commented May 10, 2023

basil commented May 10, 2023

basil commented May 11, 2023

basil left a comment

daniel-beck May 12, 2023 •

edited

timja May 12, 2023

balakine Jul 31, 2023

daniel-beck Jul 31, 2023

balakine Jul 31, 2023

basil Jul 31, 2023

		@@ -242,36 +242,65 @@ var FormChecker = {
		},

		sendRequest: function (url, params) {

[JENKINS-70931] Remove Prototype Ajax.Request usages from hudson-behavior.js #7951

[JENKINS-70931] Remove Prototype Ajax.Request usages from hudson-behavior.js #7951

Conversation

timja commented May 6, 2023 • edited

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

basil left a comment

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

yaroslavafenkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja commented May 8, 2023

Wadeck commented May 8, 2023

timja commented May 8, 2023

timja commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

basil commented May 10, 2023

basil commented May 10, 2023

basil commented May 11, 2023

basil left a comment

Choose a reason for hiding this comment

daniel-beck May 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[JENKINS-70931] Remove Prototype `Ajax.Request` usages from `hudson-behavior.js` #7951

[JENKINS-70931] Remove Prototype `Ajax.Request` usages from `hudson-behavior.js` #7951

timja commented May 6, 2023 •

edited

daniel-beck May 12, 2023 •

edited