[
https://issues.apache.org/jira/browse/GROOVY-12238?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18103193#comment-18103193
]
ASF GitHub Bot commented on GROOVY-12238:
-----------------------------------------
testlens-app[bot] commented on PR #2771:
URL: https://github.com/apache/groovy/pull/2771#issuecomment-5234330511
## ✅ All tests passed ✅
🏷️ Commit: 29c82d84d20e3783361cccf9bdb0cda77b7166a3
▶️ Tests: 108906 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
> SecureASTCustomizer does not check constructors, initializer blocks or field
> initializers
> -----------------------------------------------------------------------------------------
>
> Key: GROOVY-12238
> URL: https://issues.apache.org/jira/browse/GROOVY-12238
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Problem
> {{SecureASTCustomizer}} visits the script statement block and method bodies
> only. Code which lives outside a method body is never handed to the securing
> visitor, so none of the configured restrictions apply to it - not
> {{disallowedReceivers}}, not the statement or expression allow/deny lists,
> and not any registered {{StatementChecker}} or {{ExpressionChecker}}.
> With {{disallowedReceivers = ['java.lang.System']}} configured, every one of
> the following compiles and runs today, while the same call in the script body
> is correctly rejected:
> {code:groovy}
> class A { A() { System.getProperty('x') } } // constructor
> class B { static { System.getProperty('x') } } // static initializer
> class C { { System.getProperty('x') } } // instance
> initializer
> class D { def f = System.getProperty('x') } // field initializer
> class E { static def f = System.getProperty('x') } // static field
> initializer
> @TupleConstructor(pre={ System.getProperty('x') }) // relocated into the
> class F { String s } // generated
> constructor
> {code}
> The existing filters cannot be reused to reach these, for three separate
> reasons:
> * a static initializer block ends up inside a {{<clinit>}} method, and
> {{<clinit>}} is *synthetic*, so it is excluded both by {{filterMethods}} and
> by the {{!isSynthetic()}} test in the sibling-class loop;
> * instance initializer blocks live in
> {{ClassNode.getObjectInitializerStatements()}}, a separate list which is
> never read;
> * field initializers live in {{FieldNode.getInitialExpression()}}, and the
> backing fields of Groovy properties are themselves *synthetic*, so the
> synthetic flag cannot be used to filter them either.
> h2. Change
> Adds {{SecureASTCustomizer.visitConstructorsAndInitializers(ClassNode,
> GroovyCodeVisitor)}}, called for every class in the module, which applies the
> existing securing visitor to:
> * declared constructors (non-synthetic, with a body)
> * {{getObjectInitializerStatements()}}
> * the statements inside {{<clinit>}}
> * {{FieldNode.getInitialExpression()}}
> The method is {{protected}} so subclasses can adjust it, consistent with
> {{createGroovyCodeVisitor}} and {{filterMethods}}.
> h2. Distinguishing generated code
> This is the part worth reviewing closely.
> Constructors and initializers are not written solely by the author of the
> source being secured. The compiler generates constructors for every script
> class, and AST transformations add constructors, fields and initializer
> statements of their own. Checking those rejects valid programs rather than
> restricting the author.
> A first cut which visited everything broke 4 of the 81 existing customizer
> tests, all on generated code:
> {noformat}
> ConstructorCallExpressions are not allowed: super (context)
> Usage of variables of type [groovy.lang.Binding] is not allowed
> Indirect import checks prevents usage of expression (x2)
> {noformat}
> Those are the script class's generated {{Script()}} and {{Script(Binding)}}
> constructors - and they are *not* marked synthetic, so there is no flag
> available to exclude them. The discriminator used instead is the source
> position: a node is visited only when {{getLineNumber() > 0}}, expressed as
> an {{isFromSource(ASTNode)}} helper. Generated nodes normally carry no source
> position.
> Caveats a reviewer should weigh:
> * This is a heuristic, not a guarantee. There are ~220 {{setSourcePosition}}
> calls in the main source; a transformation which copies a source position
> onto a generated constructor, field initializer or initializer statement
> would have that code checked. Nothing found in testing does so, and {{@Grab}}
> looks safe by inspection - it injects via {{addStaticInitializerStatements}}
> and {{addObjectInitializerStatements}} using {{stmt()}}/{{callX()}} helpers,
> which do not set positions - but {{@Grab}} was not exercised directly. If the
> heuristic misfires, the failure mode is a false rejection of a valid program,
> not a silent hole.
> * The {{<clinit>}} method's wrapper {{BlockStatement}} is itself generated
> even when its statements are not, so the filter has to be applied per
> statement rather than to the method body. Applying it at the method level
> leaves static initializer blocks open.
> h2. Authored code relocated into generated members
> A generated member may nonetheless *contain* code the author wrote, because a
> transformation can move it there. {{@TupleConstructor(pre=...)}} and
> {{@MapConstructor(pre=...)}} relocate the supplied closure body into the
> constructor they generate. Dumping the AST at CANONICALIZATION shows what
> happens:
> {noformat}
> @TupleConstructor(pre={ System.getProperty('x') })
> ctor line=-1 synthetic=false
> stmt[0] line=2 java.lang.System.getProperty(x) <- authored, relocated
> here
> stmt[1] line=-1 (this.s = s) <- generated
> {noformat}
> The author's statement keeps the source position it had in the original
> source. So a member with no source position of its own is filtered *statement
> by statement* rather than skipped outright - the same treatment {{<clinit>}}
> already needs. Without this, {{@TupleConstructor(pre=...)}} and
> {{@MapConstructor(pre=...)}} escape the restrictions entirely.
> This also answers a question raised separately on GROOVY-12239, which
> proposed visiting annotation members. Annotation members turn out to be the
> wrong lever: by CANONICALIZATION the closure supplied to an annotation has
> usually been *moved* - into a constructor or a method - where it remains
> reachable by source position without touching annotations at all.
> GROOVY-12239 was closed on that basis.
> h2. Testing
> {{SecureASTCustomizerTest}} goes from 39 to 49 tests:
> ||Test||Purpose||
> |{{testDisallowedReceiverInScriptBody}}|control; unchanged behaviour|
> |{{testDisallowedReceiverInConstructor}}|closed gap|
> |{{testDisallowedReceiverInStaticInitializer}}|closed gap|
> |{{testDisallowedReceiverInObjectInitializer}}|closed gap|
> |{{testDisallowedReceiverInFieldInitializer}}|closed gap|
> |{{testDisallowedReceiverInStaticFieldInitializer}}|closed gap|
> |{{testDisallowedReceiverMovedIntoGeneratedConstructor}}|{{@TupleConstructor(pre=...)}}
> relocation|
> |{{testDisallowedReceiverMovedIntoGeneratedMapConstructor}}|{{@MapConstructor(pre=...)}}
> relocation|
> |{{testGeneratedScriptConstructorsAreNotChecked}}|generated
> {{super(Binding)}} stays exempt|
> |{{testTransformGeneratedConstructorIsNotChecked}}|{{@TupleConstructor}}
> output stays exempt|
> Verified:
> * the five gap tests and the two relocation tests fail against unmodified
> master and pass with the change; the script-body control passes in both
> * the two exemption tests pass in both, by design - they exist to stop a
> later simplification from dropping the source-position check, which is the
> property most likely to regress silently
> * probe scenarios run during development ({{@Singleton}}, {{@Immutable}},
> {{@TupleConstructor}}, {{@Canonical}}, {{@Lazy}}, {{@Delegate}}, traits,
> enums, records, inner classes, user constructors) under both a
> receiver-restriction config and an allow-list config showed results identical
> to baseline
> * full test suite: 16553 tests, no failures
> h2. Compatibility
> This is a behavioural change: scripts which compile today under a
> {{SecureASTCustomizer}} will be rejected if their constructors, initializers
> or relocated closure bodies violate the configured restrictions. That is the
> intent, but per
> [COMPATIBILITY.md|https://github.com/apache/groovy/blob/master/COMPATIBILITY.md]
> it is a breaking change and wants a dev@ decision plus a major version.
> Targeted at 6.0.
> Both {{Limitations}} sections - the user guide and the
> {{SecureASTCustomizer}} javadoc - are updated in the same change, since they
> currently document these gaps as behaviour.
> h2. Out of scope
> * *Annotation members* remain unvisited - see GROOVY-12239 for the audit and
> the reasoning above.
> * *Synthetic methods* remain unvisited. A transformation may relocate
> authored code there too: {{ConditionalInterruptibleASTTransformation}} moves
> its condition into a method created by {{addSyntheticMethod}}, and applying
> the same per-statement filter there would catch {{@ConditionalInterrupt}}.
> That was prototyped and works, but it buys one annotation and "synthetic"
> covers a great deal of compiler-generated code, so it deserves broader
> evidence before shipping. Deliberately deferred.
> * *Constructors still do not count towards {{methodDefinitionAllowed}}*.
> Their bodies are now checked, but declaring a constructor remains permitted.
> Making constructors count is a second, separable breaking change.
> * *{{@ASTTest}} is unreachable on every path*, because its transformation
> moves the closure out of the AST into node metadata and reconstructs it from
> the raw source text.
> h2. Scope note
> {{SecureASTCustomizer}} is a best-effort grammar filter, not a security
> boundary - see THREAT_MODEL.md sections 3, 9 and 11a. This change is
> hardening which removes behaviour that is surprising to a developer following
> the documentation; it does not alter that position, and a demonstrated bypass
> remains by design rather than a vulnerability.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
