New issue
Advanced search Search tips

Issue 896233 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Chrome's header parser skips empty values

Project Member Reported by mmenke@chromium.org, Oct 17

Issue description

This results in some weirdness:

Content-Length: ,10,

is considered valid, for instance.

Also, it means the code to walk through the original "Header lines" ignores leading and trailing commas in header values, instead of actually returning the full original header line.

Changes here do have the potential to break sites, unfortunately, but we should work towards a more standardized header parser.
 
Summary: Chrome's header parser skips empty values (was: Chromes header parser skips empty values)
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7913a74b0e22d99c3425dcea22e68783919a9667

commit 7913a74b0e22d99c3425dcea22e68783919a9667
Author: Matt Menke <mmenke@chromium.org>
Date: Thu Oct 25 17:14:13 2018

Header parser:  Don't allow single quotes.

No HTTP header specs allow single quotes to be used instead of double
quotes, so this CL better aligns Chrome with the spec.

Testing current behavior, using
http://test.greenbytes.de/tech/tc2231/attwithfntokensq.asis:
FireFox doesn't support single quotes, but Chrome and Edge do.

Bug:  896233 , 179825
Change-Id: I29f034180d3dec06d767e6dff0ce938f48e47147
Reviewed-on: https://chromium-review.googlesource.com/c/1286733
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602768}
[modify] https://crrev.com/7913a74b0e22d99c3425dcea22e68783919a9667/components/download/internal/common/download_stats.cc
[modify] https://crrev.com/7913a74b0e22d99c3425dcea22e68783919a9667/components/gcm_driver/crypto/encryption_header_parsers_unittest.cc
[modify] https://crrev.com/7913a74b0e22d99c3425dcea22e68783919a9667/net/http/http_content_disposition.cc
[modify] https://crrev.com/7913a74b0e22d99c3425dcea22e68783919a9667/net/http/http_content_disposition.h
[modify] https://crrev.com/7913a74b0e22d99c3425dcea22e68783919a9667/net/http/http_content_disposition_unittest.cc
[modify] https://crrev.com/7913a74b0e22d99c3425dcea22e68783919a9667/net/http/http_util.cc
[modify] https://crrev.com/7913a74b0e22d99c3425dcea22e68783919a9667/net/http/http_util.h
[modify] https://crrev.com/7913a74b0e22d99c3425dcea22e68783919a9667/net/http/http_util_unittest.cc
[modify] https://crrev.com/7913a74b0e22d99c3425dcea22e68783919a9667/tools/metrics/histograms/enums.xml

Status: Fixed (was: Assigned)
Forgot to mention on the commit message that Safari also does not support single quotes.
Unless I'm missing something this fixed  bug 896232 , not this bug?
Status: Assigned (was: Fixed)
Thanks!
For Access-Control-Allow-Origin it seems Chrome is alone with this kind of handling, FWIW: https://github.com/web-platform-tests/wpt/pull/13815.
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90

commit 56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90
Author: Matt Menke <mmenke@chromium.org>
Date: Wed Oct 31 23:48:27 2018

Fix handling of LWS separated by commas in HTTP/1.x headers.

Previously, each the following was treated the same:
  Content-Length: 7
  Content-Length: ,7
  Content-Length: 7,
  Content-Length: , ,7, ,

And all condensed into a single header with a value of 7. There's
nothing in any spec that instructs us to do this. Moreover, the spec
explicitly states that the two lines:

  Content-Length: 7
  Content-Length:

Can be safely merged into:

  Content-Length: 7,

Without changing meaning, but in the former case, Chrome treats two
headers lines as if they were separate header lines, while in the
latter, as mentioned, it ignores the existence of the comma. Also,
the original header line could never be retrieved in this case,
since the extra information was just silently purged.

This CL treats both responses as having two separate Content-Length
headers (And this applies to all other non-coalescing headers as
well).  This behavior seems to match that of all other browsers, so
breakages should be minimal.

Bug:  896233 
Change-Id: I1acb26a3face0eef8d47c1dbe727aab5ec7dd7bd
Reviewed-on: https://chromium-review.googlesource.com/c/1297251
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604450}
[modify] https://crrev.com/56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90/base/strings/string_tokenizer.h
[modify] https://crrev.com/56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90/base/strings/string_tokenizer_fuzzer.cc
[modify] https://crrev.com/56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90/base/strings/string_tokenizer_unittest.cc
[modify] https://crrev.com/56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90/net/http/http_response_headers.cc
[modify] https://crrev.com/56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90/net/http/http_response_headers_unittest.cc
[modify] https://crrev.com/56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90/net/http/http_util.cc
[modify] https://crrev.com/56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90/net/http/http_util.h
[modify] https://crrev.com/56f794cd1a0f6f5dbd3b28c60b030ffdd0106b90/net/http/http_util_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment