Add safe XML building and parsing to Script mediator#2531
Merged
GDLMadushanka merged 1 commit intowso2:masterfrom Apr 8, 2026
Merged
Add safe XML building and parsing to Script mediator#2531GDLMadushanka merged 1 commit intowso2:masterfrom
GDLMadushanka merged 1 commit intowso2:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a security-hardened XML parser utility (ScriptUtils.parseXml), refactors multiple JavaScript message contexts to delegate XML parsing to it, switches OM builder usage to the security-enabled Changes
Sequence Diagram(s)sequenceDiagram
participant Context as JavaScript Message Context
participant ScriptUtils as ScriptUtils.parseXml
participant DOMParser as Hardened DOMParser
participant OMBuilder as OMXMLBuilderFactory.createOMBuilderWithSec
rect rgba(200,200,150,0.5)
Note over Context,DOMParser: Old inline parsing flow (prior to change)
Context->>DOMParser: new DOMParser()\nparse(InputSource)
DOMParser-->>Context: Document
end
rect rgba(150,200,255,0.5)
Note over Context,ScriptUtils: New delegated, hardened flow
Context->>ScriptUtils: parseXml(text)
ScriptUtils->>DOMParser: new DOMParser() with secure features\nparse(InputSource)
DOMParser-->>ScriptUtils: Document (normalized)
ScriptUtils-->>Context: Document
Context->>OMBuilder: createOMBuilderWithSec(stream)
OMBuilder-->>Context: OMElement
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.java (1)
31-45: Please lock this in with XXE regression tests.This helper is now the single DOM parsing path for the script engines. A happy-path XML test plus explicit
<!DOCTYPE>/ external-entity rejection cases would make future dependency changes much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.java` around lines 31 - 45, Add JUnit XXE regression tests for ScriptUtils.parseXml: one happy-path test that passes a simple well-formed XML string and asserts a non-null Document with expected root element, and two negative tests that supply XML containing a DOCTYPE with an external ENTITY and a DOCTYPE with parameter/external entities and assert that parseXml throws a SAXException/IOException (or otherwise rejects the input) due to the configured features; implement tests using the same class/method name ScriptUtils.parseXml and include explicit assertions about exception type/message to lock in the XXE protections.modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/GraalVMJavaScriptMessageContext.java (1)
278-280: Consider centralizing secure OM parsing toScriptUtils.The call
OMXMLBuilderFactory.createOMBuilderWithSec(stream)is duplicated identically acrossGraalVMJavaScriptMessageContext,NashornJavaScriptMessageContext, andOpenJDKNashornJavaScriptMessageContext. SinceparseXmlwas centralized toScriptUtils, extracting this secure OM builder call there as well would keep the XML parsing logic consistent and reduce maintenance drift across engine-specific implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/GraalVMJavaScriptMessageContext.java` around lines 278 - 280, getParsedOMElement currently duplicates OMXMLBuilderFactory.createOMBuilderWithSec(stream) — centralize this secure OM parsing in ScriptUtils to avoid repetition; add a new helper in ScriptUtils (e.g., parseSecureOm(InputStream) or reuse parseXml to call OMXMLBuilderFactory.createOMBuilderWithSec and return builder.getDocumentElement()), then update GraalVMJavaScriptMessageContext.getParsedOMElement (and the other engine-specific classes) to call the new ScriptUtils helper instead of calling OMXMLBuilderFactory.createOMBuilderWithSec directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/GraalVMJavaScriptMessageContext.java`:
- Around line 278-280: getParsedOMElement currently duplicates
OMXMLBuilderFactory.createOMBuilderWithSec(stream) — centralize this secure OM
parsing in ScriptUtils to avoid repetition; add a new helper in ScriptUtils
(e.g., parseSecureOm(InputStream) or reuse parseXml to call
OMXMLBuilderFactory.createOMBuilderWithSec and return
builder.getDocumentElement()), then update
GraalVMJavaScriptMessageContext.getParsedOMElement (and the other
engine-specific classes) to call the new ScriptUtils helper instead of calling
OMXMLBuilderFactory.createOMBuilderWithSec directly.
In
`@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.java`:
- Around line 31-45: Add JUnit XXE regression tests for ScriptUtils.parseXml:
one happy-path test that passes a simple well-formed XML string and asserts a
non-null Document with expected root element, and two negative tests that supply
XML containing a DOCTYPE with an external ENTITY and a DOCTYPE with
parameter/external entities and assert that parseXml throws a
SAXException/IOException (or otherwise rejects the input) due to the configured
features; implement tests using the same class/method name ScriptUtils.parseXml
and include explicit assertions about exception type/message to lock in the XXE
protections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d6f34b5-28d6-4dc5-b06c-6ceee04306c7
📒 Files selected for processing (7)
modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ExtendedJavaScriptXmlHelper.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/GraalVMJavaScriptMessageContext.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/JavaScriptXmlHelper.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/NashornJavaScriptMessageContext.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/OpenJDKNashornJavaScriptMessageContext.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.javapom.xml
GDLMadushanka
force-pushed
the
improveXMLBuildMaster
branch
2 times, most recently
from
f63c4a9 to
c43f941
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.java (1)
31-45: Add regression coverage for the hardened XML paths.This is the core XXE hardening point now. Please add tests that reject
DOCTYPE/external-entity payloads and still accept normal XML, so a later refactor can't silently weaken the parser configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.java` around lines 31 - 45, Add regression tests for ScriptUtils.parseXml to ensure the hardened XML parser rejects DOCTYPE/external-entity payloads and still accepts normal XML: create unit tests that call ScriptUtils.parseXml with a malicious XML string containing a DOCTYPE/external-entity and assert it throws a parsing exception (e.g., SAXException or an IOException), and another test that calls parseXml with a simple well-formed XML and asserts a non-null Document with expected root element; place tests alongside other module tests and reference parseXml (ScriptUtils) to prevent future regressions of the security features.modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/GraalVMJavaScriptMessageContext.java (1)
259-262: Nice parser consolidation; consider sharing the secure OM builder too.
parseXml(...)is centralized now, butgetParsedOMElement(...)is still duplicated here,OpenJDKNashornJavaScriptMessageContext, andNashornJavaScriptMessageContext. Moving thecreateOMBuilderWithSec(...)call intoScriptUtilsas well would keep the hardening choice from drifting across runtimes.Also applies to: 278-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/GraalVMJavaScriptMessageContext.java` around lines 259 - 262, The getParsedOMElement duplication should call a centralized secure OM builder in ScriptUtils: add a new method in ScriptUtils (e.g., createOMBuilderWithSec or getSecureOMBuilder) that encapsulates the hardening/secure builder logic, then update GraalVMJavaScriptMessageContext.getParsedOMElement, OpenJDKNashornJavaScriptMessageContext.getParsedOMElement, and NashornJavaScriptMessageContext.getParsedOMElement to use ScriptUtils.createOMBuilderWithSec instead of each creating their own builder; also ensure parseXml continues to call ScriptUtils.parseXml and reuse the same secure builder helper where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.java`:
- Around line 31-32: parseXml currently constructs an InputSource from the
incoming String without guarding for a null payload which causes a
NullPointerException to bubble up; update ScriptUtils.parseXml to check if the
incoming text is null (or empty if desired) and explicitly throw a SAXException
(or another checked exception already handled by the JavaScriptMessageContext
wrappers) with a clear message instead of letting an NPE occur so that parse
failures are translated into the expected ScriptException; locate the parseXml
method and add the null check before creating the InputSource.
---
Nitpick comments:
In
`@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/GraalVMJavaScriptMessageContext.java`:
- Around line 259-262: The getParsedOMElement duplication should call a
centralized secure OM builder in ScriptUtils: add a new method in ScriptUtils
(e.g., createOMBuilderWithSec or getSecureOMBuilder) that encapsulates the
hardening/secure builder logic, then update
GraalVMJavaScriptMessageContext.getParsedOMElement,
OpenJDKNashornJavaScriptMessageContext.getParsedOMElement, and
NashornJavaScriptMessageContext.getParsedOMElement to use
ScriptUtils.createOMBuilderWithSec instead of each creating their own builder;
also ensure parseXml continues to call ScriptUtils.parseXml and reuse the same
secure builder helper where applicable.
In
`@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.java`:
- Around line 31-45: Add regression tests for ScriptUtils.parseXml to ensure the
hardened XML parser rejects DOCTYPE/external-entity payloads and still accepts
normal XML: create unit tests that call ScriptUtils.parseXml with a malicious
XML string containing a DOCTYPE/external-entity and assert it throws a parsing
exception (e.g., SAXException or an IOException), and another test that calls
parseXml with a simple well-formed XML and asserts a non-null Document with
expected root element; place tests alongside other module tests and reference
parseXml (ScriptUtils) to prevent future regressions of the security features.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af88c162-ef75-45b0-ac6f-1e01a8fbea90
📒 Files selected for processing (7)
modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ExtendedJavaScriptXmlHelper.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/GraalVMJavaScriptMessageContext.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/JavaScriptXmlHelper.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/NashornJavaScriptMessageContext.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/OpenJDKNashornJavaScriptMessageContext.javamodules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.javapom.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/JavaScriptXmlHelper.java
- modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/ExtendedJavaScriptXmlHelper.java
- pom.xml
Comment on lines
+31
to
+32
| public static Document parseXml(String text) throws IOException, SAXException { | ||
| InputSource sax = new InputSource(new java.io.StringReader(text)); |
There was a problem hiding this comment.
Guard null before constructing the parser input.
Line 32 will throw NullPointerException for a null payload, and the parseXml(...) wrappers in the *JavaScriptMessageContext classes only translate SAXException | IOException into ScriptException. That leaks an unexpected runtime exception back to scripts instead of a normal parse failure.
Proposed fix
public static Document parseXml(String text) throws IOException, SAXException {
+ if (text == null) {
+ throw new SAXException("XML payload cannot be null");
+ }
InputSource sax = new InputSource(new java.io.StringReader(text));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static Document parseXml(String text) throws IOException, SAXException { | |
| InputSource sax = new InputSource(new java.io.StringReader(text)); | |
| public static Document parseXml(String text) throws IOException, SAXException { | |
| if (text == null) { | |
| throw new SAXException("XML payload cannot be null"); | |
| } | |
| InputSource sax = new InputSource(new java.io.StringReader(text)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/extensions/src/main/java/org/apache/synapse/mediators/bsf/utils/ScriptUtils.java`
around lines 31 - 32, parseXml currently constructs an InputSource from the
incoming String without guarding for a null payload which causes a
NullPointerException to bubble up; update ScriptUtils.parseXml to check if the
incoming text is null (or empty if desired) and explicitly throw a SAXException
(or another checked exception already handled by the JavaScriptMessageContext
wrappers) with a clear message instead of letting an NPE occur so that parse
failures are translated into the expected ScriptException; locate the parseXml
method and add the null check before creating the InputSource.
Use safe methods from Axis2 and Axiom to build and parse XML inputs in the script mediator.
GDLMadushanka
force-pushed
the
improveXMLBuildMaster
branch
from
c43f941 to
3f9bb26
Compare
SanojPunchihewa
approved these changes
Apr 8, 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.
Purpose
Summary by CodeRabbit
Security
New Features
Chores