Skip to content

_format parameter handling enabled on default, potentially causing 500 errors #8966

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.

Sign up for GitHub

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

Jump to bottom
Closed
fieg opened this issue Sep 9, 2013 · 4 comments
Closed

_format parameter handling enabled on default, potentially causing 500 errors #8966

fieg opened this issue Sep 9, 2013 · 4 comments
Labels
HttpFoundation

Comments

@fieg
Copy link
Contributor

fieg commented Sep 9, 2013

I think the handling of the _format get parameter is faulty. The handling of the parameter seems to be enabled on default, causing 500 errors when humans (or bots) append the ?_format=json to any url of a symfony application.

As I see it, the handling of the _format parameter should only be handled as I as a developer specify it.

To reproduce:

When you run the following in a standard symfony-standard 2.3 checkout

/app_dev.php/hello/fieg?_format=json

You'll receive a 500 error page, this this error:

[2013-09-09 11:17:42] request.CRITICAL: Uncaught PHP Exception InvalidArgumentException: "Unable to find template "FiegFiegBundle:Default:index.json.twig"." at /vagrant/vendor/symfony/symfony/src/Symfony/Bridge/Twig/TwigEngine.php line 133 {"exception":"[object] (InvalidArgumentException: Unable to find template \"FiegFiegBundle:Default:index.json.twig\". at /vagrant/vendor/symfony/symfony/src/Symfony/Bridge/Twig/TwigEngine.php:133, Twig_Error_Loader: Unable to find template \"FiegFiegBundle:Default:index.json.twig\". at /vagrant/vendor/symfony/symfony/src/Symfony/Bundle/TwigBundle/Loader/FilesystemLoader.php:104, InvalidArgumentException: Unable to find template \"FiegFiegBundle:Default:index.json.twig\" : \"Unable to find file \"@FiegFiegBundle/Resources/views/Default/index.json.twig\".\". at /vagrant/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Templating/Loader/TemplateLocator.php:81, InvalidArgumentException: Unable to find file \"@FiegFiegBundle/Resources/views/Default/index.json.twig\". at /vagrant/app/bootstrap.php.cache:2292)"} []

This is the case for every route in your application!

I asked some people on IRC (#symfony channel) about this and they agreed that this must be a bug.
@stof
Copy link
Member

stof commented Sep 9, 2013

I think the Request should use only the attribute to get the format instead of checking the query string and the posted values: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Request.php#L1287

@pvandommelen pvandommelen mentioned this issue Sep 9, 2013
Closed
@pvandommelen
Copy link
Contributor

@stof This is my idea as well.

@fabpot
Copy link
Member

fabpot commented Sep 9, 2013

That's a tough one. I totally agree with the PR and limiting the _format handling to only attributes. That said, that might break some existing applications (I'm pretty sure that someone somewhere created an app relying on this behavior).

@hubgit
Copy link

hubgit commented Jun 26, 2014

It's also potentially problematic that the _format parameter changes the Content-Type header even when the content is not changed.

Here's a trivial example that forces HTML to be served as application/json.

Perhaps Symfony should return a 406 error if there's no template for the given format, rather than serving a different template with the wrong Content-Type header?

@fabpot fabpot removed the hasPR label Feb 12, 2015
@fabpot fabpot closed this as completed Oct 2, 2015
fabpot added a commit that referenced this issue Oct 2, 2015
@fabpot
feature #8967 [HttpFoundation] Request->getRequestFormat should only …
8fdb412 
…rely on the request attributes (pvandommelen)

This PR was squashed before being merged into the 3.0-dev branch (closes #8967).

Discussion
----------

[HttpFoundation] Request->getRequestFormat should only rely on the request attributes

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | possibly
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #8966
| License       | MIT
| Doc PR        |

Added test case and fix for #8966. Is this functionality relied on somewhere?

Commits
-------

7115c1e [HttpFoundation] Request->getRequestFormat should only rely on the request attributes
@Tobion Tobion mentioned this issue Oct 2, 2015
Merged
fabpot added a commit that referenced this issue Oct 5, 2015
@fabpot
feature #16076 [HttpFoundation] change precedence of parameters in Re…
88e2d70 
…quest::get (Tobion)

This PR was merged into the 3.0-dev branch.

Discussion
----------

[HttpFoundation] change precedence of parameters in Request::get

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Allowing the request attributes to be overwritten via GET parameters is risky and made #8966 even worse.
It is even more risky because it skips the requirements checks as configured in routing. So people that set requirements for routing placeholders like `\d+` or `html|json` can be sure it is validated when using the routing variables. But if developers use `$request->get()` to retrieve them, anybody from outside can set any value for those.

Commits
-------

e8d6764 [HttpFoundation] change precedence of parameters in Request::get
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
HttpFoundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@hubgit @fabpot @stof @fieg @pvandommelen