[
https://issues.apache.org/jira/browse/GROOVY-12238?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18102763#comment-18102763
]
ASF GitHub Bot commented on GROOVY-12238:
-----------------------------------------
codecov-commenter commented on PR #2771:
URL: https://github.com/apache/groovy/pull/2771#issuecomment-5217437448
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2771?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `54.54545%` with `10 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 69.9955%. Comparing base
([`384633b`](https://app.codecov.io/gh/apache/groovy/commit/384633b18d56cea3de4393d79a06e27e8803182e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`2abfcb8`](https://app.codecov.io/gh/apache/groovy/commit/2abfcb8ce257b5a16e09137c284af80dc68f0a4b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on master.
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2771?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...roovy/control/customizers/SecureASTCustomizer.java](https://app.codecov.io/gh/apache/groovy/pull/2771?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fcontrol%2Fcustomizers%2FSecureASTCustomizer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NvbnRyb2wvY3VzdG9taXplcnMvU2VjdXJlQVNUQ3VzdG9taXplci5qYXZh)
| 54.5454% | [6 Missing and 4 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2771?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2771?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2771 +/- ##
==================================================
+ Coverage 69.9863% 69.9955% +0.0093%
- Complexity 35528 35548 +20
==================================================
Files 1557 1557
Lines 131693 131717 +24
Branches 24175 24183 +8
==================================================
+ Hits 92167 92196 +29
+ Misses 31192 31175 -17
- Partials 8334 8346 +12
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2771?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...roovy/control/customizers/SecureASTCustomizer.java](https://app.codecov.io/gh/apache/groovy/pull/2771?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fcontrol%2Fcustomizers%2FSecureASTCustomizer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NvbnRyb2wvY3VzdG9taXplcnMvU2VjdXJlQVNUQ3VzdG9taXplci5qYXZh)
| `51.7471% <54.5454%> (+0.8850%)` | :arrow_up: |
... and [9 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2771/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> 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}}.
> Four constructs are affected: constructor bodies, static initializer blocks,
> instance (object) initializer blocks, and field initializer expressions
> (static and instance).
> 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
> {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.
> Two 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 does misfire, the failure mode is a false rejection of a valid
> program, not a silent hole.
> * The {{<clinit>}} method's wrapper {{BlockStatement}} is itself synthetic
> 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. Testing
> {{SecureASTCustomizerTest}} goes from 39 to 47 tests:
> ||Test||Purpose||
> |{{testDisallowedReceiverInScriptBody}}|control; unchanged behaviour|
> |{{testDisallowedReceiverInConstructor}}|closed gap|
> |{{testDisallowedReceiverInStaticInitializer}}|closed gap|
> |{{testDisallowedReceiverInObjectInitializer}}|closed gap|
> |{{testDisallowedReceiverInFieldInitializer}}|closed gap|
> |{{testDisallowedReceiverInStaticFieldInitializer}}|closed gap|
> |{{testGeneratedScriptConstructorsAreNotChecked}}|generated
> {{super(Binding)}} stays exempt|
> |{{testTransformGeneratedConstructorIsNotChecked}}|{{@TupleConstructor}}
> output stays exempt|
> Verified:
> * the five gap 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
> * 27 further scenarios probed during development ({{@Singleton}},
> {{@Immutable}}, {{@TupleConstructor}}, {{@Canonical}}, {{@Lazy}},
> {{@Delegate}}, traits, enums, records, inner classes) under both a
> receiver-restriction config and an allow-list config showed results
> byte-identical to baseline
> * full test suite: 16551 tests, no failures
> h2. Compatibility
> This is a behavioural change: scripts which compile today under a
> {{SecureASTCustomizer}} will be rejected if their constructors or
> initializers 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. Higher false-positive risk, and it
> does not help the {{@ASTTest}} case that prompted this investigation: that
> transformation moves its closure out of the AST into node metadata and
> reconstructs it from raw source text, so no AST-level filter can see it at
> any phase.
> * *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.
> h2. Note on scope
> {{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)
