1499356 - Incorrect header combining (flattening?)

Status: NEW

(Core :: Networking: HTTP, enhancement, P3)

Description

[Anne (:annevk)]

I've observed through fetch() and XMLHttpRequest that

  Foo: x
  Foo:
  Foo: y

becomes

  Foo: x, y

instead of

  Foo: x, , y

as you'd expect. However, https://tools.ietf.org/html/rfc7230#section-7 can only be used if you know the syntax of the header, which isn't the case for the more general purpose low-level library.

As far as I can tell from https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.cpp there's some assumption that empty header values can be elided, but that's not backed up by the HTTP standard and could even be a security problem of sorts.

I wrote wpt tests: https://github.com/web-platform-tests/wpt/pull/13471.

Comment 1

[Anne (:annevk)]

Another test this causes to fail: https://github.com/web-platform-tests/wpt/pull/13559.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Honza, can you take this?

Comment 3

[Kershaw Chang [:kershaw]]

I've had a talk with Anne yesterday.
There are some ongoing disscussions about parsing for some headers below.
 - Content-Length: https://github.com/web-platform-tests/wpt/pull/10548
 - X-Content-Type-Options: https://github.com/whatwg/fetch/pull/818
 - Content-Type: https://github.com/whatwg/mimesniff/issues/30#issuecomment-382721437
                 https://github.com/web-platform-tests/wpt/pull/10525

Anne said it would be great if someone from necko team can join the disscussions and share opions from our perspective.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #2)
> Honza, can you take this?

not any time soon, no.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

So, the Necko meeting consensus was that changing the parsing behaviour is a very risky move. We would not be comfortable making a potentially webcompat-breaking change before other UAs do.

We do have VisitOriginalResponseHeaders (bug 669259) to surface the exact headers we got back from the server - in case we want fetch to use a different header parsing/visualizing algorithm.

Comment 6

[Anne (:annevk)]

Assuming this is about comment 0, the architecture is presumably already different from other browsers, given how we end up exposing different values to XHR and fetch(). It also seems rather brittle to encourage a different code path for XHR/fetch() and other web content consumers. That seems highly likely to come back and haunt us.

Comment 7

[Sunil Mayya]

We have made changes with regard to parsing of response headers for XHR and fetch for the Bug 1697421.
The changes have fixed the General header combining problem mentioned in the bug Description.
As a result we could see that the wpt cases added for this bug (https://github.com/web-platform-tests/wpt/pull/13471) are passing:

https://wpt.fyi/results/fetch/api/basic/header-value-combining.any.html?label=master&label=experimental&aligned&view=subtest&q=header-value-combining
https://wpt.fyi/results/xhr/getallresponseheaders.htm?label=master&label=experimental&aligned&view=subtest&q=getallresponseheaders.htm
https://wpt.fyi/results/xhr/getresponseheader.any.worker.html?label=master&label=experimental&aligned&view=subtest&q=getresponseheader.any

However, we still have the following issues within the scope of this Bug:
Issue 1. content-length parsing
https://wpt.fyi/results/fetch/content-length/parsing.window.html?label=master&label=experimental&aligned&view=subtest&q=parsing.window

Issue 2. x-content-type options parsing
https://wpt.fyi/results/fetch/nosniff/parsing-nosniff.window.html?label=master&label=experimental&aligned&view=subtest&q=parsing-nosniff.window

Issue 3. Content-type parsing in general does not completely adhere to the fetch spec.

Issue 3 is tracked by Bug 1510180.

I propose to fix only Issue 1 and Issue 2 with this Bug.

Comment 8

[Anne (:annevk)]

That sounds good to me Sunil, thanks for tackling this!

Comment 9

[Sunil Mayya]

Unassigned from myself, will revisit this when I have some cycles.