Fix Content-Range response when body size equals expected range length #28
Conversation
Signed-off-by: Antonio J. García Lagar <ajgarcia@c17.net>
|
This looks pretty good. It does not modify existing behavior I guess (at least not a behavior which is covered by tests). |
| @@ -86,7 +86,7 @@ private function emitBodyRange(array $range, ResponseInterface $response): void | |||
| $length = $last - $first + 1; | |||
|
|
|||
| if ($body->isSeekable()) { | |||
| $body->seek($first); | |||
| $body->seek($body->getSize() === $length ? 0 : $first); | |||
There was a problem hiding this comment.
Should this be ===, or <=? In other words, what hapepns when the body size is shorter than the requested length?
There was a problem hiding this comment.
In that case, I think that the code should throw an exception but I preferred to make the minimal edition to fix #22 without modifying any other execution path.
We can fix that case in another PR.
There was a problem hiding this comment.
After thinking about this, definitely, we should throw an exception in that case.
When the emitter receives a response object with a Content-Range header, we only have two options here:
- The response producer computes the
Content-Rangeheader and delegates to the emitter the responsibility of fixing the body content length. - The response producer computes the
Content-Rangeheader and fixes the response body content so the received body size equals the length defined in theContent-Rangeheader. (option fixed by this PR).
In the first case, the body size can be greater, equal, or lesser than the length defined in the Content-Range header. If the given body size is lesser than the length defined in the Content-Range header, the response producer failed at computing the Content-Range header and we should throw an exception.
Anyway, as I wrote previously, this PR only fixes option 2 but does not modify any behavior related to option 1, so I think it can be merged safely, and fix the problem with shorter body size in other PR.
@weierophinney What do you think?
There was a problem hiding this comment.
About exceptions: would that leave the door open for DDoS attacks? Having an app crash (on purpose) via generic header manipulation can be problematic.
There was a problem hiding this comment.
You are right: throwing an exception could be a bad idea here. Maybe we can return a 502 response instead because the emitter got an invalid response.
There was a problem hiding this comment.
I think we're trying to solve the problem at the wrong level.
The root of the problem is that the emitter doesn't have the full context to provide the correct range split (this comes on the request only).
If approaching this from a generic PoV (as we currently do), a middleware would be more suitable for parsing the requested ranges and converting the headers/body to provide support for single and multiple ranges.
Trying to make the emitter limit the body but also ignore things if the body was already truncated sounds unnecessary. To me, the emitter should focus on doing a single job: stream the response.
I'd suggest we create a new SapiStreamEmitter that doesn't do anything with Content-Range and deprecate the current one. We should also ask ourselves if the responsibility of parsing ranges and limiting the body belongs to this library.
There was a problem hiding this comment.
I agree 💯 with @lcobucci here, and would gladly accept a PR that:
- Creates a new
SapiStreamEmitterimplementation that doesn't do anything with theContent-Rangeheader. - Deprecates the current
SapiStreamEmitter. - Creates middleware for managing ranges.
Signed-off-by: Antonio J. García Lagar ajgarcia@c17.net
Description
When the response body has already been limited to the requested range, the emitter should ignore the Content-Range header and emit the full response body.
This is an alternative to #23 to fix #22