file_body succeeds opening bad file path

[benstadin]

I'm running into an issue when the server receives a bogus file path like ".//v1/mapview/". file_body is able to open that path without error (see below), later on in Async Write an error code is set with message "Is a directory".

Should file_body.open check if the path is a directory?

void codeSnippetSendHandler() {
// snip
case RT_FILE: {
                    if (req.method() != http::verb::get &&
                        req.method() != http::verb::head ) {
                        return send(_defaultController.prepareBadRequest(std::move(req), "HTTP-method not accepted"));
                    }
                    
                    // Attempt to open the file
                    beast::error_code ec;
                    http::file_body::value_type body;
                    body.open(response->data.c_str(), beast::file_mode::scan, ec);
                    
                    // Handle the case where the file doesn't exist
                    if(ec == beast::errc::no_such_file_or_directory || !body.is_open()) {
                        return send(_defaultController.prepareNotFound(std::move(req), response->data));
                    }
                    
                    // Handle an unknown error
                    if (ec) {
                        return send(_defaultController.prepareServerError(std::move(req), ec.message()));
                    }
                    
                    // Respond to HEAD request
                    if (std::move(req).method() == http::verb::head) {
                        return send(_defaultController.prepareHeadResponse(std::move(req), body.size(), mimeType(response->data)));
                    }
                    
                    // Respond to GET request
                    if (std::move(req).method() == http::verb::get) {
                        return send(_defaultController.prepareFileResponse(std::move(req), body, mimeType(response->data), http::status::ok));
                    }
                    break;
                }
}


        template<class Body, class Allocator>
        http::response<http::file_body> prepareFileResponse(http::request<Body, http::basic_fields<Allocator>>&& req,
                                                                      http::file_body::value_type &body,
                                                                      boost::string_view mimeType,
                                                                      http::status statusCode) {
            // Cache the size since we need it after the move
            auto const size = body.size();
            
            http::response<http::file_body> res {
                std::piecewise_construct,
                std::make_tuple(std::move(body)),
                std::make_tuple(statusCode, req.version())
            };
            
            setFields(res);
            
            res.set(http::field::server, LOCATION_MASTER_SERVER_VERSION_STRING);
            res.set(http::field::content_type, mimeType);
            res.content_length(size);
            res.keep_alive(std::move(req).keep_alive());
            return res;
        }

[vinniefalco]

That's perplexing... which implementation is it using for file?

[benstadin]

I've updated the code snippet with the method preparing the file response.

[benstadin]

I should eventually mention that ./v1/mapview is an existing path on the server. A url to a non existing path like /v1/mapview/test correctly handles file not found.

[madmongo1]

Hmm, this is an interesting one. A file could be a number of things - symlink, Unix device, etc. Should the file body interface deal in file names (and therefore have to disambiguate) or file descriptors?

[benstadin]

My interpretation is that the file modes described in file_base.hpp need to be considered in file_body.open(). E.g. if file mode is beast::file_mode::scan or beast::file_mode::read then file_body.open() should fail in case the file is not readable.

[vinniefalco]

Beast File is never intended for directories

[benstadin]

Should it then fail to open?

[madmongo1]

@benstadin I'll have a design discussion with Vinnie about the behaviour of our object. I fear that like all seemingly simple design decisions, this one will end up more nuanced than was originally imagined.

In the meantime, are you comfortable adding a workaround in your code to stat the file and behave accordingly?

[benstadin]

@madmongo1 Sure, that will work for me. I already have a workaround in place and was raising it as a general topic.

[madmongo1]

I've had a think about this.

The #1 concern around sending files is security. Are you sending the file you intend to send? This applies whether you are acting as client or server.

Checking the program's intent is well beyond the scope of Beast. Beast has to assume that if you offer a file-name, then the expectation is that the file exists, is of the correct type, and that you do intend to send it.

It is therefore my view that checking that the file exists (and may be legally transmitted) should already have been checked by the application. I see no reason to change Beast's behaviour here.

I'll leave the issue open for a few more hours in case someone wants to argue, but I am minded to close it.

[benstadin]

I can follow your thoughts on this generally. Though from an API user perspective, it seems counter intuitive to what the library seems to offer:

// Attempt to open the file beast::error_code ec; http::file_body::value_type body; body.open(response->data.c_str(), beast::file_mode::scan, ec);

                // Handle the case where the file doesn't exist
                if(ec == beast::errc::no_such_file_or_directory || !body.is_open()) {
                    return send(_defaultController.prepareNotFound(std::move(req), response->data));
                }

I mean the sbove code reads like: Open file at path, otherwise handle the case when there is no such file or directory. It doesn‘t seem very intuitive that this check is never done.

[madmongo1]

Can you suggest an alternative that we can implement without breaking compatibility?

[benstadin]

Putting all pieces together from what I as user would assume, design decisions and what Vinnie said („ Beast File is never intended for directories“), I think after a call to open() body.is_open() should be set to false for „directories“.

Arguably, it changes the result. But in my opinion this is not an API change, but rather a corrective measure due to the expectation that file_body is by definition dealing with files only, and an attempt to open a directory should by this definition never have succeeded.

That said, I can live with any decision.

[vinniefalco]

it isn't meant for directories

[madmongo1]

Are you saying that if the call to open() fails, it should be a guarantee that is_open() should return false? This would seem reasonable to me.

[benstadin]

Yes.

[ashtum]

To check whether a file is a directory within the open function, an additional system call is necessary, such as fstat in POSIX or GetFileAttributesA in Win32. However, some users may find the added overhead of this step unacceptable because it might be unnecessary in their application.

Given the possibility of accessing the underlying file handle, one can create a free function to perform the check:

bool is_directory(beast::file_posix::native_handle_type fd, beast::error_code& ec)
{
  struct stat st;
  if (::fstat(fd, &st) != 0)
  {
    ec.assign(errno, beast::system_category());
    return false;
  }

  return S_ISDIR(st.st_mode);
}

// Usage
beast::error_code ec;
if (is_directory(body.file().native_handle(), ec))
{
  // ...
}