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

multipart::message from request causes Segmentation fault when Content-Type does not contain boundary value #916

Closed
BlrSystem opened this issue Oct 14, 2024 · 3 comments · Fixed by #931
Labels
bug Something isn't working
Milestone

Comments

@BlrSystem
Copy link

BlrSystem commented Oct 14, 2024

Implementing a file upload request from rust using awc::Client and actix_multipart_rfc7578::client::multipart to a crow server, I forgot to set the Content-Type header returned by multipart::Form::default(), leaving it with only the value “multipart/form-data”.

Rust bad request example

let mut form = multipart::Form::default();
let result = form.add_file("file", file_path);

client
.post(url.to_string() + "/file/upload")
.insert_header((CONTENT_TYPE,HeaderValue::from_static("multipart/form-data")))
.send_body(multipart::Body::from(form))
.await;

Crow server

void WebServer::handle_post_file_upload(
    const crow::request& req,
    crow::response& res
) 
const
{
 // without boundary the app crash crow::multipart::message
 crow::multipart::message multipart_message(req);
}

Rust good example

let mut form = multipart::Form::default();
let result = form.add_file("file", file_path);

let result = HeaderValue::try_from(form.content_type());
let Ok(content_type) = result else {
    return;
};

client
.post(url.to_string() + "/license/upload")
.insert_header((CONTENT_TYPE,content_type))
.send_body(multipart::Body::from(form))
.await;

Crow server workaround

void WebServer::handle_post_file_upload(
    const crow::request& req,
    crow::response& res
) 
const
{
    std::string content_type = req.get_header_value("Content-Type");

    if (content_type.find("multipart/form-data") == std::string::npos) {
        res.code = crow::BAD_REQUEST;
        res.body = (crow::json::wvalue){
            {"id",1},
            {"message","No multipart/form-data"}
        }.dump();
        return res.end();
    }

    if (content_type.find("boundary=") == std::string::npos) {
        res.code = crow::BAD_REQUEST;
        res.body = (crow::json::wvalue){
            {"id",1},
            {"message","No boundary= found in multipart body"}
        }.dump();
        return res.end();
    }

 // without boundary the app crash crow::multipart::message
 crow::multipart::message multipart_message(req);
}
@gittiver gittiver added the bug Something isn't working label Oct 15, 2024
@gittiver gittiver added this to the v1.3 milestone Oct 23, 2024
@Iuliean
Copy link
Contributor

Iuliean commented Oct 24, 2024

Hello, i investigated the issue submitted by op
image

It looks like an actual boundary is generated by actix/awc but not added to the header (not sure if this is the fault of actix/awc i am not an web expert by anymeans. my common sense says that it's weird at the very least)

Stepping forward with the debugger you can see that it then tries to parse some headers from "Content-Disposition" and then access the "name" attribute which does not exist. Hence the segmentation.
image

I'd say it's a combination of mistakes. What fixed things for me was to return in the "parse_body" method when the boundary was empty
image

Judging from the fact that the error gets to the user's handler and he is parsing the headers manually i would say that this is intended and maybe throwing an exception is desired. Also adding a unit test for this case would be nice

Let me know what you think i'd be happy to help with both :)

@gittiver
Copy link
Member

I agree, the missing boundary is a malformed request, therefore leaving function with raising an exception may be the appropriate reaction.
If you would want to spent time on the bugfix and testing I would be happy.

@BlrSystem
Copy link
Author

@Iuliean Thank you very much for your debug!
Yes, the missing boundary is a malformed request and it shouldn't happen, anyway raising an exception could be great.

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

Successfully merging a pull request may close this issue.

3 participants