GROOVY-12238: SecureASTCustomizer does not check constructors, initia…

src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java

@@ -18,8 +18,11 @@
  */
 package org.codehaus.groovy.control.customizers;
 
+import org.codehaus.groovy.ast.ASTNode;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.CodeVisitorSupport;
+import org.codehaus.groovy.ast.ConstructorNode;
+import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.GroovyCodeVisitor;
 import org.codehaus.groovy.ast.ImportNode;
 import org.codehaus.groovy.ast.MethodNode;
@@ -193,17 +196,20 @@
  *  </pre>
  * <p>
  *  <b>Limitations.</b> Coverage is partial by design, so it is worth knowing where the checks do and
- *  do not reach. This customizer visits the script statement block and method bodies. It does
+ *  do not reach. This customizer visits the script statement block, method and constructor bodies,
+ *  static and instance initializer blocks, and field initializer expressions. It does
  *  <b>not</b> visit the following, so restrictions such as {@code disallowedReceivers}, the statement
  *  and expression allowed/disallowed lists, and any registered {@link StatementChecker} or
  *  {@link ExpressionChecker} do not apply to code appearing there:
  *  <ul>
  *  <li>annotation members, including closure arguments to annotations</li>
- *  <li>constructor bodies; note also that a constructor is not a "method definition" as far as
- *      {@link #setMethodDefinitionAllowed(boolean)} is concerned</li>
- *  <li>static and instance initializer blocks</li>
- *  <li>field initializer expressions</li>
+ *  <li>code carrying no source position, which is how constructors, initializers and fields added by
+ *      the compiler or by an AST transformation are told apart from those written by the author of
+ *      the source being secured</li>
  *  </ul>
+ *  Note that a constructor is not a "method definition" as far as
+ *  {@link #setMethodDefinitionAllowed(boolean)} is concerned: its body is checked, but declaring one
+ *  remains permitted.
  *  Import restrictions apply to actual {@code import} statements, so they have no effect on a
  *  fully-qualified reference such as {@code new java.lang.ProcessBuilder(...)}. The
  *  {@link #setIndirectImportCheckEnabled(boolean)} flag exists to catch some of those, but only
@@ -1197,6 +1203,7 @@ public void call(final SourceUnit source, final GeneratorContext context, final
                         methodNode.getCode().visit(visitor);
                     }
                 }
+                visitConstructorsAndInitializers(clNode, visitor);
             }
         }
 
@@ -1208,6 +1215,56 @@ public void call(final SourceUnit source, final GeneratorContext context, final
                 }
             }
         }
