Update HAPI to 2.6.0 to close XXE vulnerability

[mgaffigan]

No description provided.

[github-actions]

Test Results

127 files  + 4  127 suites  +4   2m 19s ⏱️ +30s
713 tests +23  712 ✅ +22  0 💤 ±0  1 ❌ +1 
749 runs  +47  748 ✅ +46  0 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 602cede. ± Comparison against base commit 9359d9a.

♻️ This comment has been updated with latest results.

[gibson9583]

This upgrade introduces silent data loss for valid HL7 XML containing CDATA. Reproduced through OIE's ER7Serializer on Java 17 and 21, changing only the HAPI libraries:

• <HD.1><![CDATA[clinical]]></HD.1>: HAPI 2.3 preserves clinical; 2.6.0 produces an empty MSH-3 field.
• <HD.1>before<![CDATA[clinical]]>after</HD.1>: 2.3 preserves beforeclinicalafter; 2.6.0 returns beforeafter.

Both conversions report success. These inputs contain no DOCTYPE or external entity.

Affected paths are fromXML with strict parsing enabled (validation on or off), and toXML for XML input with strict parsing and validation both enabled. HAPI 2.6.0's XMLUtils.parseDocument uses a DOM builder without enabling coalescing, while XMLParser.parsePrimitive only appends text nodes, so CDATA nodes are discarded.

The targeted XXE fix works in the tested engine paths: baseline file disclosure and network requests were reproduced, and 2.6.0 prevented both for external entity and DTD payloads. Please preserve CDATA field contents while retaining those restrictions, and add regression coverage for pure and mixed CDATA on both affected paths before merging.

[mgaffigan]

Seems like an upstream bug. I agree with adding regression tests, but can someone get an upstream but open or linked?

[pacmano1]

On the upstream question: this traces to HAPI #98, "Fix XXE issue in XML parser", merged 2023-08-28 and shipped in 2.4. That PR swapped XMLUtils from LSParser to DocumentBuilderFactory without setting coalescing, and XMLParser.parsePrimitive has only ever read text nodes. Nothing exists upstream for it under CDATA, coalescing or parsePrimitive, so I will open one.

The fix I pushed keeps 2.6.0's factory configuration exactly and adds setCoalescing(true), with regression coverage for pure and mixed CDATA on fromXML with validation off and on, and on toXML with validation on.

[pacmano1]

Filed upstream as hapifhir/hapi-hl7v2#151.

[pacmano1]

Unrecognised message types change from rejected to accepted. A message whose MSH-9 has no generated structure class errors on 2.3 and comes back TRANSFORMED on 2.6.0, with strict validation on or off. Measured on a running engine on both versions, with a known type as the control. The transformer then receives a document rooted at GenericMessageV24, so a channel that rejects unknown types today quietly accepts them.

HAPI removed the guard rather than making it configurable: the GenericMessage check in XMLParser.doEncode is commented out in 2.6.0. Four lines in CustomDefaultXMLParser restore it:

@Override
protected String doEncode(Message source) throws HL7Exception {
    if (source instanceof GenericMessage) {
        throw new HL7Exception("Can't XML-encode a GenericMessage.  Message must have a recognized structure.");
    }
    return super.doEncode(source);
}

For the CDATA loss @gibson9583 reported, overriding parseStringIntoDocument in the same class and calling setCoalescing(true) on the factory fixes it. parsePrimitive only reads TEXT_NODEs and never handled CDATA in either version; 2.3 never produced those nodes because it parsed through an LSParser. #408 overrides that same method for DOCTYPE hardening, so those want to be one override rather than two.

With both applied, the behaviour differences I can find between 2.3 and 2.6.0 across 40 HL7 message cases drop from 10 to 2. Both remaining are the same case counted twice: XML with whitespace before the declaration throws SAXParseException on 2.6.0 where 2.3 threw XMLParseException, same message text, and the input is malformed so it fails on both. I can put the test corpus up separately if it's useful.

On the upstream question, HAPI's own parser hands parsePrimitive a node type its own code discards, so it is an upstream bug and worth filing. The override above is a workaround so the upgrade doesn't have to wait on that. I can open the issue if nobody has.

[pacmano1]

Pushed the two fixes from my earlier comment, with tests. Six cover pure and mixed CDATA on each strict path that reaches the parser, and disabling the setCoalescing call alone fails all six. Three cover the unrecognised-message-type behaviour in both positions.

One change of shape worth flagging: that guard is behind a new setting, "Allow Unrecognized Message Types", defaulting to off, so existing channels are unchanged and XStream leaves old ones false. Same shape as allowXml in #406. Strike it if you would rather it were unconditional, that is a one-line change.

CDATA is unconditional. That one is a defect rather than a choice.

The GenericMessage change upstream looks unintentional, incidentally. Their #20 asked for something narrower, the $ in GenericMessage$V23 producing an invalid element name, and its reproduction calls encodeDocument rather than the doEncode path where the guard lives. The guard is simply commented out, with no issue or PR behind it.