Skip to content

Replace Aalto with Woodstox and require native Stax2 for StaxMate - #2840

Merged
mprins merged 2 commits into
harden-xml-parsers-and-transformersfrom
require-stax2-for-staxmate
Oct 23, 2026
Merged

mprins merged 2 commits into
harden-xml-parsers-and-transformersfrom
require-stax2-for-staxmate

Conversation

@matthijsln

Copy link
Copy Markdown
Member

Summary

Stacked on #2839 (base branch harden-xml-parsers-and-transformers); review only the last two commits.

  • Replace Aalto with Woodstox for the BGT and BAG2 loaders. Aalto was added for performance, which is not critical anymore: XML parsing CPU usage is small compared to network and database IO, and loading is multithreaded. brmo-service already excluded Aalto and so already ran these loaders on Woodstox. woodstox-core is now managed at 7.3.0, which also upgrades the 6.x version pulled in transitively (CXF, xmlsec).
  • New StaxMateFactories (bgt-loader, also used by bag2-loader): creates the StaxMate input factory secured by Apache Commons Secure XML (coalescing, no DTD), and fails with an IllegalStateException if the StAX implementation does not natively implement Stax2.

Why the Stax2 check

StaxMate works with any StAX parser, but for a non-Stax2 parser (such as the JDK default) it wraps the reader in Stax2ReaderAdapter, which counts the element depth itself in next() and getElementText() but not in nextTag().

The GeoTools XmlStreamGeometryReader reads geometries directly from SMInputCursor.getStreamReader() and uses nextTag(). StaxMate re-synchronizes cursors using getDepth() of the reader, so after a curved geometry (gml:Arc) the depth is off and the cursors silently skip all following attributes of the object. Polygons happen to balance out, so only the bgt_wijk_curve.gml test caught it when Aalto was removed.