+        visitConstructorsAndInitializers(classNode, visitor);
+    }
+
+    /**
+     * Applies the security checks to code which lives outside method bodies: constructors, instance
+     * and static initializer blocks, and field initializer expressions. These are not reachable from
+     * {@link ModuleNode#getStatementBlock()} or {@link ClassNode#getMethods()}, so without this they
+     * would escape the configured restrictions entirely.
+     * <p>
+     * Only nodes carrying a source position are visited. The compiler and AST transformations add
+     * constructors, initializers and fields of their own — a script class always has generated
+     * constructors, for example — and those are not written by the author of the source being
+     * secured, so checking them would reject valid programs rather than restrict the author.
+     * Generated nodes normally carry no source position, which is what distinguishes them here.
+     *
+     * @param clNode the class to inspect
+     * @param visitor the security-checking visitor to apply
+     */
+    protected void visitConstructorsAndInitializers(final ClassNode clNode, final GroovyCodeVisitor visitor) {
+        for (ConstructorNode constructor : clNode.getDeclaredConstructors()) {
+            if (!constructor.isSynthetic() && constructor.getCode() != null && isFromSource(constructor)) {
+                constructor.getCode().visit(visitor);
+            }
+        }
+        for (Statement statement : clNode.getObjectInitializerStatements()) {
+            if (isFromSource(statement)) statement.visit(visitor);
+        }
+        for (MethodNode staticInitializer : clNode.getMethods("<clinit>")) {
+            // the <clinit> method is always synthetic, but the statements within it need not be
+            if (staticInitializer.getCode() instanceof BlockStatement block) {
+                for (Statement statement : block.getStatements()) {
+                    if (isFromSource(statement)) statement.visit(visitor);
+                }
+            }
+        }
+        for (FieldNode field : clNode.getFields()) {
+            Expression initialValue = field.getInitialExpression();
+            if (initialValue != null && isFromSource(initialValue)) initialValue.visit(visitor);
+        }
+    }
+
+    /**
+     * Indicates whether a node originates from the source being compiled rather than from the
+     * compiler or an AST transformation.
+     *
+     * @param node the node to test
+     * @return {@code true} if the node carries a source position
+     */
+    private static boolean isFromSource(final ASTNode node) {
+        return node.getLineNumber() > 0;
     }
 
     /**

src/spec/doc/core-domain-specific-languages.adoc

@@ -817,16 +817,20 @@ Expressions can be checked using gapi:org.codehaus.groovy.control.customizers.Se
 ==== Limitations of the secure AST customizer
 
 Coverage is partial by design, so it is worth knowing where the checks do and do not
-reach. The customizer visits the script statement block and method bodies. It does *not*
+reach. The customizer visits the script statement block, method and constructor bodies,
+static and instance initializer blocks, and field initializer expressions. It does *not*
 visit the following, so restrictions such as `disallowedReceivers`, the statement and
 expression allow/disallow lists, and your own custom checkers do not apply to code
 appearing there:
 
 * annotation members, including closure arguments to annotations
-* constructor bodies — note also that a constructor is not a ``method definition'' as far
-  as `methodDefinitionAllowed` is concerned
-* static and instance initializer blocks
-* field initializer expressions
+* code carrying no source position, which is how constructors, initializers and fields
+  added by the compiler or by an AST transformation are told apart from those written by
+  the author of the source being secured — a script class always has generated
+  constructors, for example, and checking those would reject valid programs
+
+Note that a constructor is not a ``method definition'' as far as `methodDefinitionAllowed`
+is concerned: its body is checked, but declaring one remains permitted.
 
 Import restrictions apply to actual `import` statements, so they have no effect on a
 fully-qualified reference such as `new java.lang.ProcessBuilder(...)`. The

src/test/groovy/org/codehaus/groovy/control/customizers/SecureASTCustomizerTest.groovy

@@ -754,4 +754,109 @@ final class SecureASTCustomizerTest {
             '''
         }
     }
+
+    //--------------------------------------------------------------------------
+    // code outside method bodies: constructors and initializers
+
+    private void disallowSystemReceiver() {
+        customizer.disallowedReceivers = ['java.lang.System']
+    }
+
+    @Test
+    void testDisallowedReceiverInScriptBody() {
+        disallowSystemReceiver()
+        def shell = new GroovyShell(configuration)
+        assert hasSecurityException {
+            shell.evaluate "System.getProperty('java.version')"
+        }
+    }
+
+    @Test
+    void testDisallowedReceiverInConstructor() {
+        disallowSystemReceiver()
+        def shell = new GroovyShell(configuration)
+        assert hasSecurityException {
+            shell.evaluate '''
+                class A { A() { System.getProperty('java.version') } }
+                new A()
+            '''
+        }
+    }
+
+    @Test
+    void testDisallowedReceiverInStaticInitializer() {
+        disallowSystemReceiver()
+        def shell = new GroovyShell(configuration)
+        assert hasSecurityException {
+            shell.evaluate '''
+                class A { static { System.getProperty('java.version') } }
+                new A()
+            '''
+        }
+    }
+
+    @Test
+    void testDisallowedReceiverInObjectInitializer() {
+        disallowSystemReceiver()
+        def shell = new GroovyShell(configuration)
+        assert hasSecurityException {
+            shell.evaluate '''
+                class A { { System.getProperty('java.version') } }
+                new A()
+            '''
+        }
+    }
+
+    @Test
+    void testDisallowedReceiverInFieldInitializer() {
+        disallowSystemReceiver()
+        def shell = new GroovyShell(configuration)
+        assert hasSecurityException {
+            shell.evaluate '''
+                class A { def f = System.getProperty('java.version') }
+                new A()
+            '''
+        }
+    }
+
+    @Test
+    void testDisallowedReceiverInStaticFieldInitializer() {
+        disallowSystemReceiver()
+        def shell = new GroovyShell(configuration)
+        assert hasSecurityException {
+            shell.evaluate '''
+                class A { static def f = System.getProperty('java.version') }
+                new A()
+            '''
+        }
+    }
+
+    @Test
+    void testGeneratedScriptConstructorsAreNotChecked() {
+        // every script class has generated constructors which call super(Binding); they are not
+        // written by the author of the script, so they must not be subject to the restrictions
+        customizer.with {
+            disallowedReceivers = ['java.lang.System']
+            allowedExpressions = [BinaryExpression, ConstantExpression]
+        }
+        def shell = new GroovyShell(configuration)
+        shell.evaluate '1 + 1'
+        // no error means success
+    }
+
+    @Test
+    void testTransformGeneratedConstructorIsNotChecked() {
+        // @TupleConstructor generates a constructor, which likewise is not authored source
+        customizer.with {
+            disallowedReceivers = ['java.lang.System']
+            indirectImportCheckEnabled = true
+        }
+        def shell = new GroovyShell(configuration)
+        shell.evaluate '''
+            @groovy.transform.TupleConstructor
+            class A { String a }
+            new A('x')
+        '''
+        // no error means success
+    }
 }