return a 4xx status code on non-ascii headers

[ririsoft]

Hello,

server::decode panics on valid UTF-8 but none ASCII headers and return 500 Internal Server Error on none valid UTF-8 headers.
I believe a 400 Bad Request should 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_unchecked could remove the performance penalty while remaining 100% safe. I have not benchmarked this at all however.

What do you think ?
Cheers.

[jbr]

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)?

[ririsoft]

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

[jbr]

On further examination, how is this a panic? The current code uses a ? to return an Err, which is not a panic. It might not have the desired status code, but a 500 is not a panic

[ririsoft]

Hi @jbr ,
Thank you for your time on this.

On further examination, how is this a panic? The current code uses a ? to return an Err, which is not a panic. It might not have the desired status code, but a 500 is not a panic

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
Error comes from here:
https://github.com/http-rs/http-types/blob/41655460b0229064535308e329417f8de014c2a8/src/headers/header_value.rs#L80

I believe the original title was valid, there is truly a panic.

[jbr]

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

[jbr]

This seems more complex than "only ascii utf8 allowed in headers" — https://www.rfc-editor.org/rfc/rfc8187.html

[ririsoft]

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.

[jbr]

I believe this is the same issue as #187

[shanipribadi]

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.

[ririsoft]

I am not using http-rs anymore and I am thus not interested in working on this PR further.