[JENKINS-73875] CSP compatibility for NewNodeConsoleNote

[shlomomdahan]

Created a ticket in JIRA to track this issue: JENKINS-73875

Removed inline onClick handler from script.js to comply with Content Security Policy and replaced with event delegation.

Testing done

STATUS QUO: No changes implemented. This is the intended functionality.

https://www.loom.com/share/da8a1ab0f1d8431e858da2ae3ac7215f?sid=2dedd5ac-eb73-472b-a0ae-11e39daff80c

NON RESTRICTIVE: This is the behavior after implementing the CSP changes. The code is working just as before and there is no regression in functionality.

https://www.loom.com/share/1f9479eeb2cf4829bdcf524d6f4d1a37?sid=cfd60732-705b-4874-9ae0-97ae1747b2d2

RESTRICTIVE: This is with CSP restrictive mode enabled. It is working correctly as intended without any issues.

https://www.loom.com/share/cad50c0d86af4e5d9ea467152a54ab08

Submitter checklist

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

[basil]

I checked some other plugins in jenkinsci and found that the typical pattern is to iterate over all elements of a given class with e.g. document.querySelectorAll and then add click handlers to just those elements. That seems a little cleaner than adding a click listener on the body element which is a no-op in the vast majority of cases. For example, from jenkins/core/src/main/resources/hudson/PluginManager/_installed.js:

  document.addEventListener("DOMContentLoaded", function () {
    document
      .querySelectorAll(".plugin-manager-toggle-switch")
      .forEach(function (toggle) {
        toggle.addEventListener("click", flip);
      });
  });

[shlomomdahan]

Hi @basil, I am running into an issue when trying to do it as you described above.

The links to for the toggles do not appear directly after DOMContentLoaded is triggered. For example, in the below console logs, you can see that those elements don't actually appear in the html tree until all of the dependent resources are loaded.

DOMContentLoaded: Number of .pipeline-toggle elements: 0
Window load: Number of .pipeline-toggle elements: 6

What do you think is the best approach in this situation?

is it to use window.addEventListener("load", function() {}

or perhaps continue to use the event delegation approach but narrow down the parent element to the one that contains the toggles instead of the entire body?

Thank you

[basil]

Not sure offhand, but the event handler at the top of the file (the third argument to Behaviour.specify) seems to run every time a span.pipeline-new-node is added to the DOM (and itself iterates over all existing nodes with document.querySelectorAll('.pipeline-new-node')) so perhaps that would be a good place to fish out the <a> element we are interested in and manipulate its onclick handler. We're adding the <a> element by setting e.innerHTML on line 17, so perhaps we can just add the click event handler right after that?

[shlomomdahan]

@basil
That makes sense. thank you.

https://www.loom.com/share/d8f0b84fca3f4b218f7d656f268394eb?sid=8b5e1d8e-5de0-43b3-a511-ba3c3737823e

It is working correctly with the new change

[basil]

Why would this element ever not be present if we just added it a few lines earlier? I cannot see the purpose of this if statement.

[basil]

Are you sure this is always matching the <span class="pipeline-show-hide"> that we just added on line 17 above? That code is appending <span class="pipeline-show-hide"> elements as children of e, so I am wondering if there might ever be two such children or if there is always one as in your test. Did you try testing nested parallel blocks?

[shlomomdahan]

@basil

using the below script configuration

parallel first: {
    stage('First Nested') {
        parallel nestedA: {}, nestedB: {}
    }
}, second: {
    stage('"full" details') {
        parallel nestedC: {}, nestedD: {}
    }
}

I recorded the behavior of the toggle links:

https://www.loom.com/share/2acb8c6b041a449793132e3f4a433d70

Everything seems to be working okay. Can you confirm if this is the intended behavior?

[basil]

OK, I am glad the testing is passing. To make this bulletproof, I think we should select the last child with the pipeline-show-hide class rather than the first. Since the /.+/ regular expression is always appending the new <span> element to the end of the list of child nodes, selecting the last child should always match the node we just added.

[shlomomdahan]

Thanks for explaining that detail. New commit addresses this

[basil]

Prefer querySelectorAll to reliably select the last .pipeline-toggle element, as it works regardless of the DOM structure:

Suggested change

	const toggle = e.querySelector('.pipeline-show-hide:last-child .pipeline-toggle');
	const toggles = e.querySelectorAll('.pipeline-show-hide .pipeline-toggle');
	const toggle = toggles[toggles.length - 1];

Picking the last item in the list ensures the correct element is always selected, even if e contains multiple .pipeline-show-hide elements. In contrast, querySelector('.pipeline-show-hide:last-child .pipeline-toggle') only works if .pipeline-show-hide is the last child of its parent, making it fragile if the DOM changes.

[shlomomdahan]

Understood, sorry for the back and forth with this change.

I appreciate your explanation and will note it moving forward.

[basil]

Thanks @shlomomdahan! Approving pending one last piece of feedback. Can you please test this in ATH as in jenkinsci/acceptance-test-harness#1772 and then I will be ready to merge/release this.

[basil]

As in jenkinsci/job-config-history-plugin#338 (comment), I think it would be more readable to use toggle instead of button here.