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

Feat: Add a feature flag to ignore connection headers #705

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eaufavor
Copy link
Contributor

According to the RFC, when encountering connection headers, H2 should treat them as protocol errors. However, in reality there are servers just setting these headers but only for informational purpose.

This feature allow the receiving end to just ignore these headers without bailing the entire stream.

I'm just putting this out here to see whether it is possible for things like this to be accepted.

@seanmonstar
Copy link
Member

Are there any other widely used implementations that support this? I initially think the pressure should instead be pushed back onto those servers to change, since weakening the implementations is how HTTP/1.1 got to have so many edge cases.

@eaufavor
Copy link
Contributor Author

eaufavor commented Aug 15, 2023

Good question.
I've test a few user agents:
The server: nginx conf

location /test_h2_header/ {
   add_header connection "keep-alive";
   return 200 "hello"
}

https://origin.eaufavor.net/test_h2_header/

  • Chrome via h2: accepts the connection header (the header shows in network debug tab), no error.
  • Firefox via h2: ignores the header (not in the network debug tab), no error. Interestingly it also shows that "Connection: keep-alive" is sent by Firefox as a request header (if we trust it own debug tool but it doesn't actually send it per my test on the server side).
  • Curl: stream error.

I have not yet tested other connection headers.

@nox
Copy link
Contributor

nox commented Sep 18, 2023

Why is "CI / Check MSRV (pull_request)" failing?

@eaufavor
Copy link
Contributor Author

This #708?

@nox
Copy link
Contributor

nox commented Sep 19, 2023

Ah, could you rebase then?

According to the RFC, when encountering connection headers, H2 should
treat them as protocol errors. However, in reality there are servers
just setting these headers but only for informational purpose.

This feature allow the receiving end to just ignore these headers
without bailing the entire stream.
@eaufavor eaufavor force-pushed the ignore_connection_headers branch from 86ea93f to 9dbf011 Compare September 19, 2023 22:06
|| name == header::UPGRADE
|| name == "keep-alive"
|| name == "proxy-connection"
{
tracing::trace!("load_hpack; connection level header");
malformed = true;
if cfg!(feature = "ignore_connection_header") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be a cargo feature, instead it should be a runtime option, so that each part of a large system can decide whether or not they want to enable it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I can try to update (no promises when). Just wanted to make sure there is a chance for features like this to be accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants