diff --git a/mcpp.toml b/mcpp.toml index ec01e93..8ea45b7 100644 --- a/mcpp.toml +++ b/mcpp.toml @@ -1,7 +1,7 @@ [package] namespace = "mcpplibs" name = "tinyhttps" -version = "0.2.9" +version = "0.2.10" description = "Minimal C++23 HTTP/HTTPS client with SSE streaming support" license = "Apache-2.0" repo = "https://github.com/mcpplibs/tinyhttps" diff --git a/src/http.cppm b/src/http.cppm index 67d700e..8220e16 100644 --- a/src/http.cppm +++ b/src/http.cppm @@ -242,6 +242,37 @@ static int parse_hex(std::string_view s) { return result; } +// `Content-Length`, rejected rather than salvaged. +// +// Both readers parsed this by walking the characters and keeping the digits, so +// `12abc` was 12, `abc` was 0 — indistinguishable from a genuine `0` — and a +// value past the width of the accumulator wrapped in silence. A length is the +// number of bytes the reader will then trust, so a wrong one is not a cosmetic +// error: too small leaves the next response's bytes in the stream, and too +// large waits for bytes that are not coming. +// +// Shaped like `parse_chunk_size_line` above, and exported for the same reason: +// it is the half of the body framing that can be examined without a server. +export std::optional +parse_content_length(std::string_view value) { + // A field value may carry optional whitespace on either side (RFC 9110). + while (!value.empty() && (value.front() == ' ' || value.front() == '\t')) + value.remove_prefix(1); + while (!value.empty() && (value.back() == ' ' || value.back() == '\t')) + value.remove_suffix(1); + if (value.empty()) return std::nullopt; + + std::uint64_t parsed {}; + auto [end, error] = std::from_chars( + value.data(), value.data() + value.size(), parsed, 10); + if (error != std::errc{} || end != value.data() + value.size() + || parsed > static_cast( + std::numeric_limits::max())) { + return std::nullopt; + } + return static_cast(parsed); +} + export std::optional parse_chunk_size_line(std::string_view line) { if (line.empty()) return std::nullopt; @@ -256,6 +287,25 @@ parse_chunk_size_line(std::string_view line) { return static_cast(value); } +// A streaming request that fails is answered with an error document, not an +// event stream, so SseParser finds no event boundary in it and yields nothing. +// The bytes are still worth keeping: without them a caller can report the +// status line but never the reason. Bounded, so a server answering 5xx with an +// endless body cannot grow the buffer without limit. +export inline constexpr std::size_t stream_error_body_limit = 1024 * 1024; + +// Appends as much of `data` as `limit` still allows. Returns false once the +// buffer is full, so a caller can stop copying without tracking sizes itself. +export bool append_within_limit(std::string& buffer, std::string_view data, + std::size_t limit) { + if (buffer.size() >= limit) return false; + const std::size_t room = limit - buffer.size(); + // Not std::min: defines a `min` macro on Windows. + const std::size_t take = data.size() < room ? data.size() : room; + buffer.append(data.substr(0, take)); + return take == data.size(); +} + // Case-insensitive string comparison static bool iequals(std::string_view a, std::string_view b) { if (a.size() != b.size()) return false; @@ -459,12 +509,11 @@ private: chunked = true; } if (iequals(key, "Content-Length")) { - contentLength = 0; - for (char c : valStr) { - if (c >= '0' && c <= '9') { - contentLength = contentLength * 10 + (c - '0'); - } - } + // Rejected rather than salvaged; parse_content_length says + // why. A malformed value leaves this at -1, which is the + // same state as an absent header and is a framing this + // reader already handles. + contentLength = parse_content_length(valStr).value_or(-1); } if (iequals(key, "Connection") && iequals(valStr, "close")) { connectionClose = true; @@ -704,6 +753,7 @@ public: // Read headers bool chunked = false; bool connectionClose = false; + std::int64_t contentLength = -1; while (true) { std::string headerLine = read_line(*sock, config_.readTimeoutMs); @@ -726,14 +776,27 @@ public: if (iequals(key, "Connection") && iequals(valStr, "close")) { connectionClose = true; } + // send() reads this and send_stream() did not, which is the + // same asymmetry this change exists to remove. See the body + // loop below for what its absence cost. + if (iequals(key, "Content-Length")) { + contentLength = parse_content_length(valStr).value_or(-1); + } } } // Stream body incrementally, feeding chunks to SseParser SseParser parser; bool stopped = false; + // send() fills `body` on every path including failures; without this + // send_stream would be the one entry point that drops it. Capturing is + // additive — events are still parsed and dispatched exactly as before. + const bool captureBody = !response.ok(); auto dispatch = [&](std::string_view data) -> bool { + if (captureBody) { + append_within_limit(response.body, data, stream_error_body_limit); + } auto events = parser.feed(data); for (const auto& ev : events) { if (!callback(ev)) { @@ -756,7 +819,22 @@ public: sizeLine.pop_back(); } - int chunkSize = parse_hex(sizeLine); + // A CHUNK HEADER THAT DOES NOT PARSE IS NOT A TERMINAL CHUNK. + // + // `parse_hex` returned what it had accumulated when it met a + // character it did not recognise, and zero for an empty line — + // and `read_line` returns an empty line on a timeout or a + // closed connection. So a stream that was cut short read as a + // stream that ended cleanly, and this loop reported success. + // #9 established `parse_chunk_size_line` for exactly this and + // it reached only `download_to_file`. + auto parsedChunkSize = parse_chunk_size_line(sizeLine); + if (!parsedChunkSize) { + response.statusText = "Invalid chunk size: " + sizeLine; + connectionClose = true; + break; + } + int chunkSize = static_cast(*parsedChunkSize); if (chunkSize == 0) { // Terminal chunk — read trailing \r\n read_line(*sock, config_.readTimeoutMs); @@ -775,8 +853,44 @@ public: break; } } + } else if (contentLength >= 0) { + // A DECLARED LENGTH IS READ AND THE READER THEN STOPS. + // + // This branch did not exist: a response that was not chunked was + // read until the connection closed, whatever its headers said. On + // the library's own defaults — `keepAlive = true`, so the request + // carries `Connection: keep-alive` — the server does not close, + // and the loop below ran until `readTimeoutMs` expired. + // + // Measured against httpbin's `/status/418`, which answers with a + // Content-Length and keeps the connection: + // + // keepAlive = false status=418 body=135 elapsed 1370 ms + // keepAlive = true status=418 body=135 elapsed 9379 ms (readTimeoutMs = 8000) + // + // The error body arrived either way, and on the defaults it arrived + // a full read timeout late — sixty seconds, as the defaults stand. + // `send()` has had this branch throughout, which is why the same + // request through it returns at once. + std::int64_t remaining = contentLength; + char buf[4096]; + while (!stopped && remaining > 0) { + if (!sock->wait_readable(config_.readTimeoutMs)) { + break; + } + const auto want = static_cast( + remaining < static_cast(sizeof buf) + ? remaining : static_cast(sizeof buf)); + int ret = sock->read(buf, want); + if (ret <= 0) break; + remaining -= ret; + if (!dispatch(std::string_view(buf, static_cast(ret)))) { + break; + } + } } else { - // Not chunked — read until connection closes + // Neither chunked nor a declared length: the end of the body is the + // end of the connection, so the connection must not be reused. connectionClose = true; char buf[4096]; while (!stopped) { diff --git a/tests/test_download.cpp b/tests/test_download.cpp index c862fbe..54c1655 100644 --- a/tests/test_download.cpp +++ b/tests/test_download.cpp @@ -20,6 +20,75 @@ TEST(ChunkedProtocol, AcceptsValidSizeAndTerminalChunk) { EXPECT_EQ(*https::parse_chunk_size_line("0"), 0); } +// `Content-Length` decides how many bytes a reader will trust, so a wrong +// answer is not cosmetic: too small leaves the next response's bytes in the +// stream and too large waits for bytes that are not coming. +// +// Both readers used to keep the digits and discard everything else. Measured, +// by compiling that parser on its own: +// +// "135" -> 135 +// "0" -> 0 +// "abc" -> 0 <- a refusal read as a real zero +// "12abc" -> 12 <- stops twelve bytes in +// "" -> 0 +// "-1" -> 1 <- the sign is discarded +// "99999999999999999999" -> 7766279631452241919 <- wraps, in silence +// +// The last two are the ones no amount of care at the call site could recover +// from, because the value it receives is a plausible number. +TEST(ContentLength, AcceptsAWellFormedValue) { + ASSERT_TRUE(https::parse_content_length("135").has_value()); + EXPECT_EQ(*https::parse_content_length("135"), 135); + // A field value may carry optional whitespace on either side. + ASSERT_TRUE(https::parse_content_length(" 135\t").has_value()); + EXPECT_EQ(*https::parse_content_length(" 135\t"), 135); +} + +// The one a salvaging parser cannot express. `abc` used to yield 0, which is +// indistinguishable from a server that genuinely declared an empty body — and +// the two call for opposite behaviour. +TEST(ContentLength, AZeroIsDistinguishableFromARefusal) { + ASSERT_TRUE(https::parse_content_length("0").has_value()); + EXPECT_EQ(*https::parse_content_length("0"), 0); + EXPECT_FALSE(https::parse_content_length("abc").has_value()); +} + +TEST(ContentLength, RejectsEmptyTrailingGarbageAndOverflow) { + EXPECT_FALSE(https::parse_content_length("").has_value()); + EXPECT_FALSE(https::parse_content_length(" ").has_value()); + // `12abc` used to be 12: the reader would then stop twelve bytes in and + // leave the rest of the body to be read as the next response. + EXPECT_FALSE(https::parse_content_length("12abc").has_value()); + EXPECT_FALSE(https::parse_content_length("-1").has_value()); + EXPECT_FALSE(https::parse_content_length("+1").has_value()); + // Past the width of the accumulator, which used to wrap in silence. + EXPECT_FALSE(https::parse_content_length("99999999999999999999").has_value()); +} + +TEST(StreamErrorBody, KeepsEverythingWhileUnderLimit) { + std::string buffer; + EXPECT_TRUE(https::append_within_limit(buffer, "abc", 8)); + EXPECT_TRUE(https::append_within_limit(buffer, "de", 8)); + EXPECT_EQ(buffer, "abcde"); +} + +TEST(StreamErrorBody, TruncatesTheChunkThatCrossesTheLimit) { + std::string buffer = "abc"; + EXPECT_FALSE(https::append_within_limit(buffer, "defgh", 5)); + EXPECT_EQ(buffer, "abcde"); +} + +TEST(StreamErrorBody, RefusesFurtherDataOnceFull) { + std::string buffer = "abcde"; + EXPECT_FALSE(https::append_within_limit(buffer, "f", 5)); + EXPECT_EQ(buffer, "abcde"); + // A zero limit must not append anything, not even an empty append. + std::string empty; + EXPECT_FALSE(https::append_within_limit(empty, "a", 0)); + EXPECT_TRUE(empty.empty()); +} + TEST(DownloadResultContract, CarriesTransferAndResponseMetadata) { https::DownloadToFileResult result; result.bytesWritten = 42; @@ -36,6 +105,86 @@ TEST(DownloadResultContract, CarriesTransferAndResponseMetadata) { EXPECT_FALSE(result.lastModified.empty()); } +// Test that a failed streaming request carries its error body, against a real +// HTTPS endpoint. httpbin's /status/418 answers non-2xx with a body, which is +// exactly the shape SseParser cannot turn into events. + +class StreamErrorBodyLiveTest : public ::testing::Test { +protected: + void SetUp() override { https::Socket::platform_init(); } +}; + +// ON THE LIBRARY'S OWN DEFAULTS, AND THE READ TIMEOUT IS PART OF THE +// OBSERVATION RATHER THAN A SAFETY NET. +// +// The first form of this test set `keepAlive = false`, "so the server closes +// and the read loop ends". That comment was the defect: `send_stream` had no +// branch for a declared `Content-Length`, so a response that was not chunked +// was read until the connection closed — and on the defaults the server does +// not close. The body arrived, one full `readTimeoutMs` late. +// +// keepAlive = false status=418 body=135 elapsed 1370 ms +// keepAlive = true status=418 body=135 elapsed 9379 ms (timeout 8000) +// +// So the configuration under test is the default one, and the elapsed time is +// asserted. A test that closed the connection to make the loop end would be +// examining the one arrangement in which the defect does not appear. +TEST_F(StreamErrorBodyLiveTest, FailedStreamKeepsTheErrorBody) { + https::HttpClientConfig cfg; + cfg.connectTimeoutMs = 15000; + cfg.readTimeoutMs = 30000; + // keepAlive is left at its default, which is true. + https::HttpClient client(cfg); + + https::HttpRequest req; + req.method = https::Method::GET; + req.url = "https://httpbin.org/status/418"; + + int events = 0; + const auto started = std::chrono::steady_clock::now(); + auto res = client.send_stream(req, [&](const https::SseEvent&) { + ++events; + return true; + }); + const auto elapsed = std::chrono::duration_cast( + std::chrono::steady_clock::now() - started).count(); + + EXPECT_EQ(res.statusCode, 418); + EXPECT_EQ(events, 0) << "an error document is not an event stream"; + EXPECT_FALSE(res.body.empty()) << "error body was dropped"; + EXPECT_NE(res.body.find("teapot"), std::string::npos); + // Generous against a slow runner and still an order of magnitude below the + // read timeout, which is what the defect consumed. + EXPECT_LT(elapsed, 15000) + << "the body arrived after " << elapsed + << " ms; a declared Content-Length was not honoured and the reader " + "waited for a close that keep-alive was never going to bring"; +} + +// The success path, on the framing an event stream actually uses. A 2xx is not +// captured, so `body` stays empty and the bytes reach the parser — and it must +// still return promptly, since the chunked branch is the one this change did +// not restructure. +TEST_F(StreamErrorBodyLiveTest, AChunkedSuccessIsUnchangedAndPrompt) { + https::HttpClientConfig cfg; + cfg.connectTimeoutMs = 15000; + cfg.readTimeoutMs = 30000; + https::HttpClient client(cfg); + + https::HttpRequest req; + req.method = https::Method::GET; + req.url = "https://httpbin.org/stream/3"; + + const auto started = std::chrono::steady_clock::now(); + auto res = client.send_stream(req, [](const https::SseEvent&) { return true; }); + const auto elapsed = std::chrono::duration_cast( + std::chrono::steady_clock::now() - started).count(); + + EXPECT_EQ(res.statusCode, 200); + EXPECT_TRUE(res.body.empty()) << "a 2xx body is not captured; it is the caller's stream"; + EXPECT_LT(elapsed, 15000) << "the chunked reader did not terminate promptly"; +} + // Test download_to_file against a real HTTPS endpoint. // Uses httpbin.org which returns known-size responses.