matrei commented on PR #16331:
URL: https://github.com/apache/grails-core/pull/16331#issuecomment-5615601359
# AI Review Findings (round 3)
Head `d88d15ea15` on `8.0.x` base `0980623481`. The 13 files this PR touches
are byte-identical to the round-2 head `840aea9b91`
(jamesfredley/grails-core#4); the only new commit is the merge of `8.0.x`. So
this round is a status check against the two earlier reviews rather than a
fresh read. I re-ran everything on this head: `:grails-gradle-common:test`
(12), `:grails-gradle-model:test` (56),
`:grails-testing-support-http-client:test` (105), all green, plus `codeStyle`
on the three modules. I also re-ran the JSTL spec from round 2 under
`grails-gsp/plugin`: on this head `resolveTagLibrary('jakarta.tags.core')`
still throws `SAXParseException: DOCTYPE is disallowed` when `c-1_0-rt.tld` is
in the scan pattern, and passes with `grails.xml.allowDocTypeDeclaration=true`.
The three replies on the Copilot threads are fine as answers to Copilot.
None of the findings from the two earlier rounds has a reply or a code change,
so they are listed again below with their current status.
## [P1] Still open: the opt-in is global, so using JSTL also relaxes
request-body parsing
**Files:**
-
`grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:462-475`
-
`grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/TldReader.groovy:49`
-
`grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/WebXmlTagLibraryReader.groovy:47`
-
`grails-core/src/main/groovy/org/apache/grails/core/plugins/PluginUtils.java:141`
-
`grails-doc/src/en/guide/theWebLayer/gsp/taglibs/usingJSPTagLibraries.adoc:54-64`
Unchanged from round 2. The default `grails.gsp.tldScanPattern` includes
`c-1_0-rt.tld`, that file carries a JSP 1.2 DOCTYPE, so every application that
adds JSTL breaks on its first JSP tag unless it sets the flag, and the flag
then also admits DOCTYPE in `application/xml` request bodies. The enum's own
javadoc on `DISALLOW_DOCTYPE_DECL` ("a parser shared with those callers must
leave this disabled") describes the problem the code now has.
The ask is the same: give the three trusted-descriptor readers a
`createXmlSlurper(true)` / `newSAXParser(true)` overload backed by the
`docTypeParserFactory` this PR already builds, keep the strict factory for
request bodies unconditionally, and drop the JSP doc paragraph. Both cached
factories and `buildParserFactory(boolean)` carry over as they are. Whether
`grails.xml.allowDocTypeDeclaration` survives as a request-body-only switch is
a separate decision; if it does, it should only select which factory the no-arg
methods return.
If the maintainers decide the global switch is the intended shape, that is a
legitimate call, but it should be said on the PR so the next reviewer does not
re-raise it, and the JSP doc paragraph then needs to say plainly that the
setting also affects request bodies.
## [P2] Still open: the setting is resolved through `Metadata`, and the docs
do not say what that excludes
**Files:**
-
`grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:477-480`
-
`grails-gradle/model/src/main/groovy/grails/util/Metadata.groovy:126-165,168-173`
- `grails-doc/src/en/guide/upgrading.adoc:34-51`
I re-read `Metadata` on this head to make sure the round-2 claim holds.
`loadFromDefault()` reads the classpath `application.yml`, `grails.build.info`
and system properties, and `loadYml()` keeps only `propertySources[0]`, so
`environments:` blocks map to `environments.production.grails.xml...` and are
never consulted, `application.groovy` is never evaluated, and there is no
relaxed-binding path for `GRAILS_XML_ALLOWDOCTYPEDECLARATION`. The upgrade note
says "in `application.yml`, or as a system property", which a reader will take
to mean the normal Grails configuration rules. One sentence stating that only a
top-level `application.yml` key or the system property is honoured, and that
`environments:` and `application.groovy` are not, closes this. Under the P1
shape the flag might go away entirely, which also closes it.
One new observation while reading this: `createParserFactory()` calls
`Metadata.getCurrent()` on every parse, and `Metadata` is held in a
`SoftReference`. Under memory pressure the reference is cleared and the next
XML request body re-reads `application.yml` and `grails.build.info` from the
classpath before parsing. Cheap, but it is a new classpath read on the request
path, and one more reason to resolve the choice once at the call site rather
than per parse.
## [P2] Still open: the regression path has no test in the repository
**Files:**
-
`grails-gsp/plugin/src/test/groovy/org/grails/gsp/jsp/TagLibraryResolverTests.groovy:30-37`
- `grails-gsp/plugin/build.gradle:189`
`SpringIOUtilsSpec` parses a string shaped like a JSP 1.2 descriptor;
nothing scans the real `c-1_0-rt.tld` through `TagLibraryResolverImpl`.
`grails-gsp/plugin` already has `org.glassfish.web:jakarta.servlet.jsp.jstl` on
`testRuntimeOnly`, and `TagLibraryResolverTests` already builds the resolver by
hand, so the two-case spec from round 2 drops in unchanged. It is the test that
would have caught #16310 and the one that keeps the default `tldScanPattern`
honest. I ran it again on this head; both cases behave as described above.
## [P2] Still open: a rejected hardening feature leaves no trace
**Files:**
-
`grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:481-503`
-
`grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy:223-237`
The reply to Copilot on this thread says failing fast would change supported
behaviour, which is true, and neither earlier review asked for that. The ask is
a `LOG.warn` naming the factory class and the feature inside each catch.
`XmlParserFeatureSpec` pins the identifiers against the JDK parser at test
time; the warning covers the runtime case where another SAX provider is first
on the classpath and silently fails open, which is exactly how this went
unnoticed for two years. `grails-gradle-model` already has `slf4j-api` as `api`.
## [P2] Still open: `THREAT_MODEL.md` §9 and §14 Q13 contradict the code
**Files:**
- `THREAT_MODEL.md:346`
- `THREAT_MODEL.md:470`
Both lines are unchanged on this head. §9 still says the framework does not
impose a parser configuration and Q13 still proposes confirming that. After
this PR the framework imposes one on every XML request body it binds. Leaving
the text as is means the next security review re-reports f002 against a
document that says the fix does not exist. This can be a one-paragraph change
in this PR or a follow-up, but it should be tracked.
## [P3] Still open: the small items from #16310
None of these moved since round 1:
- `XmlUtils.groovy:116` javadoc drops "disables external entity expansion
plus external DTD loading" although both features are still set; the README and
`integrationTesting.adoc` kept the sentence.
- `XmlUtilsSpec.groovy:281,292`: `def parsed =` is assigned and never read.
- `TestHttpResponseSpec.groovy:201-215` and `XmlUtilsSpec.groovy:279-300`:
both DOCTYPE tests assert a bare `thrown(SAXParseException)`, which a malformed
document also satisfies. `e.message.contains('DOCTYPE is disallowed')` pins
them to what their names claim.
- The external-entity fixtures indent `<!ENTITY` and `]>` by one space; the
internal-entity fixtures next to them do not.
- `grails-doc/src/en/guide/upgrading/upgrading80x.adoc` has no pointer to
the new "XML Parsing Defaults" section on the top-level page.
## [P3] New: one more caller of the strict factory
**Files:**
-
`grails-test-core/src/main/groovy/org/grails/plugins/testing/AbstractGrailsMockHttpServletResponse.groovy:84-86`
`response.xml` in controller unit tests goes through
`SpringIOUtils.createXmlSlurper()` too. A controller that renders XML with a
DOCTYPE, which is the application's own output, now makes its unit test throw
`DOCTYPE is disallowed` instead of returning a `GPathResult`. Not a security
concern and rare, but it is a behaviour change the upgrade note does not
mention, and it is another caller whose trust level does not match the
request-body default.
## Verified
- `grails-gradle`: `:grails-gradle-common:test` 12 cases,
`:grails-gradle-model:test` 56 cases, both `codeStyle` tasks, all green on
`d88d15ea15`.
- Root: `:grails-testing-support-http-client:test` 105 cases and
`codeStyle`, green.
- `:grails-gsp:test --tests org.grails.gsp.jsp.JstlDocTypeTldSpec` (my
two-case spec, not committed): strict default throws `SAXParseException`
containing `DOCTYPE is disallowed` from
`resolveTagLibrary('jakarta.tags.core')`; with the opt-in both
`jakarta.tags.core` and `http://java.sun.com/jstl/core_rt` resolve.
- `git diff 840aea9b91 HEAD` over the 13 PR files is empty; the merge commit
resolved nothing in them.
- `THREAT_MODEL.md:346` and `:470` are unchanged on this head.
- The `grails-gradle-common` coordinate is managed by every `grails-bom`
variant (`grails-bom/*/build.gradle:48`), so the versionless dependency in the
test client resolves for published consumers; the round-2 remark about the
module's placement stands as a preference only.
--
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]
