Skip to content

Harden multi-threaded http server #5149

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

Open
SirTyson wants to merge 1 commit into
base: master
Choose a base branch
from
+115 −1
Open

Harden multi-threaded http server #5149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion lib/httpthreaded/request_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,14 @@ request_parser::initializeForBody(request const& req)
{
if (header.name == "Content-Length")
{
body_content_length_ = stoi(header.value);
try
{
body_content_length_ = stoull(header.value);
Copy link
Contributor

@marta-lokhova marta-lokhova Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but can we ask claude for a quick unit test that repros the issue and confirms the fix?

Copy link
Contributor Author

@SirTyson SirTyson Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
catch (std::exception const&)
{
return bad;
}
Comment on lines +301 to +304
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception handler catches std::exception const&, which is overly broad. It would be more precise to catch the specific exceptions that std::stoull can throw: std::invalid_argument (for invalid format) and std::out_of_range (for values that don't fit in unsigned long long). Catching only these specific exceptions makes the error handling more explicit and prevents inadvertently catching unrelated exceptions.

Suggested change
catch (std::exception const&)
{
return bad;
}
catch (std::invalid_argument const&)
{
return bad;
}
catch (std::out_of_range const&)
{
return bad;
}

Copilot uses AI. Check for mistakes.
if (body_content_length_ == 0)
{
return bad;
Expand Down
107 changes: 107 additions & 0 deletions src/main/test/HttpThreadedTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
// Copyright 2026 Stellar Development Foundation and contributors. Licensed
// under the Apache License, Version 2.0. See the COPYING file at the root
// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0

#include "lib/catch.hpp"
#include "lib/httpthreaded/request.hpp"
#include "lib/httpthreaded/request_parser.hpp"

using namespace httpThreaded::server;

namespace
{
// Helper: feed a raw HTTP request string into the parser and return
// the final result_type.
request_parser::result_type
parseRaw(std::string const& raw, request& req)
{
request_parser parser;
auto [result, iter] = parser.parse(req, raw.begin(), raw.end());
return result;
}
}

TEST_CASE("request parser rejects malformed Content-Length", "[http]")
{
// non-numeric Content-Length
{
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: garbage\r\n"
"\r\n"
"body";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::bad);
}

// negative Content-Length wraps to huge value
{
// stoull("-1") wraps to ULLONG_MAX rather than throwing, so the
// parser accepts the header but will never accumulate enough body
// bytes — it stays indeterminate.
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: -1\r\n"
"\r\n"
"body";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::indeterminate);
Comment on lines +37 to +48
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test comment and expectation for negative Content-Length is incorrect. The comment states that stoull("-1") wraps to ULLONG_MAX rather than throwing, but this is not accurate. According to the C++ standard, std::stoull throws std::invalid_argument when given a string with a leading minus sign (negative number). Therefore, this test case should expect request_parser::bad rather than request_parser::indeterminate, as the exception will be caught and bad will be returned.

Suggested change
// negative Content-Length wraps to huge value
{
// stoull("-1") wraps to ULLONG_MAX rather than throwing, so the
// parser accepts the header but will never accumulate enough body
// bytes — it stays indeterminate.
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: -1\r\n"
"\r\n"
"body";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::indeterminate);
// negative Content-Length is invalid
{
// stoull("-1") throws std::invalid_argument, so the parser
// treats this as a malformed header and rejects the request.
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: -1\r\n"
"\r\n"
"body";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::bad);

Copilot uses AI. Check for mistakes.
}

// overflow Content-Length
{
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: 99999999999999999999999\r\n"
"\r\n"
"body";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::bad);
}

// empty Content-Length
{
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: \r\n"
"\r\n"
"body";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::bad);
}

// Content-Length zero
{
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: 0\r\n"
"\r\n";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::bad);
Comment on lines +73 to +80
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejecting Content-Length: 0 may not be correct according to HTTP specifications. A Content-Length of 0 is valid for HTTP POST requests and indicates an empty body. Unless there is a specific requirement in this codebase to reject empty-bodied POST requests, this validation at line 305-308 in request_parser.cpp should be reconsidered. The test at lines 73-81 enforces this behavior, but it may be overly restrictive.

Suggested change
// Content-Length zero
{
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: 0\r\n"
"\r\n";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::bad);
// Content-Length zero should be accepted as an empty body
{
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: 0\r\n"
"\r\n";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::good);
REQUIRE(req.method == "POST");
REQUIRE(req.uri == "/test");
REQUIRE(req.body.empty());

Copilot uses AI. Check for mistakes.
}
}

TEST_CASE("request parser accepts valid POST", "[http]")
{
std::string raw = "POST /test HTTP/1.1\r\n"
"Content-Length: 4\r\n"
"\r\n"
"body";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::good);
REQUIRE(req.method == "POST");
REQUIRE(req.uri == "/test");
REQUIRE(req.body == "body");
}

TEST_CASE("request parser accepts valid GET", "[http]")
{
std::string raw = "GET /info HTTP/1.1\r\n"
"\r\n";
request req;
auto result = parseRaw(raw, req);
REQUIRE(result == request_parser::good);
REQUIRE(req.method == "GET");
REQUIRE(req.uri == "/info");
}
Loading