fix: _range() log spam, Content-Length clamping, multipart terminator off-by-one#11
Draft
troglodyne-bot wants to merge 1 commit into
Conversation
…nator length
Three bugs in _range(), none covered by open PRs:
1. INFO("GET 416") was called unconditionally before the range validation
check, so every valid range request logged "GET 416 <path>". Besides
filling logs with false 416 entries, any fail2ban rule on 416 responses
would ban clients making legitimate ranged requests. Moved the INFO call
inside the failure branch.
2. When a Range header specifies an end byte past the file size (e.g.
bytes=0-9999 on a 5-byte file), RFC 7233 §2.1 says to clamp to the last
byte. The Content-Length calculation used the unclamped $range->[1],
producing a declared length far larger than the actual bytes sent. Clients
would stall waiting for data that never arrives. Added an explicit clamp
before the Content-Length sum. The streaming write already used
List::Util::min($sz, ...) so actual output was correct — only the header
was wrong. Content-Range in the response is also now correct.
3. The multipart terminator Content-Length calculation used "\--" (with a
literal backslash, length N+1) while the streaming write already uses
"--" (N). The off-by-one made declared Content-Length one byte larger
than the actual response body for every multipart range response.
Tests: t/01-range-response.t — 19 tests covering 416 validation, single and
multipart range bodies, suffix ranges, overlong range clamping (Content-Length,
body, Content-Range header), and multipart Content-Length accuracy.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
Three bugs in
_range(), none addressed by open PRs #4–#10.Why
INFO "GET 416" log spam —
$self->INFO("GET 416 $fullpath")fired unconditionally before the range validation check, meaning every valid byte-range request logged a false 416. Fail2ban rules watching for 416s would incorrectly ban clients making legitimate ranged GETs.Content-Length not clamped to file size — When a client requests
bytes=0-9999on a 5-byte file, RFC 7233 §2.1 says to silently cap the range end. The streaming write already usedList::Util::min($sz, ...)so the actual bytes sent were correct, butContent-Lengthwas computed from the unclamped$range->[1]. Clients received a header promising 10,000 bytes and then only 5 — hanging indefinitely waiting for data that would never come. Content-Range in the response was also wrong (claimedbytes 0-9999/5instead ofbytes 0-4/5).Multipart terminator Content-Length off by one — The Content-Length calculation used
length("\n--$CHUNK_SEP\--\n")where\-in a double-quoted Perl string passes through as a literal backslash. The streaming write (fixed separately by PR Fix multipart boundary, HTTP::Body dep, NYTProf overhead, error fallback #4) uses"\n--$CHUNK_SEP--\n"without the backslash. The mismatch made declared Content-Length one byte larger than the actual body for every multipart range response.How
INFO("GET 416")inside the failure branch$range->[1] = $sz - 1 if $range->[1] >= $szimmediately after the//=default, before the Content-Length sum and Content-Range header — one clamp fixes all three header valueslength("\n--$CHUNK_SEP\--\n")→length("\n--$CHUNK_SEP--\n")in the Content-Length accumulationTesting
t/01-range-response.t— 19 tests:bytes=N-with no end): defaults to last byte