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

[nox]

No description provided.

[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]

#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.

[acfoltzer]

Thank you again for spinning off #115, I've left my review over there.

As for the new proposed feature ignore_invalid_header_lines_in_responses in 7244d92, I have some concerns before we move ahead.

Overall thoughts

Beside the two specific areas of concern described below, I'm also wondering whether this level of permissiveness belongs in this API. The interaction with obs-fold was one that happened to occur to me as I read the patch, but it would not surprise me if there are other potential interactions that could lead to undesirable behavior just within the parser, let alone downstream effects from clients or servers observing incomplete sets of headers without awareness of the ignored field(s).

The other options we support for parsers at least have precedent in current or past versions of the HTTP/1 specs, but the proposed behavior steps entirely outside of those. Can you share more about the use case you have in mind for this? Are there clients and servers emitting such messages in practice today? Is this a hedge against data corruption?

Even with a compelling use case, this flag should be very heavily caveated in documentation along with specific descriptions of the cases where its use is required.

Scope of the new option

If we move ahead with adding this flag, the name should be changed and documentation added to more narrowly describe the behavior, or the implementation should expand to also allow ignoring errors that arise during parsing of the header value. In the current implementation, parse failures only lead to an ignored line if they occur at or before the colon separating the name from the value. Other errors that can arise after this point are not ignored. For example, modifying one of the new tests in 7244d92 to put the invalid character in the value does not yield the same behavior:

static RESPONSE_WITH_INVALID_CHAR_IN_HEADER_VALUE: &'static [u8] =
    b"HTTP/1.1 200 OK\r\nAccess-Control-Allow-Credentials: tr\x0Bue\r\nBread: baguette\r\n\r\n";

#[test]
fn test_ignore_header_line_with_invalid_char_in_value() {
    let mut headers = [EMPTY_HEADER; 2];
    let mut response = Response::new(&mut headers[..]);
    let result = ::ParserConfig::default()
        .allow_spaces_after_header_name_in_responses(true)
        .ignore_invalid_header_lines_in_responses(true)
        .parse_response(&mut response, RESPONSE_WITH_INVALID_CHAR_IN_HEADER_VALUE);

    // This assertion fails:
    assert_eq!(result, Ok(Status::Complete(81)));
    assert_eq!(response.version.unwrap(), 1);
    assert_eq!(response.code.unwrap(), 200);
    assert_eq!(response.reason.unwrap(), "OK");
    assert_eq!(response.headers.len(), 1);
    assert_eq!(response.headers[0].name, "Bread");
    assert_eq!(response.headers[0].value, &b"baguette"[..]);
}

Interaction with obs-fold

Ignoring up to the following newline does not mix well with allow_obsolete_multiline_headers_in_responses:

static RESPONSE_WITH_OBSOLETE_LINE_FOLDING_AND_INVALID_NAME: &'static [u8] =
    b"HTTP/1.1 200 OK\r\nLine-Folded-Header\xFF: hello  \r\n \r\n there\r\nBread: baguette\r\n";

/// This test passes; obs-fold is accepted but the invalid header name is not
#[test]
fn test_forbid_response_with_obsolete_line_folding_in_middle_and_invalid_name() {
    let mut headers = [EMPTY_HEADER; 1];
    let mut response = Response::new(&mut headers[..]);
    let result = ::ParserConfig::default()
        .allow_obsolete_multiline_headers_in_responses(true)
        .parse_response(&mut response, RESPONSE_WITH_OBSOLETE_LINE_FOLDING_AND_INVALID_NAME);

    assert_eq!(result, Err(::Error::HeaderName));
}

/// This should pass if we're ignoring the invalid folded line, but does not
#[test]
fn test_ignore_folded_line_with_invalid_name() {
    let mut headers = [EMPTY_HEADER; 1];
    let mut response = Response::new(&mut headers[..]);
    let result = ::ParserConfig::default()
        .allow_obsolete_multiline_headers_in_responses(true)
        .ignore_invalid_header_lines_in_responses(true)
        .parse_response(&mut response, RESPONSE_WITH_OBSOLETE_LINE_FOLDING_AND_INVALID_NAME);

    assert_eq!(
        result,
        Ok(Status::Complete(
            RESPONSE_WITH_OBSOLETE_LINE_FOLDING_IN_MIDDLE.len()
        ))
    );
    assert_eq!(response.version.unwrap(), 1);
    assert_eq!(response.code.unwrap(), 200);
    assert_eq!(response.reason.unwrap(), "OK");
    assert_eq!(response.headers.len(), 1);
    assert_eq!(response.headers[0].name, "Bread");
    assert_eq!(response.headers[0].value, &b"baguette"[..]);
}

I don't believe this would lead to a situation where the second line of a folded line could be confused for a legit field-line all on its own, as a valid folded line must be preceded by RWS whitespace. But it's close enough that I would want to heavily scrutinize any logic that determines when the ignored header line is truly over.

[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 😞

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.

[acfoltzer]

A couple small documentation tweaks, but then the remaining issue is whether to be more defensive about making this only apply to responses as described in #114 (comment)

[nox]

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

[acfoltzer]

Looking good to me now, thank you! As a followup, I will try to think about how best to make requests fully configurable without breaking the current response-only flags.

[seanmonstar]

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