get-request-data: detect malformed request line better

[jjkola]

and also detect if request protocol is unsupported.

[stassats]

When does this happen? Is returning an error better than continuing working?

[jjkola]

For me this happened when the body of the request was not read but I would assume that if somebody made specifically crafted web request then it could cause wide array of problems. As I'm currently using radiance as the framework and hunchentoot as the server component I had a problem with unsupported request (it was json request) and I didn't realized that I need to read the body. This lead to situation that two of the following requests were handled incorrectly leading to undefined behaviour. After this fix the same situation only lead to one request being rejected and no undefined behaviour. I did propose a fix to i-hunchentoot in radiance contribs which will prevent the situation but I would still like to see hunchentoot to behave more correctly.

[stassats]

Checking the method might be a good idea, but i'm not sure that the protocol needs to be checked. It's not hunchentoot's job to validate http requests for conformance.

[jjkola]

I would say that as hunchentoot is parsing the request line and request headers then it should be expected that it will ensure that those are valid at least in some sense. Also as both method and protocol will be converted keywords thus if there is random data they may cause huge memory leak especially in the case of malicious request thus leading to denial of service. If you don't like the idea of hard-coding those values then there could be an interface to add user defined methods/protocols.

[stassats]

But you're making it fail when the protocol part is missing.

[jjkola]

I think it should nowadays be expected the protocol being specified as there is already 3 versions of HTTP protocols in use if we don't count pre 1.0 versions. If it is not specified then we should default to HTTP/1.0. This is because depending on the specification version the meaning of the absence of some headers are interpreted differently. So either require protocol or default to HTTP/1.0 in the absence of protocol.

Also, now that I read the code better the code currently defaults to HTTP/1.1 but based on HTTP 1.1 specification the default should be HTTP/1.0 in the absence of protocol. And HTTP/2 expects the protocol to be always specified thus HTTP 1.1 spec is the one which should be followed in this case. So that should be changed even if the requiring the protocol is removed.

[stassats]

Don't require protocol for the request and default to HTTP 1.0

But it already gets default below to :http/0.9

[jjkola]

Oh, I didn't even notice that. I'll remove that and I realized that I should trim the whitespace before testing against the accepted protocol versions. I'll provide the changes shortly.

[jjkola]

Now all default versions point to HTTP/1.0 so they are now consistent.

[stassats]

Since it's performing validation it'd be a good idea to map it directly to a keyword, instead of using intern.