1188932 - Implement fetch user-agent header spec changes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The fetch spec changed to allow user-agent headers to be set:

  https://github.com/whatwg/fetch/issues/37

Comment 1

[Anne (:annevk)]

Note that this change also impacts XMLHttpRequest due to the sharing of infrastructure.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Allow the User-Agent header to be explicitly set by requests

I just removed the user-agent from the list of disallowed custom headers, and it seems to work.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a4710ea25de

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8654890 [details] [diff] [review]
Allow the User-Agent header to be explicitly set by requests

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

This looks good, but I want to make sure we handle cross-origin correctly.  Can you add a test to verify user-agent header is only permitted for CORS when the server has opted in with "Access-Control-Allow-Headers: user-agent"?

Thanks!

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Allow the User-Agent header to be explicitly set by requests

Added some CORS tests using User-Agent explicit headers

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8654909 [details] [diff] [review]
Allow the User-Agent header to be explicitly set by requests

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

Thanks! r=me

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Allow the User-Agent header to be explicitly set by requests

Updated - combined try with bug 1200337: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91a2ecf1ed04

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Allow the User-Agent header to be explicitly set by requests

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8656262 [details] [diff] [review]
Allow the User-Agent header to be explicitly set by requests

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

::: testing/web-platform/meta/XMLHttpRequest/setrequestheader-header-forbidden.htm.ini
@@ +1,4 @@
> +[setrequestheader-header-forbidden.htm]
> +  type: testharness
> +  [XMLHttpRequest: setRequestHeader() - headers that are forbidden]
> +    expected: FAIL

Can you file a bug or issue to get this fixed upstream?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8656262 [details] [diff] [review]
Allow the User-Agent header to be explicitly set by requests

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

::: testing/web-platform/meta/XMLHttpRequest/setrequestheader-header-forbidden.htm.ini
@@ +1,4 @@
> +[setrequestheader-header-forbidden.htm]
> +  type: testharness
> +  [XMLHttpRequest: setRequestHeader() - headers that are forbidden]
> +    expected: FAIL

Actually, James tells me changes are automatically upstreamed.  So you can just fix the test directly.  Maybe flag jgraham for review, though.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Allow the User-Agent header to be explicitly set by requests

Comment 11

[James Graham [:jgraham]]

Comment on attachment 8656855 [details] [diff] [review]
Allow the User-Agent header to be explicitly set by requests

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

The wpt parts of this look fine. So fine, in fact, that they were already changed in the sync I just landed and so should disappear from this patch if you rebase on top of inbound.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

(In reply to James Graham [:jgraham] from comment #11)
> Comment on attachment 8656855 [details] [diff] [review]
> Allow the User-Agent header to be explicitly set by requests
> 
> Review of attachment 8656855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The wpt parts of this look fine. So fine, in fact, that they were already
> changed in the sync I just landed and so should disappear from this patch if
> you rebase on top of inbound.

Awesome :) Will I have to remove the failures when I rebase as well then?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

This will allow user-agent header to be set on XHR and fetch() Request.  Flag to update the docs on MDN.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86fd2ab7d9f6

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

It occurs to me we probably should have gotten a DOM peer sign off on this since it changed script visible behavior of XHR.

Boris, do you have any problems with script being able to set the user-agent header in XHR and fetch()?

Comment 16

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ben Kelly [:bkelly] from comment #15)
> It occurs to me we probably should have gotten a DOM peer sign off on this
> since it changed script visible behavior of XHR.
> 
> Boris, do you have any problems with script being able to set the user-agent
> header in XHR and fetch()?

Oops, yeah we probably should've done that - this patch is pretty easy to back out if we decide that we don't want it, but we'll also have to adjust the wpt introduced in bug 1200337 to not check for the case where we set user-agent.

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/86fd2ab7d9f6

Comment 18

[James Graham [:jgraham]]

(In reply to Michael Layzell [:mystor] from comment #16)
> (In reply to Ben Kelly [:bkelly] from comment #15)
> > It occurs to me we probably should have gotten a DOM peer sign off on this
> > since it changed script visible behavior of XHR.
> > 
> > Boris, do you have any problems with script being able to set the user-agent
> > header in XHR and fetch()?
> 
> Oops, yeah we probably should've done that - this patch is pretty easy to
> back out if we decide that we don't want it, but we'll also have to adjust
> the wpt introduced in bug 1200337 to not check for the case where we set
> user-agent.

If you back this out you don't change the wpt test from upstream to avoid the case we fail, you change the metadata to indicate that we fail it. The whole point of web-platform-tests is that they are supposed to reflect the spec behaviour, not the particular state of our implementation.

Comment 19

[Boris Zbarsky [:bzbarsky]]

So this only allows setting user-agent on same-origin requests or cross-origin ones that get preflighted and for which the preflight says it's allowed?  That seems reasonable, though I'm not sure I see where having this header set forces a preflight... Does it?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #19)
> That seems reasonable, though I'm not sure I see where having this
> header set forces a preflight... Does it?

It should per spec.  Step 8 of main fetch:

  https://fetch.spec.whatwg.org/#concept-main-fetch

The list of "unsafe" headers is passed into the channel with SetCorsPreflightParameters().

To get the unsafe headers fetch uses Headers::GetUnsafeHeaders().  This basically checks to see if its not a simple header:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalHeaders.cpp#170

XHR checks a similar list of simple headers:

  https://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#3035

Since user-agent isn't in this list, then a preflight is forced.  I believe our tests check this as well.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Sounds good.

Comment 22

[Chris Mills [:cmills]]

To document this, I've added a note about the header being removed from the forbidden list to here:

https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name

I've also updated the Fetch and XHR docs to link to this list, so readers can easily find out which headers are forbidden and can't be set:

https://developer.mozilla.org/en-US/docs/Web/API/Headers
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#setRequestHeader%28%29

I've also added a note to the Firefox 43 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/43#Miscellaneous

Let me know if these updates look ok — a quick tech review would be appreciated. Thanks!

Comment 23

[Kohei Yoshino]

Has this been reverted or disabled at some point? I'm using the latest Nightly and getting an error when using xhr.setRequestHeader('User-Agent', 'CUSTOM-UA-STRING');

Comment 24

[Kohei Yoshino]

Please discard my last comment. My app [1] is using the Bugzilla API so that it's a cross-origin, preflight case. Therefore, Bugzilla should return the Access-Control-Allow-Headers header with User-Agent included.

[1] https://github.com/bzdeck/bzdeck/issues/393