diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java index 554765a40..9303365cd 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java @@ -459,7 +459,7 @@ private final class CleanClassCollector extends ClassCollector { @RequirePOST public FormValidation doCheckScript(@QueryParameter String value, @QueryParameter boolean sandbox) { FormValidation validationResult = GroovySandbox.checkScriptForCompilationErrors(value, - new GroovyClassLoader(Jenkins.getInstance().getPluginManager().uberClassLoader)); + new GroovyClassLoader(Jenkins.get().getPluginManager().uberClassLoader)); if (validationResult.kind != FormValidation.Kind.OK) { return validationResult; } diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ApprovalContext.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ApprovalContext.java index dd8b5dd30..9d82d415b 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ApprovalContext.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ApprovalContext.java @@ -92,7 +92,7 @@ public ApprovalContext withItem(@CheckForNull Item item) { */ public @CheckForNull Item getItem() { // TODO if getItemByFullName == null, we should removal the approval - return item != null ? Jenkins.getInstance().getItemByFullName(item) : null; + return item != null ? Jenkins.get().getItemByFullName(item) : null; } /** diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 78a607070..1ff5ed692 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -104,7 +104,7 @@ public class ScriptApproval extends GlobalConfiguration implements RootAction { @Override protected XmlFile getConfigFile() { - return new XmlFile(XSTREAM2, new File(Jenkins.getInstance().getRootDir(),getUrlName() + ".xml")); + return new XmlFile(XSTREAM2, new File(Jenkins.get().getRootDir(),getUrlName() + ".xml")); } @Override @@ -435,7 +435,7 @@ static String hashClasspathEntry(URL entry) throws IOException { public synchronized String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context) { final String hash = hash(script, language.getName()); if (!approvedScriptHashes.contains(hash)) { - if (!Jenkins.getInstance().isUseSecurity() || Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.getInstance().hasPermission(Jenkins.ADMINISTER)) { + if (!Jenkins.get().isUseSecurity() || Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.get().hasPermission(Jenkins.ADMINISTER)) { approvedScriptHashes.add(hash); } else { String key = context.getKey(); @@ -507,7 +507,7 @@ public synchronized void configuring(@NonNull ClasspathEntry entry, @NonNull App if (!approvedClasspathEntries.contains(acp)) { boolean shouldSave = false; PendingClasspathEntry pcp = new PendingClasspathEntry(hash, url, context); - if (!Jenkins.getInstance().isUseSecurity() || (Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.getInstance().hasPermission(Jenkins.ADMINISTER))) { + if (!Jenkins.get().isUseSecurity() || (Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.get().hasPermission(Jenkins.ADMINISTER))) { LOG.log(Level.FINE, "Classpath entry {0} ({1}) is approved as configured with ADMINISTER permission.", new Object[] {url, hash}); pendingClasspathEntries.remove(pcp); approvedClasspathEntries.add(acp); @@ -538,7 +538,7 @@ public synchronized FormValidation checking(@NonNull ClasspathEntry entry) { } URL url = entry.getURL(); try { - if (!Jenkins.getInstance().hasPermission(Jenkins.ADMINISTER) && !approvedClasspathEntries.contains(new ApprovedClasspathEntry(hashClasspathEntry(url), url))) { + if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER) && !approvedClasspathEntries.contains(new ApprovedClasspathEntry(hashClasspathEntry(url), url))) { return FormValidation.error(Messages.ClasspathEntry_path_notApproved()); } else { return FormValidation.ok(); @@ -587,7 +587,7 @@ public synchronized void using(@NonNull ClasspathEntry entry) throws IOException * @return a warning in case the script is not yet approved and this user lacks {@link Jenkins#ADMINISTER}, else {@link FormValidation#ok()} */ public synchronized FormValidation checking(@NonNull String script, @NonNull Language language) { - if (!Jenkins.getInstance().hasPermission(Jenkins.ADMINISTER) && !approvedScriptHashes.contains(hash(script, language.getName()))) { + if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER) && !approvedScriptHashes.contains(hash(script, language.getName()))) { return FormValidation.warningWithMarkup("A Jenkins administrator will need to approve this script before it can be used."); } else { return FormValidation.ok(); @@ -657,7 +657,7 @@ public static void popRegistrationCallback() { @DataBoundSetter public synchronized void setApprovedSignatures(String[] signatures) throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); approvedSignatures.clear(); List goodSignatures = new ArrayList<>(signatures.length); for (String signature : signatures) { @@ -750,7 +750,7 @@ public Set getPendingScripts() { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public void approveScript(String hash) throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); synchronized (this) { approvedScriptHashes.add(hash); removePendingScript(hash); @@ -768,7 +768,7 @@ public Set getPendingScripts() { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public synchronized void denyScript(String hash) throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); approvedScriptHashes.remove(hash); removePendingScript(hash); save(); @@ -786,7 +786,7 @@ private synchronized void removePendingScript(String hash) { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public synchronized void clearApprovedScripts() throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); approvedScriptHashes.clear(); save(); } @@ -808,7 +808,7 @@ private String[][] reconfigure() throws IOException { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public synchronized String[][] approveSignature(String signature) throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); pendingSignatures.remove(new PendingSignature(signature, false, ApprovalContext.create())); approvedSignatures.add(signature); save(); @@ -817,7 +817,7 @@ private String[][] reconfigure() throws IOException { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public synchronized String[][] aclApproveSignature(String signature) throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); pendingSignatures.remove(new PendingSignature(signature, false, ApprovalContext.create())); aclApprovedSignatures.add(signature); save(); @@ -826,7 +826,7 @@ private String[][] reconfigure() throws IOException { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public synchronized void denySignature(String signature) throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); pendingSignatures.remove(new PendingSignature(signature, false, ApprovalContext.create())); save(); } @@ -834,7 +834,7 @@ private String[][] reconfigure() throws IOException { // TODO nicer would be to allow the user to actually edit the list directly (with syntax checks) @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public synchronized String[][] clearApprovedSignatures() throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); approvedSignatures.clear(); aclApprovedSignatures.clear(); save(); @@ -844,7 +844,7 @@ private String[][] reconfigure() throws IOException { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public synchronized String[][] clearDangerousApprovedSignatures() throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); Iterator it = approvedSignatures.iterator(); while (it.hasNext()) { @@ -895,7 +895,7 @@ public JSON getClasspathRenderInfo() { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public JSON approveClasspathEntry(String hash) throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); URL url = null; synchronized (this) { final PendingClasspathEntry cp = getPendingClasspathEntry(hash); @@ -922,7 +922,7 @@ public JSON approveClasspathEntry(String hash) throws IOException { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public JSON denyClasspathEntry(String hash) throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); PendingClasspathEntry cp = getPendingClasspathEntry(hash); if (cp != null) { pendingClasspathEntries.remove(cp); @@ -934,7 +934,7 @@ public JSON denyClasspathEntry(String hash) throws IOException { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public JSON denyApprovedClasspathEntry(String hash) throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); if (approvedClasspathEntries.remove(new ApprovedClasspathEntry(hash, null))) { save(); } @@ -944,7 +944,7 @@ public JSON denyApprovedClasspathEntry(String hash) throws IOException { @Restricted(NoExternalUse.class) // for use from AJAX @JavaScriptMethod public synchronized JSON clearApprovedClasspathEntries() throws IOException { - Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + Jenkins.get().checkPermission(Jenkins.ADMINISTER); approvedClasspathEntries.clear(); save(); return getClasspathRenderInfo(); diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java index b7e6ed091..eb03de04b 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java @@ -66,7 +66,7 @@ private ScriptApprovalNote(int length) { @Override public ConsoleAnnotator annotate(Object context, MarkupText text, int charPos) { - if (Jenkins.getInstance().hasPermission(Jenkins.ADMINISTER)) { + if (Jenkins.get().hasPermission(Jenkins.ADMINISTER)) { String url = ScriptApproval.get().getUrlName(); StaplerRequest req = Stapler.getCurrentRequest(); if (req != null) { @@ -74,7 +74,7 @@ public ConsoleAnnotator annotate(Object context, MarkupText text, int ch url = req.getContextPath() + "/" + url; } else { // otherwise presumably this is rendered for e-mails and other non-HTTP stuff - url = Jenkins.getInstance().getRootUrl() + url; + url = Jenkins.get().getRootUrl() + url; } text.addMarkup(charPos, charPos + length, "", ""); } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java index af6f1233c..402b89745 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java @@ -100,7 +100,7 @@ public static void m2(long x) {} @Issue("JENKINS-38908") @Test public void main() throws Exception { - Script receiver = (Script) new SecureGroovyScript("def main() {}; this", true, null).configuring(ApprovalContext.create()).evaluate(GroovyCallSiteSelectorTest.class.getClassLoader(), new Binding()); + Script receiver = (Script) new SecureGroovyScript("def main() {}; this", true, null).configuring(ApprovalContext.create()).evaluate(GroovyCallSiteSelectorTest.class.getClassLoader(), new Binding(), null); assertEquals(receiver.getClass().getMethod("main"), GroovyCallSiteSelector.method(receiver, "main", new Object[0])); assertEquals(receiver.getClass().getMethod("main", String[].class), GroovyCallSiteSelector.method(receiver, "main", new Object[] {"somearg"})); } @@ -128,14 +128,11 @@ public static void m2(long x) {} @Test public void varargsFailureCases() throws Exception { // If there's a partial match, we should get a ClassCastException - try { - assertNull(GroovyCallSiteSelector.constructor(ParametersAction.class, - new Object[]{new BooleanParameterValue("someBool", true), "x"})); - } catch (Exception e) { - assertTrue(e instanceof ClassCastException); - assertEquals("Cannot cast object 'x' with class 'java.lang.String' to class 'hudson.model.ParameterValue'", - e.getMessage()); - } + final ClassCastException e = assertThrows(ClassCastException.class, + () -> assertNull(GroovyCallSiteSelector.constructor(ParametersAction.class, + new Object[]{new BooleanParameterValue("someBool", true), "x"}))); + assertEquals("Cannot cast object 'x' with class 'java.lang.String' to class 'hudson.model.ParameterValue'", + e.getMessage()); // If it's a complete non-match, we just shouldn't get a constructor. assertNull(GroovyCallSiteSelector.constructor(ParametersAction.class, new Object[]{"a", "b"})); } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java index 01487d99d..281fccbbd 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java @@ -40,12 +40,12 @@ import groovy.text.Template; import groovy.transform.ASTTest; import hudson.Functions; -import hudson.util.IOUtils; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Method; import java.net.URL; +import java.nio.charset.StandardCharsets; import java.text.DateFormat; import java.util.ArrayList; import java.util.Arrays; @@ -58,12 +58,23 @@ import java.util.concurrent.Callable; import java.util.regex.Pattern; +import org.apache.commons.io.IOUtils; import org.apache.commons.lang.StringUtils; import org.codehaus.groovy.control.CompilerConfiguration; import org.codehaus.groovy.control.MultipleCompilationErrorsException; import org.codehaus.groovy.runtime.GStringImpl; import org.codehaus.groovy.runtime.InvokerHelper; + +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist; import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.AbstractWhitelist; @@ -73,7 +84,6 @@ import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist; import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.StaticWhitelist; import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.Whitelisted; -import static org.junit.Assert.*; import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; @@ -142,14 +152,12 @@ public boolean permitsMethod(Method method, Object receiver, Object[] args) { // "method groovy.json.JsonBuilder toString", // "method groovy.json.JsonBuilder invokeMethod java.lang.String java.lang.Object" )), expected, script); - try { - evaluate(new ProxyWhitelist(), "class Real {}; def real = new Real(); real.nonexistent(42)"); - fail(); - } catch (RejectedAccessException x) { - String message = x.getMessage(); - assertEquals(message, "method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object", x.getSignature()); - assertTrue(message, message.contains("Real nonexistent java.lang.Integer")); - } + + final RejectedAccessException x = assertThrows(RejectedAccessException.class, + () -> evaluate(new ProxyWhitelist(), "class Real {}; def real = new Real(); real.nonexistent(42)")); + final String message = x.getMessage(); + assertEquals(message, "method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object", x.getSignature()); + assertTrue(message, message.contains("Real nonexistent java.lang.Integer")); } @Ignore("TODO there are various unhandled cases, such as Closure → SAM, or numeric conversions, or number → String, or boxing/unboxing.") @@ -227,20 +235,16 @@ public boolean permitsMethod(Method method, Object receiver, Object[] args) { assertRejected(new StaticWhitelist("new " + clazz), "method " + clazz + " isProp3", "new " + clazz + "().prop3"); assertEvaluate(new StaticWhitelist("staticMethod " + clazz + " isProp4"), true, clazz + ".prop4"); assertRejected(new StaticWhitelist(), "staticMethod " + clazz + " isProp4", clazz + ".prop4"); - try { - evaluate(new StaticWhitelist("new " + clazz), "new " + clazz + "().nonexistent"); - fail(); - } catch (RejectedAccessException x) { - assertEquals(null, x.getSignature()); - assertEquals("No such field found: field " + clazz + " nonexistent", x.getMessage()); - } - try { - evaluate(new StaticWhitelist("new " + clazz), "new " + clazz + "().nonexistent = 'edited'"); - fail(); - } catch (RejectedAccessException x) { - assertEquals(null, x.getSignature()); - assertEquals("No such field found: field " + clazz + " nonexistent", x.getMessage()); - } + final RejectedAccessException x = assertThrows(RejectedAccessException.class, + () -> evaluate(new StaticWhitelist("new " + clazz), "new " + clazz + "().nonexistent")); + assertNull(x.getSignature()); + assertEquals("No such field found: field " + clazz + " nonexistent", x.getMessage()); + + final RejectedAccessException x2 = assertThrows(RejectedAccessException.class, + () -> evaluate(new StaticWhitelist("new " + clazz), "new " + clazz + "().nonexistent = 'edited'")); + assertNull(x.getSignature()); + assertEquals("No such field found: field " + clazz + " nonexistent", x2.getMessage()); + assertRejected(new StaticWhitelist("new " + clazz), "method " + clazz + " getProp5", "new " + clazz + "().prop5"); assertEvaluate(new StaticWhitelist("new " + clazz, "method " + clazz + " getProp5"), "DEFAULT", "new " + clazz + "().prop5"); assertRejected(new StaticWhitelist("new " + clazz, "method " + clazz + " getProp5"), "method " + clazz + " setProp5 java.lang.String", "def c = new " + clazz + "(); c.prop5 = 'EDITED'; c.prop5"); @@ -403,24 +407,17 @@ private Unsafe() {} /** Expect errors from {@link org.codehaus.groovy.runtime.NullObject}. */ @Issue("kohsuke/groovy-sandbox #15") @Test public void nullPointerException() throws Exception { - try { - evaluate(new ProxyWhitelist(), "def x = null; x.member"); - fail(); - } catch (NullPointerException x) { - assertEquals(Functions.printThrowable(x), "Cannot get property 'member' on null object", x.getMessage()); - } - try { - evaluate(new ProxyWhitelist(), "def x = null; x.member = 42"); - fail(); - } catch (NullPointerException x) { - assertEquals(Functions.printThrowable(x), "Cannot set property 'member' on null object", x.getMessage()); - } - try { - evaluate(new ProxyWhitelist(), "def x = null; x.member()"); - fail(); - } catch (NullPointerException x) { - assertEquals(Functions.printThrowable(x), "Cannot invoke method member() on null object", x.getMessage()); - } + final NullPointerException e = assertThrows(NullPointerException.class, + () -> evaluate(new ProxyWhitelist(), "def x = null; x.member")); + assertEquals(Functions.printThrowable(e), "Cannot get property 'member' on null object", e.getMessage()); + + final NullPointerException e2 = assertThrows(NullPointerException.class, + () -> evaluate(new ProxyWhitelist(), "def x = null; x.member = 42")); + assertEquals(Functions.printThrowable(e2), "Cannot set property 'member' on null object", e2.getMessage()); + + final NullPointerException e3 = assertThrows(NullPointerException.class, + () -> evaluate(new ProxyWhitelist(), "def x = null; x.member()")); + assertEquals(Functions.printThrowable(e3), "Cannot invoke method member() on null object", e3.getMessage()); } /** @@ -583,12 +580,9 @@ public String toString() { } @Test public void missingPropertyException() throws Exception { - try { - evaluate(new ProxyWhitelist(), "GOOP"); - fail(); - } catch (MissingPropertyException x) { - assertEquals("GOOP", x.getProperty()); - } + final MissingPropertyException x = assertThrows(MissingPropertyException.class, + () -> evaluate(new ProxyWhitelist(), "GOOP")); + assertEquals("GOOP", x.getProperty()); } @Test public void specialScript() throws Exception { @@ -604,12 +598,11 @@ public String toString() { } }; assertEquals(42, GroovySandbox.run(shell, "magic", wl)); - try { - GroovySandbox.run(shell, "boring", wl); - } catch (MissingPropertyException x) { - assertEquals("boring", x.getProperty()); - } + final MissingPropertyException x = assertThrows(MissingPropertyException.class, + () -> GroovySandbox.run(shell, "boring", wl)); + assertEquals("boring", x.getProperty()); } + public static abstract class SpecialScript extends Script { @Override public Object getProperty(String property) { if (property.equals("magic")) { @@ -700,12 +693,8 @@ public static abstract class SpecialScript extends Script { } @Test public void ambiguousOverloads() { - try { - evaluate(new AnnotatedWhitelist(), Ambiguity.class.getName() + ".m(null)"); - fail("Ambiguous overload is an error in Groovy 2"); - } catch(GroovyRuntimeException e) { - // OK - } + assertThrows("Ambiguous overload is an error in Groovy 2", GroovyRuntimeException.class, + () -> evaluate(new AnnotatedWhitelist(), Ambiguity.class.getName() + ".m(null)")); } public static final class Ambiguity { @@ -774,7 +763,7 @@ public void setSecure(boolean x) {} } @Test public void keywordsAndOperators() throws Exception { - String script = IOUtils.toString(this.getClass().getResourceAsStream("SandboxInterceptorTest/all.groovy")); + String script = IOUtils.toString(this.getClass().getResourceAsStream("SandboxInterceptorTest/all.groovy"), StandardCharsets.UTF_8); assertEvaluate(new GenericWhitelist(), null, script); } @@ -924,13 +913,10 @@ public static void assertRejected(Whitelist whitelist, String expectedSignature, @Issue("JENKINS-37129") @Test public void methodMissingException() throws Exception { // test: trying to call a nonexistent method - try { - evaluate(new GenericWhitelist(), "[].noSuchMethod()"); - fail(); - } catch (MissingMethodException e) { - assertEquals(e.getType(),ArrayList.class); - assertThat(e.getMethod(),is("noSuchMethod")); - } + final MissingMethodException e = assertThrows(MissingMethodException.class, + () -> evaluate(new GenericWhitelist(), "[].noSuchMethod()")); + assertEquals(e.getType(),ArrayList.class); + assertThat(e.getMethod(),is("noSuchMethod")); // control: trying to call an existing method that's not safe assertRejected(new GenericWhitelist(), "method java.lang.Class getClassLoader", "[].class.classLoader"); @@ -1169,15 +1155,12 @@ public void dateTimeApi() throws Exception { @Issue("SECURITY-1186") @Test public void finalizer() throws Exception { - try { - evaluate(new GenericWhitelist(), "class Test { public void finalize() { } }; null"); - fail("Finalizers should be rejected"); - } catch (MultipleCompilationErrorsException e) { - assertThat(e.getErrorCollector().getErrorCount(), equalTo(1)); - Exception innerE = e.getErrorCollector().getException(0); - assertThat(innerE, instanceOf(SecurityException.class)); - assertThat(innerE.getMessage(), containsString("Object.finalize()")); - } + final MultipleCompilationErrorsException e = assertThrows(MultipleCompilationErrorsException.class, + () -> evaluate(new GenericWhitelist(), "class Test { public void finalize() { } }; null")); + assertThat(e.getErrorCollector().getErrorCount(), equalTo(1)); + Exception innerE = e.getErrorCollector().getException(0); + assertThat(innerE, instanceOf(SecurityException.class)); + assertThat(innerE.getMessage(), containsString("Object.finalize()")); } @Test @@ -1298,56 +1281,47 @@ public void blockIllegalAnnotationsInAnnotations() throws Exception { @Issue("SECURITY-1754") @Test public void blockDirectCallsToSyntheticConstructors() throws Exception { - try { - // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted. - evaluate(new GenericWhitelist(), - "class Superclass { }\n" + - "class Subclass extends Superclass { }\n" + - "new Subclass(null)"); - fail("Script should have failed"); - } catch (SecurityException e) { - assertThat(e.getMessage(), equalTo( - "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + - "Perhaps you meant to use one of these constructors instead: public Subclass()")); - } + // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted. + final SecurityException e = assertThrows(SecurityException.class, + () -> evaluate(new GenericWhitelist(), + "class Superclass { }\n" + + "class Subclass extends Superclass { }\n" + + "new Subclass(null)")); + assertThat(e.getMessage(), equalTo( + "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + + "Perhaps you meant to use one of these constructors instead: public Subclass()")); } @Issue("SECURITY-1754") @Test public void blockMisinterceptedCallsToSyntheticConstructors() throws Exception { - try { - // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted. - evaluate(new GenericWhitelist(), - "class Superclass { }\n" + - "class Subclass extends Superclass {\n" + - " Subclass() { def x = 1 }\n" + - " Subclass(Subclass s) { def x = 1 }\n" + - "}\n" + - "new Subclass(null)"); // Intercepted as a call to the second constructor before SECURITY-1754, but actually calls synthetic constructor. - fail("Script should have failed"); - } catch (SecurityException e) { - assertThat(e.getMessage(), equalTo( - "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + - "Perhaps you meant to use one of these constructors instead: public Subclass(), public Subclass(Subclass)")); - } + // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted. + final SecurityException e = assertThrows(SecurityException.class, + () -> evaluate(new GenericWhitelist(), + "class Superclass { }\n" + + "class Subclass extends Superclass {\n" + + " Subclass() { def x = 1 }\n" + + " Subclass(Subclass s) { def x = 1 }\n" + + "}\n" + + "new Subclass(null)")); // Intercepted as a call to the second constructor before SECURITY-1754, but actually calls synthetic constructor. + assertThat(e.getMessage(), equalTo( + "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + + "Perhaps you meant to use one of these constructors instead: public Subclass(), public Subclass(Subclass)")); } @Issue("SECURITY-1754") @Test public void blockCallsToSyntheticConstructorsViaOtherConstructors() throws Exception { - try { - // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted. - evaluate(new GenericWhitelist(), - "class Superclass { }\n" + - "class Subclass extends Superclass {\n" + - " Subclass() { }\n" + - " Subclass(int x, int y) { this(null) }\n" + // Calls synthetic constructor - "}\n" + - "new Subclass(1, 2)"); - fail("Script should have failed"); - } catch (SecurityException e) { - assertThat(e.getMessage(), equalTo( - "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + - "Perhaps you meant to use one of these constructors instead: public Subclass(), public Subclass(int,int)")); - } + // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted. + final SecurityException e = assertThrows(SecurityException.class, + () -> evaluate(new GenericWhitelist(), + "class Superclass { }\n" + + "class Subclass extends Superclass {\n" + + " Subclass() { }\n" + + " Subclass(int x, int y) { this(null) }\n" + // Calls synthetic constructor + "}\n" + + "new Subclass(1, 2)")); + assertThat(e.getMessage(), equalTo( + "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + + "Perhaps you meant to use one of these constructors instead: public Subclass(), public Subclass(int,int)")); } @Issue("SECURITY-1754") @@ -1397,24 +1371,22 @@ public void blockIllegalAnnotationsInAnnotations() throws Exception { @Issue("SECURITY-2020") @Test public void unsafeReturnValue() throws Throwable { - try { + final SecurityException e = assertThrows(SecurityException.class, () -> { Object result = evaluate(new GenericWhitelist(), "class Test {\n" + - " @Override public String toString() {\n" + - " jenkins.model.Jenkins.get().setSystemMessage('Hello, world!')\n" + - " 'test'\n" + - " }\n" + - "}\n" + - "new Test()"); + " @Override public String toString() {\n" + + " jenkins.model.Jenkins.get().setSystemMessage('Hello, world!')\n" + + " 'test'\n" + + " }\n" + + "}\n" + + "new Test()"); // Test.equals and Test.getClass are inherited and not sandbox-transformed, so they can be called outside of the sandbox. assertFalse(result.equals(new Object())); assertThat(result.getClass().getSimpleName(), equalTo("Test")); // Test.toString is defined in the sandbox, so it cannot be called outside of the sandbox. result.toString(); - fail("Test.toString should throw a SecurityException"); - } catch (SecurityException e) { - assertThat(e.getMessage(), equalTo("Rejecting unsandboxed static method call: jenkins.model.Jenkins.get()")); - } + }); + assertThat(e.getMessage(), equalTo("Rejecting unsandboxed static method call: jenkins.model.Jenkins.get()")); } /** @@ -1432,14 +1404,9 @@ private void assertAnnotationBlocked(Class annotation, String script) { private void assertAnnotationBlockedInternal(Class annotation, String script) { GroovyShell shell = new GroovyShell(GroovySandbox.createSecureCompilerConfiguration()); - try { - shell.parse(script); - fail("Compilation should have failed"); - } catch (Exception e) { - assertThat(e, instanceOf(MultipleCompilationErrorsException.class)); - assertThat(e.getMessage(), anyOf( - containsString("Annotation " + annotation.getName() + " cannot be used in the sandbox"), - containsString("Annotation " + annotation.getSimpleName() + " cannot be used in the sandbox"))); - } + final MultipleCompilationErrorsException e = assertThrows(MultipleCompilationErrorsException.class, () -> shell.parse(script)); + assertThat(e.getMessage(), anyOf( + containsString("Annotation " + annotation.getName() + " cannot be used in the sandbox"), + containsString("Annotation " + annotation.getSimpleName() + " cannot be used in the sandbox"))); } } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java index bc2c68915..6436973d9 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java @@ -28,11 +28,12 @@ import org.junit.Test; import org.jvnet.hudson.test.Issue; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxResolvingClassLoader.CLASS_NOT_FOUND; import static org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxResolvingClassLoader.parentClassCache; -import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; public class SandboxResolvingClassLoaderTest { @@ -45,12 +46,9 @@ public class SandboxResolvingClassLoaderTest { // Load a class that does exist. assertThat(loader.loadClass("java.lang.String", false), equalTo(String.class)); // Load a class that does not exist. - try { - loader.loadClass("this.does.not.Exist", false); - fail("Class should not exist"); - } catch (ClassNotFoundException e) { - assertThat(e.getMessage(), containsString("this.does.not.Exist")); - } + final ClassNotFoundException e = assertThrows(ClassNotFoundException.class, + () -> loader.loadClass("this.does.not.Exist", false)); + assertThat(e.getMessage(), containsString("this.does.not.Exist")); // The result of both of the class loading attempts should exist in the cache. assertThat(parentClassCache.get(parentLoader).getIfPresent("java.lang.String"), equalTo(String.class)); assertThat(parentClassCache.get(parentLoader).getIfPresent("this.does.not.Exist"), equalTo(CLASS_NOT_FOUND)); diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java index 0b9c8a7af..9217d61e3 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java @@ -59,8 +59,16 @@ import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval; import org.jenkinsci.plugins.scriptsecurity.scripts.UnapprovedUsageException; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import org.jenkinsci.plugins.scriptsecurity.scripts.languages.GroovyLanguage; import org.junit.Rule; @@ -234,12 +242,9 @@ public class SecureGroovyScriptTest { assertEquals(0, pendingScripts.size()); // We didn't add the approved classpath so ... - try { - ScriptApproval.get().using(groovy, GroovyLanguage.get()); - fail("Expected UnapprovedUsageException"); - } catch (UnapprovedUsageException e) { - assertEquals("script not yet approved for use", e.getMessage()); - } + final UnapprovedUsageException e = assertThrows(UnapprovedUsageException.class, + () -> ScriptApproval.get().using(groovy, GroovyLanguage.get())); + assertEquals("script not yet approved for use", e.getMessage()); } private List getAllJarFiles() throws URISyntaxException { @@ -809,12 +814,9 @@ public void testSandboxClassResolution() throws Exception { assertNotSame(Checker.class, a.loadClass(Checker.class.getName())); SecureGroovyScript sgs = new SecureGroovyScript("System.gc()", true, null); - try { - sgs.configuringWithKeyItem().evaluate(a, new Binding()); - fail("Expecting a rejection"); - } catch (RejectedAccessException e) { - assertTrue(e.getMessage().contains("staticMethod java.lang.System gc")); - } + final RejectedAccessException e = assertThrows(RejectedAccessException.class, + () -> sgs.configuringWithKeyItem().evaluate(a, new Binding())); + assertTrue(e.getMessage().contains("staticMethod java.lang.System gc")); } @Issue("SECURITY-1186") diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/TestGroovyRecorder.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/TestGroovyRecorder.java index feb979ceb..4071a376d 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/TestGroovyRecorder.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/TestGroovyRecorder.java @@ -59,7 +59,7 @@ public SecureGroovyScript getScript() { try { Binding binding = new Binding(); binding.setVariable("build", build); - build.setDescription(String.valueOf(script.evaluate(Jenkins.getInstance().getPluginManager().uberClassLoader, binding, listener))); + build.setDescription(String.valueOf(script.evaluate(Jenkins.get().getPluginManager().uberClassLoader, binding, listener))); } catch (Exception x) { throw new IOException(x); } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelistTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelistTest.java index a19e2a177..470eabf9e 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelistTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelistTest.java @@ -30,6 +30,7 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.net.URL; +import java.nio.charset.StandardCharsets; import java.text.MessageFormat; import java.util.ArrayList; import java.util.Arrays; @@ -57,10 +58,8 @@ public class StaticWhitelistTest { static void sanity(URL definition) throws Exception { StaticWhitelist wl = StaticWhitelist.from(definition); - List sigs = new ArrayList<>(); - InputStream is = definition.openStream(); - try { - BufferedReader br = new BufferedReader(new InputStreamReader(is, "UTF-8")); + List sigs = new ArrayList(); + try (InputStream is = definition.openStream(); InputStreamReader isr = new InputStreamReader(is, StandardCharsets.UTF_8); BufferedReader br = new BufferedReader(isr)) { String line; while ((line = br.readLine()) != null) { line = StaticWhitelist.filter(line); @@ -69,8 +68,6 @@ static void sanity(URL definition) throws Exception { } sigs.add(StaticWhitelist.parse(line)); } - } finally { - is.close(); } HashSet existingSigs = new HashSet<>(sigs.size()); diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntryTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntryTest.java index 1290cad53..58926a68f 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntryTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntryTest.java @@ -72,24 +72,10 @@ private static void assertRoundTrip(String path, String url) throws Exception { ClasspathEntry ignore = new ClasspathEntry("http://nowhere.net/"); ignore = new ClasspathEntry(rule.newFile("x.jar").getAbsolutePath()); ignore = new ClasspathEntry(rule.newFolder().getAbsolutePath()); - try { - ignore = new ClasspathEntry(""); - fail(); - } catch (MalformedURLException x) { - // good - } - try { - ignore = new ClasspathEntry(" "); - fail(); - } catch (MalformedURLException x) { - // good - } - try { - ignore = new ClasspathEntry("relative"); - fail(); - } catch (MalformedURLException x) { - // good - } + + assertThrows(MalformedURLException.class, () -> new ClasspathEntry("")); + assertThrows(MalformedURLException.class, () -> new ClasspathEntry(" ")); + assertThrows(MalformedURLException.class, () -> new ClasspathEntry("relative")); } } \ No newline at end of file diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNoteTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNoteTest.java index e915ce5b0..a603ebec7 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNoteTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNoteTest.java @@ -32,12 +32,15 @@ import hudson.model.Item; import hudson.model.Result; import jenkins.model.Jenkins; + +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; +import static org.junit.Assert.assertEquals; + import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript; import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.TestGroovyRecorder; import org.junit.ClassRule; import org.junit.Test; -import static org.junit.Assert.*; import org.junit.Rule; import org.jvnet.hudson.test.BuildWatcher; import org.jvnet.hudson.test.Issue; diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index 33856024f..1473e63f6 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -48,9 +48,10 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Level; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.nullValue; -import static org.junit.Assert.assertThat; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; public class ScriptApprovalTest extends AbstractApprovalTest { @@ -118,19 +119,15 @@ public void malformedScriptApproval() throws Exception { @Issue("SECURITY-1866") @Test public void classpathEntriesEscaped() throws Exception { // Add pending classpath entry. - String hash = null; - try { - ScriptApproval.get().using(new ClasspathEntry("https://www.example.com/#value=HackHack")); - fail("Classpath should not already be approved"); - } catch (UnapprovedClasspathException e) { - hash = e.getHash(); - } + final UnapprovedClasspathException e = assertThrows(UnapprovedClasspathException.class, () -> + ScriptApproval.get().using(new ClasspathEntry("https://www.example.com/#value=HackHack"))); + // Check for XSS in pending approvals. JenkinsRule.WebClient wc = r.createWebClient(); HtmlPage approvalPage = wc.goTo("scriptApproval"); assertThat(approvalPage.getElementById("xss"), nullValue()); // Approve classpath entry. - ScriptApproval.get().approveClasspathEntry(hash); + ScriptApproval.get().approveClasspathEntry(e.getHash()); // Check for XSS in approved classpath entries. HtmlPage approvedPage = wc.goTo("scriptApproval"); assertThat(approvedPage.getElementById("xss"), nullValue());