add checks for inline javascript #868

mawinter69 · 2024-10-30T08:28:47Z

Detects usage of inline event handlers like onclick, usage of script elements (except for json type), usage of checkUrl without checkDependsOn

Same as jenkins-infra/repository-permissions-updater#4049 but for maven

Testing done

Manually tested that a plugin that contains any of the offending patterns properly fails the tests.

Sample output

08:50:52 [INFO] Results:
08:50:52 [INFO]
08:50:52 [ERROR] Failures:
08:50:52 [ERROR] org.jvnet.hudson.test.JellyTestSuiteBuilder$JellyCheck.null
08:50:52 [INFO]   Run 1: PASS
08:50:52 [INFO]   Run 2: PASS
08:50:52 [ERROR]   Run 3: JellyTestSuiteBuilder$JellyCheck.runTest:122 Usage of 'checkUrl' without checkDependsOn in file:/c:/SAPDevelop/jenkins/os/collapsing-console-sections-plugin/target/classes/org/jvnet/hudson/plugins/collapsingconsolesections/CollapsingSectionNote/DescriptorImpl/outline.jelly
Usage of inline event handler 'onclick' in file:/c:/SAPDevelop/jenkins/os/collapsing-console-sections-plugin/target/classes/org/jvnet/hudson/plugins/collapsingconsolesections/CollapsingSectionNote/DescriptorImpl/outline.jelly
Usage of inline event handler 'onblur' in file:/c:/SAPDevelop/jenkins/os/collapsing-console-sections-plugin/target/classes/org/jvnet/hudson/plugins/collapsingconsolesections/CollapsingSectionNote/DescriptorImpl/outline.jelly

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

Detects usage of inline event handlers like onclick, usage of script elements (except for json type), usage of checkUrl without checkDependsOn Same as jenkins-infra/repository-permissions-updater#4049 but for maven builds

mawinter69 · 2024-10-30T08:33:23Z

Should this be guarded by a switch so that the checks can be disabled (like the requirePI switch)?
Would we need some way to handle false positives (not sure if there could be any), e.g. by adding some comment or attribute?
Enabling that feature as is would probably break the build for a lot of plugins, but it would force plugin owners to react when they want to upgrade

timja · 2024-10-30T08:46:58Z

Should this be guarded by a switch so that the checks can be disabled (like the requirePI switch)? Would we need some way to handle false positives (not sure if there could be any), e.g. by adding some comment or attribute? Enabling that feature as is would probably break the build for a lot of plugins, but it would force plugin owners to react when they want to upgrade

Best to run it through bom by force updating the test harness version in pct.sh (similar to how this updated the hpi plugin):
https://github.com/jenkinsci/bom/blob/81216a901a1846f7c78f88dd4f7e46021c1a4403/pct.sh#L37

To see what the impact is

timja · 2024-10-30T09:48:25Z

src/main/java/org/jvnet/hudson/test/JellyTestSuiteBuilder.java

+                    errors.add("Usage of 'checkUrl' without 'checkDependsOn' in "+jelly);
+                }
+                if (onclick != null && element.getNamespace() != Namespace.NO_NAMESPACE) {
+                    errors.add("Usage of 'onclick' from a taglib in "+jelly);


could we link to a page explaining how to fix this?

When used via a taglib it mean the resulting tag will also have that onclick handler. Afaik for all places in core that define an onclick in a taglib it is marked as deprecated. So the usage of onclick from a taglib is not really different from an onclick on a plain html element.

timja · 2024-10-30T09:49:35Z

src/main/java/org/jvnet/hudson/test/JellyTestSuiteBuilder.java

+                Attribute checkUrl = element.attribute("checkUrl");
+                Attribute checkDependsOn = element.attribute("checkDependsOn");
+                if (checkUrl != null  && checkDependsOn == null) {
+                    errors.add("Usage of 'checkUrl' without 'checkDependsOn' in "+jelly);


I'm wondering if this is clear enough to people, e.g. in most cases people need to add a checkDependsOn="".

Personally I think this isn't clear enough on its own.

(It would be good if this could be fixed so it doesn't need the empty checkDependsOn)

Probably when we have CSP enabled by default and all plugins are adjusted we could remove an empty checkDependsOn, until then we need it to find out if we have to run the eval on the checkUrl.
We could add a link to https://www.jenkins.io/doc/developer/security/csp/ whenever there is an inline javascript found.

timja · 2024-10-30T09:51:06Z

Could you add sample output to the PR description showing what it looks like to a developer please?

mawinter69 · 2024-10-30T11:33:24Z

Should this be guarded by a switch so that the checks can be disabled (like the requirePI switch)? Would we need some way to handle false positives (not sure if there could be any), e.g. by adding some comment or attribute? Enabling that feature as is would probably break the build for a lot of plugins, but it would force plugin owners to react when they want to upgrade

Best to run it through bom by force updating the test harness version in pct.sh (similar to how this updated the hpi plugin): https://github.com/jenkinsci/bom/blob/81216a901a1846f7c78f88dd4f7e46021c1a4403/pct.sh#L37

To see what the impact is

That'll be tricky. The project here is already requiring 2.479 of core. So when I want to build a plugin I need the plugin parent pom in version 5.x at least. Without that it's not building

timja · 2024-10-30T12:37:23Z

Should this be guarded by a switch so that the checks can be disabled (like the requirePI switch)? Would we need some way to handle false positives (not sure if there could be any), e.g. by adding some comment or attribute? Enabling that feature as is would probably break the build for a lot of plugins, but it would force plugin owners to react when they want to upgrade

Best to run it through bom by force updating the test harness version in pct.sh (similar to how this updated the hpi plugin): https://github.com/jenkinsci/bom/blob/81216a901a1846f7c78f88dd4f7e46021c1a4403/pct.sh#L37
To see what the impact is

That'll be tricky. The project here is already requiring 2.479 of core. So when I want to build a plugin I need the plugin parent pom in version 5.x at least. Without that it's not building

Could do a backport PR?

timja · 2024-10-30T12:37:59Z

But i guess that will cause problems for ones that have upgraded

add checks for inline javascript

f45a7c8

Detects usage of inline event handlers like onclick, usage of script elements (except for json type), usage of checkUrl without checkDependsOn Same as jenkins-infra/repository-permissions-updater#4049 but for maven builds

Merge branch 'master' into jelly-verifier

4b74eb8

timja reviewed Oct 30, 2024

View reviewed changes

mawinter69 mentioned this pull request Oct 30, 2024

add a hook to be able to overwrite the jenkins-test-harness version jenkinsci/plugin-compat-tester#724

Draft

6 tasks

add link to csp page

261c314

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add checks for inline javascript #868

add checks for inline javascript #868

mawinter69 commented Oct 30, 2024 •

edited

Loading

mawinter69 commented Oct 30, 2024

timja commented Oct 30, 2024

timja Oct 30, 2024

mawinter69 Oct 30, 2024

timja Oct 30, 2024

mawinter69 Oct 30, 2024

timja commented Oct 30, 2024 •

edited

Loading

mawinter69 commented Oct 30, 2024

timja commented Oct 30, 2024

timja commented Oct 30, 2024

add checks for inline javascript #868

Are you sure you want to change the base?

add checks for inline javascript #868

Conversation

mawinter69 commented Oct 30, 2024 • edited Loading

Testing done

Submitter checklist

mawinter69 commented Oct 30, 2024

timja commented Oct 30, 2024

timja Oct 30, 2024

Choose a reason for hiding this comment

mawinter69 Oct 30, 2024

Choose a reason for hiding this comment

timja Oct 30, 2024

Choose a reason for hiding this comment

mawinter69 Oct 30, 2024

Choose a reason for hiding this comment

timja commented Oct 30, 2024 • edited Loading

mawinter69 commented Oct 30, 2024

timja commented Oct 30, 2024

timja commented Oct 30, 2024

mawinter69 commented Oct 30, 2024 •

edited

Loading

timja commented Oct 30, 2024 •

edited

Loading