HTTP-version related improvements & cleanups #133
Conversation
RFC 7230 section 3.1.1 defines the first line of a request as
request-line = method SP request-target SP HTTP-version CRLF
and RFC 7230 section 2.6 defines the HTTP-version part as
HTTP-version = HTTP-name "/" DIGIT "." DIGIT
HTTP-name = %x48.54.54.50 ; "HTTP", case-sensitive
in other words, HTTP-version is intended to be two single digits.
The two most common values encountered are HTTP/1.0 and HTTP/1.1 and
so we provide direct matching for those.
This implicitly also fixes the bug of absurdly big version numbers
wrapping over into negative HTTP version integer parts.
Previously, `HttpParseException`s on the HTTP request would silently terminate the connection without any explanation being sent to the client. With this commit, `HttpParseException`s occuring during request parsing are transformed into basic code-400 HTTP error responses before terminating the connection.
According to RFC 7230 section 2.6 "A server can send a 505 (HTTP Version Not Supported) response if it wishes, for any reason, to refuse service of the client's major protocol version." Since HTTP/2 with is significantly different on-wire message format has been released, we *know* that a major version larger than 1 is definitely not supported by snap-server currently and so it's reasonable to reject such doomed to fail requests with the appropriate 505 response code early on.
Some of the HTTP-version tests were assuming that if version does not match "HTTP/1.1" it is to be treated as "HTTP/1.0" which is not proper for dealing with a hypothetical HTTP/1.2 request; this patch changes the checks to be more in accordance with RFC 7230 section 2.6 which denotes > The minor version advertises the sender's > communication capabilities even when the sender is only using a > backwards-compatible subset of the protocol, thereby letting the > recipient know that more advanced features can be used in response > (by servers) or in future requests (by clients). and > A server SHOULD send a response version equal to the highest version > to which the server is conformant that has a major version less than > or equal to the one received in the request. A server MUST NOT send > a version to which it is not conformant. Fwiw, the section also states that in principle it is legitimate for a HTTP server to reply to a HTTP/1.0 message with ah HTTP/1.1 response with the caveat > When an HTTP/1.1 message is sent to an HTTP/1.0 recipient [RFC1945] > or a recipient whose version is unknown, the HTTP/1.1 message is > constructed such that it can be interpreted as a valid HTTP/1.0 > message if all of the newer features are ignored. However, the current patch merely avoids treating a HTTP/1.2 request incorrectly as a HTTP/1.0 style request, whereas it is more appropriate to treat a HTTP/1.2 request as a HTTP/1.1 request with unknown features ignored and responding with HTTP/1.1 replies to inform the (hypothetical) HTTP/1.2 client that HTTP/1.1 is the highest version understood by the server.
| -- HTTP-version = HTTP-name "/" DIGIT "." DIGIT | ||
| -- HTTP-name = %x48.54.54.50 ; "HTTP", case-sensitive | ||
| -- | ||
| pVer s = case bsStripHttpPrefix s of |
There was a problem hiding this comment.
This code is in the core accept loop and so we can't tolerate code that's significantly slower here. E.g. bsStripHttpPrefix can't return Maybe because that's an extra allocation you can't afford; instead you would need to return null string as your empty case.
| Just "1.0" -> return (1, 0) | ||
| Just vstr | ||
| | [mjs,'.',mns] <- S.unpack vstr | ||
| , Just mj <- digitToInt mjs |
There was a problem hiding this comment.
Same here with calling digitToInt. We are more okay with a misparse returning version '0.0' than we are with adding another x bytes of allocation overhead to this function
There was a problem hiding this comment.
even if this is the unhappy path, which would only occur for "bad" clients? NB: in the cases above I've made sure to handle the happy path with lowest overhead by direct matching w/o any dynamic allocations.
There was a problem hiding this comment.
e.g. bsStripHttpPrefix that returns a Maybe - that's in the hot path, so we'd prefer to return ByteString there to save three words and a pointer indirection. Null string is as good as Nothing there. caseing on the resulting bytestrings using string equality in the hot path is probably faster than splitting on . like I was doing before, though.
| -- snap-server currently and so it's reasonable to reject such | ||
| -- doomed to fail requests with the appropriate 505 response | ||
| -- code early on. | ||
| when (fst version >= 2) return505 |
There was a problem hiding this comment.
This part is fine and we can do it without modifying the parsing code. If you want you can reject fst version /= 1 because we don't support HTTP 0.x either :)
These are a couple of the issues I noticed with the HTTP-version handling in snap-server