Hardened XML parsers and validators#3072
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR adds hardened SAX, schema, and StAX XML processing, routes validation and XML consumers through shared utilities, improves external DTD handling in ChangesXXE Hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSchemaFactory.java (1)
41-47: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winEnable secure processing on the schema factory as well.
ACCESS_EXTERNAL_DTDblocks external DTD fetches, butFEATURE_SECURE_PROCESSINGshould also be enabled here to apply JAXP secure-processing limits during schema compilation.Suggested fix
import static javax.xml.XMLConstants.ACCESS_EXTERNAL_DTD; +import static javax.xml.XMLConstants.FEATURE_SECURE_PROCESSING; @@ SchemaFactory sf = SchemaFactory.newInstance(schemaLanguage); try { + sf.setFeature(FEATURE_SECURE_PROCESSING, true); sf.setProperty(ACCESS_EXTERNAL_DTD, ""); } catch (org.xml.sax.SAXNotRecognizedException | org.xml.sax.SAXNotSupportedException e) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSchemaFactory.java` around lines 41 - 47, Enable secure processing in HardenedSchemaFactory.newInstance in addition to ACCESS_EXTERNAL_DTD. Update the SchemaFactory setup to turn on FEATURE_SECURE_PROCESSING before returning the factory, and keep the existing error handling path for unsupported secure settings. Use the existing newInstance method and ACCESS_EXTERNAL_DTD constant as the place to add the additional hardening.core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.java (1)
111-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the parser outcome, not only the missing request.
Line 114 ignores
protect(...)’s result, so a regression that simply rejects/parsing-fails before any fetch would still pass Line 118. Assert the intended behavior explicitly:assertFalse(...)if external DTDs should be rejected, orassertTrue(...)plus output checks if they should be removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.java` around lines 111 - 118, The XMLProtector test currently only checks that no external DTD request was made, but it ignores the outcome of protect(...), so a parsing failure could still pass the test. Update XMLProtectorTest to assert the actual result of protector.protect(...) using the XMLProtector instance, and then align the assertion with the intended behavior: use assertFalse(...) if external DTDs should be rejected, or assertTrue(...) together with output verification if they should be stripped/removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java`:
- Around line 165-166: The XMLProtector XMLResolver setup still allows an
external DOCTYPE event to survive, so fix the external DTD handling in
XMLProtector by rejecting the external DTD before it is written rather than
returning an empty stream. Update the resolver logic around f.setXMLResolver so
it does not preserve a resolvable external reference, and make sure the external
DTD path is fully blocked in a way that checkExternalEntities() and the
downstream serialization both cannot emit it when removeDTD is false.
In
`@core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSaxParser.java`:
- Line 36: The shared SAXParserFactory used by HardenedSaxParser can be accessed
concurrently, so parser creation in newSAXParser() should be synchronized.
Update the newSAXParser() path to guard factory.newSAXParser() with a
synchronized block on the shared factory field (or equivalent lock) so
concurrent callers do not race when creating parsers.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSchemaFactory.java`:
- Around line 41-47: Enable secure processing in
HardenedSchemaFactory.newInstance in addition to ACCESS_EXTERNAL_DTD. Update the
SchemaFactory setup to turn on FEATURE_SECURE_PROCESSING before returning the
factory, and keep the existing error handling path for unsupported secure
settings. Use the existing newInstance method and ACCESS_EXTERNAL_DTD constant
as the place to add the additional hardening.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.java`:
- Around line 111-118: The XMLProtector test currently only checks that no
external DTD request was made, but it ignores the outcome of protect(...), so a
parsing failure could still pass the test. Update XMLProtectorTest to assert the
actual result of protector.protect(...) using the XMLProtector instance, and
then align the assertion with the intended behavior: use assertFalse(...) if
external DTDs should be rejected, or assertTrue(...) together with output
verification if they should be stripped/removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 128f46f7-226a-4402-a6aa-c208b12a24c7
📒 Files selected for processing (14)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractXMLSchemaValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/XMLSchemaValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/soap/SoapOperationExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.javacore/src/main/java/com/predic8/membrane/core/util/MessageUtil.javacore/src/main/java/com/predic8/membrane/core/util/SOAPUtil.javacore/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSaxParser.javacore/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSchemaFactory.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPUtilTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/XMLSchemaValidatorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/soap/SoapOperationExtractorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.javacore/src/test/java/com/predic8/membrane/core/util/xml/parser/HardenedSaxParserTest.javacore/src/test/java/com/predic8/membrane/core/util/xml/parser/HardenedSchemaFactoryTest.java
…ed concurrency and simplify instantiation
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSchemaFactory.java (1)
45-56: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
ACCESS_EXTERNAL_SCHEMAhere too.SchemaFactoryexternal imports/includes are governed byACCESS_EXTERNAL_SCHEMA, so the DTD guard doesn’t cover the factory’s own schema fetches. Add the same empty-string guard for defense in depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSchemaFactory.java` around lines 45 - 56, The HardenedSchemaFactory.newInstance method only guards ACCESS_EXTERNAL_DTD, so schema imports/includes can still fetch external resources. Update the same SchemaFactory setup to also set ACCESS_EXTERNAL_SCHEMA to an empty string alongside FEATURE_SECURE_PROCESSING and the existing DTD guard, keeping the same exception handling for unsupported properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSchemaFactory.java`:
- Around line 45-56: The HardenedSchemaFactory.newInstance method only guards
ACCESS_EXTERNAL_DTD, so schema imports/includes can still fetch external
resources. Update the same SchemaFactory setup to also set
ACCESS_EXTERNAL_SCHEMA to an empty string alongside FEATURE_SECURE_PROCESSING
and the existing DTD guard, keeping the same exception handling for unsupported
properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43194f83-2227-4286-aca6-dc1d34bda9bb
📒 Files selected for processing (7)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/XMLSchemaValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.javacore/src/main/java/com/predic8/membrane/core/util/MessageUtil.javacore/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSaxParser.javacore/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedSchemaFactory.javacore/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.javacore/src/test/java/com/predic8/membrane/core/util/xml/parser/HardenedSaxParserTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/util/MessageUtil.java
- core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/XMLSchemaValidator.java
Member
Author
|
/ok-to-test |
Previously the attack-detection check was skipped when removeDTD=true, so a malicious bare DOCTYPE with an external SYSTEM/PUBLIC reference was silently stripped without any log trace. Now it's logged (info) with the raw DOCTYPE declaration attached at debug level, in both the reject (removeDTD=false) and strip (removeDTD=true) paths.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java (1)
140-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated external-subset log message across the strip and reject paths.
Lines 141-142 replicate the
log.info/log.debugpair already incheckExternalSubset()(lines 87-88). Consider extracting a small helper (e.g.logExternalSubset(dtd, boolean removed)) so the message and debug payload stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java` around lines 140 - 147, Deduplicate the external-subset logging currently repeated in the DTD stripping branch and checkExternalSubset(). Extract a helper such as logExternalSubset(dtd, boolean removed) that emits the shared info/debug messages while indicating whether the DTD was removed, then call it from both paths to keep the messages and payload consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java`:
- Around line 93-98: Update hasExternalSubsetReference to exclude the DOCTYPE
root name before applying EXTERNAL_ID_KEYWORD, ensuring matches occur only after
the declared element name and flag actual SYSTEM/PUBLIC external identifiers
without rejecting valid names such as SYSTEM or PUBLIC.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java`:
- Around line 140-147: Deduplicate the external-subset logging currently
repeated in the DTD stripping branch and checkExternalSubset(). Extract a helper
such as logExternalSubset(dtd, boolean removed) that emits the shared info/debug
messages while indicating whether the DTD was removed, then call it from both
paths to keep the messages and payload consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ea13a8b-cd19-4cf1-ad84-d56b0b38d465
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
…utFactory` for improved security and consistency
|
This pull request needs "/ok-to-test" from an authorized committer. |
rrayst
requested changes
Jul 10, 2026
rrayst
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…bilities
Summary by CodeRabbit