Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Improved an error message related to controllers#27499
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ScullWM commentedJun 5, 2018
Great for newcomers! |
javiereguiluz commentedJun 5, 2018
@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. |
lyrixx commentedJun 5, 2018
@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. |
javiereguiluz commentedJun 5, 2018
@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 🙈 |
linaori commentedJun 5, 2018
You can always create your own listener that transforms the string into a response. The SensioFrameworkExtraBundle does this for arrays with the |
| $response =$event->getResponse(); | ||
| }else { | ||
| $msg =sprintf('The controller must return a response (%s given).',$this->varToString($response)); | ||
| $msg =sprintf('The controller must return aSymfony\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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd suggest moving theis_string check inside thevarToString method
| } | ||
| if (is_string($var)) { | ||
| returnsprintf('"%s" string',$var); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Using a generated string in an exception message makes me nervous (it can contain end-user submitted values).
javiereguiluz commentedJun 26, 2018
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! |
javiereguiluz commentedJun 27, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
| $response =$event->getResponse(); | ||
| }else { | ||
| $msg =sprintf('The controller must return aresponse (%s given).',$this->varToString($response)); | ||
| $msg =sprintf('The controller must return aSymfony\Component\HttpFoundation\Response object but it returned %s.',$this->varToString($response)); |
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.
Maybe add double quotes around the ...\Request
fabpot commentedJun 27, 2018
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)