whatwg / fetch

Fetch Standard
https://fetch.spec.whatwg.org/
Other
2.1k stars 325 forks source link

Define behavior for `Priority` request header #1718

Open pmeenan opened 10 months ago

pmeenan commented 10 months ago

What is the issue with the Fetch Standard?

The Priority request header is part of RFC 9218 (Extensible HTTP Priorities) and sent by the networking layer when appropriate (for most browsers this is when using HTTP/3, Chrome may start sending it for HTTP/2 as well).

The header is not currently on the list of forbidden request headers and the behavior is undefined for how it interacts with a user-provided Priority header in fetch.

For actual prioritization, fetch provides RequestPriority (though it is not as granular) but applications may have their own use for the header name if they are already sending it.

It would be helpful to specify the behavior either by adding it to the forbidden header list or defining how the extensible priorities header should be treated if an application provides an explicit Priority header.

The current behavior in Firefox is to send both headers. In Chrome (behind a flag) the header will only be set by the networking stack if the application didn't include a Priority header as part of the request.

annevk commented 10 months ago

This is why Sec- exists. Why wasn't this considered when the Priority request header was being implemented? I.e., how can we avoid this in the future?

cc @whatwg/http

pmeenan commented 10 months ago

Sec- makes sense if the header is meant to be forbidden.

I can't speak directly to the extensible priorities spec since I was mostly an observer while the process went through but the end-to-end header intentionally allows for overrides by the origin and it's not immediately clear that the intent wasn't also to allow for the value to be application-specified or at least overridden.

That said, it wouldn't hurt for both groups to align on when Sec- should be used and when it shouldn't. It also came up during discussions on compression dictionaries.

annevk commented 10 months ago

In that case we should probably not set the Priority header when it's already set, similar to what we do with User-Agent.

dessant commented 3 months ago

This new header and the lack of control over it in browsers is breaking websites and browser extensions: https://github.com/dessant/buster/issues/405#issuecomment-2143898748

pmeenan commented 3 months ago

The Priority HTTP header is defined in RFC 9218 and Safari and Firefox have always sent it for requests over HTTP/3. Chrome recently added it for HTTP/2 and HTTP/3 (previously only sent the priority frame).

At least in the case of Chrome, if the Priority header is set before it gets to the networking stack then the user-supplied value will be used (like was discussed above).

annevk commented 3 months ago

To fix this:

  1. Tests need to be written in web-platform-tests to ensure the header does not get overwritten when set directly.
  2. https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch needs to be patched to account for this header, similar to how it already accounts for User-Agent and other headers.

cc @domfarolino

valenting commented 3 months ago

At least in the case of Chrome, if the Priority header is set before it gets to the networking stack then the user-supplied value will be used (like was discussed above).

Starting with Firefox 126 we send the header for all requests, and override any user set header: https://phabricator.services.mozilla.com/D201265 We're happy to align on the Chrome behaviour, as stated above. I filed 1900362 to do just that.

pmeenan commented 3 months ago
  1. Tests need to be written in web-platform-tests to ensure the header does not get overwritten when set directly.

It doesn't look like web-platform-tests support HTTP/3 which is where it is commonly used. I can create HTTP/1 and HTTP/2 tests which will test one of Chrome's paths but I'm not sure it will work for either Firefox or Safari (as best as I could tell, the change in Firefox 126 is still only setting the header in HTTP/3).

annevk commented 3 months ago

Would you mind filing an issue requesting HTTP/3 support? I think an issue (might want to reference https://github.com/web-platform-tests/rfcs/blob/master/rfcs/quic.md) and 1/2 coverage is sufficient for now. The Fetch PR will result in implementation bugs being filed which will at least make Gecko and WebKit aware of the problem.

annevk commented 3 months ago

Oh, hmm, there is https://github.com/web-platform-tests/rfcs/blob/master/rfcs/webtransport_h3_test_server.md as well, but I guess it's currently WebTransport specific? I haven't tried to play with it. An issue is prolly a good next step either way.

dessant commented 3 months ago

Chrome 124 and Firefox 126 both set the Priority header for HTTP/2 requests, but Safari 17.5 does not.

The Priority header implementation in Chrome and Firefox ignores the existence of the request filtering engine available to extensions. declarativeNetRequest and webRequest do not see the header because it is set by the browser after request filtering occurs, so extensions cannot remove the header, they will only be able to set a custom value.

Any new HTTP headers you introduce should be set before request filtering, so extensions can have full control over them. If setting the Priority header later on in the network stack is indeed crucial and cannot be done earlier, please reach out to the folks at https://github.com/w3c/webextensions to discuss a solution for having control over such headers.

pmeenan commented 3 months ago

@annevk do you have suggestions on how the process flow would look when the value of the Priority header itself is decided and defined at transport-time and the actual priority values are internal breowser-specific (and a bit opaque in the spec)?

I could see something like "If httpRequest’s header list does not contain 'Priority', then user agents may append an appropriate Priority to httpRequest’s header list." but the steps for determining the value to set would require unwinding the browser-specific treatment of internal-priority.

Maybe a some text that references the extensible priorities RFC and call it a mapping of internal-priority to the RFC?

The timing of the processing might not be appropriate though (as we move towards browser literally implementing the process steps directly) and maybe something like a boolean flag on request that indicates if the Priority header is allowed to be set by the transport layer or not and leave the actual setting opaque?

annevk commented 3 months ago

I think something along those lines seems fine. Probably needs to use "implementation-defined".

Since the exact time cannot be observed, the user agent doing it slightly later seems fine and doesn't have to be accounted for.

valenting commented 3 months ago

as best as I could tell, the change in Firefox 126 is still only setting the header in HTTP/3

No, the Priority header is all requests. You can check by loading https://example.com over H2.

KershawChang commented 2 months ago

At least in the case of Chrome, if the Priority header is set before it gets to the networking stack then the user-supplied value will be used (like was discussed above).

Starting with Firefox 126 we send the header for all requests, and override any user set header: https://phabricator.services.mozilla.com/D201265 We're happy to align on the Chrome behaviour, as stated above. I filed 1900362 to do just that.

While working on bug 1900362, we found that we need to include the priority header in the CORS-safelisted request-header list to pass some tests. I'm not sure if we implemented it incorrectly or if the priority header actually needs to be in the list.

@annevk , what do you think? Thanks.

annevk commented 2 months ago

I think that means you set the header too early. I would not expect the header to show up in Access-Control-Request-Headers. We don't include User-Agent there either. See #1757 for the proposed model.