fix: strip transport-owned headers to prevent duplicate Content-Length (closes #579) - #602
Merged
Merged
Conversation
…-Length (closes #579) ResponseConverter::extractHeaders() now strips Content-Length, Accept-Ranges and Transfer-Encoding centrally so Workerman's transport layer is the sole authority on message framing. Previously array_merge_recursive in Response::withHeaders() combined app-supplied framing headers with the values encode()/__toString() compute, emitting Content-Length twice — a protocol violation on file downloads and a response-desync primitive when the two values disagreed. Single-valued headers are also flattened from list<string|null> to string (except Set-Cookie) to prevent array_merge_recursive from ever producing arrays of conflicting values. Headers with all-null/empty values are dropped. Content-Range and 206 status on ranged responses are preserved. Existing strcasecmp guards in buildHeaderString methods kept as belt-and-braces. Added ContentLengthDesyncTest covering all four paths (file, small body, large body, streamed) plus ranged 206, Set-Cookie, Content-Range, empty body, 304, and the TRANSPORT_HEADERS list.
- Add testRangedFileDownloadThroughEncodeEmitsCorrectBodyRange: drives the real Workerman Http::encode() and asserts on the wire bytes — status 206, exactly one Content-Length: 100, Content-Range: bytes 0-99/5000, and the body is the requested 100-byte range from the fixture (closes AC 2). - Add testFileDownloadThroughEncodeEmitsSingleFramingHeaders: end-to-end wire test for plain file download — exactly one Content-Length and one Accept-Ranges, plus the full fixture as body. - Add testHeadRequestEmitsNoBodyAndSingleContentLength: constructs a HEAD request via Symfony's prepare(), asserts no body and a single Content-Length: 0 (closes AC 7). - Strengthen testStreamedResponseEmitsNoContentLength: set an app-supplied Content-Length so the test guards against a leaked app CL on the streamed path (closes AC 4 caveat). - Remove the two hand-simulated ranged/file tests that duplicated encode() logic; the end-to-end tests supersede them.
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
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.
Description
Closes #579
Changes
ResponseConverter::extractHeaders()now strips transport-owned headers (Content-Length,Accept-Ranges,Transfer-Encoding) centrally so Workerman's transport layer (Http::encode()/Response::__toString()) is the sole authority on message framinglist<string|null>tostring(exceptSet-Cookie, which legitimately needs multiple values) to preventarray_merge_recursiveinResponse::withHeaders()from ever producing arrays of conflicting valuesContent-Rangeand the206status on ranged responses are preserved (Workerman sets them viaheader()which overwrites)strcasecmpguards inDefaultResponseStrategy::buildHeaderString()andStreamedResponseStrategy::buildHeaderString()kept as belt-and-braces with explanatory commentsResponseConverterStrategyInterfaceand both strategies updated toarray<string, string|list<string|null>>getHeader()now returnsstringfor single-valued headers)ContentLengthDesyncTestwith 9 tests covering all four body paths (file, small body, large body, streamed), ranged 206, multiple Set-Cookie, Content-Range preservation, empty body, 304, and theTRANSPORT_HEADERSconstanttestConvertDropsAllNullHeaderValuesinResponseConverterTestRoot cause
Workerman's
Response::withHeaders()usesarray_merge_recursive, which on a string key present in both arrays produces an array of both values rather than a replacement.Http::encode()adds its ownContent-Length/Accept-RangesviawithHeaders()for file responses, andResponse::__toString()appends its ownContent-Lengthunconditionally for the small-body path — so an application-suppliedContent-Lengthwas emitted twice. When the two values disagreed, the conflict is a response-desync primitive (RFC 9110 §8.6) that can poison caches and leak responses across keep-alive connections.Changelog
Security fix entry added under
[Unreleased]→### Security.Code Review
composer lintpasses (PHPStan level 8, php-cs-fixer, rector)composer testpasses (1566 tests)