matrei commented on PR #16310:
URL: https://github.com/apache/grails-core/pull/16310#issuecomment-5598308166
# AI Review Findings
Head `6fdc50880a` on `8.0.x` base `55d5076aa5`. I reproduced the premise
independently against the JDK 21 parser
(`com.sun.org.apache.xerces.internal.jaxp.SAXParserFactoryImpl`): every
`http://` identifier is accepted and every `https://` spelling is answered with
`SAXNotRecognizedException`. The sweep that introduced the `https` forms is
`eed8df3594` (#13478, 79 files), so since April 2024 `createParserFactory()`
has configured nothing beyond `FEATURE_SECURE_PROCESSING`, and the `catch
(Exception)` around each call kept that invisible. The correction is real and
the direction is right.
There is one blocking problem with the default this PR ships, and it is not
covered by any test in the repository. The follow-up in
jamesfredley/grails-core#4 addresses it with a global opt-in; I would take that
PR with one change in shape (see the review on jamesfredley/grails-core#4).
## [P1] Rejecting DOCTYPE in the shared factory breaks JSP tag library
resolution for every application that uses JSTL
**Files:**
-
`grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:433`
-
`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/TagLibraryResolverImpl.groovy:71-90`
-
`grails-gsp/spring-boot/src/main/java/grails/gsp/boot/GspAutoConfiguration.java:323`
`createParserFactory()` returns one cached factory for every caller of
`createXmlSlurper()` and `newSAXParser()`. Those callers fall into two trust
levels:
| Caller | Input |
|---|---|
| `XmlDataBindingSourceCreator`, `HalXmlDataBindingSourceCreator`,
`grails.converters.XML.parse` | HTTP request bodies (untrusted) |
| `TldReader`, `WebXmlTagLibraryReader`, `PluginUtils` | Descriptors on the
application classpath (trusted) |
The descriptors are where the strict default bites. `GspAutoConfiguration`
defaults `grails.gsp.tldScanPattern` to a list that ends with
`classpath*:/META-INF/c-1_0-rt.tld`. In
`org.glassfish.web:jakarta.servlet.jsp.jstl:3.0.1` that file opens with a JSP
1.2 DOCTYPE, and so do 8 of the 22 descriptors in the jar (`c-1_0*.tld`,
`fmt-1_0*.tld`, `sql-1_0*.tld`, `x-1_0*.tld`).
`TagLibraryResolverImpl.initialize()` scans every pattern in one loop and
catches nothing, so the first `resolveTagLibrary(uri)` call throws
`SAXParseException: DOCTYPE is disallowed ...` and no JSP tag library resolves,
including `jakarta.tags.core` from the DOCTYPE-free `c.tld` that was scanned
earlier in the same loop.
I confirmed this with a spec in `grails-gsp/plugin` (JSTL is already on that
module's test runtime classpath) that scans `c-1_0-rt.tld` plus `c.tld` and
resolves `jakarta.tags.core`. On this head it fails with `DOCTYPE is
disallowed`. The existing GSP tests stay green only because
`GroovyPageWithJSPTagsTests`, `AbstractGrailsTagTests` and
`TagLibraryResolverTests` scan `c.tld`, `fmt.tld`, `core.tld` and
`spring*.tld`, none of which carries a DOCTYPE; `TldReaderTests` uses a fixture
without one. The two test examples that put JSTL on the runtime classpath
(`gsp-layout`, `gsp-sitemesh3`) contain no `<%@ taglib %>` directive, so the
resolver is never initialised there either.
The documented custom pattern `classpath*:/META-INF/*.tld` in
`usingJSPTagLibraries.adoc` hits all eight.
The user-visible symptom is the worst kind: a 500 on the first page that
uses a JSP tag, with a parser error that names a feature URI nobody set in the
application.
**What I would change.** The two trust levels want two parsers, and the
readers of trusted descriptors know they are reading trusted descriptors. Keep
the strict default for request bodies and let the descriptor readers ask for
DOCTYPE tolerance explicitly:
```java
// SpringIOUtils
public static XmlSlurper createXmlSlurper() throws ... { //
strict, for request bodies
return createXmlSlurper(false);
}
public static XmlSlurper createXmlSlurper(boolean allowDocTypeDeclaration)
throws ... {
return new
XmlSlurper(createParserFactory(allowDocTypeDeclaration).newSAXParser());
}
```
with `TldReader`, `WebXmlTagLibraryReader` and `PluginUtils` passing `true`.
External general and parameter entities, DTD grammar loading and external DTD
retrieval stay off on both factories, so the tolerant parser still resolves a
`file://` entity to nothing and skips the `web-jsptaglibrary_1_2.dtd` reference
instead of fetching it. That needs no configuration key, no upgrade note for
JSP users, and does not relax request-body parsing as a side effect of using
JSTL.
jamesfredley/grails-core#4 instead adds
`grails.xml.allowDocTypeDeclaration`, read through `Metadata`, that switches
the single shared factory. It works, and I ran its tests, but it makes "I use
JSTL" and "accept DOCTYPE in request bodies" the same switch. Details in that
review.
## [P2] A rejected hardening feature is still swallowed silently
**Files:**
-
`grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:431-461`
-
`grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy:235-243`
The PR exists because `catch (Exception) { /* ignore */ }` hid a
`SAXNotRecognizedException` for two years, and the catch blocks are unchanged.
The tolerance for parsers lacking a feature is reasonable, but it should leave
a trace. `grails-gradle-model` already has `slf4j-api` as an `api` dependency;
a `LOG.warn("Parser {} does not support feature {}", factory.getClass(), name)`
inside each catch costs nothing and would have surfaced this in every
application log since 2024. Copilot's suggestion to make
`disallow-doctype-decl` mandatory and propagate is too strong for a shared
factory, but a warning is not.
jamesfredley/grails-core#4 adds `XmlParserFeatureSpec`, which asserts each
identifier is recognised by the JDK parser. That pins the identifiers at test
time; the warning covers the runtime case where a different SAX provider is
first on the classpath.
## [P2] The four external-entity features lose their only behavioural
coverage
**Files:**
-
`grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy`
-
`grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy:279-300`
-
`grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy:201-227`
Every new test asserts DOCTYPE rejection, and once a DOCTYPE is refused no
document reaches `external-general-entities`, `external-parameter-entities`,
`load-dtd-grammar` or `load-external-dtd`. The deleted `newXmlSlurper blocks
external entities` and `xml uses a secure default slurper that does not resolve
external entities` were the only tests that drove an entity through the parser
and asserted the file contents did not appear. After this PR the same wrong
identifier in any of those four lines passes CI again.
jamesfredley/grails-core#4 restores this coverage for `SpringIOUtils` by
testing with DOCTYPE permitted; the split-factory shape above makes that
natural, because the tolerant factory is the one that can be tested for entity
blocking.
## [P2] `THREAT_MODEL.md` §9 now states something the code no longer does
**Files:**
- `THREAT_MODEL.md:346`
- `THREAT_MODEL.md:470`
The description says "No threat-model change in this PR", but §9 currently
reads "XXE in XML data binding. XML parsing is delegated to the underlying
parser; the framework does not impose a parser configuration. *(inferred)*",
and §14 question 13 proposes confirming exactly that. After this PR the
framework does impose a configuration on every XML request body it binds, and
the strict DOCTYPE default is a behaviour change users will hit. The sentence
in §9 should move to §8 or be rewritten to describe what is now guaranteed
(external entities and external DTDs refused, DOCTYPE refused unless opted in),
and Q13 should be answered rather than left open. Leaving the disclaimer in
place means the next security review will re-report f002 against a document
that says the fix does not exist.
Related: the upgrade notes need to say that `application/xml`, `text/xml`
and HAL XML request bodies carrying a DOCTYPE are now refused.
`integrationTesting.adoc` only covers the test client.
jamesfredley/grails-core#4 adds an "XML Parsing Defaults" section to
`upgrading.adoc` that does this.
## [P3] Small things in the test client, still present on
jamesfredley/grails-core#4's head
- `XmlUtils.groovy:116` javadoc drops "disables external entity expansion
plus external DTD loading" although those 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-227` and `XmlUtilsSpec.groovy:279-300`:
both tests now assert a bare `thrown(SAXParseException)`, which a malformed
document also satisfies. `e.message.contains('DOCTYPE is disallowed')` pins
them to the behaviour their names claim.
- The external-entity fixtures indent `<!ENTITY` and `]>` by one space; the
internal-entity fixtures next to them do not.
## Verified
- Feature recognition probe against JDK 21: 5/5 `http://` accepted, 5/5
`https://` rejected with `SAXNotRecognizedException`.
- JSTL 3.0.1 jar: 8 of 22 `.tld` files contain `<!DOCTYPE`; `c-1_0-rt.tld`
(in the default scan pattern) is one of them, uri
`http://java.sun.com/jstl/core_rt`.
- No `https://apache.org/xml/features` or `https://xml.org/sax/features`
strings remain anywhere in the tree after this PR. The other `http://` sites
(`GrailsUpdater`, `GrailsViolationAggregationPlugin`,
`grails-test-report/build.gradle`) were already correct.
- `:grails-gsp:test` with a two-case spec scanning `c-1_0-rt.tld` plus
`c.tld` (run on jamesfredley/grails-core#4's head, whose strict default is
identical to this PR's): `resolveTagLibrary('jakarta.tags.core')` throws
`SAXParseException` containing `DOCTYPE is disallowed`; with
jamesfredley/grails-core#4's opt-in both `jakarta.tags.core` and
`http://java.sun.com/jstl/core_rt` resolve.
- `:grails-gradle-model:test`, `:grails-gradle-common:test`,
`:grails-testing-support-http-client:test` and the matching `codeStyle` tasks
on jamesfredley/grails-core#4's head: 173 cases, all green.
- Commit-level checks on `6fdc50880a` show only CodeQL and the release
drafter; the PR workflow runs are what CI reports, and none of them scans a
DOCTYPE-bearing TLD.
--
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]
