GROOVY-5277: SecureASTCustomizer fails to visit classes

groovy · Feb 6, 2012 · dda7e0a · dda7e0a
1 parent 5f5973e
commit dda7e0a
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 9 deletions.
diff --git a/src/main/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java b/src/main/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java
@@ -516,10 +516,7 @@ public void call(final SourceUnit source, final GeneratorContext context, final
         if (!isPackageAllowed && ast.getPackage() != null) {
             throw new SecurityException("Package definitions are not allowed");
         }
-        final List<MethodNode> methods = ast.getMethods();
-        if (!isMethodDefinitionAllowed && methods !=null && methods.size()>0) {
-            throw new SecurityException("Method definitions are not allowed");
-        }
+        checkMethodDefinitionAllowed(classNode);
 
         // verify imports
         if (importsBlacklist != null || importsWhitelist != null || starImportsBlacklist != null || starImportsWhitelist != null) {
@@ -547,13 +544,42 @@ public void call(final SourceUnit source, final GeneratorContext context, final
 
         final SecuringCodeVisitor visitor = new SecuringCodeVisitor();
         ast.getStatementBlock().visit(visitor);
+        for (ClassNode clNode : ast.getClasses()) {
+            if (clNode!=classNode) {
+                checkMethodDefinitionAllowed(clNode);
+                for (MethodNode methodNode : clNode.getMethods()) {
+                    if (!methodNode.isSynthetic()) {
+                        methodNode.getCode().visit(visitor);
+                    }
+                }
+            }
+        }
 
-        if (isMethodDefinitionAllowed && methods !=null) {
+        List<MethodNode> methods = filterMethods(classNode);
+        if (isMethodDefinitionAllowed) {
             for (MethodNode method : methods) {
-                method.getCode().visit(visitor);
+                if (method.getDeclaringClass()==classNode) method.getCode().visit(visitor);
             }
         }
     }
+
+    private void checkMethodDefinitionAllowed(ClassNode owner) {
+        if (isMethodDefinitionAllowed) return;
+        List<MethodNode> methods = filterMethods(owner);
+        if (!methods.isEmpty()) throw new SecurityException("Method definitions are not allowed");
+    }
+
+    private List<MethodNode> filterMethods(ClassNode owner) {
+        List<MethodNode> result = new LinkedList<MethodNode>();
+        List<MethodNode> methods = owner.getMethods();
+        for (MethodNode method : methods) {
+            if (method.getDeclaringClass() == owner && !method.isSynthetic()) {
+                if ("main".equals(method.getName()) || "run".equals(method.getName()) && owner.isScriptBody()) continue;
+                result.add(method);
+            }
+        }
+        return result;
+    }
 
     private void assertStarImportIsAllowed(final String packageName) {
         if (starImportsWhitelist != null && !starImportsWhitelist.contains(packageName)) {
@@ -649,13 +675,13 @@ private void assertStatementAuthorized(final Statement statement) throws Securit
         private void assertExpressionAuthorized(final Expression expression) throws SecurityException {
             final Class<? extends Expression> clazz = expression.getClass();
             if (expressionsBlacklist != null && expressionsBlacklist.contains(clazz)) {
-                throw new SecurityException(clazz.getSimpleName() + "s are not allowed");
+                throw new SecurityException(clazz.getSimpleName() + "s are not allowed: " + expression.getText());
             } else if (expressionsWhitelist != null && !expressionsWhitelist.contains(clazz)) {
-                throw new SecurityException(clazz.getSimpleName() + "s are not allowed");
+                throw new SecurityException(clazz.getSimpleName() + "s are not allowed: " + expression.getText());
             }
             for (ExpressionChecker expressionChecker : expressionCheckers) {
                 if (!expressionChecker.isAuthorized(expression)) {
-                    throw new SecurityException("Expression [" + clazz.getSimpleName() + "] is not allowed");
+                    throw new SecurityException("Expression [" + clazz.getSimpleName() + "] is not allowed: " + expression.getText());
                 }
             }
             if (isIndirectImportCheckEnabled) {

diff --git a/src/test/org/codehaus/groovy/control/customizers/SecureASTCustomizerTest.groovy b/src/test/org/codehaus/groovy/control/customizers/SecureASTCustomizerTest.groovy
@@ -81,6 +81,37 @@ class SecureASTCustomizerTest extends GroovyTestCase {
         }
     }
 
+    void testMethodDefinitionInClass() {
+        def shell = new GroovyShell(configuration)
+        String script = """
+            class A {
+                def method() {
+                    true
+                }
+            }
+            new A()
+        """
+        shell.evaluate(script)
+        // no error means success
+        customizer.methodDefinitionAllowed = false
+        assert hasSecurityException {
+            shell.evaluate(script)
+        }
+    }
+
+    void testClassExtendingClassWithMethods() {
+        def shell = new GroovyShell(configuration)
+        String script = """
+            class A extends LinkedList {
+            }
+            new A()
+        """
+        shell.evaluate(script)
+        // no error means success
+        customizer.methodDefinitionAllowed = false
+        shell.evaluate(script)
+    }
+
     void testExpressionWhiteList() {
         def shell = new GroovyShell(configuration)
         customizer.expressionsWhitelist = [BinaryExpression, ConstantExpression]