-
-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RequestBodyParserMiddleware does not reject requests that exceed post_max_size #263
Conversation
// request body of unknown size exceeding limit during buffering | ||
if ($error instanceof OverflowException) { | ||
return new Response(413, array('Content-Type' => 'text/plain'), 'Request body exceeds allowed limit'); | ||
return $stack($request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I think these changes make perfect sense 👍
One question that remains is, when should the next handler be called? It's my understanding that this middleware handler should still wait for the complete request to end
before proceeding. This basically means that this middleware will wait for the complete request body anyway and that we simply ignore its contents.
Also, I wonder what should happen with Content-Length
(remove?) and getBody()->getSize()
(set to 0
?) etc. We should probably ensure to only pass updated values to the next consumer.
Can you update this to include some tests and documentation for this? 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question that remains is, when should the next handler be called? It's my understanding that this middleware handler should still wait for the complete request to end before proceeding. This basically means that this middleware will wait for the complete request body anyway and that we simply ignore its contents.
Adding tests next so we can ensure the right behavior.
Can you update this to include some tests and documentation for this? 👍
It's on the list, but wanted code out first so we can discuss before spending time on documentation.
@clue added test that ensures the request isn't handled until the full request is in, also implemented that 🎉 . Updating documentation next |
$body->on('close', function () use ($stack, $request, $resolve) { | ||
$resolve($stack($request)); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, but this is currently missing a cancellation handler. Maybe use Stream\first()
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I'll look into it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how relevant this is here, what do you think about reactphp/promise-stream#7?
@clue using |
} | ||
|
||
public function testExcessiveSizeReturnsError413() | ||
public function testExcessiveSizeBodyIsDisgardedTheRequestIsPassedDownToTheNextMiddleware() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disgarded -> DiscardedAnd
README.md
Outdated
rejected with a `413` (Request Entity Too Large) error message without calling | ||
the next middleware handlers. | ||
accepted but their request body will not be added to the request. Browsers consider | ||
a request failed when a response is sent before the entire request has gone out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is valid remark, but it is actually unrelated to this PR. I will remove this line for now and will make sure file a new ticket to keep track of this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #273 for follow-up discussion about this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this! The changes LGTM, I've squashed some of your changes and updated the documentation slightly, now let's get this in 👍
@jsor ping! |
Implements / closes #254