-
Notifications
You must be signed in to change notification settings - Fork 565
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
Host and :authority must agree #968
Changes from 1 commit
2cfe2e9
bad442d
b4eec03
6444758
577a84a
af9a402
ca28eef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2934,16 +2934,29 @@ cookie: e=f | |
pseudo-header field to convey authority information, unless there is no authority | ||
information to convey (in which case it MUST NOT generate :authority). | ||
</t> | ||
<t> | ||
Clients MUST NOT generate a request with a <tt>Host</tt> header field that differs | ||
from the <tt>:authority</tt> pseudo-header field. A server MAY treat a request as | ||
malformed if it contains a <tt>Host</tt> header field that is different from the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is 'different' here? If one is present and one isn't, is that different? What if they have case differences? Etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't specify that deliberately, allowing someone to treat "example.com" and "example.com:443" and "Example.com" each as different to the other. Would it be best to point that out?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or just add 'when their values are compared, byte-for-byte' There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They should not be compared byte-for-byte, we did this mistake recently and broke a few clients :-) I think I'm fine with a SHOULD though. As Stefan (I think) mentioned, most implementations currently do not perform the comparison and a MUST would instantly make them non-compliant. HTTP/1.1 messaging proceeds differently (in 3.2). It enumerates what a client must send, then says that a recipient MUST reject as badreq any request with invalid or missing Host. This leaves a bit of gray area about what you consider as invalid but does mandate that some checks are performed. I'm not sure I like this approach better, but I wanted to note it if that can fuel the discussion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I definitely think we should leave a breadcrumb to, say, scheme-based normalization, as well as to the relevant bit of the -semantics draft (§ 7.2 Host and Authority). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with citing RFC 3986 is that it doesn't actually specify anything in Section 6.2. It's all examples. Section 7.2 of -semantics is similarly unhelpful. However, as the server is authoritative, perhaps we can rely on its own definitions of authority and say if the fields identify different authorities - by its own definition - it can reject the request. Would that work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we'd apply the same guidance to intermediaries? I think that's probably acceptable from where I'm sitting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I've worked something out. It's pretty gross though. Let me know what you think. |
||
value of the <tt>:authority</tt> pseudo-header field. | ||
</t> | ||
<t> | ||
An intermediary that forwards a request over HTTP/2 MUST construct an | ||
<tt>:authority</tt> pseudo-header field using the authority information from the | ||
control data of the original request, unless the the original request's target URI | ||
does not contain authority information (in which case it MUST NOT generate | ||
<tt>:authority</tt>). Note that the Host header field is not the sole source of this | ||
information; see <xref target="HTTP" section="7.2"/>. | ||
<tt>:authority</tt>). Note that the <tt>Host</tt> header field is not the sole | ||
source of this information; see <xref target="HTTP" section="7.2"/>. | ||
</t> | ||
<t> | ||
An intermediary that forwards a request received in HTTP/2 via HTTP/1.1 MUST set the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still embarrassed by the normative language involving H1 here. What about this variant: An intermediary that needs to produce a Host header field (e.g. to translate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, that's a good suggestion. I've taken it. |
||
<tt>Host</tt> field in the forwarded request to the value from the | ||
<tt>:authority</tt> pseudo-header field, unless it changes the request target. This | ||
martinthomson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
replaces any existing <tt>Host</tt> field to avoid potential vulnerabilities in HTTP | ||
routing. | ||
</t> | ||
<t> | ||
An intermediary that forwards a request over HTTP/2 MUST retain any <tt>Host</tt> | ||
An intermediary that forwards a request over HTTP/2 MAY retain any <tt>Host</tt> | ||
header field. | ||
</t> | ||
<t> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I get your reasoning for MAY here instead of something stronger. It seems that this is a security issue, so we should have stronger reasoning for optionality here. Personally I think this should be at least SHOULD, if not MUST.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning is that old implementations won't apply this rule (so it can't be MUST) and some might choose not to (as it requires an extra comparison). I'm OK with "SHOULD", but "MAY" seemed safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO if there's significant security impact, it can be a MUST - especially since this is about recipient behaviour, not sender behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @mnot. I also think that for senders we can safely say that the fields MUST be byte-for-byte identical.