Copilot commented on code in PR #2819:
URL: https://github.com/apache/groovy/pull/2819#discussion_r3811707528
##########
src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java:
##########
@@ -1557,6 +1576,44 @@ protected ClassNode getExpressionType(ClassNode
objectExpressionType) {
return objectExpressionType.isArray() ?
getExpressionType(objectExpressionType.getComponentType()) :
objectExpressionType;
}
+ /**
+ * Whether a cast constructs an instance of its type by coercing a
literal operand — a list
+ * or map (invoking a constructor) or a closure (creating a SAM proxy)
— as opposed to
+ * converting a value that already exists. Such a cast is treated like
a constructor call by
+ * the indirect import check (GROOVY-12283).
+ *
+ * @param cast the cast expression
+ * @return {@code true} if the cast materialises a new instance of its
type
+ */
+ private static boolean constructsByCoercion(final CastExpression cast)
{
+ Expression operand = cast.getExpression();
+ return operand instanceof ListExpression
+ || operand instanceof MapExpression
+ || operand instanceof ClosureExpression;
+ }
+
+ /**
+ * Whether a subscript is a named-argument construction such as
+ * {@code Foo[name: 'x', size: 2]} rather than an ordinary index
access. Map entries are not
+ * valid in a real subscript, so their presence uniquely marks the
construction form
+ * (GROOVY-12283).
+ *
+ * @param expression the binary expression
+ * @return {@code true} if the expression constructs by named arguments
+ */
+ private static boolean isNamedArgConstruction(final BinaryExpression
expression) {
+ if (!"[".equals(expression.getOperation().getText())) {
+ return false;
+ }
+ Expression right = expression.getRightExpression();
+ if (right instanceof SpreadMapExpression) {
+ return true;
+ }
+ return right instanceof ListExpression
+ && ((ListExpression) right).getExpressions().stream()
+ .anyMatch(e -> e instanceof MapEntryExpression ||
e instanceof SpreadMapExpression);
Review Comment:
This check runs during AST traversal and may be executed very frequently.
Using a stream here allocates extra objects and can add overhead; a simple
indexed/foreach loop over `getExpressions()` can perform the same check with
less allocation and typically better performance in compiler-phase code.
##########
src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java:
##########
@@ -1540,6 +1545,20 @@ protected void assertExpressionAuthorized(final
Expression expression) throws Se
final String typename = expr.getType().getName();
assertImportIsAllowed(typename);
assertStaticImportIsAllowed(expr.getText(), typename);
+ } else if (expression instanceof CastExpression expr &&
constructsByCoercion(expr)) {
+ // GROOVY-12283: a cast whose operand is a list, map
or closure literal
+ // constructs an instance of the cast type (list/map
-> constructor,
+ // closure -> SAM proxy) rather than converting an
existing value, so it
+ // is checked like a constructor call. Covers `(Foo)
[..]` and `[..] as Foo`.
+ ClassNode target = getExpressionType(expr.getType());
// array -> component
+ if (!ClassHelper.isPrimitiveType(target)) { // e.g.
(int[]) [1, 2] has no class to check
+ assertImportIsAllowed(target.getName());
+ }
+ } else if (expression instanceof BinaryExpression expr &&
isNamedArgConstruction(expr)) {
+ // GROOVY-12283: `Foo[name: 'x', ..]` is a
named-argument construction of
+ // Foo, not a subscript (map entries are not valid in
a real subscript). The
+ // receiver type is dynamic here, so the class is
named by its source text.
+
assertImportIsAllowed(expr.getLeftExpression().getText());
}
} catch (SecurityException e) {
throw new SecurityException("Indirect import checks
prevents usage of expression", e);
Review Comment:
The exception message has a grammatical error (“checks prevents”) and is
very generic. Since this code path will now trigger for additional expression
kinds (casts/subscripts), it would be more actionable to fix the wording (e.g.,
“checks prevent”) and include some minimal context (such as the offending
expression text or its class) to help users identify what was blocked.
##########
src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java:
##########
@@ -1557,6 +1576,44 @@ protected ClassNode getExpressionType(ClassNode
objectExpressionType) {
return objectExpressionType.isArray() ?
getExpressionType(objectExpressionType.getComponentType()) :
objectExpressionType;
}
+ /**
+ * Whether a cast constructs an instance of its type by coercing a
literal operand — a list
+ * or map (invoking a constructor) or a closure (creating a SAM proxy)
— as opposed to
+ * converting a value that already exists. Such a cast is treated like
a constructor call by
+ * the indirect import check (GROOVY-12283).
+ *
+ * @param cast the cast expression
+ * @return {@code true} if the cast materialises a new instance of its
type
+ */
+ private static boolean constructsByCoercion(final CastExpression cast)
{
+ Expression operand = cast.getExpression();
+ return operand instanceof ListExpression
+ || operand instanceof MapExpression
+ || operand instanceof ClosureExpression;
+ }
+
+ /**
+ * Whether a subscript is a named-argument construction such as
+ * {@code Foo[name: 'x', size: 2]} rather than an ordinary index
access. Map entries are not
+ * valid in a real subscript, so their presence uniquely marks the
construction form
+ * (GROOVY-12283).
+ *
+ * @param expression the binary expression
+ * @return {@code true} if the expression constructs by named arguments
+ */
+ private static boolean isNamedArgConstruction(final BinaryExpression
expression) {
+ if (!"[".equals(expression.getOperation().getText())) {
+ return false;
+ }
+ Expression right = expression.getRightExpression();
+ if (right instanceof SpreadMapExpression) {
+ return true;
+ }
Review Comment:
`SpreadMapExpression` is now explicitly treated as named-argument
construction, but the added tests only cover map-entry style (`Foo[a: 1]`) and
not the spread-map forms (e.g., `Foo[*: someMap]` or mixtures like `Foo[a: 1,
*: someMap]`). Adding a regression test for the spread-map variant would ensure
this branch stays correct and prevents future regressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
