[NicoPiel]

What

HTTPUtil.parseHeaders no longer uses commons-httpclient 3's HttpParser. It now delegates to httpcore 4.4's AbstractMessageParser.parseHeaders (httpcore is already on the server classpath), instead of using a custom parser.

Why

This is one step toward dropping commons-httpclient 3.0.1. Following review, it uses an existing, maintained parser rather than hand-written parsing code.

Behaviour changes against httpclient 3

HTTPUtil is user-script API, so these are worth a line in the release notes. Well-formed headers (Name: value lines separated by CRLF or LF, ending at a blank line or EOF) parse the same as before. These corner cases change:

Input	httpclient 3	httpcore 4.4
Malformed header (no colon, status line, empty name)	org.apache.commons.httpclient.ProtocolException (an IOException), Unable to parse header: <line>	org.apache.http.ProtocolException (not an IOException), Invalid header: <line>
Empty header name (:v)	accepted as name ""	rejected
Folded line before the first header ( orphan)	silently ignored	rejected
Whitespace-only line (\t)	ends the header block	treated as a folded continuation, so parsing continues
Line that is only a bare CR (\r\r\n)	ends the header block	rejected (Invalid header: [0x10])
Folded continuation of an empty value (X: then b)	" b"	"b"
Trailing whitespace on a folded line (a, b , \tc)	"a b c"	"a b c" (only leading whitespace of each fold is dropped)
Control characters around a value (\u000ba\u0001)	trimmed (String.trim)	kept; only spaces and tabs are trimmed

Reviewer notes

• The first commit adds characterization tests that pass against the old httpclient 3 implementation. The last commit updates them to pin httpcore's behaviour; each changed assertion is one of the rows above. 21 tests now.
• str.getBytes() still uses the platform charset and the bytes are decoded as UTF-8, as before.
• No header count or line length limit is applied (-1), matching the old unlimited behaviour.
• This does not remove the jar yet. That needs the HttpStatus change and the WebDAV rewrite merged too.

🤖 Generated with Claude Code

[github-actions]

Test Results

130 files  ± 0  130 suites  ±0   3m 58s ⏱️ +47s
810 tests +21  810 ✅ +21  0 💤 ±0  0 ❌ ±0 
936 runs  +21  930 ✅ +21  6 💤 ±0  0 ❌ ±0 

Results for commit 90c8e09. ± Comparison against base commit 4589238.

This pull request removes 2 and adds 23 tests. Note that renamed tests count towards both.

com.mirth.connect.donkey.server.data.jdbc.OracleJdbcDaoTest ‑ testBatchInsertMessageContentKeepsStatementOpenUntilExecuted
com.mirth.connect.donkey.server.data.jdbc.OracleJdbcDaoTest ‑ testFailedBatchInsertClearsAndClosesTheStatement

com.mirth.connect.donkey.server.data.jdbc.JdbcDaoTest ‑ insertMessageContentBatchBatchesEveryRowThenExecutesOnce
com.mirth.connect.donkey.server.data.jdbc.JdbcDaoTest ‑ insertMessageContentBatchClosesTheStatementWhenBindingFails
com.mirth.connect.server.userutil.HTTPUtilTest ‑ testBareCarriageReturnLineIsRejected
com.mirth.connect.server.userutil.HTTPUtilTest ‑ testCarriageReturnHandling
com.mirth.connect.server.userutil.HTTPUtilTest ‑ testContinuationOfEmptyValue
com.mirth.connect.server.userutil.HTTPUtilTest ‑ testCrlfLines
com.mirth.connect.server.userutil.HTTPUtilTest ‑ testDuplicateNamesLastWins
com.mirth.connect.server.userutil.HTTPUtilTest ‑ testEmptyNameIsRejected
com.mirth.connect.server.userutil.HTTPUtilTest ‑ testEmptyString
com.mirth.connect.server.userutil.HTTPUtilTest ‑ testEmptyValue
…

♻️ This comment has been updated with latest results.

[gibson9583]

Looks safe to merge.

[mgaffigan]

We should not be implementing an HTTP parser. This is how you get more request smuggling vulnerabilities.

Change to some existing parser we're already using. That may break some corner case where a client is sending nonstandard requests - but such things are not reasonably avoidable.

[NicoPiel]

Agreed. parseHeaders now delegates to httpcore 4.4's AbstractMessageParser.parseHeaders, so there's no custom parsing code left. I updated the characterization tests to pin httpcore's behaviour and listed every corner case that changes against httpclient 3 in the PR description. They're all malformed or unusual inputs: an empty name, an orphan folded line, whitespace-only and bare-CR lines, fold whitespace, control-character trimming, and the exception type and message.

[mgaffigan]

Looks better - can we deprecate this method? Seems like it should not be used - doesn't handle repeated headers - and how is someone getting a set of http headers as a string?