Skip to content

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

Merged
NotMyFault merged 24 commits into
jenkinsci:masterfrom
daniel-beck:csp-bind
Feb 24, 2024
Merged

[JENKINS-60866][JENKINS-71513] Apply Stapler update for CSP-compliant st:bind and renderOnDemand#6865
NotMyFault merged 24 commits into
jenkinsci:masterfrom
daniel-beck:csp-bind

Conversation

@daniel-beck

@daniel-beck daniel-beck commented Jul 16, 2022

Copy link
Copy Markdown
Member

See JENKINS-60866.

Downstream of jenkinsci/stapler#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
    var proxy = eval(e.getAttribute("proxy"));

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
daniel-beck requested a review from Wadeck July 16, 2022 21:45
@daniel-beck

This comment was marked as outdated.

@daniel-beck daniel-beck added the work-in-progress The PR is under active development, not ready to the final review label Jul 16, 2022
@daniel-beck daniel-beck changed the title [JENKINS-60866] Apply Stapler update for CSP-compliant st:bind [JENKINS-60866] Apply Stapler update for CSP-compliant st:bind and renderOnDemand Jul 17, 2022
@daniel-beck

daniel-beck commented Jul 17, 2022

Copy link
Copy Markdown
Member Author

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

@basil

basil commented Jul 17, 2022

Copy link
Copy Markdown
Member

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 #6801 for 9 days.

@github-actions github-actions Bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 3, 2022
@github-actions github-actions Bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 10, 2022
@github-actions github-actions Bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 26, 2022
@basil basil added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Aug 26, 2022
@github-actions github-actions Bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 7, 2022
@daniel-beck daniel-beck changed the title [JENKINS-60866] Apply Stapler update for CSP-compliant st:bind and renderOnDemand [JENKINS-60866][JENKINS-71491] Apply Stapler update for CSP-compliant st:bind and renderOnDemand Aug 11, 2023
@github-actions github-actions Bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 11, 2023
@daniel-beck
daniel-beck marked this pull request as ready for review August 14, 2023 10:00
@daniel-beck daniel-beck added the needs-security-review Awaiting review by a security team member label Aug 14, 2023
Comment thread core/src/main/java/hudson/Functions.java
Comment thread core/src/main/java/hudson/Functions.java
Comment thread core/src/main/java/hudson/Functions.java Outdated
Comment thread war/src/main/webapp/scripts/hudson-behavior.js Outdated
@github-actions github-actions Bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Nov 1, 2023
@github-actions

github-actions Bot commented Nov 1, 2023

Copy link
Copy Markdown
Contributor

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

@github-actions github-actions Bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Nov 1, 2023
@github-actions github-actions Bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jan 1, 2024
@github-actions

github-actions Bot commented Jan 1, 2024

Copy link
Copy Markdown
Contributor

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

@github-actions github-actions Bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Feb 12, 2024
@daniel-beck
daniel-beck marked this pull request as ready for review February 12, 2024 20:59
@daniel-beck daniel-beck added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Feb 12, 2024
@daniel-beck

daniel-beck commented Feb 12, 2024

Copy link
Copy Markdown
Member Author

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 daniel-beck removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Feb 14, 2024
Comment on lines +1000 to +1003
let proxyMethod = e.getAttribute("data-proxy-method");
let proxyUrl = e.getAttribute("data-proxy-url");
let proxyCrumb = e.getAttribute("data-proxy-crumb");
let proxyUrlNames = e.getAttribute("data-proxy-url-names").split(",");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't these be const?

Suggested change
let proxyMethod = e.getAttribute("data-proxy-method");
let proxyUrl = e.getAttribute("data-proxy-url");
let proxyCrumb = e.getAttribute("data-proxy-crumb");
let proxyUrlNames = e.getAttribute("data-proxy-url-names").split(",");
const proxyMethod = e.getAttribute("data-proxy-method");
const proxyUrl = e.getAttribute("data-proxy-url");
const proxyCrumb = e.getAttribute("data-proxy-crumb");
const proxyUrlNames = e.getAttribute("data-proxy-url-names").split(",");


@Override
public String getUrlName() {
return "theWellKnownRoot";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a very realistic example of course, because it is a singleton (the point of WithWellKnownURL would be to look up a specific object based on parsing out a path), but good enough for purposes of tests here I suppose.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is the only implementation of WithWellKnownURL, we'll survive — if it weren't for your comments recently in the context of session serialization, I might have ripped it out entirely.

(Yes, there's another one in CloudBees, but it's long dead.)

@MarkEWaite MarkEWaite added the skip-changelog Should not be shown in the changelog label Feb 14, 2024
@daniel-beck daniel-beck added developer Changes which impact plugin developers and removed skip-changelog Should not be shown in the changelog labels Feb 14, 2024
@timja timja removed the needs-more-reviews Complex change, which would benefit from more eyes label Feb 20, 2024
@timja

timja commented Feb 20, 2024

Copy link
Copy Markdown
Member

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

@comment-ops-bot 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 Feb 20, 2024
@NotMyFault
NotMyFault merged commit 6460778 into jenkinsci:master Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues squash-merge-me Unclean or useless commit history, should be merged only with squash-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants