hyperium / http

Rust HTTP types
Apache License 2.0
1.15k stars 285 forks source link

Remove trailing whitespaces from header values #245

Open Eijebong opened 6 years ago

Eijebong commented 6 years ago

As stated here,

The field value does not include any leading or trailing whitespace: OWS occurring before the first non-whitespace octet of the field value or after the last non-whitespace octet of the field value ought to be excluded by parsers when extracting the field value from a header field.

Here's a test case that should be passing:

#[test]
fn test_trailing_space() {
    let value = HeaderValue::from_bytes("coucou \t ".as_bytes()).unwrap();
    assert_eq!(value.to_str().unwrap(), "coucou");
}
seanmonstar commented 6 years ago

Some implementation notes to remember:

hannesg commented 6 years ago

Hi Sean, Hi Bastien

I considered writing a patch for that but I have one question. Shouldn't HeaderValue::from_bytes("coucou \t ".as_bytes()) rather return an error?

The rfc is about what parsers are expected to do. A correct parser should never create a value with leading or trailing whitespace anyway. So it rather makes sense to signal an incorrect usage instead of silently sanitizing.

Moreover, I've checked the implementation in hyper and it already trims leading whitespace but not trailing whitespace ( https://github.com/seanmonstar/httparse/blob/3aad4fac259e944b82aaf1af233624b2f98b85d9/src/lib.rs#L621 ) AND it uses the unsafe constructor for HeaderValue ( https://github.com/hyperium/hyper/blob/8f9174746660c52f7aa9ae67dd77dbd5200a136f/src/proto/h1/role.rs#L50 ) which is not expected to trim whitespace according to this description. I think we rather have to fix trailing whitespace in httparse and make constructing a HeaderValue with leading or trailing whitespace an error.

What's you opinion?

seanmonstar commented 6 years ago

I think it's probably fair to return an error instead of trimming automatically.