-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Request exceptions #20962
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
Request exceptions #20962
Conversation
ff3a4a5
to
32ec288
Compare
👍 |
2 similar comments
👍 |
👍 |
@@ -1220,7 +1229,12 @@ public function getHost() | |||
// check that it does not contain forbidden characters (see RFC 952 and RFC 2181) | |||
// use preg_replace() instead of preg_match() to prevent DoS attacks with long host names | |||
if ($host && '' !== preg_replace('/(?:^\[)?[a-zA-Z0-9-:\]_]+\.?/', '', $host)) { | |||
throw new \UnexpectedValueException(sprintf('Invalid Host "%s"', $host)); | |||
if (!$this->isHostValid) { |
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.
Why method produce a different outcome depending on whether it was called before?
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.
That's what I tried to explained in the description.
The first time you try to get the host (it happens in the request validation or router listener for instance), you get the exception, which means that you should then not access the request again. But, when handling the exception in the kernel, you might handle another request in development or production to convert it to a proper HTTP response (think API, ...). The problem comes from the fact that the request used to handle this exception is a duplicate of the original request. If we throw an exception again here, which will happen without this ugly code, then exception handling fails and the original exception bubbles. This is what we are trying to avoid here.
👍 |
Thank you @thewilkybarkid. |
This PR was merged into the 3.3-dev branch. Discussion ---------- Request exceptions | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20389, #20615, #20662 | License | MIT | Doc PR | n/a Replaces #20389 and #20662. The idea is to generically manage 400 responses when an exception implements `RequestExceptionInterface`. The "weird" caches on the request for the host and the clients IPs allows to correctly manage exceptions in an exception listener/controller (as we are duplicating the request there, but we don't want to throw an exception there). Commits ------- 32ec288 [HttpFoundation] refactored Request exceptions d876809 Return a 400 response for suspicious operations
As of symfony/symfony#20962, the user will receive a `400 Bad Request`.
This PR was merged into the master branch. Discussion ---------- Update status code for untrusted hosts As of symfony/symfony#20962, the user will receive a `400 Bad Request`. Commits ------- b1887d9 Update status code for untrusted hosts
Replaces #20389 and #20662. The idea is to generically manage 400 responses when an exception implements
RequestExceptionInterface
.The "weird" caches on the request for the host and the clients IPs allows to correctly manage exceptions in an exception listener/controller (as we are duplicating the request there, but we don't want to throw an exception there).