Closed Bug 1188932 Opened 4 years ago Closed 4 years ago

Implement fetch user-agent header spec changes

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: bkelly, Assigned: Nika)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

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

  https://github.com/whatwg/fetch/issues/37
Note that this change also impacts XMLHttpRequest due to the sharing of infrastructure.
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
Attachment #8654890 - Flags: review?(bkelly)
Assignee: nobody → michael
Status: NEW → ASSIGNED
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!
Attachment #8654890 - Flags: review?(bkelly)
Added some CORS tests using User-Agent explicit headers
Attachment #8654890 - Attachment is obsolete: true
Attachment #8654909 - Flags: review?(bkelly)
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
Attachment #8654909 - Flags: review?(bkelly) → review+
Depends on: 1200337
Attachment #8655028 - Attachment is obsolete: true
Attachment #8656262 - Flags: review?(bkelly)
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?
Attachment #8656262 - Flags: review?(bkelly) → review+
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.
Attachment #8656262 - Attachment is obsolete: true
Attachment #8656855 - Flags: review?(james)
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.
Attachment #8656855 - Flags: review?(james)
(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?
This will allow user-agent header to be set on XHR and fetch() Request.  Flag to update the docs on MDN.
Keywords: dev-doc-needed
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()?
Flags: needinfo?(bzbarsky)
(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.
https://hg.mozilla.org/mozilla-central/rev/86fd2ab7d9f6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(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.
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?
Flags: needinfo?(bzbarsky)
(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.
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!
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');
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
Duplicate of this bug: 405904
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.