Sanitize uploaded filenames in upload example to prevent path traversal

[superm1]

The upload example wrote each uploaded file using the filename supplied verbatim in the multipart Content-Disposition header. A client could set that filename to an absolute path or one containing ../ components and cause the server to create or overwrite files outside the working directory.

This reduces each client-supplied filename to its base name, rejects the request with 400 Bad Request if the result is empty, ., .., or still contains a path separator, and writes the files into a dedicated uploads directory.

[yhirose]

Thanks for the fix — the upload example is easy to copy-paste, so sanitizing the client-supplied filename here is worthwhile.

A few points before merging:

1. This breaks the example build (must fix)

example/Makefile compiles with -std=c++11, but <filesystem> requires C++17, so make in example/ now fails with:

error: no member named 'filesystem' in namespace 'std'

Since the library itself targets C++11, I'd prefer avoiding <filesystem> in the example rather than bumping the Makefile. A plain C++11 base-name extraction works fine:

auto sanitize = [](const string &filename) -> string {
  auto name = filename.substr(filename.find_last_of("/\\") + 1);
  if (name.empty() || name == "." || name == ".." ||
      name.find(':') != string::npos) {
    return string();
  }
  return name;
};

This also handles the POSIX case uniformly: ..\..\evil.txt gets reduced to evil.txt instead of being rejected, and there's no platform asymmetry in how separators are treated.

2. The .. substring check is overly strict

name.find("..") != string::npos rejects legitimate filenames like report..final.txt. Once the name is reduced to a base name, an embedded .. is harmless — the exact-match name == ".." check is sufficient (and the substring check makes that exact-match comparison redundant).

3. Minor: consider rejecting :

On Windows, a name like foo.txt:stream could create an NTFS alternate data stream. Adding : to the reject list is a one-character change (included in the snippet above).

Everything else looks good — writing into a dedicated uploads/ directory and returning 400 on suspicious names are both improvements over the previous behavior.

[superm1]

Adjusted, thanks!

[yhirose]

Thanks, but there is another issue when writing into uploads/ assumes that directory already exists. It doesn't, and nothing in this PR creates it. If you run the example fresh, ofstream ofs("uploads/" + image_name, ...) opens in a failed state (I verified locally that ofs.good() is false), but since there's no error check, the handler still returns "done" and 200 OK. So the example silently stops working: no file gets written, no error surfaces.

Before this PR, files were written directly into the cwd, so this dependency on a pre-existing directory is new here. It's not a pre-existing issue. So it would be better to actually make sure that directory exists rather than assume it. Otherwise, users would be confused.

You could do either:

1. Create the directory at startup (e.g. via a portable mkdir call), or at minimum
2. Check ofs's state after opening/writing and return a 500 on failure, so it's not a silent no-op

Thanks!

[superm1]

Thanks, I've added an error code for this case.

[yhirose]

filesystem::create_directories isn't supported in C++11...

[superm1]

filesystem::create_directories isn't supported in C++11...

Gah.

[superm1]

I guess we can do something like this where we just call mkdir with an OS specific backing?

[yhirose]

The uploads/ directory is what's causing most of the trouble. It forced the directory creation, which forced create_directories (C++17), which forced the OS-specific mkdir. But once the filename is a safe base name, writing into uploads/ is no safer than writing into the current directory. It's only adding platform code.

Since this is just an example, I'd like to keep it minimal. Please drop uploads/ and the whole mkdir/<direct.h>/<sys/stat.h> block, write into the current directory as before, and keep only the filename sanitization which is the real point. That also lets us drop <filesystem> (the example still builds with -std=c++11):

auto sanitize = [](const string &filename) -> string {
  auto name = filename.substr(filename.find_last_of("/\\") + 1);
  if (name.empty() || name == "." || name == ".." ||
      name.find(':') != string::npos) {
    return string();
  }
  return name;
};

const auto image_name = sanitize(image_file.filename);
const auto text_name = sanitize(text_file.filename);
if (image_name.empty() || text_name.empty()) {
  res.status = StatusCode::BadRequest_400;
  return;
}

Then ofstream ofs(image_name, ios::binary); as before. This is simpler, easier to understand, and better conveys the original intention of this example. Thanks!