Skip to content

GROOVY-12283: SecureASTCustomizer: apply import rules to construction-coercion casts and s… - #2819

Open
paulk-asert wants to merge 1 commit into
apache:masterfrom
paulk-asert:groovy12283
Open

GROOVY-12283: SecureASTCustomizer: apply import rules to construction-coercion casts and s…#2819
paulk-asert wants to merge 1 commit into
apache:masterfrom
paulk-asert:groovy12283

Conversation

@paulk-asert

@paulk-asert paulk-asert commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

…ubscripts

When using SecureASTCustomizer, the indirect import check inspected constructor, method, static-method and method-pointer expressions, so a class forbidden by the import rules could still be built through a construction that is neither a constructor call nor a method call: a cast whose operand is a list, map or closure literal ((Foo) [..], [..] as Foo, (Runnable) { }), and a named-argument subscript (Foo[a: 1]). Each builds an instance of the named type.

The check is extended to both. A cast constructs when its operand is a list, map or closure literal, as opposed to converting a value that already exists; its target type is checked like a constructor call (array component unwrapped, primitive components skipped as they name no class). A subscript constructs when its arguments are map entries, which are not valid in an ordinary subscript, so their presence marks the form unambiguously; the receiver type is dynamic at this phase, so the class is named by its source text.

Plain converting casts ((String) x, (int) n) and positional subscripts stay unexamined. The residual is the non-literal coercion ((Foo) var, var as Foo), where an overridden asType could construct at runtime; that is statically invisible and out of scope, consistent with this customizer being a hardening aid rather than a security boundary.

@paulk-asert
paulk-asert requested a lite review from Copilot August 19, 2026 09:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Extends SecureASTCustomizer’s indirect import checks to cover additional Groovy construction forms that can instantiate types without explicit constructor/method calls (list/map/closure coercion casts and named-argument subscripts), closing an import-rule bypass.

Changes:

  • Apply indirect import checks to coercion casts that materialize instances from list/map/closure literals.
  • Apply indirect import checks to named-argument subscript “construction” form (Foo[a: 1]).
  • Add regression tests and a helper class to validate the new blocking/allowing behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java Adds detection for construction-by-coercion casts and named-argument subscript construction, enforcing import rules in those cases.
src/test/groovy/org/codehaus/groovy/control/customizers/SecureASTCustomizerTest.groovy Adds tests covering the newly blocked coercion/subscript construction paths and permitted coercions.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java Outdated
Comment thread src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java Outdated
Comment thread src/main/java/org/codehaus/groovy/control/customizers/SecureASTCustomizer.java Outdated
@paulk-asert paulk-asert changed the title GROOVY-12283: apply import rules to construction-coercion casts and s… GROOVY-12283: SecureASTCustomizer - apply import rules to construction-coercion casts and s… Aug 19, 2026
@paulk-asert paulk-asert changed the title GROOVY-12283: SecureASTCustomizer - apply import rules to construction-coercion casts and s… GROOVY-12283: SecureASTCustomizer: apply import rules to construction-coercion casts and s… Aug 19, 2026
@codecov-commenter

codecov-commenter commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.1817%. Comparing base (1c2f0f2) to head (86607de).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
...roovy/control/customizers/SecureASTCustomizer.java 70.5882% 3 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##               master      #2819        +/-   ##
==================================================
+ Coverage     70.1794%   70.1817%   +0.0023%     
- Complexity      35853      35885        +32     
==================================================
  Files            1563       1564         +1     
  Lines          132546     132620        +74     
  Branches        24385      24407        +22     
==================================================
+ Hits            93020      93075        +55     
- Misses          31113      31131        +18     
- Partials         8413       8414         +1     
Files with missing lines Coverage Δ
...roovy/control/customizers/SecureASTCustomizer.java 55.5556% <70.5882%> (+1.3210%) ⬆️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ubscripts

The indirect import check inspected constructor, method, static-method and
method-pointer expressions, so a class forbidden by the import rules could
still be built through a construction that is neither a constructor call nor
a method call: a cast whose operand is a list, map or closure literal
((Foo) [..], [..] as Foo, (Runnable) { }), and a named-argument subscript
(Foo[a: 1]). Each builds an instance of the named type.

The check is extended to both. A cast constructs when its operand is a list,
map or closure literal, as opposed to converting a value that already exists;
its target type is checked like a constructor call (array component unwrapped,
primitive components skipped as they name no class). A subscript constructs
when its arguments are map entries, which are not valid in an ordinary
subscript, so their presence marks the form unambiguously; the receiver type
is dynamic at this phase, so the class is named by its source text.

Plain converting casts ((String) x, (int) n) and positional subscripts stay
unexamined. The residual is the non-literal coercion ((Foo) var, var as Foo),
where an overridden asType could construct at runtime; that is statically
invisible and out of scope, consistent with this customizer being a hardening
aid rather than a security boundary.
@testlens-app

testlens-app Bot commented Aug 19, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 86607de
▶️ Tests: 102689 executed
⚪️ Checks: 31/31 completed


Learn more about TestLens at testlens.app/docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants