workflow-job-plugin icon indicating copy to clipboard operation
workflow-job-plugin copied to clipboard

JENKINS-73875 CSP compatibility for script.js

Open shlomomdahan opened this issue 1 year ago • 3 comments

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

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

shlomomdahan avatar Oct 08 '24 00:10 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

shlomomdahan avatar Oct 08 '24 01:10 shlomomdahan

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?

basil avatar Oct 08 '24 02:10 basil

@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

shlomomdahan avatar Oct 08 '24 14:10 shlomomdahan