fix(http): reject incomplete chunked transfers - #9
Merged
Conversation
Sunrisepeak
marked this pull request as ready for review
July 11, 2026 18:16
Sunrisepeak
added a commit
that referenced
this pull request
Aug 29, 2026
…framing defects the review found (#14) * fix(http): keep the error body of a failed streaming request send() fills HttpResponse::body on every path including failures; send_stream() was the one entry point that dropped it. A non-2xx answer to a streaming request is an error document, not an event stream: SseParser finds no event boundary in it, emits nothing, and the bytes stay in its private buffer. Callers were left with a status line and no reason. Capture the body when the status is not 2xx. Events are still parsed and dispatched exactly as before, and nothing is copied on a 2xx stream, so the success path is byte-identical. The copy is bounded by stream_error_body_limit (1 MiB) so a server answering 5xx with an endless body cannot grow the buffer without limit. Truncation lives in an exported append_within_limit, in the same spirit as parse_chunk_size_line, with three unit tests for under, across and past the limit; a live test against httpbin's /status/418 covers the wiring. * Honour a declared Content-Length in send_stream, and reject a chunk size rather than salvaging it Review of the change this branch already carries. The defect it reports is real and the fix is placed correctly --- `dispatch` is the single funnel for every body byte on both framing paths, and `captureBody` is decided after the headers are read, where the status is final. Measured against master, one program, one source file: master status=418 events=0 body.size()=0 this status=418 events=0 body.size()=135 What follows is what that fix could not do on its own. --- 1. A DECLARED LENGTH, WHICH IS WHY THE TEST HAD TO CLOSE THE CONNECTION ---- `send_stream` had no branch for `Content-Length`: a response that was not chunked was read until the connection closed, whatever its headers said. On this library's own defaults --- `keepAlive = true`, so the request carries `Connection: keep-alive` --- the server does not close, and the read loop ran until `readTimeoutMs` expired. Measured against httpbin's `/status/418`: keepAlive = false status=418 body=135 elapsed 1370 ms keepAlive = true status=418 body=135 elapsed 9379 ms (timeout 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 the same asymmetry between the two entry points that this branch exists to remove. The live test set `keepAlive = false`, "so the server closes and the read loop ends". That comment was the defect, and the test was examining the one arrangement in which it does not appear. It now runs on the defaults and asserts the elapsed time. after: keepAlive = true status=418 body=135 elapsed 1192 ms --- 2. A CHUNK SIZE THAT DOES NOT PARSE IS NOT A TERMINAL CHUNK --------------- `parse_hex` returns what it accumulated when it meets a character it does 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 `download_to_file` alone; `send` and `send_stream` were left on the old one. --- 3. Content-Length WAS PARSED BY KEEPING THE DIGITS ------------------------ Measured, by compiling that parser on its own: "135" -> 135 "abc" -> 0 <- a refusal read as a real zero "12abc" -> 12 <- stops twelve bytes in "-1" -> 1 <- the sign is discarded "99999999999999999999" -> 7766279631452241919 <- wraps, in silence The last two are the ones no care at the call site could recover from, because what it receives is a plausible number. `parse_content_length` is exported and shaped like `parse_chunk_size_line`, for the reason #9 gave: it is the half of the body framing that can be examined without a server. Both readers use it. --- criteria ----------------------------------------------------------------- Six unit tests over the two pure parsers, and two live ones: the failed stream now runs on the DEFAULT configuration with the elapsed time asserted, and a chunked 2xx is asserted to leave `body` empty and to return promptly --- the success path is the one this change restructured around, so it is observed rather than assumed. 17 tests from 6 suites pass, plus 3 in test_resolver. --------- Co-authored-by: Cloud_Yun <yunfeng66645@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
注册 for free
to join this conversation on GitHub.
Already have an account?
登录 to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
mcpp test— 2 test binaries passed; 12 tests passedgit diff --checkRefs openxlings/xlings#356