-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dart:io HttpClient's request force-transforms the header names to lowercase #33501
Comments
Sounds like #25120 |
Yeah, very similar problem. |
If the RFC-2616 states that header names are case-insensitive... why force the header names transformation to lowercase? shouldn't that be a decision taken by the developer? |
I think the reason for the lowercase transform is HTTP/2: it standardizes all header names to be lowercase, and with that it makes somewhat sense to do the same here. |
I've spent half a day debugging to find out http client forces headers as lowercase. |
Same here, I ended up copying over a bunch of code from the SDK after debugging a broken WebSocket connection for a few days. I get the rationale but this isn't documented anywhere which is really annoying. |
Since I can't say to thousands of users to update their receivers or fix their HTTP server implementation, and I don't expect Dart team to change this behavior, I had no other choice but to change default HTTP client. I took current HttpClient from master branch (requires Dart 2.5, Flutter 1.8+), changed it to leave headers alone and packed in the package. Hopefully it will save someone someone from crying and pulling up their hair. Just use it as a drop in replacement for HttpClient. Tested and works with Dio. You can find package HERE. |
There seems to be a suggestion that we should add an option to not change the case. |
@lrhn Any idea? This would be a breaking change if adding one more flag. |
To be more specific, A flag is probably needed to be passed from httpclient all the way down to httpheader. sdk/sdk/lib/_http/http_impl.dart Line 2071 in a1fdfb8
sdk/sdk/lib/_http/http_impl.dart Line 770 in a1fdfb8
sdk/sdk/lib/_http/http_impl.dart Line 1078 in a1fdfb8
Inside headers, sdk/sdk/lib/_http/http_headers.dart Line 41 in 4102e63
sdk/sdk/lib/_http/http_headers.dart Line 44 in 4102e63
sdk/sdk/lib/_http/http_headers.dart Line 44 in 4102e63
|
Thats the one way to look at it.
Last two can be done as optional class constructor with default value params (better) or as public properties (less transparent IMHO). Just my 2 cents. |
any update? |
Still in progress. I expect this cl to be landed within next few weeks. |
Is there any ETA when this will land in a Dart release? |
The cl is pretty much done. But it has to wait for breaking change request getting approved. |
Creating an API Client using HTTPClient with the following request headers:
And the server (nginx/1.15.8) is returning a 400 error:
Using the same headers in android with the following request headers:
And the same server is returning a 200 response, so I'm really looking forward to integrating the change by @zichangg : #39657 (comment) |
It looks like the colon |
Thanks @accek, that was definitely an oversight. I just tried it again with these headers:
But the result was still the same:
I didn't think to add the nginx logs before, but here are all the nginx logging from last night:
Currently there are no logs showing, but I think that error was from using the wrong url protocol. |
@accek We're working to make the breaking change, but it's a workaround for bad servers. The standard requires the protocol headers are case insensitive and you get the server fixed instead of debugging the issue here. I assume those bearer tokens are for debugging and have no actual access. |
Yes the token is not usable @accek , it only worked for a private internal server. Could the solution be to use the HTTP/2 standard optionally @isoos , similar to @zichangg 's solution (but an opt in solution)? @sortie I looked into the Server side, and according to the creator of Laravel, Laravel literally does not touch request headers in any form or fashion. |
This is a breaking change. Request: #33501 HttpHeaders use lowercase by default for all headers, since it is supposed to be case insensitive. Some servers incorrectly treat case as significant, however, and expect headers with capitalization or in uppercase. The current implementation forces headers to be lower cases when adding values. Users cannot even manually modify the headers. This change removes this restriction here so that users can modify the headers to whatever form they want. The new behavior is backwards compatible except if class was implemented. All headers inside http.dart are written as lower cases, adding values to HttpHeaders is still receiving lower cases input. The other cl (https://dart-review.googlesource.com/c/http_multi_server/+/121411) migrates multi_headers.dart to be compatible with this change. Bug: #33501 Change-Id: I6f7f2ef907b229773c283140c07f2de4cd500981 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/119100 Commit-Queue: Zichang Guo <[email protected]> Reviewed-by: Lasse R.H. Nielsen <[email protected]>
This reverts commit b2b7337. Reason for revert: flutter build broke!! Original change's description: > [dart:io] Stop forcing lower case on HttpHeaders > > This is a breaking change. Request: #33501 > > HttpHeaders use lowercase by default for all headers, since it is supposed to be case insensitive. Some servers incorrectly treat case as significant, however, and expect headers with capitalization or in uppercase. The current implementation forces headers to be lower cases when adding values. Users cannot even manually modify the headers. > > This change removes this restriction here so that users can modify the headers to whatever form they want. The new behavior is backwards compatible except if class was implemented. All headers inside http.dart are written as lower cases, adding values to HttpHeaders is still receiving lower cases input. > > The other cl (https://dart-review.googlesource.com/c/http_multi_server/+/121411) migrates multi_headers.dart to be compatible with this change. > > Bug: #33501 > Change-Id: I6f7f2ef907b229773c283140c07f2de4cd500981 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/119100 > Commit-Queue: Zichang Guo <[email protected]> > Reviewed-by: Lasse R.H. Nielsen <[email protected]> [email protected],[email protected],[email protected],[email protected] Change-Id: I4d4299393ad6549b250053df8823e726855e2baf No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: #33501 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134102 Reviewed-by: Zichang Guo <[email protected]> Commit-Queue: Zichang Guo <[email protected]>
The flutter channel dev dart sdk 2.8.0-dev.15.0 has to much bug and stable has ver dart sdk 2.7.0 max |
Sorry, you will have to wait for the dev channel to stabilize then. |
ok thnx for explanation |
I'm also struggling with a server only accepting cased headers. Tried: final body = '{"hello":"world"}';
final client = new HttpClient();
final req = await client.put('localhost', 8000, '/');
req.headers.add('Content-Type', 'application/json', preserveHeaderCase: true);
req.headers.add('Content-Length', body.length, preserveHeaderCase: true);
req.write(body);
await req.close(); Versions: > flutter --version
Flutter 1.15.21 • channel master • /home/eirikb/.cache/yay/flutter-git/flutter
Framework • revision e2b4edd286 (3 weeks ago) • 2020-03-13 02:46:02 -0400
Engine • revision d7a00b8b09
Tools • Dart 2.8.0 (build 2.8.0-dev.14.0 eb9c26bd37) But headers are still lower-cased. > /opt/flutter/bin/cache/dart-sdk/bin/dart --version
Dart VM version: 2.8.0-dev.14.0.flutter-eb9c26bd37 (Fri Mar 13 01:41:41 2020 +0000) on "linux_x64"
> /opt/flutter/bin/cache/dart-sdk/bin/dart api_test.dart Output from > nc -l -p 8000
PUT / HTTP/1.1
user-agent: Dart/2.8 (dart:io)
content-type: application/json
accept-encoding: gzip
content-length: 17
host: localhost:8000
{"hello":"world"} Printing headers show they were correctly set in print(req.headers);
|
Bug: #33501 Change-Id: I57d9bba251b76314bf40b81d1b09bd4643dce4d2 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/141911 Commit-Queue: Zichang Guo <[email protected]> Reviewed-by: Siva Annamalai <[email protected]> Reviewed-by: Jonas Termansen <[email protected]>
This reverts commit af19f96. Reason for revert: Breaks package:shelf tests. Original change's description: > [dart:io] Preserve header case in http header _builds() > > Bug: #33501 > Change-Id: I57d9bba251b76314bf40b81d1b09bd4643dce4d2 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/141911 > Commit-Queue: Zichang Guo <[email protected]> > Reviewed-by: Siva Annamalai <[email protected]> > Reviewed-by: Jonas Termansen <[email protected]> [email protected],[email protected],[email protected] Change-Id: I9c0418a256cb53e415ed0d5aeab84c4b0b4a161d No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: #33501 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/142980 Reviewed-by: David Morgan <[email protected]> Commit-Queue: David Morgan <[email protected]>
@mit-mit I'm having exactly the same issue as @eirikb .
Can someone please explain what's current status on this, is it working, if yes how, if not what's happening with it? Should this be re-opened? |
@shaxxx Sorry for the delayed reply. Code reviews for my fix took some time, I'll land the fix today. |
@zichangg Thanks for the update, as far I can understand preserveHeaderCase should work in the next flutter stable? |
reopening issue so that we can track when the bug fix CL lands and update on when it gets rolled into Flutter. You will probably see it in the dev channel first. |
Outgoing HttpHeaders will use _build() to build headers. This cl make _build() uses case-sensitive HttpHeaders. Original change's description: > [dart:io] Preserve header case in http header _builds() > > Bug: #33501 > Change-Id: I57d9bba251b76314bf40b81d1b09bd4643dce4d2 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/141911 > Commit-Queue: Zichang Guo <[email protected]> > Reviewed-by: Siva Annamalai <[email protected]> > Reviewed-by: Jonas Termansen <[email protected]> Bug: #33501 Change-Id: I2d42351b9593d86ad1b2ea248faaea625d321e96 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/143081 Commit-Queue: Zichang Guo <[email protected]> Reviewed-by: Siva Annamalai <[email protected]>
reopening this issue because force lowercase http header make it breakable from legacy server that don't check for small letter. and http1 header is sensitive case. |
@talregev im okay with you , they should insert support for http 1.1 |
Hey, I also miss the case sensitive version of the headers. Any update in this topic? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@cesuper @talregev this issue was closed because If this does not work for you (meaning: you set There are no plans to switch to case-sensitive headers by default. |
Any update on this? |
The toLowerCase() transformation may be useful when processing the response headers, but when sending, the server could be checking the cased version and fail if it doesn't find it that way. (Yeah, bad server.)
The text was updated successfully, but these errors were encountered: