Invalid URL character cause panic

[DCjanus]

Bug Report

Version

aws_sdk_s3 0.2.0 and main branch on github which commit hash is 6feb50d

Platform

Linux DCjanusArchLinuxNUC10 5.15.7-arch1-1 #1 SMP PREEMPT Wed, 08 Dec 2021 14:33:16 +0000 x86_64 GNU/Linux

AWS Services

S3

Description

This code cause panic

#[tokio::test]
async fn test_invalid_key() {
    let config = aws_sdk_s3::Config::builder().build();

    aws_sdk_s3::input::HeadObjectInput::builder()
        .bucket("test")
        .key("<")
        .build()
        .unwrap()
        .make_operation(&config)
        .await;
}

Panic message:

thread 'test_invalid_key' panicked at 'should be valid request: http::Error(InvalidUri(InvalidUriChar))', sdk\s3\src\input.rs:14854:28

[rcoh]

thanks for the report! we'll investigate and get a fix out ASAP

[DCjanus]

Code from here:

aws-sdk-rust/sdk/aws-smithy-http/src/label.rs

Line 18 in 6feb50d

percent_encoding::utf8_percent_encode(t.as_ref(), uri_set).to_string()

Looks like we percent encode when character hit the blacklist, but in the real world, input could be really unexpected, and we should encode characters that not on the whitelist.

Since we are using reqwest and http, there is a builtin whitelist

[DCjanus]

BTW, maybe we should avoid unwrap or expect as much as we can, since most AWS SDK type is not UnwindSafe.

Which means, I can't use futures::future::FutureExt::catch_unwind to recover my server from panic, which is really horrible.

I don't think expect like this is a good idea, maybe we should return error rather than panic

    fn assemble(
        builder: http::request::Builder,
        body: aws_smithy_http::body::SdkBody,
    ) -> http::request::Request<aws_smithy_http::body::SdkBody> {
        builder.body(body).expect("should be valid request")
    }

[rcoh]

yeah we will add more exhaustive testing—generally the SDK should never panic when dispatching requests, so this is a bug (and the expect here quickly alerted you to the issue)

[DCjanus]

Sounds good to me, thanks for your help.

[rcoh]

Thanks for reporting & debugging! the fix for this has landed upstream and will go out in the next release!

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.