Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Response with multiple Transfer-Encoding: chunked header fields not parsed as chunk #683

Closed
nham opened this issue Nov 18, 2015 · 4 comments
Labels
A-headers Area: headers. C-bug Category: bug. Something is wrong. This is bad!

Comments

@nham
Copy link

nham commented Nov 18, 2015

I've adapted this code from client/response.rs:

fn main() {
    let stream = MockStream::with_input(b"\
        HTTP/1.1 200 OK\r\n\
        Transfer-Encoding: chunked\r\n\
        Transfer-Encoding: chunked\r\n\
        \r\n\
        1\r\n\
        q\r\n\
        2\r\n\
        we\r\n\
        2\r\n\
        rt\r\n\
        0\r\n\
        \r\n"
    );

    let url = Url::parse("http://hyper.rs").unwrap();
    let mut res = Response::new(url, Box::new(stream)).unwrap();

    let mut s = String::new();
    res.read_to_string(&mut s).unwrap();
    println!("{:?}", s);
}

The result is:

"1\r\nq\r\n2\r\nwe\r\n2\r\nrt\r\n0\r\n\r\n"

Unless I'm misreading the HTTP spec, it seems

HTTP/1.1 200 OK\r\n
Transfer-Encoding: chunked\r\n
Transfer-Encoding: chunked\r\n

should be the same as

HTTP/1.1 200 OK\r\n
Transfer-Encoding: chunked, chunked\r\n

The latter correctly results in "qwert" being printed out.

@seanmonstar seanmonstar added C-bug Category: bug. Something is wrong. This is bad! A-headers Area: headers. labels Nov 19, 2015
@seanmonstar
Copy link
Member

Indeed, it would seem the bug is in the parsing of the TransferEncoding header. It should handle more than 1 field occurrence.

@dignifiedquire
Copy link

@seanmonstar I'm running into this issue myself, but I'm not sure how/where to best fix it. Any pointers where I should start looking?

@seanmonstar
Copy link
Member

This occurs in the parsing. TransferEncoding uses parsing::from_comma_delimited(), which currently only allows 1 line of a header.

I cannot recall if there are any headers that are comma separated but should be rejected if there is more than one line. If not, then the fix would be in the from_comma_delimited method to check all the lines, not just the first.

@pyfisch
Copy link
Contributor

pyfisch commented Mar 10, 2016

I have written a parser for comma-delimited headers that should catch all cases like multiline headers, too much whitespace and superfluos commas.
https://github.com/pyfisch/kinglet/blob/master/src/headers.rs#L54-L100

Stebalien added a commit to Stebalien/hyper that referenced this issue Mar 11, 2016
HeaderX: a
HeaderX: b

MUST be interpreted as

HeaderX: a, b

See: https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

Fixes hyperium#683
Stebalien added a commit to Stebalien/hyper that referenced this issue Mar 11, 2016
Stebalien added a commit to Stebalien/hyper that referenced this issue Mar 24, 2016
Stebalien added a commit to Stebalien/hyper that referenced this issue Mar 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-headers Area: headers. C-bug Category: bug. Something is wrong. This is bad!
Projects
None yet
Development

No branches or pull requests

4 participants