Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 9 years ago

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

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

	Anne (:annevk)
	Comment 1 • 9 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 2 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 3 • 9 years ago

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!

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 4 • 9 years ago

Added some CORS tests using User-Agent explicit headers

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 5 • 9 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 6 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 8 • 9 years ago

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?

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 9 • 9 years ago

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.

	James Graham [:jgraham]
	Comment 11 • 9 years ago

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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 12 • 9 years ago

(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?

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 13 • 9 years ago

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

	Pulsebot
	Comment 14 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 15 • 9 years ago

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()?

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 16 • 9 years ago

(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.

	Phil Ringnalda (:philor)
	Comment 17 • 9 years ago

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

	James Graham [:jgraham]
	Comment 18 • 9 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 9 years ago

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?

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 20 • 9 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 9 years ago

Sounds good.

	Chris Mills [:cmills]
	Comment 22 • 9 years ago

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!

	Kohei Yoshino [:kohei]
	Comment 23 • 8 years ago

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');

	Kohei Yoshino [:kohei]
	Comment 24 • 8 years ago

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