Allow ignoring invalid header lines (fixes #61, #83)

[seanmonstar]

It'd probably be better to have the separate commits as different PRs, as that would make the review panel a bit clear-er... But I looked at each commit individually, and I think it looks right.

@acfoltzer wanna take a look?

[acfoltzer]

@seanmonstar sure thing, can you tag me as a reviewer so I don't lose track? Should be able to dig in early next week if that's alright

[nox]

There is an actual bug now in httparse which makes invalid HTTP requests be accepted by the crate, could we hurry to get a new release?

[Coder-256]

I wonder if the existing behavior is vulnerable to response splitting, e.g. GET / HTTP/1.1\n\n: Foo\n\nBody; if config.allow_spaces_after_header_name_in_responses is enabled, would : Foo be parsed as a header instead of part of the body?

[acfoltzer]

There is an actual bug now in httparse which makes invalid HTTP requests be accepted by the crate, could we hurry to get a new release?

@nox would it be possible to split the invalid HTTP request fix from the new option? That would make it easier to review quickly. I will start taking a look at dcb18ab.

[nox]

Will do, but there isn't anything in this PR that should be long to review 🤷🏻‍♂️

[nox]

https://github.com/seanmonstar/httparse/pull/115

[nox]

I wonder if the existing behavior is vulnerable to response splitting, e.g. GET / HTTP/1.1\n\n: Foo\n\nBody; if config.allow_spaces_after_header_name_in_responses is enabled, would : Foo be parsed as a header instead of part of the body?

No, because the check for the first char being a valid header name char is separate, but now that you say this, I just realised I forgot to handle the first char being invalid, in this PR. I guess there was some nasty stuff to review after all hah!

[acfoltzer]

Thank you!

Will do, but there isn't anything in this PR that should be long to review 🤷🏻‍♂️

I want to make sure we have time to carefully understand the changes being made to the parser, and give some of my fuzzers time to chug on the new branch. After all, the impact if we get something wrong is pretty significant.

[nox]

Beside the two specific areas of concern described below, I'm also wondering whether this level of permissiveness belongs in this API.

Yes it does, browsers do that so httparse should to.

---

Will modify my patch to take care of errors in values.

---

The interaction with obsolete line folding is a non-issue because additional lines must start with whitespace, and that's never considered a header name. I'll add a test which involves an invalid header that uses obsolete line folding though.

[nox]

I also noticed yesterday that I didn't take care of header names whose very first char is invalid.

[nox]

I've studied Chromium's code a bit more and the one thing it rejects is NUL chars. Everything else is ok.

[acfoltzer]

Yes it does, browsers do that so httparse should to.

D'oh, of course browsers allow this :disappointed:

The interaction with obsolete line folding is a non-issue because additional lines must start with whitespace, and that's never considered a header name. I'll add a test which involves an invalid header that uses obsolete line folding though.

It looks like the new tests with folding address the problems I had in the original patch, nice!

I've studied Chromium's code a bit more and the one thing it rejects is NUL chars. Everything else is ok.

I see that there's now more of a description of behavior in the doc comment, thank you! I think it'd be worth enumerating a few more details there, like how it determines the end of the line to ignore, and how it interacts with folding and possibly the other options. Not quite to the level of an RFC with ABNF etc, but it would be nice to be very specific about what this allows.

[nox]

@acfoltzer Improved the docs and added a test for request parsing.

[seanmonstar]

Thanks to both of you for working through this! <3