jenkins icon indicating copy to clipboard operation
jenkins copied to clipboard

[JENKINS-60866][JENKINS-71513] Apply Stapler update for CSP-compliant st:bind and renderOnDemand

Open daniel-beck opened this issue 2 years ago • 11 comments

See JENKINS-60866.

Downstream of https://github.com/jenkinsci/stapler/pull/385

This takes care of two different previously inline or eval'ed JS snippets that ended up calling makeStaplerProxy to bind objects to JS:

  • st:bind emitted a script tag with inline script that assigns the result of makeStaplerProxy to a variable (if used with a variable name -- the other mode cannot be fixed, as it's intended for nesting in an inline script).
  • renderOnDemand.jelly added the makeStaplerProxy call it needed to an HTML attribute, later eval'ing it in https://github.com/jenkinsci/jenkins/blob/d977ee8741ed6362dac282e69a317a070ee9d813/war/src/main/webapp/scripts/hudson-behavior.js#L793

This PR changes Stapler and Jenkins to no longer need either:

  • st:bind now emits script tags with src. Stapler has been improved to have a separate URL that provides the previously inline script standalone.
  • renderOnDemand now adds HTML attributes for the function name and its argument values, which are then read in hudson-behavior.js and used to invoke the specified method with the correct arguments.

Testing notes

  • st:bind is used in core by progressiveRendering, which is used by the "Build History" view.
  • renderOnDemand is used whenever a form has nontrivial dynamic content (e.g. adding build steps to a freestyle job, or the configuration of a security realm, authorization strategy, or markup formatter after selecting the type in a dropdown box.

Proposed changelog entries

  • Developer: Update Stapler from 1822.v120278426e1c to 1839.ved17667b_a_eb_5 to no longer generate line JavaScript with Stapler bound objects to improve compatibility with Content-Security-Policy Plugin

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) and are in the imperative mood. 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
  • [ ] New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • [ ] New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • [ ] 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 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, 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).

daniel-beck avatar Jul 16 '22 21:07 daniel-beck

Probably need to do renderOnDemand.jelly at the same time as these are closely related.

daniel-beck avatar Jul 16 '22 21:07 daniel-beck

Looks like the default branch of Stapler with a bunch of unreleased commits (for incrementals) is incompatible with core. Nice.

[2022-07-17T10:08:49.977Z] [INFO] --- maven-enforcer-plugin:3.1.0:enforce (display-info) @ jenkins-war ---
[2022-07-17T10:08:49.977Z] [INFO] Adding ignore: module-info
[2022-07-17T10:08:49.977Z] [INFO] Restricted to JDK 8 yet org.kohsuke.stapler:stapler:jar:1721.v6298feb_6eb_8f:compile contains org/kohsuke/stapler/framework/io/ByteBuffer$1.class targeted to JDK 11
[2022-07-17T10:08:49.977Z] [INFO] Restricted to JDK 8 yet org.kohsuke.stapler:stapler-groovy:jar:1721.v6298feb_6eb_8f:compile contains org/kohsuke/stapler/jelly/groovy/GroovyClassLoaderTearOff$1.class targeted to JDK 11
[2022-07-17T10:08:49.977Z] [INFO] Restricted to JDK 8 yet org.jenkins-ci:commons-jelly:jar:1.1-jenkins-20220630:compile contains org/apache/commons/jelly/util/TagUtils.class targeted to JDK 11
[2022-07-17T10:08:49.977Z] [INFO] Restricted to JDK 8 yet org.kohsuke.stapler:stapler-jelly:jar:1721.v6298feb_6eb_8f:compile contains org/kohsuke/stapler/jelly/package-info.class targeted to JDK 11
[2022-07-17T10:08:49.977Z] [ERROR] Rule 1: org.apache.maven.plugins.enforcer.EnforceBytecodeVersion failed with message:
[2022-07-17T10:08:49.977Z] Found Banned Dependency: org.kohsuke.stapler:stapler:jar:1721.v6298feb_6eb_8f
[2022-07-17T10:08:49.977Z] Found Banned Dependency: org.kohsuke.stapler:stapler-groovy:jar:1721.v6298feb_6eb_8f
[2022-07-17T10:08:49.977Z] Found Banned Dependency: org.jenkins-ci:commons-jelly:jar:1.1-jenkins-20220630
[2022-07-17T10:08:49.977Z] Found Banned Dependency: org.kohsuke.stapler:stapler-jelly:jar:1721.v6298feb_6eb_8f

daniel-beck avatar Jul 17 '22 10:07 daniel-beck

Looks like the default branch of Stapler with a bunch of unreleased commits (for incrementals) is incompatible with core. Nice.

@daniel-beck Why would it be “nice”? Your sarcasm is not appreciated. The corresponding changes have been ready for review in https://github.com/jenkinsci/jenkins/pull/6801 for 9 days.

basil avatar Jul 17 '22 15:07 basil

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Aug 02 '22 18:08 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Aug 10 '22 16:08 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Sep 07 '22 15:09 github-actions[bot]

Back to draft per https://github.com/jenkinsci/jenkins/pull/6865#discussion_r1293718459

daniel-beck avatar Aug 21 '23 17:08 daniel-beck

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Sep 11 '23 17:09 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Sep 26 '23 07:09 github-actions[bot]

https://github.com/jenkinsci/build-monitor-plugin/pull/830 demonstrates how one can adapt a plugin that currently assigns to a variable with a name that is rejected.

daniel-beck avatar Sep 28 '23 10:09 daniel-beck

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Oct 11 '23 18:10 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Nov 01 '23 05:11 github-actions[bot]

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions[bot] avatar Jan 01 '24 07:01 github-actions[bot]

I plan to merge the upstream change and finalize the bom/pom.xml here shortly. Please review this PR if you're interested in it :)

daniel-beck avatar Feb 12 '24 21:02 daniel-beck

/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!

timja avatar Feb 20 '24 22:02 timja