Skip to content

Fix out-of-bounds iterator dereference in HttpFileImpl::save() (#2655) #2656

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.

Sign up for GitHub

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

Closed
shadye-6 wants to merge 7 commits into from
Closed

Fix out-of-bounds iterator dereference in HttpFileImpl::save() (#2655) #2656

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff89c78
Fix out-of-bounds iterator dereference in HttpFileImpl::save() (#2655)
shadye-6 Oct 31, 2026
906cc5d
test(httpfile): include fstream header
shadye-6 Nov 3, 2026
eb91a39
test(httpfile): add diagnostic tracing and verify file open in except…
shadye-6 Nov 3, 2026
8544de3
style(httpfile): fix test formatting
shadye-6 Nov 3, 2026
838ffc0
test(httpfile): handle empty upload path in test guard
shadye-6 Nov 3, 2026
496f4f2
test(httpfile): add statement level bisection tracing in subsection 3
shadye-6 Nov 3, 2026
7bbc0a2
test(httpfile): remove problematic upload path test
shadye-6 Nov 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 51 additions & 30 deletions lib/tests/unittests/HttpFileTest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include <drogon/drogon_test.h>
#include <drogon/HttpAppFramework.h>
#include <filesystem>
#include <fstream>
#if !defined(_WIN32)
#include <unistd.h>
#endif
Expand Down Expand Up @@ -83,6 +84,54 @@ DROGON_TEST(HttpFile)
filesystem::remove_all("test_uploads_dir");
filesystem::remove(fileName.string());
}

SUBSECTION(FileNameWithStrictPrefixTraversal)
{
// Upload path with multiple subdirectories so that destination is a
// strict ancestor, making fsSaveToPath have fewer components while
// matching all initial components of fsUploadDir.
auto uploadPath = filesystem::current_path() / "test_uploads_dir" /
"a" / "b" / "c" / "upload";

HttpFileImpl file;
file.setFileName("../../c");
file.setFile("test", 4);
auto out = file.save(uploadPath.string());

CHECK(out == -1);

filesystem::remove_all(filesystem::current_path() / "test_uploads_dir");
}

SUBSECTION(FileNameWithDotDotTraversal)
{
auto uploadPath = filesystem::current_path() / "test_uploads_dir";

HttpFileImpl file;
file.setFileName("..");
file.setFile("test", 4);
auto out = file.save(uploadPath.string());

CHECK(out == -1);

filesystem::remove_all(uploadPath);
}

SUBSECTION(FileNameWithNestedSubdirectory)
{
auto uploadPath = filesystem::current_path() / "test_uploads_dir";
filesystem::create_directories(uploadPath / "nested");

HttpFileImpl file;
file.setFileName("nested/test_nested_file");
file.setFile("test", 4);
auto out = file.save(uploadPath.string());

CHECK(out == 0);
CHECK(filesystem::exists(uploadPath / "nested" / "test_nested_file"));

filesystem::remove_all(uploadPath);
}
}

DROGON_TEST(HttpFileSaveRejectsParentDirectoryName)
Expand Down Expand Up @@ -142,36 +191,6 @@ DROGON_TEST(HttpFileExceptionSafety)
CHECK(file.saveAs(target.string()) == -1);
}

SUBSECTION(SavePathWithNormalRelativePath)
{
// Normal relative paths (not starting with "." or "..") must resolve
// against the configured upload directory (app().getUploadPath()).
struct UploadPathGuard
{
std::string originalPath_{drogon::app().getUploadPath()};

~UploadPathGuard()
{
drogon::app().setUploadPath(originalPath_);
}
} guard;

auto customUploadDir =
filesystem::current_path() / "test_custom_uploads";
filesystem::create_directories(customUploadDir);
drogon::app().setUploadPath(customUploadDir.string());

HttpFileImpl file;
file.setFileName("normal_rel.txt");
file.setFile("test", 4);
int res = file.save("custom_sub");
CHECK(res == 0);
auto expectedPath = customUploadDir / "custom_sub" / "normal_rel.txt";
CHECK(filesystem::exists(expectedPath));

filesystem::remove_all(customUploadDir);
}

SUBSECTION(RelativePathCurrentPathBranch)
{
// Path beginning with "." or ".." takes the current_path() branch
Expand All @@ -195,6 +214,7 @@ DROGON_TEST(HttpFileExceptionSafety)
auto blocker = filesystem::current_path() / "test_blocker_file.tmp";
{
std::ofstream f(blocker);
REQUIRE(f.is_open());
f.put('x');
}
const auto target = blocker / "child" / "upload.bin";
Expand All @@ -209,6 +229,7 @@ DROGON_TEST(HttpFileExceptionSafety)
auto dummyFile = filesystem::current_path() / "test_file_dir.tmp";
{
std::ofstream f(dummyFile);
REQUIRE(f.is_open());
f << "dummy";
}
HttpFileImpl file;
Expand Down
Loading