_parse_request_range: accept range unit in any case per RFC 7233#3660
Open
HrachShah wants to merge 1 commit into
Open
_parse_request_range: accept range unit in any case per RFC 7233#3660HrachShah wants to merge 1 commit into
HrachShah wants to merge 1 commit into
Conversation
The pre-fix code compared the range unit with `unit != "bytes"`, which rejected any Range header that didn't spell the unit in exactly lowercase. RFC 7233 section 2.1 is explicit: "all rules derived from token are to be compared case-insensitively, like range-unit and acceptable-ranges." A conforming client that sends `Range: BYTES=1-2` was being silently rejected, and the request was treated as having no Range header at all. Switching the comparison to `unit.lower() != "bytes"` makes the parser match the RFC, while still rejecting unknown units (which is the only behaviour the rest of the function relies on downstream of this check). Added a doctest and a small ParseRequestRangeTest class covering the lowercase baseline, the uppercase case, the mixed-case form, the suffix and -N range shapes, and the negative case (an unknown unit like "items" still returns None).
Member
|
LGTM. I think the test failure just needs a rebase onto #3678 |
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
2 participants
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
The pre-fix code compared the range unit with a case-sensitive equality check (
if unit != "bytes"), so an incomingRange: BYTES=1-2(or any non-lowercase variant) was treated as an unknown unit and silently returnedNone.RFC 7233 §2.1 explicitly says:
The fix lowercases the unit before the comparison. The actual
start/endnumeric values are still parsed from the same string and are unaffected.Repro
Tests
test_lowercase_unitbaseline (still passes).test_uppercase_unit_accepted(new):BYTES=1-2→(1, 3). Fails on master.test_mixed_case_unit_accepted(new):Bytes=1-2andbYtEs=1-2both pass. Fails on master.test_uppercase_unit_with_suffix_range(new):BYTES=6-→(6, None),BYTES=-6→(-6, None). Fails on master.test_non_bytes_unit_still_rejected(new):items=1-2→None(passes on master too — sanity check).All 5 new tests pass with the fix; the three uppercase/mixed-case tests fail on master. The full
tornado.test.httputil_testsuite (62 tests) still passes.Docstring
Added a doctest line demonstrating the new behaviour:
Closes the gap that a perfectly-spec-compliant
Range: BYTES=1-2header was being ignored, even though Tornado would happily serve a 206 Partial Content response to a client that had the wrong case in the request.