Skip to content

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

Merged
merged 2 commits into from
Dec 17, 2016
Merged

Request exceptions #20962

merged 2 commits into from
Dec 17, 2016

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Dec 16, 2016

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).

@fabpot fabpot force-pushed the request-exceptions branch from ff3a4a5 to 32ec288 Compare December 16, 2016 16:05
@nicolas-grekas
Copy link
Member

👍

2 similar comments
@stof
Copy link
Member

stof commented Dec 16, 2016

👍

@dunglas
Copy link
Member

dunglas commented Dec 16, 2016

👍

@@ -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) {
Copy link
Member

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?

Copy link
Member Author

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.

@xabbuh
Copy link
Member

xabbuh commented Dec 17, 2016

👍

@fabpot
Copy link
Member Author

fabpot commented Dec 17, 2016

Thank you @thewilkybarkid.

@fabpot fabpot merged commit 32ec288 into symfony:master Dec 17, 2016
fabpot added a commit that referenced this pull request Dec 17, 2016
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
@fabpot fabpot deleted the request-exceptions branch January 7, 2017 00:34
thewilkybarkid added a commit to thewilkybarkid/symfony-docs that referenced this pull request Apr 6, 2017
As of symfony/symfony#20962, the user will receive a `400 Bad Request`.
xabbuh added a commit to symfony/symfony-docs that referenced this pull request Apr 8, 2017
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
@fabpot fabpot mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants