Remove trailing whitespace from HTTP headers #53005

brianquinlan · 2023-07-21T00:27:43Z

Change Intent

The intent of this change is to make the headers field values contained (through HttpHeaders) in HttpRequest and HttpClientResponse never contain trailing spaces and tabs.

For example, an HTTP request like:

POST /test HTTP/1.1
Header-A:  \t  cat \t

Would be parsed like:

// CURRENT BEHAVIOR: leading whitespace removed, trailing preserved
assert(headers['header-a'] == 'cat \t ');
// PROPOSED BEHAVIOR: leading and trailing whitespace removed.
assert(headers['header-a'] == 'cat');

Justification

It is easier to write correct code if the whitespace has been removed from the field values.

The Dart HTTP parser itself has issues with trailing whitespace. For example, the chunked content encoding detector currently checks for the exact string 'chunked' rather than for 'chunked[ \t]*'.

RFC 2616 section 4.2 specifically says that leading and trailing whitespace is not semantic:

The field-content does not include any leading or trailing LWS:
linear white space occurring before the first non-whitespace
character of the field-value or after the last non-whitespace
character of the field-value. Such leading or trailing LWS MAY be
removed without changing the semantics of the field value.

Finally, this behavior would be consistent with the behavior of XMLHTTPRequest (dart:html HttpRequest) , Cronet (package:cronet_http) and Apple's Foundation URL loading system (package:cupertino_http).

Impact

There are three scenarios that I can imagine:

usage that is currently incorrect but is fixed by this change, e.g., if (header['header-a'] == 'cat') ...
usage that already accounts for the current behavior but will not be broken, e.g., if (header['header-a'].trim() == 'cat') ...
usage that already accounts for the current behavior in a broken way, e.g., if (header['header-a'] == 'cat \t ') ...

I'm guessing that case (1) is the most common and case (3) is the least common.

Mitigation

I think that this is unlikely to have significant impact.

The text was updated successfully, but these errors were encountered:

aam · 2023-07-21T14:37:21Z

I don't think http client library should be in business of trimming and cleansing header values, should not attempt to apply semantic meaning to them. It should try to get content it received into developers hands as is and let them decide whether to trim values or not.
Beside relatively recent dart internal issue #51532 where Content-Length value was not trimmed before parsing, do we have any user reports of having a problem with current behavior?

itsjustkevin · 2023-07-21T17:02:42Z

@Hixie @grouma @vsmenon breaking change request, hot off of the presses!

brianquinlan · 2023-07-21T17:26:45Z

I don't think http client library should be in business of trimming and cleansing header values, should not attempt to apply semantic meaning to them. It should try to get content it received into developers hands as is and let them decide whether to trim values or not.

We already normalize the header name for case. We also remove leading whitespace from the field value.

Beside relatively recent dart internal issue #51532 where Content-Length value was not trimmed before parsing, do we have any user reports of having a problem with current behavior?

No. But I suspect that is because most servers do not append additional whitespace to field values.

aam · 2023-07-21T17:48:10Z

I don't think http client library should be in business of trimming and cleansing header values, should not attempt to apply semantic meaning to them. It should try to get content it received into developers hands as is and let them decide whether to trim values or not.
We already normalize the header name for case. We also remove leading whitespace from the field value.

Normalizing header name is kind of different thing compared to "normalizing" the value(note also that we do have an option preserveHeaderCase on https://api.dart.dev/stable/3.0.6/dart-io/HttpHeaders/add.html).

a-siva · 2023-07-24T17:38:57Z

I don't think http client library should be in business of trimming and cleansing header values, should not attempt to apply semantic meaning to them. It should try to get content it received into developers hands as is and let them decide whether to trim values or not.

Could you summarize as to why this is important and what potential pitfalls you see from the removal of trailing whitespaces from HTTP headers.

sgjesse · 2023-07-25T08:25:27Z

As already mentioned "RFC 2616 section 4.2 specifically says that leading and trailing whitespace is not semantic", so any intermediate (cache server, proxy server, etc.) MAY already remove this whitespace. So in practice one can not rely on whitespace added by the server being preserved anyway. The current behavior in principle require trimming all header values which seems error prone.

aam · 2023-07-25T16:37:19Z

@a-siva wrote

Could you summarize as to why this is important and what potential pitfalls you see from the removal of trailing whitespaces from HTTP headers.

When dart developer controls both server and client they might want to use trailing spaces as they are not prohibited by the RFC. Also it is hard to rule out possibility of some legacy server sending out trailing spaces as part of the header value content.

Note #33501 that resulted in dart HttpHeaders add and set getting preserveHeaderCase option when representing header names - developers asked for flexibility around that even though it's not per RFC.

https://datatracker.ietf.org/doc/html/rfc2616#section-4.2 talks about several things that MAY happen to the content(trimming of leading/trailing WS, longer WS sequences replaced with single WS, combination of multiple header values into single comma-separated list) - I don't think we have done none of that so far(well, except of leading WS). If we implement trimming of trailing WS, It would be good to clarify our strategy with other items.

itsjustkevin · 2023-08-10T14:31:59Z

Checking in, have we come to a decision here, or is this still being worked through?

brianquinlan · 2023-08-10T22:04:09Z

@itsjustkevin

We are ready for approval.

itsjustkevin · 2023-08-11T13:00:41Z

Calling all reviewers (@Hixie, @grouma, @vsmenon) another breaking change, hot off the press!

grouma · 2023-08-11T16:35:56Z

SGTM. I can see this potentially impacting our internal web development workflow but we should have sufficient E2E coverage to catch any issues.

Hixie · 2023-08-11T19:50:08Z

fine by me

vsmenon · 2023-08-14T22:10:08Z

lgtm

brianquinlan · 2023-08-14T22:14:28Z

@grouma I already have a PR in progress and it passes the CBUILD.

grouma · 2023-08-14T22:19:54Z

@grouma I already have a PR in progress and it passes the CBUILD.

I'm not very familiar with CBUILD but I assume it doesn't do a TGP?

I'm concerned about our internal workflows as we have a handful of Dart based web servers / tools. Any issues will hopefully be caught when this change is rolled into Google3 though.

brianquinlan · 2023-08-14T22:28:58Z

@grouma I don't know much about CBUILD but you seem to be right - it tests <5,000 Google3 targets.

Bug: #53005 Bug: #51532 Change-Id: I8a2fc04f48d50103819d655ccd300e73d59fbecc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/319903 Reviewed-by: Alexander Aprelev <aam@google.com> Commit-Queue: Brian Quinlan <bquinlan@google.com>

brianquinlan added library-io breaking-change-request This tracks requests for feedback on breaking changes labels Jul 21, 2023

brianquinlan assigned kevmoo and itsjustkevin Jul 21, 2023

a-siva added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Jul 21, 2023

a-siva mentioned this issue Jul 24, 2023

Allow for spaces after chunk size when parsing HTTP with Transfer-Encoding: chunked #52887

Closed

brianquinlan closed this as completed Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove trailing whitespace from HTTP headers #53005

Remove trailing whitespace from HTTP headers #53005

brianquinlan commented Jul 21, 2023 •

edited

aam commented Jul 21, 2023 •

edited

itsjustkevin commented Jul 21, 2023

brianquinlan commented Jul 21, 2023

aam commented Jul 21, 2023

a-siva commented Jul 24, 2023

sgjesse commented Jul 25, 2023

aam commented Jul 25, 2023

itsjustkevin commented Aug 10, 2023

brianquinlan commented Aug 10, 2023

itsjustkevin commented Aug 11, 2023

grouma commented Aug 11, 2023

Hixie commented Aug 11, 2023

vsmenon commented Aug 14, 2023

brianquinlan commented Aug 14, 2023

grouma commented Aug 14, 2023

brianquinlan commented Aug 14, 2023

Remove trailing whitespace from HTTP headers #53005

Remove trailing whitespace from HTTP headers #53005

Comments

brianquinlan commented Jul 21, 2023 • edited

Change Intent

Justification

Impact

Mitigation

aam commented Jul 21, 2023 • edited

itsjustkevin commented Jul 21, 2023

brianquinlan commented Jul 21, 2023

aam commented Jul 21, 2023

a-siva commented Jul 24, 2023

sgjesse commented Jul 25, 2023

aam commented Jul 25, 2023

itsjustkevin commented Aug 10, 2023

brianquinlan commented Aug 10, 2023

itsjustkevin commented Aug 11, 2023

grouma commented Aug 11, 2023

Hixie commented Aug 11, 2023

vsmenon commented Aug 14, 2023

brianquinlan commented Aug 14, 2023

grouma commented Aug 14, 2023

brianquinlan commented Aug 14, 2023

brianquinlan commented Jul 21, 2023 •

edited

aam commented Jul 21, 2023 •

edited