fix: reject multipart ranges before validation - #661
Conversation
I think it would make sense to keep the specific error message body. Can you bring it back? |
|
Thanks. I’ll keep the multipart-specific body so the response stays behavior-preserving. That will require a small |
ca0e6ea to
4448c39
Compare
|
Added the small |
There was a problem hiding this comment.
@shblue21 do you see a reason not to support the behavior of flattening to single-part GET instead? Even if opt-in due to DOS risk (in which case your approach seems fine for rejection, though I have a small nit on internal error modeling).
IETF seems to prefer that,the RFC states:
A server MAY ignore the Range header field. However, origin servers
and intermediate caches ought to support byte ranges when possible,
since Range supports efficient recovery from partially failed
transfers and partial retrieval of large representations. A server
MUST ignore a Range header field received with a request method other
than GET.
Range requests are an OPTIONAL feature of HTTP, designed so that recipients not implementing this feature (or not supporting it for the target resource) can respond as if it is a normal GET request without impacting interoperability.
| if parsed.ranges.len() > 1 { | ||
| // ServeDir/ServeFile do not support multipart responses, so reject | ||
| // multi-range requests before validate() runs overlap checks. | ||
| return Err(RangeUnsatisfiableError::OverlappingRanges); |
There was a problem hiding this comment.
This doesn't seem like quite the right error, this refers to overlapping ranges within a multipart request. Meanwhile we don't support them at all.
Can we have a local error type instead that more clearly denotes what's happening, if we preserve this behavior?
|
@shblue21 are you still interested in pursuing this PR? |
|
@jlizen Sorry, I missed this. I’ll take another look at the comments now. |
bcf90c6 to
136b535
Compare
|
@jlizen For this PR, I’d prefer to keep returning 416 and only skip the unnecessary validation. We can discuss ignoring the |
Motivation
ServeDir/ServeFiledo not support multipart range responses, but they validate multi-rangeRangeheaders before later rejecting them with416 Range Not Satisfiable.This means requests that are guaranteed to be rejected still pay the cost of
validate(file_size), including its pairwise overlap checks.Solution
Reject multiple ranges before calling
validate(file_size):MultipleRangesNotSupportederror when more than one range is presentvalidate(file_size)for single-range requests416response body for valid multi-range requestsThis also adds regression coverage for the multipart-specific response body.
Note: syntactically parsed multi-range requests that would previously fail semantic validation, such as overlapping or reversed ranges, are now rejected as unsupported multiple ranges before validation. The status and
Content-Rangeremain unchanged, but the416body becomes the multipart-specific message.Refs: #660