New issue
Advanced search Search tips

Issue 801831 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Jan 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Enforce HTTP/2 invariants regarding :authority or :path

Project Member Reported by rsleevi@chromium.org, Jan 13 2018

Issue description

Note: Related to  Issue 797825 , but I didn't want to make this block on it or block it, since it's distinct enough to be its own issue.

Currently, the HTTP/2 and QUIC code uses QuicUrlUtils::IsValidUrl(), QuicUrlUtils::HostName(), and SpdyUtils::GetUrlFromHeadersBlock to handle processing of responses (and notably, PUSH_PROMISES) 

The HTTP/2 RFC makes several remarks re: client/server behaviour, namely:

https://tools.ietf.org/html/rfc7540#section-8.1.2.3
":authority" MUST be present, MAY be empty (by virtue of a 'host' that is a 'reg-name' which is allowed to be 0 characters by virtue of https://tools.ietf.org/html/rfc3986#section-3.2.2 ) , and MUST NOT contain the "userinfo" component of the authority
  - Notably, unless the scheme defines a default host, then the host portion of the authority MUST NOT be empty - namely "whereas the "http" scheme considers a missing authority or empty host invalid."

A current gap within SpdyUtils::GetUrlFromHeadersBlock() is that it does not enforce the invariant regarding 'userinfo'. It returns a (potentially invalid) URL, which then implementations like QuicUrlUtils::IsValidUrl() and QuicUrlUtils::HostName() attempt to parse into a GURL (forcing canonicalization each time they do, which is not ideal), without enforcing that the userinfo is empty.

Separately, the :path component MAY be "*" in the case of an OPTIONS request, but I couldn't find any code that validated the path / checked the '*' (although perhaps I missed it)
 
Project Member

Comment 1 by sheriffbot@chromium.org, Jan 14

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Recharge-Cold
rch: Can you look at these? It'd be great if they're now covered by tests :)
Components: -Internals>Network>HTTP2
Owner: rch@chromium.org
Status: Started (was: Untriaged)
For those following along at home...

The method SpdyUtils::GetUrlFromHeadersBlock has been renamed (to make the function's purpose more clear) to: quic::SpdyUtils::GetPromisedUrlFromHeaders. This has a couple of implications:
1) This issue only applies to QUIC, not to HTTP/2 (yay)
2) Only requests which are valid promised requests are valid for this method. In particular only GET and HEAD are supported by this method so OPTIONS is explicitly unsupported.

That being said, more sanity checking of :authority and :path are still in order and I'll give it a stab.
Status: WontFix (was: Started)
Looking more closely, this method already verifies the invariants by virtual of delegating to quic::SpdyUtils::GetPushPromiseUrl() which does the needful (and has a pile of tests). I'll add some additional tests to quic::SpdyUtils::GetPromisedUrlFromHeaders() just to verify. But I'm confident that this is working correctly today.

Sign in to add a comment