Repository navigation
Ignore a Range header whose value is not well-formed - #1672
17krishna8 wants to merge 1 commit into
Conversation
reiern70
left a comment
There was a problem hiding this comment.
Thanks, the fix itself looks right to me: every malformed form in the tests fails on master with ArrayIndexOutOfBoundsException or NumberFormatException, a well-formed range is still picked up, and the existing range tests in PackageResourceReferenceTest pass with the change.
Two things before it goes in:
The description. A malformed Range header fails only the request that carried it; the server and other clients are not affected, so this is hardening rather than a denial-of-service vector. (If you ever do believe something is a vulnerability, please report it privately as SECURITY.md describes rather than in a PR; for hardening like this a public PR is exactly right.) Also, RFC 9110 §14.2 makes ignoring mandatory only for a range unit the server doesn't understand; for an invalid ranges-specifier it says a server MAY ignore or reject it. Ignoring is fine, the description and the commit message should just say "may".
The tests. ByteArrayResource doesn't advertise byte ranges, so the new tests check the request-cycle meta data on a resource that ignores ranges anyway, and the 200 they assert would be there with or without the header. PackageResourceReferenceTest#testContentRange already goes through a range-capable resource with makeRangeRequest(). Adding the malformed cases there checks what a client actually gets back, and replaces the new class:
// malformed ranges are ignored
assertEquals("resource_var_style_en.txt", makeRangeRequest("bytes=5"));
assertEquals("resource_var_style_en.txt", makeRangeRequest("bytes=abc"));
assertEquals("resource_var_style_en.txt", makeRangeRequest("bytes=1-x"));
assertEquals("resource_var_style_en.txt",
makeRangeRequest("bytes=99999999999999999999-"));
assertEquals("resource_var_style_en.txt", makeRangeRequest("bytes 5-10"));| */ | ||
| private ByteArrayResource newRecordingResource() | ||
| { | ||
| return new ByteArrayResource("application/octet-stream", DATA.getBytes(StandardCharsets.US_ASCII)) |
There was a problem hiding this comment.
106 columns; the project wraps at 100. Moot if the cases move into PackageResourceReferenceTest.
A Range header whose value does not hold up when parsed made a resource request fail with NumberFormatException or ArrayIndexOutOfBoundsException instead of serving the resource: 'bytes=5' has no dash to split on, 'bytes=1-x' and numbers beyond the long range fail the number parsing, and a value without a '=' separator fed the whole header text to the parser. RFC 9110, section 14.2, lets a server ignore a Range header with an invalid ranges-specifier, so such a request now falls back to serving the whole resource: the content range meta data is only set for a header of the form bytes=<start>-<end> where both parts are valid long values.
c9e810b to
c227c08
Compare
|
Thanks for the review and for checking every malformed form on master. I have reworded the description and the commit message: this is hardening, and per RFC 9110 section 14.2 a server may ignore a The test class is gone; the malformed cases now live in |
A
Rangeheader whose value does not hold up when parsed made a resource request fail withNumberFormatExceptionorArrayIndexOutOfBoundsExceptioninstead of serving the resource:bytes=5has no dash to split on,bytes=1-xand numbers beyond the long range fail the number parsing, and a value without a=separator fed the whole header text to the parser.RFC 9110, section 14.2, lets a server ignore a
Rangeheader with an invalid ranges-specifier, so such a request now falls back to serving the whole resource: the content range meta data is only set for a header of the formbytes=<start>-<end>where both parts are valid long values.This is hardening: only the request that carried the malformed header is affected, the server and other clients are not.
The malformed cases are covered in
PackageResourceReferenceTest#testContentRange, next to the existing range cases, so they go through a range-capable resource.