diff --git a/lib/src/HttpRequestImpl.cc b/lib/src/HttpRequestImpl.cc index a11e4ea299..fb62d1485f 100644 --- a/lib/src/HttpRequestImpl.cc +++ b/lib/src/HttpRequestImpl.cc @@ -368,17 +368,27 @@ void HttpRequestImpl::appendToBuffer(trantor::MsgBuffer *output) const content.append("--"); } } + // The body may be stored in either content_ or cacheFilePtr_ when the + // request body exceeds clientMaxMemoryBodySize_. The two storages are + // mutually exclusive — see appendToBody()/reserveBodySize(). + std::string_view cachedBody; + if (cacheFilePtr_) + { + cachedBody = cacheFilePtr_->getStringView(); + } assert(!(!content.empty() && !content_.empty())); + assert(!(!content.empty() && !cachedBody.empty())); + assert(!(!content_.empty() && !cachedBody.empty())); if (!passThrough_) { - if (!content.empty() || !content_.empty()) + if (!content.empty() || !content_.empty() || !cachedBody.empty()) { char buf[64]; auto len = snprintf( buf, sizeof(buf), contentLengthFormatString(), - content.length() + content_.length()); + content.length() + content_.length() + cachedBody.length()); output->append(buf, len); if (contentTypeString_.empty()) { @@ -426,6 +436,8 @@ void HttpRequestImpl::appendToBuffer(trantor::MsgBuffer *output) const output->append(content); if (!content_.empty()) output->append(content_); + if (!cachedBody.empty()) + output->append(cachedBody.data(), cachedBody.length()); } void HttpRequestImpl::addHeader(const char *start, diff --git a/lib/src/HttpRequestImpl.h b/lib/src/HttpRequestImpl.h index 2d956df799..1b397b7f12 100644 --- a/lib/src/HttpRequestImpl.h +++ b/lib/src/HttpRequestImpl.h @@ -37,6 +37,9 @@ #include #include +// Forward declaration so we can befriend a global-namespace test helper. +class HttpRequestImplCacheFileTestAccess; + namespace drogon { enum class StreamDecompressStatus @@ -59,6 +62,7 @@ class HttpRequestImpl : public HttpRequest { public: friend class HttpRequestParser; + friend class ::HttpRequestImplCacheFileTestAccess; explicit HttpRequestImpl(trantor::EventLoop *loop) : creationDate_(trantor::Date::now()), loop_(loop) diff --git a/lib/tests/CMakeLists.txt b/lib/tests/CMakeLists.txt index c8249b41ab..0cd084fdb6 100644 --- a/lib/tests/CMakeLists.txt +++ b/lib/tests/CMakeLists.txt @@ -44,6 +44,7 @@ else() set(UNITTEST_SOURCES ${UNITTEST_SOURCES} ../src/HttpFileImpl.cc unittests/HttpFileTest.cc unittests/HttpMethodTest.cc + unittests/HttpRequestForwardCacheBodyTest.cc unittests/WebsocketResponseTest.cc) endif() diff --git a/lib/tests/unittests/HttpRequestForwardCacheBodyTest.cc b/lib/tests/unittests/HttpRequestForwardCacheBodyTest.cc new file mode 100644 index 0000000000..c0b94c85f3 --- /dev/null +++ b/lib/tests/unittests/HttpRequestForwardCacheBodyTest.cc @@ -0,0 +1,283 @@ +/** + * + * HttpRequestForwardCacheBodyTest.cc + * + * Regression test for https://github.com/drogonframework/drogon/issues/2482 + * + * When an inbound HTTP request body exceeds clientMaxMemoryBodySize_, the + * body is spilled to a CacheFile (HttpRequestImpl::cacheFilePtr_) instead of + * HttpRequestImpl::content_. HttpRequestImpl::appendToBuffer() is used by + * HttpClientImpl when forwarding the request to a downstream server, and it + * must include the cached body in the serialized buffer (and in the + * Content-Length header). + */ +#include +#include +#include +#include "../../lib/src/HttpRequestImpl.h" +#include "../../lib/src/CacheFile.h" + +#include +#include +#include +#include +#include +#ifdef _WIN32 +#include +#define DROGON_TEST_GETPID _getpid +#else +#include +#define DROGON_TEST_GETPID getpid +#endif + +using namespace drogon; + +// Test-only access shim that befriends HttpRequestImpl in the header. It +// lets the test inject a CacheFile-backed body, simulating the state the +// request parser leaves the request in when the body has been spilled to +// disk. +class HttpRequestImplCacheFileTestAccess +{ + public: + static void installCacheFile(HttpRequestImpl &req, + std::unique_ptr cf) + { + req.cacheFilePtr_ = std::move(cf); + } +}; + +namespace +{ +std::string makeTmpPath(const std::string &tag) +{ + // tmpnam-style path under the OS temp dir, but using fixed prefixes so we + // can run repeatedly without name collisions. +#ifdef _WIN32 + const char *base = std::getenv("TEMP"); + if (!base || !*base) + base = std::getenv("TMP"); + if (!base || !*base) + base = "C:/Windows/Temp"; + std::string out = base; + out += "/drogon-issue2482-"; +#else + std::string out = "/tmp/drogon-issue2482-"; +#endif + out += tag; + out += "-"; + out += std::to_string(DROGON_TEST_GETPID()); + out += "-"; + out += std::to_string(reinterpret_cast(&out)); + return out; +} + +std::unique_ptr makeCacheFileWith(const std::string &payload, + const std::string &tag) +{ + auto path = makeTmpPath(tag); + auto cf = std::make_unique(path, /*autoDelete=*/true); + cf->append(payload); + return cf; +} + +// Find a header value in a serialized HTTP request buffer, matching +// case-insensitively for the field name and trimming the value. +std::string findHeader(const std::string &raw, const std::string &name) +{ + auto headerEnd = raw.find("\r\n\r\n"); + if (headerEnd == std::string::npos) + return {}; + std::string headers = raw.substr(0, headerEnd + 2); + std::string lname = name; + for (auto &c : lname) + c = static_cast(::tolower(static_cast(c))); + + size_t pos = 0; + while (pos < headers.size()) + { + auto eol = headers.find("\r\n", pos); + if (eol == std::string::npos) + break; + std::string line = headers.substr(pos, eol - pos); + pos = eol + 2; + auto colon = line.find(':'); + if (colon == std::string::npos) + continue; + std::string field = line.substr(0, colon); + for (auto &c : field) + c = static_cast(::tolower(static_cast(c))); + if (field == lname) + { + std::string value = line.substr(colon + 1); + size_t s = 0; + while (s < value.size() && + ::isspace(static_cast(value[s]))) + ++s; + size_t e = value.size(); + while (e > s && ::isspace(static_cast(value[e - 1]))) + --e; + return value.substr(s, e - s); + } + } + return {}; +} + +std::string extractBody(const std::string &raw) +{ + auto headerEnd = raw.find("\r\n\r\n"); + if (headerEnd == std::string::npos) + return {}; + return raw.substr(headerEnd + 4); +} +} // namespace + +DROGON_TEST(HttpRequestForwardCacheBody_BodyEmpty) +{ + // Sanity check: with neither content_ nor a cache file, a non-body method + // must not emit a content-length header. + HttpRequestImpl req(nullptr); + req.setMethod(Get); + req.setVersion(Version::kHttp11); + req.setPath("/foo"); + + trantor::MsgBuffer buf; + req.appendToBuffer(&buf); + std::string raw(buf.peek(), buf.readableBytes()); + CHECK(findHeader(raw, "content-length").empty()); + CHECK(extractBody(raw).empty()); +} + +DROGON_TEST(HttpRequestForwardCacheBody_InMemoryStillForwarded) +{ + // Regression guard for the existing (small) body path. + const std::string body = "small-in-memory-body"; + HttpRequestImpl req(nullptr); + req.setMethod(Post); + req.setVersion(Version::kHttp11); + req.setPath("/echo"); + req.setBody(body); + + trantor::MsgBuffer buf; + req.appendToBuffer(&buf); + std::string raw(buf.peek(), buf.readableBytes()); + + CHECK(findHeader(raw, "content-length") == std::to_string(body.size())); + CHECK(extractBody(raw) == body); +} + +DROGON_TEST(HttpRequestForwardCacheBody_CachedBodyForwarded) +{ + // The bug from #2482: a body sitting in cacheFilePtr_ must be serialized + // by appendToBuffer() and accounted for in content-length. + std::string body; + body.reserve(200 * 1024); + for (int i = 0; i < 200 * 1024; ++i) + body.push_back(static_cast('A' + (i % 26))); + + HttpRequestImpl req(nullptr); + req.setMethod(Post); + req.setVersion(Version::kHttp11); + req.setPath("/forward"); + HttpRequestImplCacheFileTestAccess::installCacheFile( + req, makeCacheFileWith(body, "cached")); + + trantor::MsgBuffer buf; + req.appendToBuffer(&buf); + std::string raw(buf.peek(), buf.readableBytes()); + + CHECK(findHeader(raw, "content-length") == std::to_string(body.size())); + CHECK(extractBody(raw) == body); +} + +DROGON_TEST(HttpRequestForwardCacheBody_CachedBodyExactBoundary) +{ + // Exactly the default clientMaxMemoryBodySize boundary. Make sure we + // correctly emit the body even when its size is on the round number. + const size_t kSize = 64 * 1024; + std::string body(kSize, 'x'); + HttpRequestImpl req(nullptr); + req.setMethod(Put); + req.setVersion(Version::kHttp11); + req.setPath("/upload"); + HttpRequestImplCacheFileTestAccess::installCacheFile( + req, makeCacheFileWith(body, "boundary")); + + trantor::MsgBuffer buf; + req.appendToBuffer(&buf); + std::string raw(buf.peek(), buf.readableBytes()); + + CHECK(findHeader(raw, "content-length") == std::to_string(kSize)); + CHECK(extractBody(raw).size() == kSize); + CHECK(extractBody(raw) == body); +} + +DROGON_TEST(HttpRequestForwardCacheBody_CachedBodyBinarySafe) +{ + // The cached body must survive serialization byte-for-byte, including + // NULs and high bytes. + std::string body; + body.resize(70 * 1024); + for (size_t i = 0; i < body.size(); ++i) + body[i] = static_cast(i & 0xff); + + HttpRequestImpl req(nullptr); + req.setMethod(Post); + req.setVersion(Version::kHttp11); + req.setPath("/binary"); + HttpRequestImplCacheFileTestAccess::installCacheFile( + req, makeCacheFileWith(body, "binary")); + + trantor::MsgBuffer buf; + req.appendToBuffer(&buf); + std::string raw(buf.peek(), buf.readableBytes()); + + CHECK(findHeader(raw, "content-length") == std::to_string(body.size())); + CHECK(extractBody(raw) == body); +} + +DROGON_TEST(HttpRequestForwardCacheBody_PassThroughKeepsBody) +{ + // passThrough_ skips parameter/content-length emission, but the body + // bytes from a cache file must still appear in the serialized buffer. + const std::string body(80 * 1024, 'p'); + HttpRequestImpl req(nullptr); + req.setMethod(Post); + req.setVersion(Version::kHttp11); + req.setPath("/pass"); + req.setPassThrough(true); + HttpRequestImplCacheFileTestAccess::installCacheFile( + req, makeCacheFileWith(body, "passthrough")); + // passThrough mode expects callers to preserve the original + // content-length header. + req.addHeader("content-length", std::to_string(body.size())); + + trantor::MsgBuffer buf; + req.appendToBuffer(&buf); + std::string raw(buf.peek(), buf.readableBytes()); + + CHECK(extractBody(raw) == body); +} + +DROGON_TEST(HttpRequestForwardCacheBody_CachedBodyWithCustomHeaders) +{ + // Headers and cookies must continue to be serialized in addition to the + // cached body. + const std::string body(100 * 1024, 'h'); + HttpRequestImpl req(nullptr); + req.setMethod(Post); + req.setVersion(Version::kHttp11); + req.setPath("/with-headers"); + req.addHeader("X-Trace-Id", "abc-123"); + req.addCookie("session", "deadbeef"); + HttpRequestImplCacheFileTestAccess::installCacheFile( + req, makeCacheFileWith(body, "headers")); + + trantor::MsgBuffer buf; + req.appendToBuffer(&buf); + std::string raw(buf.peek(), buf.readableBytes()); + + CHECK(findHeader(raw, "content-length") == std::to_string(body.size())); + CHECK(findHeader(raw, "x-trace-id") == "abc-123"); + CHECK(raw.find("cookie: session=deadbeef") != std::string::npos); + CHECK(extractBody(raw) == body); +}