[bugfix] Fix non-ASCII cookie values silently dropped / crashing on Jetty 12

[duncdrum]

Summary

Independent of and unrelated to #6692/#6693 (the URL-rewrite pipeline stack) -- this investigates and fixes a separate, pre-existing bug found while reviewing a 2010-era // TODO: remove this hack after fixing HTTP 1.1 comment during that other work.

HttpResponseWrapper#encode (added 2010-10-14, commit 67612ad4be) applies the same ISO-8859-1 byte-packing scheme to both header values (response:set-header()) and cookie values (response:set-cookie()), to work around the Java Servlet API's byte-per-char header serialization for non-ASCII text.

Measured empirically against a real Jetty 12 instance (not just inferred from the spec):

• Headers: the hack still works. A non-ASCII header value's exact UTF-8 bytes reach the wire unchanged.
• Cookies: the hack does not work, and never really did on this stack. RFC 6265's cookie-octet grammar excludes any byte >= 0x80 -- stricter than headers' more permissive treatment -- and Jetty 12 enforces this strictly. Depending on request shape, a non-ASCII cookie value either:
  • vanishes from the response entirely, with no warning or error anywhere, or
  • fails the entire request with a 500: org.eclipse.jetty.http.ComplianceViolationException: ... RFC6265 Cookie values characters restricted to US-ASCII: 0xd0 (forbidden).

What Changed

Commits 1-2 (measure, then red): a unit test pinning the exact byte-packing transformation, a raw-socket wire test against real Jetty 12 confirming the header/cookie divergence above, and an end-to-end round-trip test (response:set-cookie() → request:get-cookie-value()) reproducing the 500.

Commit 3 (fix): exist-core/src/main/java/org/exist/http/servlets/HttpResponseWrapper.java -- cookie values now go through a separate encodeCookieValue() that percent-encodes instead of byte-packing, keeping the wire value strictly within cookie-octet's ASCII-safe range regardless of input text. The header path (encode()) is untouched. org.exist.xquery.functions.request.GetCookieValue#decode (the read side) is updated symmetrically to percent-decode, with a fallback to the raw value if decoding fails, so a cookie eXist didn't set itself (browser, another app on the same domain) doesn't crash a lookup over a coincidental % that isn't a valid escape.

Both historic "TODO: remove this hack after fixing HTTP 1.1" comments are replaced with what direct measurement now shows -- the header-side hack isn't a fixable HTTP/1.1 defect (it's a still-current characteristic of the Java Servlet header API) and remains genuinely necessary today, not dead code.

Test Plan

• HttpResponseWrapperEncodingTest -- unit-level, pins the header byte-packing transformation (unchanged) and the new cookie percent-encoding.
• HttpResponseWrapperEncodingWireTest -- real HTTP round trip via raw socket against live Jetty 12; confirms header behavior and documents the cookie divergence.
• NonAsciiCookieRoundTripTest -- red against unfixed code (500), green after the fix; full response:set-cookie() → request:get-cookie-value() round trip.
• Full org.exist.http.*/org.exist.http.servlets.*/org.exist.xquery.functions.request.*/org.exist.xquery.functions.response.* sweep: 132 tests, all green -- this is also the first test coverage cookies have had in this codebase at all.

[github-actions]

📊 XQTS result comparison

Comparison of this run against develop.

Metric	develop	this run	Change
➖ Passed	28,868 (90.74%)	28,868 (90.74%)	0 (0.00 pp)
➖ Failures	1,546	1,546	0
➖ Errors	134	134	0
➖ Skipped	1,267	1,267	0
🧪 Total tests	31,815	31,815	0

Relative to develop: 0 newly passing, 0 newly failing, 0 new errors, 0 newly skipped — counting only tests recorded in both runs whose outcome changed.

_{Runtime: 429.0s (-75.62s vs develop).}

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

The header/cookie split is right, and measuring the wire rather than reading the spec is what makes it convincing — RFC 6265's cookie-octet genuinely does exclude bytes >= 0x80 where header values tolerate them. This is also the first test coverage cookies have ever had here, which is worth something on its own.

One substantive finding, and a small suggestion that I think resolves it.

Switching the read side changes how existing cookies decode

URLDecoder.decode applies to every cookie already sitting in a browser, not only to ones written by the new code. Measured:

cookie value already in a browser	decodes as today	decodes as after this PR
aGVsbG8+d29ybGQ= (base64)	aGVsbG8+d29ybGQ=	aGVsbG8 d29ybGQ=
abc+def	abc+def	abc def
tok%C3%A9n	tok%C3%A9n	tokén
50%	50%	50% (unchanged — invalid escape, caught by the fallback)

Base64 is the case I would worry about: its alphabet includes +, session tokens are routinely base64, and the corruption is silent. The IllegalArgumentException fallback only catches malformed escapes, so a well-formed %C3%A9 in a foreign cookie gets decoded whether or not it was ever encoded.

The class javadoc does acknowledge the + ambiguity, but scopes it to "a cookie eXist didn't set itself". Two cases sit outside that framing: cookies this instance set before the upgrade are equally affected, and the %XX case is not mentioned at all.

Suggestion: encode space as %20 and leave + alone on the way back

URLEncoder/URLDecoder implement application/x-www-form-urlencoded, where space is +. Percent-encoding proper (RFC 3986) uses %20 and gives + no special meaning. Moving to that shape removes the + hazard entirely:

pre-existing cookie	this PR	with %20 + literal +
aGVsbG8+d29ybGQ=	aGVsbG8 d29ybGQ=	aGVsbG8+d29ybGQ=
abc+def	abc def	abc+def

and every round trip this PR already tests still passes — Cyrillic, a b, a+b, 50%, base64. Mechanically it is small: URLEncoder.encode(v, UTF_8).replace("+", "%20") on write, and on read protect a literal + before decoding (v.replace("+", "%2B")) or use a plain RFC 3986 decoder.

That leaves only the %XX ambiguity, which is inherent to decoding a namespace you do not own, and much rarer in practice than +.

Worth knowing: an RFC 3986 decodeForURI — a decoder that treats + as a literal — is already proposed in #6451 for the xmldb: URI functions, for exactly the same reason. If both land it would be good for them to share one implementation rather than grow two.

Minor

encode() still calls value.getBytes() with no charset, so it takes the platform default. That is UTF-8 on Java 18+ so it is correct in practice today, but the new javadoc above it now states "a value's UTF-8 bytes" explicitly — worth making the code say so too while you are in there: value.getBytes(UTF_8).

Test suggestion

Whatever you decide on the above, a cookie value containing a literal + (a base64 payload is the realistic case) and one containing a %XX sequence would both be worth pinning — neither is covered now, and they are the shapes that change behavior for existing deployments.

[duncdrum]

Good catch — pushed a fix in 48cd9c0. Cookie encoding now rewrites the URLEncoder-produced + (space) to %20 on write, and escapes a literal + to %2B before decoding on read, so a pre-existing + (e.g. in a base64 token) is never mistaken for an encoded space. Also made the header path's UTF-8 byte extraction explicit, and pointed to #6451 in the docs for sharing an implementation later. Added tests pinning the + and %XX cases on both the encode and decode sides.

[duncdrum]

@joewiz please have a look again

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Thanks, @duncdrum, 48cd9c0 covers everything: a space is written as %20, a literal + is protected as %2B before decoding, getBytes(UTF_8) is explicit, and the tests pin both the + and the %XX cases. I ran HttpResponseWrapperEncodingTest, GetCookieValueDecodeTest and NonAsciiCookieRoundTripTest on the branch: all 11 pass.

One wording nit, not blocking: the new Javadoc on encodeCookieValue says that rewriting the encoded space to %20 "keeps a literal + passing through unencoded". URLEncoder encodes a literal + as %2B, which is what addCookieLeavesLiteralPlusPercentEncoded asserts, so the sentence could say that a literal + is written as %2B and so can't be confused with a space.

[dizzzz]

System.err.println -> done on purpose?

[duncdrum]

No, leftover debugging. Removed in 5a66724.

[dizzzz]

System.err.println -> for debugging?

[duncdrum]

Same, removed in 5a66724.

[dizzzz]

is here a test missing?

[duncdrum]

Good catch, it was only printing. Measurement 3 now asserts that ascii-cookie=plainvalue is present, that test-cookie is present with the percent-encoded value (URLEncoder.encode of the Cyrillic string), and that every Set-Cookie byte is printable ASCII. Fixed in 5a66724; the 4 affected test classes pass (12 tests).

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Thanks @duncdrum, the rebase and JUnit 5 migration look clean, and the diff against what I approved is test-only. +1 to @dizzzz's threads: with the fix in, measurement 3 in HttpResponseWrapperEncodingWireTest could assert that test-cookie is present and percent-encoded, rather than only printing it, and the System.err debug output could go. (The encodeCookieValue Javadoc nit from my last review is still open too: a literal + is written as %2B, not passed through.)

[duncdrum]

Thx @dizzzz and @joewiz my laptop is currently blocked by me hunting a storage bug. I ll address the review once it is done.

[duncdrum]

Thanks both. Addressed and force-pushed, rebased onto current develop (history folded into the commits that last touched each file):

• HttpResponseWrapperEncodingWireTest: removed the System.err debug output, and measurement 3 now asserts the cookie is present and percent-encoded (5a66724).
• encodeCookieValue Javadoc: now says a literal + is written as %2B and so can't be confused with a space (8710960).
• Also cleaned up the four Codacy UnnecessaryFullyQualifiedName notices in the wire test.

Re-ran HttpResponseWrapperEncodingTest, HttpResponseWrapperEncodingWireTest, GetCookieValueDecodeTest and NonAsciiCookieRoundTripTest on the rebased branch: all green.