http icon indicating copy to clipboard operation
http copied to clipboard

Remove trailing whitespaces from header values

Open Eijebong opened this issue 7 years ago • 4 comments

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");
}

Eijebong avatar Sep 08 '18 15:09 Eijebong

Some implementation notes to remember:

  • This should be handled in from_str, from_bytes, and from_shared. The value in these functions should be trimmed.
  • Since from_static should usually be called with compile-time string literals, it may be best to panic if the string starts or ends with whitespace. (Though, it is also possible to trim a static string to get a shorter static string...)
  • Tests should check each of these constructors.

seanmonstar avatar Sep 11 '18 21:09 seanmonstar

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?

hannesg avatar Sep 16 '18 11:09 hannesg

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

seanmonstar avatar Sep 17 '18 21:09 seanmonstar