mcpplibs
diff --git a/‎src/http.cppm‎
Lines changed: 96 additions & 8 deletions b/‎src/http.cppm‎
Lines changed: 96 additions & 8 deletions
diff --git a/‎tests/test_download.cpp‎
Lines changed: 95 additions & 1 deletion b/‎tests/test_download.cpp‎
Lines changed: 95 additions & 1 deletion
@@ -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<std::int64_t>
+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::uint64_t>(
+            std::numeric_limits<std::int64_t>::max())) {
+        return std::nullopt;
+    }
+    return static_cast<std::int64_t>(parsed);
+}
+
 export std::optional<std::int64_t>
 parse_chunk_size_line(std::string_view line) {
     if (line.empty()) return std::nullopt;
@@ -478,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;
@@ -723,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);
@@ -745,6 +776,12 @@ 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);
+                }
             }
         }
 
@@ -782,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<int>(*parsedChunkSize);
                 if (chunkSize == 0) {
                     // Terminal chunk — read trailing \r\n
                     read_line(*sock, config_.readTimeoutMs);
@@ -801,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<std::size_t>(
+                    remaining < static_cast<std::int64_t>(sizeof buf)
+                        ? remaining : static_cast<std::int64_t>(sizeof buf));
+                int ret = sock->read(buf, want);
+                if (ret <= 0) break;
+                remaining -= ret;
+                if (!dispatch(std::string_view(buf, static_cast<std::size_t>(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) {
 
@@ -20,6 +20,52 @@ 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));
@@ -68,27 +114,75 @@ class StreamErrorBodyLiveTest : public ::testing::Test {
     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;
-    cfg.keepAlive = false;  // so the server closes and the read loop ends
+    // 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::milliseconds>(
+        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::milliseconds>(
+        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.