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

:authority is not required by spec #73

Closed
wants to merge 1 commit into from

Conversation

fredr
Copy link
Contributor

@fredr fredr commented Jan 22, 2019

If I understand the spec RFC 7540, :authority is not required

All HTTP/2 requests MUST include exactly one valid value for the
   ":method", ":scheme", and ":path" pseudo-header fields, unless it is
   a CONNECT request (Section 8.3).  An HTTP request that omits
   mandatory pseudo-header fields is malformed (Section 8.1.2.6).

@essen
Copy link
Member

essen commented Jan 22, 2019

Yes it seems to not be mandatory when a proxy translates an HTTP/1.1 request and it was using origin or asterisk form. I had not noticed that. This will need a test in rfc7540_SUITE in Cowboy though.

@essen
Copy link
Member

essen commented Mar 26, 2019

Merged, thanks!

@essen essen closed this Mar 26, 2019
@fredr
Copy link
Contributor Author

fredr commented Mar 26, 2019

🍻

@essen
Copy link
Member

essen commented Mar 27, 2019

I think I was too quick to merge this. The host header check makes better sense in this module than in Cowboy. I don't think this module should return pseudo headers without an authority. I will fix it.

@fredr
Copy link
Contributor Author

fredr commented Apr 1, 2019

Cool!

Let me know if you want me to do anything. I guess we can close ninenines/cowboy#1349 then?

@essen
Copy link
Member

essen commented Apr 1, 2019

Some parts of it are useful. I'm on it, I'll close it soon once I'm done.

@essen
Copy link
Member

essen commented Apr 1, 2019

OK after a lot of thinking I think I will leave the changes as they are currently. I don't like mixing up pseudo headers and regular headers from a semantic point of view. I will just tweak the other PR and merge.

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.

2 participants