jenkinsci / jenkins-test-harness

Unit test framework for Jenkins core and its plugins
https://www.jenkins.io/doc/developer/testing/
MIT License
110 stars 130 forks source link

add checks for inline javascript #868

Open mawinter69 opened 4 weeks ago

mawinter69 commented 4 weeks ago

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

Same as https://github.com/jenkins-infra/repository-permissions-updater/pull/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

mawinter69 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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

mawinter69 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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