Skip to content

Commit 1321eaa

Browse files
committed
GROOVY-12279: SecureASTCustomizer: apply import rules to a method pointer's target type
The indirect import check asked about a method pointer's own type. A MethodPointerExpression fixes that to groovy.lang.Closure in its constructor, so the check never concerned the class the pointer is taken on, and its second argument was the whole expression text where assertStaticImportIsAllowed expects a member name. Both modes were wrong, in opposite directions. Measured with the check enabled: disallowedImports = [java.lang.ProcessBuilder] new ProcessBuilder() blocked ProcessBuilder.&new allowed ProcessBuilder::new allowed allowedImports = [java.util.ArrayList] ArrayList.&size refused The deny list missed the pointer entirely, since nothing names Closure in one. The allow list refused every pointer, including pointers to a class it allowed, because Closure is not in an allow list either. So the check both let through what it was meant to stop and stopped what it was meant to allow. Ask about the class the pointer is taken on, unwrapping arrays through the same helper the method call branch uses, and pass the method name rather than the expression text. This changes behaviour in both directions, each toward what the flag documents: a denied target is now refused, and a pointer to an allowed target is now permitted where it was refused before. An existing assertion covered java.util.LinkedList.&size under an allow list, and passed only because Closure was absent from that list; it passes after this change too, now for the reason it appears to be testing.
1 parent 55dcfb9 commit 1321eaa

2 files changed

Lines changed: 43 additions & 2 deletions

File tree

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1508,9 +1508,14 @@ protected void assertExpressionAuthorized(final Expression expression) throws Se
15081508
assertImportIsAllowed(typename);
15091509
assertStaticImportIsAllowed(expr.getMethod(), typename);
15101510
} else if (expression instanceof MethodPointerExpression expr) {
1511-
final String typename = expr.getType().getName();
1511+
// A method pointer's own type is fixed to groovy.lang.Closure by its
1512+
// constructor, so the class the import rules are about is the one the
1513+
// pointer is taken on, and the member is the method it names.
1514+
final String typename = getExpressionType(expr.getExpression().getType()).getName();
15121515
assertImportIsAllowed(typename);
1513-
assertStaticImportIsAllowed(expr.getText(), typename);
1516+
Expression methodName = expr.getMethodName();
1517+
assertStaticImportIsAllowed(methodName instanceof ConstantExpression
1518+
? methodName.getText() : null, typename);
15141519
}
15151520
} catch (SecurityException e) {
15161521
throw new SecurityException("Indirect import checks prevents usage of expression", e);

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,42 @@ final class SecureASTCustomizerTest {
336336
}
337337
}
338338

339+
// GROOVY-12279: a method pointer's own type is fixed to groovy.lang.Closure, so the
340+
// indirect import check was asking about Closure rather than about the class the pointer
341+
// is taken on. In deny mode that let the pointer through; in allow mode it rejected every
342+
// pointer, since Closure is never in an allow list.
343+
@Test
344+
void testIndirectImportCheckUsesMethodPointerTarget_denied() {
345+
customizer.disallowedImports = ['java.util.LinkedList']
346+
customizer.indirectImportCheckEnabled = true
347+
def shell = new GroovyShell(configuration)
348+
assert hasSecurityException {
349+
shell.evaluate('return java.util.LinkedList.&size')
350+
}
351+
assert hasSecurityException {
352+
shell.evaluate('return java.util.LinkedList::size')
353+
}
354+
// The constructor form was already checked, and stays checked.
355+
assert hasSecurityException {
356+
shell.evaluate('return new java.util.LinkedList()')
357+
}
358+
}
359+
360+
@Test
361+
void testIndirectImportCheckUsesMethodPointerTarget_allowed() {
362+
customizer.allowedImports = ['java.util.ArrayList']
363+
customizer.indirectImportCheckEnabled = true
364+
def shell = new GroovyShell(configuration)
365+
// Permitted because the target is allowed. Previously refused, because the type being
366+
// asked about was Closure, which no allow list names.
367+
shell.evaluate('return java.util.ArrayList.&size')
368+
shell.evaluate('return java.util.ArrayList::size')
369+
// A target which is not allowed is still refused.
370+
assert hasSecurityException {
371+
shell.evaluate('return java.util.LinkedList.&size')
372+
}
373+
}
374+
339375
@Test
340376
void testAllowedIndirectImports() {
341377
customizer.allowedImports = ['java.util.ArrayList']

0 commit comments

Comments
 (0)