Fix http 1.0 #7

Merged
merged 5 commits into from Apr 1, 2012

Conversation

Projects
None yet
2 participants
Contributor

mcunha commented Mar 29, 2012

Once more, now with feeling! It was nagging me why HTTP 1.0 GET's were not failing in unit testing, and yet they failed with curl in a sample app.

Commits include:

  • Fix test harness to not provide EOF marker - real world clients will not disconnect/provide EOF until they have a response, so the parser needs to pass acceptable requests along, and only read to EOF if reasonably sure there's an entity body being sent. This change breaks a bunch of tests if not applied with the parser fixes below.
  • Fix HTTP 1.0 POST to include Content-Length as per RFC
  • Add HTTP 1.0 POST without Content-Length - this should not happen and if it does we should parse headers and then pass request along the stack so they can send HTTP 400/411 back. This test fails without parser fixes below.
  • Fix parser to only read to EOF when an entity body is known to be present - Requests that fail to signal entity body presence should be replied to with error messages but should not hang parser until client drops connection.
  • Minor update to gitignore to forget about test-results folders used by MonoDevelop [in current MD alpha at least]

Well known use cases triggering HTTP 1.0 usage include using nginx as a reverse proxy, possibly others.

mcunha added some commits Mar 29, 2012

Test harness should not provide EOF marker
Clients who connect should not disconnect until they have a reply, so
parsing needs to complete successfully before we reach EOF.
Fix 1.0 POST to be RFC compliant
All requests with an entity body must have Content-Length set or
Transfer-Encoding set. Failure to do should result in HTTP 400 Bad
Request or HTTP 411 Length Required.
Only read to EOF if entity body present
All valid requests with an entity body will have Content-Length or
Transfer-Encoding set. Failure to do so should result in HTTP 400 Bad
Request or HTTP 411 Length Required. Parser should not wait for EOF to
come.

Could you split this out into a separate test, and explicitly define the behavior when the Content-Length is missing?

Owner

mcunha replied Mar 31, 2012

If you look into e405487 you'll find it tests exactly the situation where the client uploads something and doesn't provide CL or TE (which is what this test was testing before). Regarding defining the behavior, I didn't put it in because I was looking at HttpMachine as a mere parser, so I thought it should bubble up for Kayak to reply. Am I assuming too much ?

Maybe there could be a flag on TestRequest that indicates to the test harness whether or not it should execute this line? Would that be a step towards explicitly nailing down the behavior for malformed requests?

Owner

mcunha replied Mar 31, 2012

The only edge case I see using that flag would be simulating a client that disconnects before we reply to it's last request [possibly with > 0 requests sent previously]. But if the client disconnects we dont' care about parsing anymore. Kayak should release the socket and move on, client will never read our response.

What kind of malformed requests are you thinking about ?

Owner

bvanderveen commented Apr 1, 2012

I've had a look at this stuff on my laptop this morning, and I'm satisfied with all of it. Thanks Marco!

bvanderveen added a commit that referenced this pull request Apr 1, 2012

@bvanderveen bvanderveen merged commit 41e85f7 into bvanderveen:master Apr 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment