jamesfredley opened a new pull request, #16328: URL: https://github.com/apache/grails-core/pull/16328
> Relocated from [jamesfredley/grails-core#4](https://github.com/jamesfredley/grails-core/pull/4). That follow-up was opened on an archive fork by mistake; Grails PRs belong on `apache/grails-core`. Parent: apache/grails-core#16310. Suggested follow-up to #16310. Keeps the feature-URI correction and the strict DOCTYPE default from that PR, and addresses three things found while reviewing it. ## Why the identifiers got rewritten `eed8df3594` ("chore: update url to HTTPS", #13478) rewrote them across 79 files in April 2024: ```diff -saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl";, false); +saxParserFactory.setFeature("https://apache.org/xml/features/disallow-doctype-decl";, false); ``` These are registered identifiers matched by exact string comparison, not addresses — nothing is fetched from them. No parser recognises the `https` spelling, so `setFeature` answers with `SAXNotRecognizedException`, and because each call is wrapped in a catch that tolerates parsers lacking a feature, the hardening switched off silently and stayed off for two years. ## Changes **Collect the identifiers once.** They were declared twice, in `SpringIOUtils` and in the HTTP test client's `XmlUtils`, as bare literals in both — which is how one rewrite became two. `XmlParserFeature` in `grails-gradle-common` holds them, and carries the reason the scheme cannot be changed. That module is in the `grails-gradle` build alongside `SpringIOUtils`, and `grails-common` already re-exposes it (`api 'org.apache.grails.gradle:grails-gradle-common'`), so the root build and the test client both see it. **Make DOCTYPE rejection configurable, still off by default.** `SpringIOUtils.createParserFactory()` returns one cached factory shared with readers of trusted classpath descriptors — `TldReader`, `WebXmlTagLibraryReader`, `PluginUtils` — and TLDs routinely carry a DOCTYPE. `jakarta.servlet.jsp.jstl:3.0.1` ships eight, including `c-1_0-rt.tld`, which the default `grails.gsp.tldScanPattern` in `GspAutoConfiguration` scans. `TagLibraryResolverImpl.initialize()` does not catch, so rejecting DOCTYPE stops JSP tag library resolution from the first `resolveTagLibrary(uri)` call. The default stays strict. Applications that need declarations opt in: ```yaml grails: xml: allowDocTypeDeclaration: true ``` Read through `Metadata`, so it works from `application.yml` or as a system property. Opting in relaxes only whether a declaration is permitted — external general entities, external parameter entities and external DTDs stay refused either way, which the tests assert. Documented in the upgrade notes and on the JSP tag library page. **Test the hardening by behaviour, not by reading flags back.** A test that asserts `getFeature(...)` has to restate the identifiers, so the same sweep rewrites the test and the production code together; it fails, but at the test, with a name-lookup error whose tempting fix is to make the test tolerate it. Driving real documents through the parser keeps the assertions free of any identifier. `XmlParserFeatureSpec` additionally derives the list from `values()` and asserts each is one a parser actually registers, so an unrecognised name fails by name instead of silently. Verified both failure modes: re-enabling DOCTYPE rejection without the opt-in fails 5 of 7 `SpringIOUtilsSpec` cases including the descriptor test, and rewriting the enum to `https` fails 4 of 7 plus all 5 `XmlParserFeatureSpec` cases. ## On adopting nohttp Not enabled here, but worth calling out, because it is directly connected and could break this again. The sweep in #13478 was preparatory work toward adopting `io.spring.nohttp`. The tool itself has never run over these modules, and would not have flagged these values: running its matcher with the default allowlist reports all five as allowed, because that allowlist carries `^http://xml\.org/.*` and `^http://apache\.org/xml/features/.*`. Spring hit the same problem and allowlisted it upstream. So the tool is not the hazard — the manual preparation for it was. Adopting nohttp needs no exception for these five, and its allowlist is a useful reference for which `http://` strings in the codebase are identifiers rather than links. But if we make another run at adopting it, the same well-meant rewrite can land again, and the empty catches around `setFeature` will hide it again exactly as they did for the last two years. That is what the comment on `XmlParserFeature` and the behavioural tests are for: the comment tells the next person why the scheme is fixed, and the tests fail loudly if it changes anyway rather than leaving the hardening quietly off. ## Testing - `:grails-gradle-model:test`, `:grails-gradle-common:test`, `:grails-testing-support-http-client:test` - Downstream consumers of `SpringIOUtils`: `:grails-web-databinding:test`, `:grails-converters:test`, `:grails-test-core:test`, `:grails-core:test`, `:grails-web-jsp:test`, `:grails-gsp:test` - `codeStyle` on each changed module -- 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]
