CAMEL-24110: camel-xslt-saxon - fix saxonReaderProperties null-guard, DOMSource URIResolver, unhardened SAXParser#24779
Conversation
…always true, DOMSource from URIResolver broken, unhardened SAXParser Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
LGTM — four well-identified bugs fixed with proper security hardening.
Verification of each fix:
-
Dead null-guard ✅ — Confirmed
saxonReaderPropertiesis initialized asnew HashMap<>()(line 76), so the!= nullcheck was always true. The!isEmpty()fix correctly skips the unnecessarycreateReaderForSource()wrapping when no reader properties are configured. -
DOMSource broken ✅ —
SAXSource.sourceToInputSource()returns null forDOMSource(by spec — DOMSource has no InputSource representation). Moving the null check to the top ofcreateReaderForSource()and returning the original source unchanged is the correct fix. The newXsltSaxonUriResolverDomSourceTestdirectly exercises this path. -
Unhardened SAXParser ✅ — The
SAXParserFactory.newInstance()without security features was exposed to XXE. The added features (FEATURE_SECURE_PROCESSING,disallow-doctype-decl, disable external entities) match the standard Camel XML hardening pattern. The Apache Xerces-specificdisallow-doctype-declfeature is safely wrapped in the try-catch — if a non-Xerces parser is used, it falls through to the catch and returns the original source. -
Unreachable null-source error ✅ — Confirmed the parent
XsltEndpoint.loadResource()checkssource == nullfirst (before any wrapping). The Saxon override had the wrapping before the null check, making theIOExceptionunreachable. Fixed by matching the parent's order.
Additional fixes ✅ — Boolean.valueOf() → Boolean.parseBoolean() avoids unnecessary boxing. Error fallback return source instead of return null prevents NPE downstream.
Static analysis: ast-grep found 3 pre-existing mutable-collection-return warnings on getter methods — these are unrelated to this PR (standard Camel endpoint configuration pattern).
Note: This PR and #24778 both modify XsltSaxonEndpoint.java from the same base. The changes are in different regions (doInit/doStart removal vs. loadResource/createReaderForSource), so they should merge cleanly.
CI: Builds still in progress.
Reviewed with Claude Code on behalf of @gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 tested, 29 compile-only — current: 11 all testedMaveniverse Scalpel detected 40 affected modules (current approach: 11).
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
LGTM — real XXE hardening on the stylesheet-parse path. createReaderForSource now sets FEATURE_SECURE_PROCESSING=true, disallow-doctype-decl=true, and external general/parameter entities =false on the SAXParserFactory before newSAXParser(), so the hardening lands on the actual parser that produces the returned SAXSource (not a throwaway). disallow-doctype-decl also makes load-external-dtd/XInclude moot. The DOMSource fix is sound too — a DOMSource from a custom URIResolver is already parsed, so returning it unwrapped introduces no re-parse/SSRF (and it fixes the previously-broken SAXSource(reader, null)).
Two non-blocking notes:
- Changing the guard from
saxonReaderProperties != nullto!saxonReaderProperties.isEmpty()meanssetSaxonReaderProperties(null)would NPE — low risk since the field defaults to an empty map, but a!= nullbelt is cheap. - The newly-hardened path has only a happy-path DOMSource test; a negative test feeding a malicious DOCTYPE/external-entity stylesheet and asserting it's now rejected would lock the hardening in against regressions.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
3 participants
Summary
Claude Code on behalf of davsclaus
Fixes four interrelated bugs in
XsltSaxonEndpointoriginating from CAMEL-14501 (2020):Dead null-guard:
saxonReaderPropertiesis initialized to an emptyHashMap, soif (saxonReaderProperties != null)is always true — every stylesheet is unconditionally re-wrapped throughcreateReaderForSource(). Fixed by guarding with!saxonReaderProperties.isEmpty().DOMSource broken: A custom
URIResolverreturning aDOMSourcegetsSAXSource.sourceToInputSource()→null, producing aSAXSourcewith nullInputSource. Fixed by checking ifsourceToInputSource()returns null and passing the original source through unchanged.Unhardened SAXParserFactory:
createReaderForSource()creates a bareSAXParserFactory.newInstance()with zero security features. Fixed by enablingFEATURE_SECURE_PROCESSING,disallow-doctype-decl, and disabling external entities — matching the hardening used elsewhere in Camel.Unreachable null-source error: In
loadResource(), the wrap happened before thesource == nullcheck, so the intendedIOException("Cannot load schema resource ...")could never fire. Fixed by checking null first, matching the parentXsltEndpoint.loadResource().Additionally:
Boolean.valueOf()→Boolean.parseBoolean()(avoids unnecessary boxing), and error fallback returns the original source instead of null.Test plan
XsltSaxonUriResolverDomSourceTest— customURIResolverreturningDOMSourcewith xslt-saxon route (was broken, now passes)🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com