Merge pull request #68 from jglick/JENKINS-34739-varargs

[JENKINS-34739] Handle varargs
jenkinsci · May 19, 2016 · 415e349017ec8b04f9400c87789141670093a182 · 415e349
2 parents 155ac3d + 02d9c3b
commit 415e349017ec8b04f9400c87789141670093a182
diff --git a/...main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java b/...main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java
@@ -26,6 +26,7 @@
 import com.google.common.primitives.Primitives;
 import groovy.lang.GString;
+  
+import java.lang.reflect.Array;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
@@ -43,7 +44,10 @@
 */
 class GroovyCallSiteSelector {
-  
-    private static boolean matches(@Nonnull Class<?>[] parameterTypes, @Nonnull Object[] parameters) {
+  
+    private static boolean matches(@Nonnull Class<?>[] parameterTypes, @Nonnull Object[] parameters, boolean varargs) {
+  
+        if (varargs) {
+  
+            parameters = parametersForVarargs(parameterTypes, parameters);
+  
+        }
        if (parameters.length != parameterTypes.length) {
            return false;
        }
@@ -80,6 +84,33 @@ private static boolean matches(@Nonnull Class<?>[] parameterTypes, @Nonnull Obje
        return true;
    }
+  
+    /**
+  
+     * Translates a method parameter list with varargs possibly spliced into the end into the actual parameters to be passed to the JVM call.
+  
+     */
+  
+    private static Object[] parametersForVarargs(Class<?>[] parameterTypes, Object[] parameters) {
+  
+        int fixedLen = parameterTypes.length - 1;
+  
+        Class<?> componentType = parameterTypes[fixedLen].getComponentType();
+  
+        assert componentType != null;
+  
+        int arrayLength = parameters.length - fixedLen;
+  
+        if (arrayLength >= 0) {
+  
+            if (arrayLength == 1 && parameterTypes[fixedLen].isInstance(parameters[fixedLen])) {
+  
+                // not a varargs call
+  
+                return parameters;
+  
+            } else {
+  
+                Object array = Array.newInstance(componentType, arrayLength);
+  
+                for (int i = 0; i < arrayLength; i++) {
+  
+                    Array.set(array, i, parameters[fixedLen + i]);
+  
+                }
+  
+                Object[] parameters2 = new Object[fixedLen + 1];
+  
+                System.arraycopy(parameters, 0, parameters2, 0, fixedLen);
+  
+                parameters2[fixedLen] = array;
+  
+                return parameters2;
+  
+            }
+  
+        } else {
+  
+            return parameters;
+  
+        }
+  
+    }
+  
+
    /**
     * {@link Class#isInstance} extended to handle some important cases of primitive types.
     */
@@ -128,7 +159,7 @@ private static boolean isInstancePrimitive(@Nonnull Class<?> type, @Nonnull Obje
    public static @CheckForNull Constructor<?> constructor(@Nonnull Class<?> receiver, @Nonnull Object[] args) {
        for (Constructor<?> c : receiver.getDeclaredConstructors()) {
-  
-            if (matches(c.getParameterTypes(), args)) {
+  
+            if (matches(c.getParameterTypes(), args, c.isVarArgs())) {
                return c;
            }
        }
@@ -140,28 +171,10 @@ private static boolean isInstancePrimitive(@Nonnull Class<?> type, @Nonnull Obje
        return findMatchingMethod(receiver, method, args);
    }
-  
-    private static boolean isEnumInitializer(Method m) {
-  
-        if (!m.getDeclaringClass().isEnum()) {
-  
-            return false;
-  
-        }
-  
-        if (!m.isSynthetic()) {
-  
-            return false;
-  
-        }
-  
-        final Class<?>[] parameterTypes = m.getParameterTypes();
-  
-        if (parameterTypes.length != 1) {
-  
-            return false;
-  
-        }
-  
-        final Class<?> type = parameterTypes[0];
-  
-        if (!type.isArray()) {
-  
-            return false;
-  
-        }
-  
-        return type.getComponentType().equals(Object.class);
-  
-    }
-  
-
    private static Method findMatchingMethod(Class<?> receiver, String method, Object[] args) {
        Method candidate = null;
        for (Method m : receiver.getDeclaredMethods()) {
-  
-            if (m.getName().equals(method) && (matches(m.getParameterTypes(), args) || isEnumInitializer(m))) {
+  
+            if (m.getName().equals(method) && (matches(m.getParameterTypes(), args, m.isVarArgs()))) {
                if (candidate == null || isMoreSpecific(m, candidate)) {
                    candidate = m;
                }

diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/generic-whitelist b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/generic-whitelist
@@ -23,10 +23,13 @@ staticField groovy.lang.Closure OWNER_FIRST
 staticField groovy.lang.Closure OWNER_ONLY
 staticField groovy.lang.Closure TO_SELF
 method groovy.lang.Closure call java.lang.Object
+  
+method groovy.lang.Closure call java.lang.Object[]
 method groovy.lang.Closure curry java.lang.Object
+  
+method groovy.lang.Closure curry java.lang.Object[]
 method groovy.lang.Closure getDelegate
 method groovy.lang.Closure getResolveStrategy
 method groovy.lang.Closure ncurry int java.lang.Object
+  
+method groovy.lang.Closure ncurry int java.lang.Object[]
 method groovy.lang.Closure setDelegate java.lang.Object
 method groovy.lang.Closure setResolveStrategy int
 method groovy.lang.Script getBinding
@@ -40,6 +43,9 @@ new java.io.StringReader java.lang.String
 method java.lang.CharSequence charAt int
 method java.lang.CharSequence length
 method java.lang.Comparable compareTo java.lang.Object
+  
+method java.lang.Enum name
+  
+method java.lang.Enum ordinal
+  
+# could add valueOf, though currently the staticField’s need to be whitelisted, which is the more likely use case
 method java.lang.Object clone
 method java.lang.Object equals java.lang.Object
 method java.lang.Object getClass

diff --git a/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java b/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java
@@ -69,7 +69,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 org.junit.AssumptionViolatedException;
 import org.junit.Ignore;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
@@ -431,7 +430,7 @@ private Unsafe() {}
        new GroovyShell().evaluate("String.metaClass.getAnswer = {-> return 42}"); // privileged operation
        assertEvaluate(new StaticWhitelist(), 42, "'existence'.getAnswer()");
        assertEvaluate(new StaticWhitelist(), 42, "'existence'.answer");
-  
-        assertRejected(new StaticWhitelist("method groovy.lang.Closure call java.lang.Object"), "staticMethod java.lang.System exit int", "def c = System.&exit; c(1)");
+  
+        assertRejected(new GenericWhitelist(), "staticMethod java.lang.System exit int", "def c = System.&exit; c(1)");
    }
    @Issue("JENKINS-28277")
@@ -441,6 +440,59 @@ private Unsafe() {}
        assertEvaluate(new GenericWhitelist(), 'h', "def charAt = {str, idx -> str.charAt(idx)}; def firstChar = charAt.ncurry(1, 0); firstChar 'hello'");
    }
+  
+    @Issue("JENKINS-34739")
+  
+    @Test public void varargs() throws Exception {
+  
+        // Control cases:
+  
+        ProxyWhitelist wl = new ProxyWhitelist(new GenericWhitelist(), new AnnotatedWhitelist());
+  
+        assertEvaluate(wl, 0, "class UsesVarargs {static int len(String... vals) {vals.length}}; UsesVarargs.len(new String[0])");
+  
+        assertEvaluate(wl, 3, "class UsesVarargs {static int len(String... vals) {vals.length}}; UsesVarargs.len(['one', 'two', 'three'] as String[])");
+  
+        String uv = UsesVarargs.class.getName();
+  
+        assertEvaluate(wl, 0, uv + ".len(new String[0])");
+  
+        assertEvaluate(wl, 3, uv + ".len(['one', 'two', 'three'] as String[])");
+  
+        assertEvaluate(wl, 0, uv + ".sum(new int[0])");
+  
+        assertEvaluate(wl, 6, uv + ".sum([1, 2, 3] as int[])");
+  
+        assertEvaluate(wl, 3, uv + ".xlen(3, new String[0])");
+  
+        assertEvaluate(wl, 6, uv + ".xlen(3, ['one', 'two', 'three'] as String[])");
+  
+        assertEvaluate(wl, "one,two,three", uv + ".join(',', ['one', 'two', 'three'] as String[])");
+  
+        assertRejected(wl, "staticMethod " + uv + " explode java.lang.String[]", uv + ".explode(new String[0])");
+  
+        assertRejected(wl, "staticMethod " + uv + " explode java.lang.String[]", uv + ".explode(['one', 'two', 'three'] as String[])");
+  
+        // Test cases:
+  
+        assertEvaluate(wl, 0, "class UsesVarargs {static int len(String... vals) {vals.length}}; UsesVarargs.len()");
+  
+        assertEvaluate(wl, 3, "class UsesVarargs {static int len(String... vals) {vals.length}}; UsesVarargs.len('one', 'two', 'three')");
+  
+        assertEvaluate(wl, 0, uv + ".len()");
+  
+        assertEvaluate(wl, 3, uv + ".xlen(3)");
+  
+        assertEvaluate(wl, 0, uv + ".sum()");
+  
+        assertEvaluate(wl, 6, uv + ".sum(1, 2, 3)");
+  
+        assertEvaluate(wl, 3, uv + ".len('one', 'two', 'three')");
+  
+        assertEvaluate(wl, 6, uv + ".xlen(3, 'one', 'two', 'three')");
+  
+        assertEvaluate(wl, "one,two,three", uv + ".join(',', 'one', 'two', 'three')");
+  
+        assertRejected(wl, "staticMethod " + uv + " explode java.lang.String[]", uv + ".explode()");
+  
+        assertRejected(wl, "staticMethod " + uv + " explode java.lang.String[]", uv + ".explode('one', 'two', 'three')");
+  
+    }
+  
+    public static class UsesVarargs {
+  
+        @Whitelisted
+  
+        public static int len(String... vals) {
+  
+            return vals.length;
+  
+        }
+  
+        @Whitelisted
+  
+        public static int sum(int... numbers) {
+  
+            int sum = 0;
+  
+            for (int number : numbers) {
+  
+                sum += number;
+  
+            }
+  
+            return sum;
+  
+        }
+  
+        @Whitelisted
+  
+        public static int xlen(int x, String... vals) {
+  
+            return x + vals.length;
+  
+        }
+  
+        @Whitelisted
+  
+        public static String join(String sep, String... vals) {
+  
+            return StringUtils.join(vals, sep);
+  
+        }
+  
+        public static void explode(String... vals) {}
+  
+    }
+  
+
    @Test public void templates() throws Exception {
        final GroovyShell shell = new GroovyShell(GroovySandbox.createSecureCompilerConfiguration());
        final Template t = new SimpleTemplateEngine(shell).createTemplate("goodbye <%= aspect.toLowerCase() %> world");
@@ -593,6 +645,26 @@ public Object invokeMethod(String name, Object args) {
            + "Thing.values()[0].description\n";
        String expected = "The first thing";
        assertEvaluate(new GenericWhitelist(), expected, script);
+  
+        String e = E.class.getName();
+  
+        ProxyWhitelist wl = new ProxyWhitelist(new GenericWhitelist(), new AnnotatedWhitelist());
+  
+        assertEvaluate(wl, 2, e + ".TWO.getN()");
+  
+        assertRejected(wl, "method " + e + " explode", e + ".TWO.explode()");
+  
+        assertEvaluate(wl, "TWO", e + ".TWO.name()");
+  
+        assertRejected(wl, "staticField " + e + " ONE", e + ".ONE.name()");
+  
+    }
+  
+    public enum E {
+  
+        ONE(1),
+  
+        @Whitelisted
+  
+        TWO(2);
+  
+        private final int n;
+  
+        private E(int n) {
+  
+            this.n = n;
+  
+        }
+  
+        @Whitelisted
+  
+        public int getN() {
+  
+            return n;
+  
+        }
+  
+        public void explode() {}
    }
    private static void assertEvaluate(Whitelist whitelist, final Object expected, final String script) {