CrowCpp / Crow

A Fast and Easy to use microframework for the web.
https://crowcpp.org
Other
3.24k stars 358 forks source link

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

Open BlrSystem opened 2 weeks ago

BlrSystem commented 2 weeks ago

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);
}
Iuliean commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

@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.