Skip to content

Conversation

hannesg
Copy link

@hannesg hannesg commented Sep 22, 2018

According to rfc7230 http parsers are expected to ignore space around header values. Trying to construct a HeaderValue with leading or trailing whitespace is therefore clearly an error.

According to [rfc7230](https://tools.ietf.org/html/rfc7230#section-3.2.4)
http parsers are expected to ignore space around header values. Trying
to construct a HeaderValue with leading or trailing whitespace is
therefore clearly an error.
@carllerche
Copy link
Collaborator

I don't know if I agree that extra work should be done to make it an error. I could probably be convinced either way. Maybe @seanmonstar has thoughts on this?

@seanmonstar
Copy link
Member

I guess it depends on how the additional checks affect performance. Is it possible to see some benchmark results of before and after?

@seanmonstar seanmonstar added the B-rfc Blocked: request for comments. More discussion would help move this along. label Oct 18, 2018
@robjtede
Copy link

robjtede commented Apr 27, 2022

It's kind of understandable for from_static but sort of a shame to add this on try_from_generic (up-to-date method) since httparse already makes sure that leading and trailing spaces are not part of the header values it parses.

@DemiMarie
Copy link

@robjtede It’s the job of the library to validate the input it provides. If the input is already validated, it might make sense to add a (possibly unsafe) function that skips the validation. Not having leading and/or trailing whitespace is an invariant that must be upheld by all HTTP field values.

@DemiMarie
Copy link

@seanmonstar I don’t have benchmarks, but I expect this to be significantly cheaper than iterating through the value to check for forbidden characters. The check can be relaxed to “is the first or last byte 0x20 or less” if one does the existing validation first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-rfc Blocked: request for comments. More discussion would help move this along.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants