2016-09-06 17:03 GMT+03:00 Amos Jeffries <[email protected]>:
> > This patch fixes Squid to return 505 (Version Not Supported) error > > code for HTTP/2+ versions. > > > > Before this change, when a syntactically valid HTTP/1 request indicated > > HTTP major version 2, Squid mangled and forwarded the request as an > > HTTP/1 message. > > That is not true. The trunk code returns false the same as this patched > version. Because the version field for HTTP/2 does not start with the > static magic string "HTTP/1. Disagree: the trunk Http::One::RequestParser::parseHttpVersionField() returns true, because it treats HTTP/2 message as HTTP/0.9 message. You can see this in my Co-Advisor attachment in "HTTP/1.1 device MUST NOT use 2.1 HTTP version in requests" test case. > > Since Squid does not and cannot correctly interpret an > > HTTP/1 request using HTTP/2 rules, returning an error and closing the > > connection appears to be the only correct action possible. > > That is not correct either. The old logic requires method to be GET for > full HTTP/0.9 handling. HTTP/2 pseudo-method is PRI. If anything other > than PRI is seen it is not valid HTTP/2. > At most we should have logic passing HTTP/2 transparently through the > proxy (configurable). Which is the relay behaviour of HTTP/0.9 > processing, but not its header mangling behaviour (because PRI, not GET > method). Not sure that is what Http::One::RequestParser::parseHttpVersionField() should care of. In my understanding, this method should parse the input with HTTP/1 rules, returning false if the input does not conform to these rules. > Also, HTTP/2 does not obsolete HTTP/1.x RFC's, so there is an explicit > possibility that HTTP/1.2+ will exist at some point. The IETF WG > expectation is/was that 1.2+ (if any) would contain compatibility for > HTTP/2+ features over HTTP/1.x syntax. OK, and the patched Squid would return 505 (HTTP Version Not Supported) for HTTP/1.2+.(it is not yet implemented). > Just noticed that the unit test for "GET / HTTP/2.0" request-line is > also failing due to this parser change now accepting that invalid > version as valid. I suspect that test should be changed to check for 505 error code instead of 400. > You should use tok.skip(Http1::MagicPrefix) instead of trying to count > digits in the major version. I count digits in order to differentiate syntactically valid(but unsupported by Squid) HTTP versions and invalid(multi-digit) ones, returning 505 or 400 codes respectively. Eduard. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
