Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enable script-security tests again #1463

Merged
merged 3 commits into from
Jan 4, 2024
Merged

Conversation

jglick
Copy link
Member

@jglick jglick commented Jan 3, 2024

Reverting #1444 since these should pass as soon as jenkinsci/script-security-plugin#549 is deployed now that it is deployed.

Fixes #1463

@jglick jglick requested a review from timja January 3, 2024 14:16
@jglick
Copy link
Member Author

jglick commented Jan 3, 2024

https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/PR-1463/2/testReport/plugins/GroovyPluginTest/latest_linux_jdk21_firefox_split9___run_system_groovy_from_file/ 👀

…	WARNING	o.k.s.HttpResponseRenderer$Default#handleJavaScriptProxyMethodCall: call to /$stapler/bound/…/approveSignature failed
java.lang.IllegalStateException: Expected 1 instance of org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval$ApprovedWhitelist but got 2
	at hudson.ExtensionList.lookupSingleton(ExtensionList.java:457)
	at org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval$ApprovedWhitelist.configurationChanged(ScriptApproval.java:974)
	at org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval.reconfigure(ScriptApproval.java:1135)
	at org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval.approveSignature(ScriptApproval.java:1145)

@jglick
Copy link
Member Author

jglick commented Jan 3, 2024

Core bug? Locally

[
    org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval$ApprovedWhitelist,
    org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist,
    org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist
].each {c ->
  println "$c: ${ExtensionList.lookup(c)}"
}; null

class org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval$ApprovedWhitelist: [org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval$ApprovedWhitelist@2a5adf3f[org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.AclAwareWhitelist@761fbf66], org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval$ApprovedWhitelist@2a5adf3f[org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.AclAwareWhitelist@761fbf66]]
class org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist: [org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval$ApprovedWhitelist@2a5adf3f[org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.AclAwareWhitelist@761fbf66]]
class org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist: [org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.AnnotatedWhitelist@79647c59, org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.StaticWhitelist@3b3da89b, org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval$ApprovedWhitelist@2a5adf3f[org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.AclAwareWhitelist@761fbf66]]

Note that the ApprovedWhitelist appears twice when looked up under itself, but not when lookup up under supertypes. And I suppose this only happens when script-security is dynamically installed. Also https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/PR-1463/2/testReport/plugins/GroovyPluginTest/lts_linux_jdk11_firefox_split9___run_system_groovy_from_file/ passed. As did https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/PR-1463/2/testReport/plugins/ScriptSecurityPluginTest/latest_linux_jdk21_firefox_split8___signatureNeedsApproval/ which uses the same idiom

ScriptApproval sa = new ScriptApproval(jenkins);
sa.open();
sa.findSignature("getProperties").approve();

@jglick
Copy link
Member Author

jglick commented Jan 3, 2024

I get the same error locally from ScriptSecurityPluginTest.signatureNeedsApproval so I suppose it is timing-sensitive.

@jglick jglick marked this pull request as ready for review January 4, 2024 15:36
@jglick jglick requested a review from Vlatombe January 4, 2024 21:53
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

thanks!

@timja timja added chore tests and removed chore labels Jan 4, 2024
@timja timja merged commit 1846668 into jenkinsci:master Jan 4, 2024
24 checks passed
@jglick jglick deleted the script-security branch January 4, 2024 23:13
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants