Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panics with non-ASCII header value #853

Open
IVogel opened this issue Oct 7, 2021 · 2 comments
Open

Panics with non-ASCII header value #853

IVogel opened this issue Oct 7, 2021 · 2 comments
Labels
bug Something isn't working

Comments

@IVogel
Copy link

IVogel commented Oct 7, 2021

It panics due to lack of the string validity check in async-h1 and due to unwrap in http-types. Also, there is second unwrap that will panic if two fields with similar values exists, but second has valid UTF-8 string.

Minimal code for replication

#[async_std::main]
async fn main() -> tide::Result<()> {
    let mut app = tide::new();
    app.at("/").get( |_| async {
        Ok(tide::Response::new(200))
    });
    app.listen("0.0.0.0:8080").await?;
    Ok(())
}

Note, header value should be an valid UTF-8 string.

curl --header "Test: тест" http://0.0.0.0:8080
# or
curl --header "Test: test" --header "Test: тест" http://0.0.0.0:8088

Sorry if this is wrong repo. I don't really know to which repo this issue related to, because tide uses async-h1 which uses http-types. And I have got this error while using tide.

@Fishrock123 Fishrock123 added the bug Something isn't working label Oct 7, 2021
@yoshuawuyts
Copy link
Member

Thanks for raising this! You're right we should probably remove the unwraps from both those locations. Unfortunately HTTP headers are required to be ASCII to reliably work (see this SO question), so that restriction cannot go away. But on our end we should make it so that we don't unwrap within http-types.

Tasks

  • change signature of {Request,Response,Headers}::append to return Result
  • change signature of {Request,Response,Headers}::insert to return Result

@IVogel
Copy link
Author

IVogel commented Oct 8, 2021

Thanks for your reply.
Yes, I know that headers should be all in ASCII.
Wrote this because yesterday somebody has crashed my auth server. ¯\_( :| )_/¯
Looking forward to updates.

But for now, I'll just drop these non-ASCII headers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants