fix(network): preserve header case in the Dio driver - #151
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. The one-line driver fix is correct and genuinely covered by the new test; the only real gap is the post-change sync this repo's Major
Minor
Tests
Checks I ran
I read both changed files in full; nothing was listed as |
Dio's preserveHeaderCase defaults to false (dio-5.9.2 lib/src/options.dart:151), and the IO adapter forwards that default straight into dart:io's HttpClientRequest.headers.set (io_adapter.dart:109), which lowercases every header key on the wire. A case-sensitive consumer (ExoPlayer's User-Agent lookup) silently receives the wrong value instead of an error. Set preserveHeaderCase: true on the driver's BaseOptions so a caller-supplied key such as User-Agent reaches the socket unchanged. This changes what goes on the wire for every existing consumer of magic's Http facade: any header whose casing previously arrived normalised to lower-case now arrives exactly as the caller wrote it. No consumer should depend on the old behaviour, since HTTP header names are case-insensitive by spec, but a consumer parsing raw header bytes rather than going through a case-insensitive lookup would be affected. Added test/network/preserve_header_case_test.dart, which opens a loopback socket and asserts the raw request bytes carry 'User-Agent:' rather than 'user-agent:'; a HttpHeaders-based assertion cannot prove this because the receiving side's HttpHeaders lowercases on parse regardless of what the client sent.
The response write and the close sat in the per-chunk callback with no guard, so a request arriving in two TCP segments would write to an already closed socket and fail the test for a reason unrelated to header casing. Both now sit behind the same completed-head condition as the completer above them.
The driver change alters wire behaviour for every Http consumer, so it needs the CHANGELOG entry the post-change sync requires. The platform split belongs in doc/ rather than only in the PR body: casing is preserved where the IO adapter runs, and the web still lowercases because dio_web_adapter writes through XMLHttpRequest.setRequestHeader. Response headers stay lowercase either way.
ae15036 to
3e65da4
Compare
|
Both findings addressed, plus a rebase onto master (the branch was two commits behind). Major, the post-change sync. Your reading of the web path is right, and I verified it against the resolved package rather than the docs: Minor, the socket write window. Fixed as suggested. The write and the close now sit behind the same completed-head guard as the completer, written as an early return so the two conditions are not duplicated: if (rawRequest.isCompleted || !buffer.toString().contains('\r\n\r\n')) {
return;
}One correction on your side. Gates after the change: |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Both earlier findings are settled and the new commits are documentation plus a test-server guard, so nothing new to flag: this looks ready. What changed since my last pass: the socket handler now returns early unless the head has landed and the completer is unfilled ( The incremental diff also listed the vault and plugin-reference files. Those came in with the rebase onto master ( TestsUnchanged from my last pass and still the right shape: raw-socket assertion on the outgoing bytes is the only thing that can prove casing. The new guard closes the split-segment window exactly as described. Checks I ran
|
|
One correction, mine this time. I told you your dio version came from a different tree; it did not. |
1 participant
What
DioNetworkDriver'sBaseOptionsnever setpreserveHeaderCase, whose default isfalse. The IO adapter forwards that flag straight intoHttpClientRequest.headers.set(...)(dio-5.9.2lib/src/adapters/io_adapter.dart:109), which lowercases every header key on the wire. A case-sensitive consumer (ExoPlayer'sUser-Agentlookup, in a downstream app) silently receives the wrong value instead of an error, because the wrong casing is not a request failure, it just ships the player's default agent instead of the caller's.Setting
preserveHeaderCase: trueon the driver'sBaseOptionsfixes it: the flag is verified at the resolveddioversion (5.9.2, not5.11.1) to default tofalsein bothHeaders(headers.dart:11) andBaseOptions(options.dart:151), and to be forwarded by the IO adapter atio_adapter.dart:109. The browser adapter (dio_web_adapter) has no equivalent: it writes headers viaXMLHttpRequest.setRequestHeader, which is a browser API with no case-preservation hook at all, so this fix only reaches non-web targets. That is a platform limitation, not something this PR can close.Behaviour change for existing consumers
Every header sent through
Httpnow reaches the socket with the exact casing the caller passed, instead of being normalised to lower-case. HTTP header names are case-insensitive by spec, so no consumer should depend on the previous lower-casing, but a consumer parsing raw header bytes directly (rather than through a case-insensitive header map) would see a different string.Testing
test/network/preserve_header_case_test.dartopens a loopbackServerSocketand asserts the raw request bytes containUser-Agent: Watchools/1.0rather thanuser-agent: .... Adart:ioHttpHeaders-based assertion on the receiving side cannot prove this: incoming headers are parsed withoutpreserveHeaderCase, so they lowercase on receipt regardless of what the client actually sent on the wire.Red phase confirmed before the fix: the same test failed with
user-agent: Watchools/1.0in the captured raw request.flutter test(full suite, 1441 tests),dart analyze,dart format --set-exit-if-changedall clean on the touched files.Not done in this PR
The
CHANGELOG.md/doc//skills/post-change sync this repo'sCLAUDE.mdcalls for is not included; this PR's scope was fixed to the driver plus its test by the calling task, and the sync would need to be added before release regardless.