StaxMate allows but discourages bypassing cursors (see FasterXML/StaxMate#2, still open). With a native Stax2 parser the depth is tracked by the parser itself, so this works. The javadoc of StaxMateFactories documents this, and that the check should stay even when StaxMate is upgraded automatically, unless that issue is verified to be resolved.

Test plan

  • BGTObjectStreamerTest (including bgt_wijk_curve.gml) and BAG2 loader tests pass with Woodstox
  • New StaxMateFactoriesTest: native Stax2 reader with Woodstox; IllegalStateException when the JDK StAX implementation is selected
  • Each commit builds and passes the unit tests on its own; brmo-service, nhr-loader and brmo-commandline (CXF with Woodstox 7.3.0) pass
  • Integration tests (database profiles) loading BGT and BAG2 data

🤖 Generated with Claude Code

matthijsln and others added 2 commits October 23, 2026 12:55
Aalto was added for performance, which is not critical anymore as XML
parsing CPU usage is small compared to network and database IO.

StaxMate needs a native Stax2 implementation: with the JDK StAX parser
attributes after a curved geometry were silently skipped (see the
bgt_wijk_curve.gml test). Woodstox was already used by brmo-service
because it excludes Aalto.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add StaxMateFactories, used by the BGT and BAG2 loaders, which creates a
StaxMate input factory secured by Apache Commons Secure XML and fails if
the StAX implementation does not natively implement Stax2.

With a non-native implementation StaxMate uses Stax2ReaderAdapter, which
does not track the element depth for nextTag(). The GeoTools geometry
reader uses nextTag() directly on the stream reader, after which the
StaxMate cursors silently skip elements following curved geometries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@matthijsln

Copy link
Copy Markdown
Member Author

Aalto was only used in standalone docker images for the bag2 and bgt loaders, Woodstox was already used in brmo-service.

The JDK-default Stax1 parser was never used in deployments so no attributes were ever dropped, this is just an added precaution.

@codecov

codecov Bot commented Oct 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 37%. Comparing base (a736883) to head (624cbec).

Files with missing lines Patch % Lines
.../main/java/nl/b3p/brmo/util/StaxMateFactories.java 84% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                          Coverage Diff                          @@
##             harden-xml-parsers-and-transformers   #2840   +/-   ##
=====================================================================
+ Coverage                                     37%     37%   +1%     
  Complexity                                    74      74           
=====================================================================
  Files                                        200     201    +1     
  Lines                                      13545   13547    +2     
  Branches                                    1431    1432    +1     
=====================================================================
+ Hits                                        4904    4905    +1     
- Misses                                      8233    8235    +2     
+ Partials                                     408     407    -1     
Files with missing lines Coverage Δ
...3p/brmo/bag2/loader/BAG2GMLMutatieGroepStream.java 86% <100%> (-<1%) ⬇️
...java/nl/b3p/brmo/bgt/loader/BGTObjectStreamer.java 82% <100%> (-<1%) ⬇️
.../main/java/nl/b3p/brmo/util/StaxMateFactories.java 84% <84%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mprins
mprins merged commit 727ea85 into master Oct 23, 2026
28 of 30 checks passed
@mprins
mprins deleted the require-stax2-for-staxmate branch October 23, 2026 13:23
mprins pushed a commit that referenced this pull request Oct 23, 2026
#2839)

## Summary

Adds `SecureXmlFactories` (brmo-loader) with hardened factory methods,
based on [Apache Commons Secure
XML](https://commons.apache.org/proper/commons-secure-xml/), and uses
them instead of `TransformerFactory.newInstance()` /
`DocumentBuilderFactory.newInstance()` everywhere, and
`XMLInputFactory.newInstance()` in brmo-loader.

Commons Secure XML secures each factory with a recipe for the actual
implementation (JDK, Xerces, Woodstox, Saxon) and resolves external
references (entities, DTDs, stylesheets, `document()`) to empty content.
On top of that:

- **`newTransformerFactory()`**: `SecureTransformerFactory` plus
`FEATURE_SECURE_PROCESSING`. Stylesheets loaded via a `URIResolver` (the
`xsl:import`s from the classpath) still work.
- **`newDocumentBuilderFactory()`**: `SecureDocumentBuilderFactory` plus
`disallow-doctype-decl`, so documents with a DOCTYPE are rejected
instead of parsed with empty entities.
- **`newXMLInputFactory()`**: `SecureXMLInputFactory` plus
`SUPPORT_DTD=false`. Used by `GbavXMLReader`, `Brk2SnapshotXMLReader`
and `DataComfortXMLReader`. These already had `SUPPORT_DTD=false`, so
this is for consistency/defence in depth.

The BGT and BAG2 loaders don't depend on brmo-loader and configure their
own StAX factory (currently Aalto via SPI, which Commons Secure XML does
not list as supported), so there `IS_SUPPORTING_EXTERNAL_ENTITIES=false`
is added inline next to the existing `SUPPORT_DTD=false`. Both
properties are accepted by the JDK parser, Woodstox and Aalto. The
follow-up PR #2840 replaces Aalto with Woodstox and uses Commons Secure
XML there too.

### Fixed XXE

- **`NhrXMLReader`**: the nHR input was parsed by the split transformer
as a `StreamSource` using an unhardened `TransformerFactory`. Setting
`source.setSystemId("")` did **not** prevent resolving an absolute
`file://` external entity. A test showed the file contents were read
before this change. Credit to Ray Sabee (Whitehat Security, @raysabee)
for finding and reporting this issue!
- Unhardened `DocumentBuilderFactory` in `RsgbTransformer`,
`RsgbBRPTransformer.merge`, `NhrXMLReader.extractBSN`, `Brk2Bericht`,
`GbavBericht` and `StUFBGasynchroon`.

`BRPXMLReader`, `WozXMLReader` and `RsgbWOZTransformer` already had
hardening inline; they now use the shared factory. An unused
`DocumentBuilderFactory` in `NHRLoadUtils` is removed.

### Behaviour change

- XML containing a DOCTYPE is now rejected by all `DocumentBuilder`
parsing (already the case for BRP and WOZ). None of the supported
formats use a DTD.
- The `TransformerFactory` does not reject a document with an external
entity but resolves it to empty content: the Commons Secure XML resolver
takes precedence over the JAXP
`accessExternalDTD`/`accessExternalStylesheet` attributes. The external
resource is never read.

## Test plan

- [x] New `SecureXmlFactoriesTest`: external entity not resolved by the
transformer (also with a `URIResolver` for `xsl:import`); rejected by
the document builder and StAX; billion laughs rejected; plain XML still
works
- [x] Unit tests pass for brmo-loader, bgt-loader, bag2-loader (with
Aalto), brmo-stufbg204, nhr-loader, brmo-service and brmo-commandline
- [ ] Integration tests (database profiles), which run the
`Rsgb*Transformer`s on real data

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@mprins mprins added the dependency update (deels) automatische updates van gebruikte libraries label Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependency update (deels) automatische updates van gebruikte libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants