Fix XXE in XML batch parsing by mgaffigan · Pull Request #452 · OpenIntegrationEngine/engine

mgaffigan · 2026-09-19T21:55:08Z

CVE-2026-82578 found an XXE vulnerability in XML batch parsing. This closes that vulnerability and adds regression tests.

Review notes:

See Fix XXE in XML batch parsing for the actual fix
Most of this is tests, since the feature was not previously tested

Note:

Breaking change: doctype parsing is disallowed by this PR. Messages with a doctype will be refused (not errored).

github-actions · 2026-09-19T22:03:56Z

Test Results

127 files + 1 127 suites +1 2m 27s ⏱️ +18s
723 tests + 6 723 ✅ + 6 0 💤 ±0 0 ❌ ±0
789 runs +24 783 ✅ +24 6 💤 ±0 0 ❌ ±0

Results for commit 77e86a5. ± Comparison against base commit 03eefcf.

♻️ This comment has been updated with latest results.

gibson9583

Requesting changes before merge.

The new batch endpoint can report success without accepting a batch, and its response collector retains all processed message payloads. Please address the two inline comments.

The Oracle integration check is also failing in the new CSV batch test: Expected source transformed content but the server stored none. Please investigate and get a passing Oracle run before merging. I have not established whether the missing content is caused by this PR.

Failing Oracle check

mgaffigan · 2026-09-21T15:51:45Z

@gibson9583, the oracle issue was opened with issue #454 and fixed in #455

Items raised are non-blocking now.

gibson9583

Items raised are non-blocking. Approved

tonygermano

My requests are for documentation improvements and not to challenge the code.

Can you expand the commit messages? Some of these changes are not small, and it would be good to have a summary of the changes and justification for why they are needed, especially when they touch multiple files or appear at a glance to change workflows.

Normally batch processing is controlled by the channel definition. What is the expected behavior when calling the new API method batchMessagesWithObj on a channel which doesn't have batch processing enabled? Is the only difference between calling this method and the existing method the return value? I think the method description implies that might be the case, but it should also describe what happens when called on a channel with batch processing disabled.

mgaffigan · 2026-09-21T20:09:59Z

Can you expand the commit messages? Some of these changes are not small, and it would be good to have a summary of the changes and justification for why they are needed, especially when they touch multiple files or appear at a glance to change workflows.

You should have write access to the branch. Feel free to reword. The commits are thematically made, already, so there should not be much explanation I can think of - and no issue is open to link to (nor do I think these are nuanced enough to warrant one).

Normally batch processing is controlled by the channel definition. What is the expected behavior when calling the new API method batchMessagesWithObj on a channel which doesn't have batch processing enabled? Is the only difference between calling this method and the existing method the return value? I think the method description implies that might be the case, but it should also describe what happens when called on a channel with batch processing disabled.

Not sure - but the change is to return the message ID's from process batch. It does not change the processing. Whatever happened previously would still happen, just now you have an endpoint which gives all of the message ID's instead of just 1.

pacmano1 · 2026-09-21T21:02:15Z

This rejects batches whose DOCTYPE declares only internal entities and points nowhere. I added a case to 210-xml-batch-xxe: a batch declaring <!ENTITY site "CLINIC-A"> with two messages. On main it splits into the two expected messages with the entity resolved; on this branch the server refuses it with a 500. 02-two-messages passes on both, so the difference is the DOCTYPE rather than the batch path.

jonbartels flagged the same behaviour change on #441 on 15 Sep and offered a changelog entry, and abhinavagarwal07 asked there about a release note. Neither got a reply, and nothing here mentions it.

#453 goes the other way, permitting the DOCTYPE and setting only ACCESS_EXTERNAL_DTD. Measured alone, that setting errors on the external-entity payload and still accepts an internal-only DOCTYPE, so both approaches close the CVE and only this one drops traffic that works today. The two PRs should probably agree.

mgaffigan · 2026-09-21T22:58:06Z

This rejects batches whose DOCTYPE declares only internal entities and points nowhere. I added a case to 210-xml-batch-xxe: a batch declaring <!ENTITY site "CLINIC-A"> with two messages. On main it splits into the two expected messages with the entity resolved; on this branch the server refuses it with a 500. 02-two-messages passes on both, so the difference is the DOCTYPE rather than the batch path.

jonbartels flagged the same behaviour change on #441 on 15 Sep and offered a changelog entry, and abhinavagarwal07 asked there about a release note. Neither got a reply, and nothing here mentions it.

#453 goes the other way, permitting the DOCTYPE and setting only ACCESS_EXTERNAL_DTD. Measured alone, that setting errors on the external-entity payload and still accepts an internal-only DOCTYPE, so both approaches close the CVE and only this one drops traffic that works today. The two PRs should probably agree.

You and Tony are right. The rest of the app blocks DTDs entirely. I had tried to leave a small hole in the XSLT on the basis that someone might be using XHTML source messages, but it did not work, and I see no reasonable way to make it work.

Updated that PR to block DTD entirely, making it consistent with this PR (and the majority of the engine, which already blocks DTDs).

tonygermano

See inline suggestion and question. I was going to have clanker write the missing commit messages bodies, but I did not know if they would be retained if other changes go through.

pacmano1

The DOCTYPE question is settled, but the behavior change still needs a release note. A batch whose DOCTYPE declares only internal entities splits on 4.5 and gets a 500 after this PR. Can the PR body call that out so it lands in the 4.6.x notes? Same ask jonbartels made on #441.

mgaffigan · 2026-09-25T13:47:34Z

@pacmano1, added breaking change notice to this and #453

jonbartels · 2026-09-29T02:03:31Z

I have a non blocking followup PR at mgaffigan#8

This PR would make some corrections to how a failed message is handled. I think its valuable but adding 500 lines to PR 452 is would further slow the review process. I am approving

processMessage returns a single message id, taken from the response handler's selected result. For a batch that is only the first or last message, leaving a caller no way to learn the ids of the others. The smoke test harness needs them to assert per-message content. POST /channels/{channelId}/batchMessagesWithObj returns every id. The response handler is already threaded through dispatchBatchMessage, so CollectingResponseHandler only has to record each id as it is set. It keeps the ids and not the DispatchResults, so a long batch does not retain the processed messages, maps and content of the messages that have already finished. Processing is unchanged. Whether a batch is split is still governed by the channel's Process Batch setting; on a channel with batch processing disabled the dispatch takes the single-message path and the endpoint returns a one-element list. EngineController.dispatchRawMessage gains a five-argument overload. The existing four-argument form delegates to it with a SimpleResponseHandler, so its callers are unaffected, but the interface gains an abstract method and out-of-tree implementations will need updating. Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>

A batch fixture produces several messages, and the harness could only submit a payload and assert against one of them. Assertion files for a fixture that expects N messages now live in numbered subdirectories, 01 through NN; a fixture that expects one message keeps its flat layout and reads exactly as before. The generator rejects numbering that does not start at 01 or that leaves gaps, and rejects a fixture that mixes loose assertion files with numbered ones. An empty source_rejected file declares that the server must refuse the submission. It cannot be combined with assertion files, since a refused submission produces no message to assert against. source_raw, destNN_raw and destNN_encoded are now assertable. OieServer.submitMessage calls the new batchMessagesWithObj endpoint so the harness learns every id the payload produced, and runMessage fails if the count does not match what the fixture expects. Failure output renders every message rather than one, and now includes the source map. ci/tests/120-delimited-batch covers the new layout with a CSV batch channel, independent of any XML change. Note that routing every submission through batchMessagesWithObj leaves processMessage without end-to-end coverage. Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>

Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>

CVE-2026-82578 found an XXE vulnerability in XML batch parsing. This closes that vulnerability and adds regression tests. setNamespaceAware(true) is not a behavior change. XPath.evaluate(String, InputSource, QName) built its DOM with a namespace aware DocumentBuilder, so the batch splitter has always been namespace aware. Without it, split messages lose the xmlns declarations they inherit from the batch element. Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>

jonbartels · 2026-09-29T02:19:15Z

I also addressed the commit updates that @tonygermano cited.

@mgaffigan hit me in Discord tomorrow and lets get this merged

Commit messages were updated.

mgaffigan requested review from a team, NicoPiel, gibson9583, jonbartels, kayyagari, kpalang, ssrowe and tonygermano September 19, 2026 21:55

jonbartels previously approved these changes Sep 21, 2026

View reviewed changes

pacmano1 mentioned this pull request Sep 21, 2026

Fix SQLi in DatabaseConnectorServlet (CVE-2026-82583) #451

Merged

gibson9583 previously requested changes Sep 21, 2026

View reviewed changes

Comment thread server/src/main/java/com/mirth/connect/server/api/servlets/MessageServlet.java

Comment thread ...y/src/main/java/com/mirth/connect/donkey/server/message/batch/CollectingResponseHandler.java Outdated

gibson9583 mentioned this pull request Sep 21, 2026

Harden XSLT step against XXE attacks #453

Merged

mgaffigan dismissed jonbartels’s stale review via 0fb4c05 September 21, 2026 15:49

mgaffigan force-pushed the maint/fix-cve-82578 branch from f1d7369 to 0fb4c05 Compare September 21, 2026 15:49

gibson9583 self-requested a review September 21, 2026 16:43

gibson9583 previously approved these changes Sep 21, 2026

View reviewed changes

tonygermano requested changes Sep 21, 2026

View reviewed changes

jonbartels added this to the 4.6.x CVE fixes milestone Sep 23, 2026

jonbartels mentioned this pull request Sep 23, 2026

Fix SQLi + XXE CVEs (CVE-2026-82583, -78224, -82578) with regression tests #441

Closed

jonbartels added the 4.6.x CVE Fixes A quick way to organize a batch of CVE issues for 4.6.x, Sept 2026 label Sep 23, 2026

jonbartels modified the milestones: 4.6.x CVE fixes, Next Release Sep 23, 2026

tonygermano previously requested changes Sep 24, 2026

View reviewed changes

Comment thread server/src/main/java/com/mirth/connect/plugins/datatypes/xml/XMLBatchAdaptor.java

Comment thread server/src/main/java/com/mirth/connect/server/api/servlets/MessageServlet.java Outdated

mgaffigan dismissed gibson9583’s stale review via 8266577 September 24, 2026 15:44

mgaffigan force-pushed the maint/fix-cve-82578 branch from 0fb4c05 to 8266577 Compare September 24, 2026 15:44

mgaffigan requested review from gibson9583 and tonygermano September 24, 2026 15:46

gibson9583 previously approved these changes Sep 24, 2026

View reviewed changes

mgaffigan dismissed gibson9583’s stale review via 822f8ee September 24, 2026 19:48

mgaffigan force-pushed the maint/fix-cve-82578 branch from 8266577 to 822f8ee Compare September 24, 2026 19:48

gibson9583 self-requested a review September 24, 2026 21:17

gibson9583 approved these changes Sep 24, 2026

View reviewed changes

pacmano1 approved these changes Sep 24, 2026

View reviewed changes

mgaffigan force-pushed the maint/fix-cve-82578 branch from 822f8ee to 551aea6 Compare September 25, 2026 13:33

jonbartels approved these changes Sep 29, 2026

View reviewed changes

mgaffigan added 4 commits September 28, 2026 22:10

Add XXE test during xml parse batch (CVE-2026-82578)

26fbdc5

Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>

jonbartels force-pushed the maint/fix-cve-82578 branch from 551aea6 to 77e86a5 Compare September 29, 2026 02:17

mgaffigan merged commit 1062766 into OpenIntegrationEngine:main Sep 29, 2026
11 checks passed

Conversation

mgaffigan commented Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

gibson9583 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mgaffigan commented Sep 21, 2026

Uh oh!

gibson9583 left a comment

Choose a reason for hiding this comment

Uh oh!

tonygermano left a comment

Choose a reason for hiding this comment

Uh oh!

mgaffigan commented Sep 21, 2026

Uh oh!

pacmano1 commented Sep 21, 2026

Uh oh!

mgaffigan commented Sep 21, 2026

Uh oh!

tonygermano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pacmano1 left a comment

Choose a reason for hiding this comment

Uh oh!

mgaffigan commented Sep 25, 2026

Uh oh!

jonbartels commented Sep 29, 2026

Uh oh!

jonbartels commented Sep 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mgaffigan commented Sep 19, 2026 •

edited

Loading

github-actions Bot commented Sep 19, 2026 •

edited

Loading