If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[XHR2] Does not concatenate multiple header values

RESOLVED DUPLICATE of bug 1285036

Status

()

Core
DOM: Core & HTML
P3
normal
RESOLVED DUPLICATE of bug 1285036
4 years ago
a year ago

People

(Reporter: hallvors, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 obsolete attachments)

(Reporter)

Description

4 years ago
Test cases:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-case-insensitive.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-header-allowed.htm

Comment 1

a year ago
Created attachment 8760556 [details] [diff] [review]
918763-fix-case-multivalue-and-permitted-name-issues-of-request-headers-in-xhrs.diff

The test links above have since changed to:

http://w3c-test.org/XMLHttpRequest/setrequestheader-case-insensitive.htm
http://w3c-test.org/XMLHttpRequest/setrequestheader-header-allowed.htm

Here's a patch that allows these tests to pass. These were the changes that had to be made;

1) Remove Content-Transfer-Encoding from the list of forbidden request headers, since it's not on the XHR/Fetch specs' list (just Transfer-Encoding).
2) Allow setting multiple values ("merging") for headers internally considered "singletons", like Authorization.
3) Allow headers that are set to default values (ie User-Agent) to be over-ridden with the user-set headers, rather than merged with them (if they aren't forbidden ones, of course).
4) Fix the XHR logic which keeps track of the user-set headers in case a redirect must be made.
5) Remove an unused hashtable variable, mAlreadySetHeaders.

I'm guessing that some of these changes (1,2) may be controversial, as they will affect more than just XMLHttpRequests. However, if I'm reading the WhatWG specs correctly, the Fetch and WebSockets APIs should also get fixes 1 and 2 (and 3 as well), so this version of the patch still seemed worth pitching.

Also note that Chrome doesn't currently allow the overriding of User-Agent in XHRs, though the spec does not forbid it and hence it fails that test case.
Attachment #8760556 - Flags: review?(jst)

Comment 2

a year ago
Created attachment 8763098 [details] [diff] [review]
918763-fix-failing-mochitest.diff

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc3a3a46b89c&selectedJob=22477374

Here's a follow-up patch to fix the mochitest failure that uncovered.
Comment on attachment 8760556 [details] [diff] [review]
918763-fix-case-multivalue-and-permitted-name-issues-of-request-headers-in-xhrs.diff

Redirecting to sicking as he'd be a better reviewer for this than me.
Attachment #8760556 - Flags: review?(jst) → review?(jonas)

Comment 4

a year ago
Created attachment 8766847 [details] [diff] [review]
918763-fix-case-multivalue-and-permitted-name-issues-of-request-headers-in-xhrs.diff

Rebased and folded into one patch.
Attachment #8760556 - Attachment is obsolete: true
Attachment #8763098 - Attachment is obsolete: true
Attachment #8760556 - Flags: review?(jonas)
Attachment #8766847 - Flags: review?(jonas)
Comment on attachment 8766847 [details] [diff] [review]
918763-fix-case-multivalue-and-permitted-name-issues-of-request-headers-in-xhrs.diff

Review of attachment 8766847 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentUtils.cpp
@@ -7011,5 @@
>  {
>    static const char *kInvalidHeaders[] = {
>      "accept-charset", "accept-encoding", "access-control-request-headers",
>      "access-control-request-method", "connection", "content-length",
> -    "cookie", "cookie2", "content-transfer-encoding", "date", "dnt",

Why remove "content-transfer-encoding"?

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2947,5 @@
> +        mModifiedRequestHeaders[isAlreadyUserSetIndex].value
> +      );
> +    } else {
> +      RequestHeader reqHeader = {
> +        nsCString(headerOverride ? *headerOverride : header),

Isn't headerOverride always null here?

@@ +2950,5 @@
> +      RequestHeader reqHeader = {
> +        nsCString(headerOverride ? *headerOverride : header),
> +        nsCString(value)
> +      };
> +      mModifiedRequestHeaders.AppendElement(reqHeader);

It might be easier to use two arrays instead. One with names and one with values. That way you can use nsTArray.indexOf to do a case-insensitive search for an already set header.

Up to you.

::: netwerk/protocol/http/nsHttpHeaderArray.cpp
@@ +51,5 @@
>      MOZ_ASSERT(!entry || variety != eVarietyRequestDefault,
>                 "Cannot set default entry which overrides existing entry!");
>      if (!entry) {
>          return SetHeader_internal(header, value, variety);
> +    } else if (merge) {

This change should be reviewed by a necko peer. Maybe :mayhemer.

At the very least I suspect that we need to ensure that when we parse a http-response, that we still don't merge headers which aren't singleton headers.
Attachment #8766847 - Flags: review?(jonas) → review+

Comment 6

a year ago
> Why remove "content-transfer-encoding"?

It's not one of the forbidden headers in the fetch spec (https://fetch.spec.whatwg.org/#forbidden-header-name)

Also the web platform test setrequestheader-header-allowed specifically checks that it isn't blocked.


>This change should be reviewed by a necko peer. Maybe :mayhemer.
>At the very least I suspect that we need to ensure that when we parse a http-response, that we still don't merge headers which aren't singleton headers.

Given that note (and the array suggestion), maybe it would be better to use my seventh patch in bug 1285036 instead? It manages the XHR request headers differently, so the nsHttpHeaderArray code won't need a change. However, it's a bigger patch, because it's attempting to align the algorithms for XHR request headers more closely with the spec (so it covers more WPTs), not just solve this specific ticket's issue.

Comment 7

a year ago
:baku just r+'d the patch I mentioned in the last comment, so I'll hold off on this one for now. If checkin of that one sticks, then I'll mark this one as obsolete.

Comment 8

a year ago
The patch in bug 1285036 that I mentioned in the last two comments has landed, fixing these web platform tests, so I'm closing this ticket as a dupe.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1285036

Updated

a year ago
Attachment #8766847 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.