Improved an error message related to controllers #27499
Conversation
|
Great for newcomers! |
|
@ScullWM allowing to return strings was discussed in the past (specially because "some frameworks" allow to do that). But it was decided to keep requiring That's why I think we should keep the current behaviour: if people learn about Request/Response soon, they will understand Symfony easier. |
|
@javiereguiluz Is your master branch a bit outdated ? Because I made a patch recently that display a "better" trace that highlight the controller. And I can not see it on your screenshot. Anyway, 👍 with this PR. |
|
@lyrixx the code change and the screenshot were done in a Symfony 4.1 app and then I copied the code to the master branch 🙈 |
|
You can always create your own listener that transforms the string into a response. The SensioFrameworkExtraBundle does this for arrays with the |
| @@ -157,7 +157,7 @@ private function handleRaw(Request $request, int $type = self::MASTER_REQUEST) | |||
| if ($event->hasResponse()) { | |||
| $response = $event->getResponse(); | |||
| } else { | |||
| $msg = sprintf('The controller must return a response (%s given).', $this->varToString($response)); | |||
| $msg = sprintf('The controller must return a Symfony\Component\HttpFoundation\Response object but it returned: %s.', is_string($response) ? sprintf('"%s" string', $response) : $this->varToString($response)); | |||
There was a problem hiding this comment.
I'd suggest moving the is_string check inside the varToString method
| @@ -269,6 +269,10 @@ private function varToString($var): string | |||
| return sprintf('Resource(%s)', get_resource_type($var)); | |||
| } | |||
|
|
|||
| if (is_string($var)) { | |||
| return sprintf('"%s" string', $var); | |||
There was a problem hiding this comment.
Here, $var can be very long, I would strip the string in such cases, maybe using the first 200 or so chars.
There was a problem hiding this comment.
Using a generated string in an exception message makes me nervous (it can contain end-user submitted values).
|
I've updated this feature to trim long strings. Before:
After:
About this other comment from Fabien (#27499 (comment)) I'm not sure how to solve it. Thanks! |
|
Thanks to Twig's automatic escaping, I think we're good here. I've tested with some of the examples given by OWASP to prevent XSS (https://www.owasp.org/index.php/Testing_for_Reflected_Cross_site_scripting_(OTG-INPVAL-001)) and it looks like it's working:
Can anyone spot other "attack vectors" in this code? Thanks |
| @@ -157,7 +157,7 @@ private function handleRaw(Request $request, int $type = self::MASTER_REQUEST) | |||
| if ($event->hasResponse()) { | |||
| $response = $event->getResponse(); | |||
| } else { | |||
| $msg = sprintf('The controller must return a response (%s given).', $this->varToString($response)); | |||
| $msg = sprintf('The controller must return a Symfony\Component\HttpFoundation\Response object but it returned %s.', $this->varToString($response)); | |||
There was a problem hiding this comment.
Maybe add double quotes around the ...\Request
|
Thank you @javiereguiluz. |
…ereguiluz) This PR was squashed before being merged into the 4.2-dev branch (closes #27499). Discussion ---------- Improved an error message related to controllers | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This proposal is irrelevant for experienced users but it may be useful for newcomers. After having delivered several introductory Symfony training, I can say that when someone adds `return "some string...";` in their controller, the error message is confusing:  Maybe we can reword it a bit? (I'm open for suggestions to improve the error message)  Commits ------- 7510c3a Improved an error message related to controllers
This proposal is irrelevant for experienced users but it may be useful for newcomers. After having delivered several introductory Symfony training, I can say that when someone adds
return "some string...";in their controller, the error message is confusing:Maybe we can reword it a bit? (I'm open for suggestions to improve the error message)