Skip to content
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

[Breaking Change Request] Allow http implementation to work with case-sensitive Http Headers #41628

Closed
zichangg opened this issue Apr 22, 2020 · 1 comment
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-request This tracks requests for feedback on breaking changes library-_http

Comments

@zichangg
Copy link
Contributor

Context

This is a follow-up for #39657. Class HttpHeaders allows case-sensitive header fields. However, there are multiple places in our Http implementation which force headers as lowercased. It blocked headers sent/received being case-sensitive.

The intended change in behavior

  • _HttpParser allows HttpHeaders field to be whatever it is instead of parsing it to lowercased field. That makes sure all incoming requests/responses have their headers fields unchanged.
  • _HttpHeaders's _build() will no longer use lowercased field name to generate outgoing headers.

Expected impact

Code that use HttpClient and HttpServer are affected.

Breakage:

  • If you have a http server, client send the request with "CapitalizedHeaders": "value" in the header. The entity in request.headers will now have "CapitalizedHeaders" as a key instead of "capitalizedheaders". Potentially, checks like headers['capitalizedheaders'] != null will fail.
       HttpServer
           .bind(InternetAddress.anyIPv6, 80)
           .then((server) {
             server.listen((HttpRequest request) {
               // Headers are no longer lowercased.
               if(request.headers['capitalizedheaders'] != null) {
               // This will break now
               }
               request.response.close();
             });
           });
  • Similarly, if you have a http client, server sends case-sensitive headers. HttpClientResponse will also be able to capture that.

Steps for mitigation

When doing a look up in headers or a string comparison for headers's field, use case-sensitive string.

Proposed implementation

https://dart-review.googlesource.com/c/sdk/+/143081

@zichangg zichangg added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-_http breaking-change-request This tracks requests for feedback on breaking changes labels Apr 22, 2020
@mit-mit mit-mit assigned franklinyow and unassigned mit-mit Apr 23, 2020
@sortie
Copy link
Contributor

sortie commented Apr 23, 2020

I'm sorry, we haven't communicated properly and I misunderstood what the change does and what it's intended to do, and there appears to be misunderstandings in the proposal above. Let's get it all cleared up, hopefully I get all my facts straight.

The original problem reported in #33501 (comment) appears to be that preserveHeaderCase is ineffective for headers set by the SDK (like Content-Length and Content-Type) since it overrides the user preference. I agree that's a bit of a bug and we should make preserveHeaderCase take effect in those cases.

However, the changelist attempting to that fix problem does two things:

  1. It makes preserveHeaderCase control the case of headers set by the SDK like Content-Type and Content-Length, i.e. what the SDK transmits. That fixes the bug.
  2. It makes the HTTP header parsing preserve the case of headers it receives and exposes it.

I approved the original changelist only thinking it fixed the bug but I didn't realize at all also made the second change. The updated test did check for that behavior but I didn't realize it.

Preserving the header case for received headers is a bad idea and we shouldn't do it. It's acceptable to build a mechanism to send headers of a particular case to misbehaving servers for the purpose of interoperability. However, we musn't ourselves become those misbehaving servers. Our HTTP parser lowercases the headers it receives, which means code cannot depend on the case of the header, and thus always follows the specification that headers are case insensitive. The proposed change lets application code depend on the sent header case, which introduces room for error. There's no reason for the server to care and we shouldn't make this change.

And then the proposal above says operator [] on HttpHeaders now become case sensitive. That's not the case. That operator is case insensitive. So there's no breaking change as far as I can tell. Even if the changelist happened to make it case sensitive, we can fix that, and no breaking change of that kind of needed. The breaking change, however, would be that if one ask for the case of a header (e.g. using toString() on HttpHeaders, or iterating through the header keys), then one might get a non-lowercase header name.

I suggest the changelist in question is rescoped to only change the transmission of headers.

I'm closing this breaking change request for these reasons:

  • There has been no rationale given for why we want the HTTP parser to lowercase the headers it receives and I consider it to be a bad idea. Unless there's some context I'm missing?
  • The original bug (that some headers sent by Dart did not respect preserveHeaderCase) can be fixed without a breaking change request. Unless there's some bug report I'm not aware of?

Or do I have any further misunderstandings?

@sortie sortie closed this as completed Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-request This tracks requests for feedback on breaking changes library-_http
Projects
None yet
Development

No branches or pull requests

4 participants