diff --git a/CHANGELOG.md b/CHANGELOG.md index 9df519eb..1d8d1ccc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ loosely while pre-1.0 (breaking changes can land on minor bumps). ## [Unreleased] ### Fixed +- **Inbound HTTP body honors Content-Length.** `parse_http_request` now + takes at most `Content-Length` bytes from the header-read leftover, so + `Content-Length: 0` stays empty and extra pipelined or smuggled bytes + past the declared length are not adopted as this request's body. - **SSE mailbox terminal after overflow.** `sse_mailbox_push(..., force_deliver=true)` now evicts oldest events and delivers a terminal envelope even when a prior push already set `overflowed`. Request-event diff --git a/CMakeLists.txt b/CMakeLists.txt index c5f43348..61f6b1dd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1310,6 +1310,17 @@ if(INDEX_BUILD_TESTS) ) add_test(NAME unit_circuit_breaker COMMAND unit_circuit_breaker) + # --- Unit tests for inbound HTTP/1.1 request framing --- + add_executable(unit_http_request + tests/test_http_request.cpp + src/api/http_request.cpp + ) + target_include_directories(unit_http_request PRIVATE + ${CMAKE_SOURCE_DIR}/include + ${CMAKE_SOURCE_DIR}/third_party/doctest + ) + add_test(NAME unit_http_request COMMAND unit_http_request) + # --- Unit tests for CORS allowlist policy --- add_executable(unit_cors tests/test_cors.cpp diff --git a/src/api/http_request.cpp b/src/api/http_request.cpp index 5f4b4e3e..311d8c64 100644 --- a/src/api/http_request.cpp +++ b/src/api/http_request.cpp @@ -110,7 +110,13 @@ bool parse_http_request(int fd, HttpRequest& req) { } static constexpr size_t kMaxBody = 16 * 1024 * 1024; // hard cap if (want > kMaxBody) return false; - req.body = leftover; + // TCP often delivers the header sentinel plus extra bytes in one + // recv. leftover is that prefix — take at most `want` so CL:0 + // stays empty and pipelined/smuggled bytes past Content-Length + // never become this request's body. Connection: close drops the + // rest (same framing as ApiClient::read_response_body). + if (leftover.size() > want) leftover.resize(want); + req.body = std::move(leftover); char buf[4096]; while (req.body.size() < want) { size_t remaining = want - req.body.size(); diff --git a/tests/test_http_request.cpp b/tests/test_http_request.cpp new file mode 100644 index 00000000..0ae5be79 --- /dev/null +++ b/tests/test_http_request.cpp @@ -0,0 +1,85 @@ +// tests/test_http_request.cpp — inbound HTTP/1.1 request framing. + +#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN +#include "doctest.h" +#include "api/http_request.h" + +#include +#include +#include + +using namespace arbiter; + +namespace { + +bool parse_from_bytes(const std::string& raw, HttpRequest& req) { + int sp[2]; + if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sp) != 0) return false; + const ssize_t w = ::write(sp[0], raw.data(), raw.size()); + ::shutdown(sp[0], SHUT_WR); + const bool ok = parse_http_request(sp[1], req); + ::close(sp[0]); + ::close(sp[1]); + return ok && w == static_cast(raw.size()); +} + +std::string request_with(const std::string& headers, const std::string& body) { + return "POST /v1/orchestrate HTTP/1.1\r\n" + headers + "\r\n" + body; +} + +} // namespace + +TEST_CASE("exact Content-Length leftover is the body") { + HttpRequest req; + REQUIRE(parse_from_bytes( + request_with("Content-Length: 5\r\n", "hello"), req)); + CHECK(req.method == "POST"); + CHECK(req.path == "/v1/orchestrate"); + CHECK(req.body == "hello"); +} + +TEST_CASE("body is capped to Content-Length when leftover overshoots") { + HttpRequest req; + REQUIRE(parse_from_bytes( + request_with("Content-Length: 5\r\n", "helloXXXXEXTRA"), req)); + CHECK(req.body == "hello"); +} + +TEST_CASE("Content-Length 0 does not adopt leftover as body") { + HttpRequest req; + REQUIRE(parse_from_bytes( + request_with("Content-Length: 0\r\n", + "{\"agent\":\"smuggled\"}"), req)); + CHECK(req.body.empty()); +} + +TEST_CASE("GET without Content-Length drops leftover bytes") { + HttpRequest req; + REQUIRE(parse_from_bytes( + "GET /v1/health HTTP/1.1\r\nHost: x\r\n\r\nPOST /v1/orchestrate HTTP/1.1\r\n\r\n", + req)); + CHECK(req.method == "GET"); + CHECK(req.path == "/v1/health"); + CHECK(req.body.empty()); +} + +TEST_CASE("short body after Content-Length is rejected") { + HttpRequest req; + CHECK_FALSE(parse_from_bytes( + request_with("Content-Length: 10\r\n", "short"), req)); +} + +TEST_CASE("duplicate Content-Length is rejected") { + HttpRequest req; + CHECK_FALSE(parse_from_bytes( + request_with("Content-Length: 5\r\nContent-Length: 5\r\n", "hello"), + req)); +} + +TEST_CASE("Transfer-Encoding is rejected") { + HttpRequest req; + CHECK_FALSE(parse_from_bytes( + request_with("Transfer-Encoding: chunked\r\nContent-Length: 5\r\n", + "hello"), + req)); +}