Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define behavior for Priority request header #1718

Open
pmeenan opened this issue Oct 16, 2023 · 16 comments · May be fixed by #1757
Open

Define behavior for Priority request header #1718

pmeenan opened this issue Oct 16, 2023 · 16 comments · May be fixed by #1757
Labels
needs concrete proposal Moving the issue forward requires someone to figure out a detailed plan needs tests Moving the issue forward requires someone to write tests topic: http

Comments

@pmeenan
Copy link
Contributor

pmeenan commented Oct 16, 2023

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
Copy link
Member

annevk commented Oct 16, 2023

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
Copy link
Contributor Author

pmeenan commented Oct 16, 2023

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
Copy link
Member

annevk commented Oct 16, 2023

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
Copy link

dessant commented Jun 2, 2024

This new header and the lack of control over it in browsers is breaking websites and browser extensions: dessant/buster#405 (comment)

@pmeenan
Copy link
Contributor Author

pmeenan commented Jun 2, 2024

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
Copy link
Member

annevk commented Jun 3, 2024

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
Copy link

valenting commented Jun 3, 2024

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.

@annevk annevk added needs tests Moving the issue forward requires someone to write tests needs concrete proposal Moving the issue forward requires someone to figure out a detailed plan labels Jun 3, 2024
@pmeenan
Copy link
Contributor Author

pmeenan commented Jun 3, 2024

  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
Copy link
Member

annevk commented Jun 3, 2024

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
Copy link
Member

annevk commented Jun 3, 2024

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
Copy link

dessant commented Jun 3, 2024

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
Copy link
Contributor Author

pmeenan commented Jun 3, 2024

@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
Copy link
Member

annevk commented Jun 3, 2024

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
Copy link

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.

@pmeenan pmeenan linked a pull request Jun 3, 2024 that will close this issue
5 tasks
@KershawChang
Copy link

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
Copy link
Member

annevk commented Jun 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs concrete proposal Moving the issue forward requires someone to figure out a detailed plan needs tests Moving the issue forward requires someone to write tests topic: http
Development

Successfully merging a pull request may close this issue.

5 participants