Skip to content
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

Query string dot is replaced with underscore by trailing slash URL redirection #29664

Closed
guilliamxavier opened this issue Dec 21, 2018 · 7 comments

Comments

@guilliamxavier
Copy link
Contributor

guilliamxavier commented Dec 21, 2018

Symfony version(s) affected: 4.1.9, 4.2.1

Description

Having defined a route without trailing slash in the path,
accessing the URL with a trailing slash automatically redirects to the URL without the trailing slash, but also replaces dots with underscores in the query string parameters names (if any).

How to reproduce

TL;DR:

Given my localhost app defines a route for method GET for path /foos (not /foos/)
When a client requests (GET) the URL http://localhost/foos/?nested.prop=bar
Then I expect the client is redirected to http://localhost/foos?nested.prop=bar
    but actually the client is redirected to http://localhost/foos?nested_prop=bar

More details:

Given the following controller (note: no trailing slash after "foos"):

src/Controller/FooController.php
<?php

namespace App\Controller;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Annotation\Route;

class FooController
{
    /**
     * @Route("/foos", methods={"GET"})
     */
    public function getFoos(Request $request)
    {
        /* Disclaimer: this is quick-and-dirty code. */

        $html = '<!DOCTYPE html><html><head><meta charset="UTF-8"></head><body>';
        $html .= '<table border="1">';
        $html .= '<tr><th>expression</th><th>value</th></tr>';
        foreach ([
            '$request->getUri()',
            '$request->getSchemeAndHttpHost().$request->getRequestUri()',
            '$request->getQueryString()',
            '$request->server->get("QUERY_STRING")',
        ] as $expression) {
            $value = eval("return $expression;");
            [$expression, $value] = array_map('htmlspecialchars', [$expression, $value]);
            $html .= "<tr><td><code>$expression</code></td><td><code>$value</code></td></tr>";
        }
        $html .= '</table>';
        $html .= '</body></html>';

        return new Response($html);
    }
}

Then

  • http://localhost/foos?nested.prop=bar displays:

    expression value
    $request->getUri() http://localhost/foos?nested_prop=bar
    $request->getSchemeAndHttpHost().$request->getRequestUri() http://localhost/foos?nested.prop=bar
    $request->getQueryString() nested_prop=bar
    $request->server->get("QUERY_STRING") nested.prop=bar

    (i.e. nested.prop is transformed to nested_prop but there are ways to get the original query string).

  • http://localhost/foos/?nested.prop=bar (note: trailing slash after "foos") redirects (301) to http://localhost/foos?nested_prop=bar with trailing slash removed but also dot replaced with underscore, which thus displays:

    expression value
    $request->getUri() http://localhost/foos?nested_prop=bar
    $request->getSchemeAndHttpHost().$request->getRequestUri() http://localhost/foos?nested_prop=bar
    $request->getQueryString() nested_prop=bar
    $request->server->get("QUERY_STRING") nested_prop=bar

    i.e. we only have nested_prop, the original query string has been lost.

Additional context

With 3.4.20,

  • http://localhost/foos?nested.prop=bar displays:

    expression value
    $request->getUri() http://localhost/foos?nested.prop=bar
    $request->getSchemeAndHttpHost().$request->getRequestUri() http://localhost/foos?nested.prop=bar
    $request->getQueryString() nested.prop=bar
    $request->server->get("QUERY_STRING") nested.prop=bar

    (i.e. nested.prop was preserved).

  • http://localhost/foos/?nested.prop=bar (note: trailing slash after "foos") gives a 404 error No route found for "GET /foos/" (no automatic redirect).

In all cases, $request->query->get("nested.prop") is null (not defined) (with 4.1, 4.2 and even 3.4), and we have to use $request->query->get("nested_prop") (or parse the original query string) to get "bar".

The real project uses API Platform, which auto-generates resource routes (without trailing slash) and parses the original query string to get nested filters (with dots). The calls with trailing slash are from automated REST clients.

@wgorczyca
Copy link

wgorczyca commented Dec 28, 2018

It's because we decided to use parse_str in Request::normalizeQueryString() instead of custom logic #26220, variables in PHP can't have dots and spaces, those are converted to underscores.

@guilliamxavier
Copy link
Contributor Author

@wgorczyca Thanks for sharing. API Platform's ReadListener uses a utility class RequestParser:

But maybe RedirectController::urlRedirectAction() (called from RedirectableUrlMatcher::redirect()) should use $request->server->get('QUERY_STRING') directly rather than $request->getQueryString()?

@guilliamxavier
Copy link
Contributor Author

(For information, for the moment I have done something like https://stackoverflow.com/questions/52705684/how-to-disable-trailing-slash-redirect-in-symfony-4-1/52786096#52786096 because a 404 error (like in 3.4) is still better than a redirect with corrupted query parameters...)

@javiereguiluz
Copy link
Member

Note that the automatic conversion of dots into underscores is a PHP behavior ... which could finally be changed/removed in PHP 8: https://externals.io/message/106213

@stof
Copy link
Member

stof commented Jul 17, 2019

Having $request->getQueryString() being consistent with the content of $request->query is a good thing IMO. I would not change it.

@guilliamxavier
Copy link
Contributor Author

@javiereguiluz But in this precise case the PHP behavior only bites because Symfony does trailing-slash redirection by using RedirectableUrlMatcher::redirect()
and that calls RedirectController::urlRedirectAction()
and that uses Request::getQueryString()
(which ultimately passes the server QUERY_STRING through PHP's parse_str())...

I found related #28806

@stof I wouldn't expect a change to be needed at that level

@drupol
Copy link
Contributor

drupol commented Jun 11, 2020

Sorry to dig up such old topics but I encountered the issue while doing the ecphp/cas-bundle and working with API Platform.

I have found a pretty nice solution for us and I made a bundle out of it: https://github.com/loophp/unaltered-psr-http-message-bridge-bundle

nicolas-grekas added a commit that referenced this issue Jun 18, 2020
…ecting (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle] preserve dots in query-string when redirecting

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #29664
| License       | MIT
| Doc PR        | -

Commits
-------

fcc0e2c [FrameworkBundle] preserve dots in query-string when redirecting
nicolas-grekas added a commit that referenced this issue Jun 18, 2020
…ecting (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[FrameworkBundle] preserve dots in query-string when redirecting

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #29664
| License       | MIT
| Doc PR        | -

Requires #37270 to pass.

Commits
-------

3d0d59d [FrameworkBundle] preserve dots in query-string when redirecting
fabpot added a commit that referenced this issue Jun 24, 2020
…oes the same as `parse_str()` but preserves dots in variable names (nicolas-grekas)

This PR was merged into the 5.2-dev branch.

Discussion
----------

[HttpFoundation] add `HeaderUtils::parseQuery()`: it does the same as `parse_str()` but preserves dots in variable names

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Inspired by symfony/psr-http-message-bridge#80
/cc @drupol

Related to #9009, #29664, #26220 but also api-platform/core#509 and https://www.drupal.org/project/drupal/issues/2984272
/cc @dunglas @alexpott

Commits
-------

dd81e32 [HttpFoundation] add `HeaderUtils::parseQuery()`: it does the same as `parse_str()` but preserves dots in variable names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants