return a 4xx status code on non-ascii headers #172
Conversation
None utf-8 or ascii headers should not make a server panic, but return 400 BadRequest instead.
ririsoft
force-pushed
the
none-ascii-header
branch
from
fe32ac2
to
729c3ac
Compare
| @@ -80,6 +80,8 @@ where | |||
| req.set_version(Some(http_types::Version::Http1_1)); | |||
|
|
|||
| for header in httparse_req.headers.iter() { | |||
| // https://tools.ietf.org/html/rfc822#section-3.1 | |||
| http_types::ensure_status!(header.value.is_ascii(), 400, "None ascii header"); | |||
There was a problem hiding this comment.
This requires two iterations over each byte — one for the is_ascii() check and the other for the from_utf8. I believe we should just map the error value for the result of from_utf8, like std::str::from_utf8(header.value).map_err(|_| Error::from_str(400, "non-ascii header"))? or using Status, std::str::from_utf8(header.value).status(400)?
There was a problem hiding this comment.
Sorry, I commented below already.
There is a panic that prevent mapping the error unfortunately.
Unwrap occurs here: https://github.com/http-rs/http-types/blob/41655460b0229064535308e329417f8de014c2a8/src/headers/headers.rs#L66
Error comes from here:
https://github.com/http-rs/http-types/blob/41655460b0229064535308e329417f8de014c2a8/src/headers/header_value.rs#L80
|
On further examination, how is this a panic? The current code uses a |
jbr
changed the title
|
Hi @jbr ,
The panic is easily reproducible with the test cases I added to the pull request. Unwrap occurs here: https://github.com/http-rs/http-types/blob/41655460b0229064535308e329417f8de014c2a8/src/headers/headers.rs#L66 I believe the original title was valid, there is truly a panic. |
|
My apologies, I misunderstood. It seems to me like we should fix this by removing the unwrap in headers.rs, especially since the http-types v3 window is open. @yoshuawuyts, what do you think about append returning Result<()>? Or doing a lossy conversion? Panicking whenever to_header_values() fails seems problematic, and fixing it by handling this specific reason it might fail doesn't address the root concern. In particular, if someone implements the ToHeaderValues trait for their type, the interface gives the impression that the Result will be handled gracefully, but then we unwrap it in the primary usage of that api. Maybe ToHeaderValues should not return a Result to make implementers address failures, or we should propagate that Result up to user code |
|
This seems more complex than "only ascii utf8 allowed in headers" — https://www.rfc-editor.org/rfc/rfc8187.html |
From my understanding, I may be wrong, RFC8187 introduce the possibility to add UTF-8 characters percent encoded, so that it remains in the ascii space. |
|
I believe this is the same issue as #187 |
|
I believe the panic thrown by http_types::headers::Headers append/insert is now handled in http-rs/http-types#385 by the functions returning a Result now. Although that version is main-only and 3.0.0 is unreleased yet. |
|
I am not using http-rs anymore and I am thus not interested in working on this PR further. |
Hello,
server::decodepanics on valid UTF-8 but none ASCII headers and return500 Internal Server Erroron none valid UTF-8 headers.I believe a
400 Bad Requestshould be returned instead in both cases.Note: Theorically the additional check will slightly decrease server side headers decoding performances. I am not sure about the project's policy, but usage of unsafe code
std::str::from_utf8_uncheckedcould remove the performance penalty while remaining 100% safe. I have not benchmarked this at all however.What do you think ?
Cheers.