Skip to content

Commit e07ebcd

Browse files
committed
fix: ignore unnamed multipart file uploads before target handling
Reported-by: AllenYen Private coordination: GHSA-q8g6-h8wv-pr43 Validation: make check -j1, 113/113 test executables and check-local gates passed.
1 parent 54ee855 commit e07ebcd

5 files changed

Lines changed: 91 additions & 1 deletion

File tree

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,4 +65,5 @@ libtool
6565
.claude
6666
CLAUDE.md
6767
.groundwork-plans/
68+
.debug/
6869
.DS_Store

‎Makefile.am‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ EXTRA_DIST = libhttpserver.pc.in $(DX_CONFIG) scripts/extract-release-notes.sh s
6363
test/headers/consumer_umbrella.cpp \
6464
test/headers/consumer_post_umbrella.cpp \
6565
test/headers/consumer_umbrella_no_backend.cpp \
66-
LICENSE RELEASE_NOTES.md CONTRIBUTING.md CODE_OF_CONDUCT.md
66+
LICENSE RELEASE_NOTES.md CONTRIBUTING.md CODE_OF_CONDUCT.md SECURITY.md
6767

6868
# ---------------------------------------------------------------------------
6969
# Header-hygiene checks (TASK-002)

‎src/detail/upload_pipeline.cpp‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,9 @@ MHD_Result upload_pipeline::process_file_upload(detail::connection_context* conn
135135
MHD_Result upload_pipeline::iterate_file(detail::connection_context* conn,
136136
const char* key, const char* filename, const char* content_type,
137137
const char* transfer_encoding, const char* data, size_t size) {
138+
// MHD supplies a null key for file parts without a name attribute.
139+
// Ignore them before touching either the memory or disk upload target.
140+
if (key == nullptr) return MHD_YES;
138141
try {
139142
if (config_.file_upload_target != FILE_UPLOAD_DISK_ONLY) {
140143
conn->request->set_arg_flat(key,

‎test/integ/file_upload.cpp‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,28 @@ class print_file_upload_resource : public http_resource {
295295
string content;
296296
};
297297

298+
// Raw multipart bytes preserve the missing name attribute; curl_mime would
299+
// supply a name and conceal this malformed-input regression.
300+
static std::pair<CURLcode, long> send_raw_multipart(int port, const string& body) {
301+
CURL* curl = curl_easy_init();
302+
const string url = "http://127.0.0.1:" + std::to_string(port) + "/anything";
303+
curl_slist* headers = curl_slist_append(nullptr,
304+
"Content-Type: multipart/form-data; boundary=BOUNDARY");
305+
curl_easy_setopt(curl, CURLOPT_URL, url.c_str());
306+
curl_easy_setopt(curl, CURLOPT_HTTPHEADER, headers);
307+
curl_easy_setopt(curl, CURLOPT_POSTFIELDS, body.data());
308+
curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, static_cast<long>(body.size()));
309+
curl_easy_setopt(curl, CURLOPT_TIMEOUT, 5L);
310+
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION,
311+
+[](char*, size_t size, size_t count, void*) -> size_t { return size * count; });
312+
const CURLcode result = curl_easy_perform(curl);
313+
long status = 0;
314+
curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &status);
315+
curl_easy_cleanup(curl);
316+
curl_slist_free_all(headers);
317+
return {result, status};
318+
}
319+
298320
LT_BEGIN_SUITE(file_upload_suite)
299321
void set_up() {
300322
}
@@ -1069,6 +1091,33 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_sanitize_keeps_basename)
10691091
rmdir(upload_directory.c_str());
10701092
LT_END_AUTO_TEST(file_upload_sanitize_keeps_basename)
10711093

1094+
LT_BEGIN_AUTO_TEST(file_upload_suite, unnamed_multipart_file_keeps_server_alive)
1095+
curl_global_init(CURL_GLOBAL_ALL);
1096+
webserver ws{create_webserver(0)};
1097+
ws.on_get("/hello", [](const http_request&) {
1098+
return http_response::string("hello");
1099+
});
1100+
ws.start(false);
1101+
LT_CHECK_EQ(ws.is_running(), true);
1102+
const int port = ws.get_bound_port();
1103+
for (const char* disposition : {"filename=\"x\"", "filename=\"\"",
1104+
"name=\"f\"; filename=\"x\""}) {
1105+
const string body = string("--BOUNDARY\r\nContent-Disposition: form-data; ")
1106+
+ disposition + "\r\n\r\ndata\r\n--BOUNDARY--\r\n";
1107+
const auto result = send_raw_multipart(port, body);
1108+
LT_CHECK_EQ(result.first, CURLE_OK);
1109+
LT_CHECK_EQ(result.second, 404L);
1110+
// A second HTTP request proves the daemon still handles connections.
1111+
const auto followup = send_raw_multipart(port,
1112+
"--BOUNDARY\r\nContent-Disposition: form-data; name=\"f\"\r\n"
1113+
"\r\nok\r\n--BOUNDARY--\r\n");
1114+
LT_CHECK_EQ(followup.first, CURLE_OK);
1115+
LT_CHECK_EQ(followup.second, 404L);
1116+
}
1117+
ws.stop();
1118+
curl_global_cleanup();
1119+
LT_END_AUTO_TEST(unnamed_multipart_file_keeps_server_alive)
1120+
10721121
LT_BEGIN_AUTO_TEST_ENV()
10731122
AUTORUN_TESTS()
10741123
LT_END_AUTO_TEST_ENV()

‎test/unit/post_iterator_null_key_test.cpp‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "httpserver/create_test_request.hpp"
2525
#include "httpserver/detail/connection_context.hpp"
2626
#include "httpserver/detail/webserver_impl.hpp"
27+
#include "httpserver/detail/upload_pipeline.hpp"
2728

2829
#include "./littletest.hpp"
2930

@@ -121,6 +122,42 @@ LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, valid_key_continuation_appends)
121122
std::string("hello"));
122123
LT_END_AUTO_TEST(valid_key_continuation_appends)
123124

125+
// An unnamed file part must be ignored before either upload target is touched.
126+
LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, unnamed_file_has_no_side_effects)
127+
for (auto target : {httpserver::FILE_UPLOAD_MEMORY_ONLY,
128+
httpserver::FILE_UPLOAD_DISK_ONLY,
129+
httpserver::FILE_UPLOAD_MEMORY_AND_DISK}) {
130+
for (const char* filename : {"x", ""}) {
131+
request_fixture f;
132+
httpserver::webserver_config config;
133+
config.file_upload_target = target;
134+
httpserver::detail::upload_pipeline uploads(config);
135+
for (int chunk = 0; chunk < 2; ++chunk) {
136+
MHD_Result r = MHD_NO;
137+
LT_CHECK_NOTHROW(r = uploads.iterate_file(
138+
&f.conn, nullptr, filename, nullptr, nullptr, "data", 4));
139+
LT_CHECK_EQ(r, MHD_YES);
140+
LT_CHECK_EQ(f.req.get_args().size(), static_cast<size_t>(0));
141+
LT_CHECK_EQ(f.req.get_files().size(), static_cast<size_t>(0));
142+
LT_CHECK_EQ(f.conn.upload_ostrm.get(), nullptr);
143+
LT_CHECK_EQ(f.conn.upload_key, std::string());
144+
LT_CHECK_EQ(f.conn.upload_filename, std::string());
145+
}
146+
}
147+
}
148+
LT_END_AUTO_TEST(unnamed_file_has_no_side_effects)
149+
150+
LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, named_file_chunks_append)
151+
request_fixture f;
152+
httpserver::webserver_config config;
153+
httpserver::detail::upload_pipeline uploads(config);
154+
LT_CHECK_EQ(uploads.iterate_file(
155+
&f.conn, "f", "x", nullptr, nullptr, "hel", 3), MHD_YES);
156+
LT_CHECK_EQ(uploads.iterate_file(
157+
&f.conn, "f", "x", nullptr, nullptr, "lo", 2), MHD_YES);
158+
LT_CHECK_EQ(std::string(f.req.get_arg_flat("f")), std::string("hello"));
159+
LT_END_AUTO_TEST(named_file_chunks_append)
160+
124161
LT_BEGIN_AUTO_TEST_ENV()
125162
AUTORUN_TESTS()
126163
LT_END_AUTO_TEST_ENV()

0 commit comments

Comments
 (0)