On 7/09/2016 1:49 a.m., Amos Jeffries wrote: > On 6/09/2016 9:06 a.m., Eduard Bagdasaryan wrote: >> Hello, >> >> 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." > > >> 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). > > In the case where it is not HTTP/1.1, HTTP/1.0, HTTP/2, or GET without a > version field. Then the version must start with "HTTP/" to be considered > invalid HTTP version and the MUST clause applicable. > > Otherwise it could be any protocol at all. HTTP/2, SMTP, Finger, > Secure-HTTP, WebSocket, SPDY and ICAP can all be sent to this parse > logic at various times. > > 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. > >> >> Also: since HTTPbis prohibits HTTP versions with multi-digits, treat >> such requests as invalid. >> >> Attached Co-Advisor result pages for patched and unpatched Squid. >> > > For some reason I'm not sure of yet these changes result in unit test > failure parsing a garbage line with various bad whitespace characters in it. > > make check with testHttp1Parser debugging enabled produces: > " > TEST @1071, in="\r \t \n" > > testHttp1Parser.cc:106:Assertion > Test name: testHttp1Parser::testParseRequestLineProtocols > equality assertion failed > - Expected: 0 > - Actual : 1 > " >
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 think this other fail is due to "2.0" meeting both the version layout and the "multiDigits' conditions. You should use tok.skip(Http1::MagicPrefix) instead of trying to count digits in the major version. This is an HTTP/1 parser, non-1.x versions (except 0.9 special case) are all unsuccessful parse if they occur here. Amos _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
