Removing default behaviour relying on `?url=` #340

Open
greut opened this Issue Feb 19, 2012 · 9 comments

4 participants

@greut

We've recently hit the wall because with a third party application sending us this kind of url ?tduid=123&url=/foo/bar. It made me thinking and I'm now seeing this ?url= thing as harmful. REQUEST_URI can be enough in most cases (the nginx documentation already uses it but not the apache one btw).

Some exemples of breaking websites:

The change:

diff --git a/action/Request.php b/action/Request.php
index 6510ae8..95eec27 100644
--- a/action/Request.php
+++ b/action/Request.php
@@ -568,9 +568,6 @@ class Request extends \lithium\net\http\Request {
                if (isset($this->_config['url'])) {
                        return rtrim($this->_config['url'], '/');
                }
-               if (!empty($_GET['url']) ) {
-                       return rtrim($_GET['url'], '/');
-               }
                if ($uri = $this->env('REQUEST_URI')) {
                        return str_replace($this->env('base'), '/', parse_url($u
                }

The backward compatibility hack

If you still need that, you can tweak your index.php Request creation. It's much difficult to do it the other way round when the $_GET['url'] is part of Request::_url().

<?php

require dirname(__DIR__) . '/config/bootstrap.php';

echo lithium\action\Dispatcher::run(new lithium\action\Request(array(
    '_config' => array(
        'url' => !empty($_GET['url']) ? $_GET['url'] : '/'
    )
)));

I know I'm asking for a big backward incompatible change here since I assume many websites are using the default .htaccess or suggested Apache configuration. Please consider it for a cleaner lithium :-)

@nateabele
Union of RAD member

What's wrong with just looking at 'REQUEST_URI' first and $_GET['url'] second? At least that way it's not a total BC break. In any case, you can do what you need to much more simply with nowhere near as much code. Just do:

new lithium\action\Request(array('url' => $_SERVER['REQUEST_URI']))
@greut

@nateabele thanks for the much better snippets!

Changing the if's order in Request::_url() looks like removing it because 'REQUEST_URI' will always be true; or I get something wrong. I haven't tested that solution though.

@greut

For the record, otherwise the query string is taken with it:

<?php
require dirname(__DIR__) . '/config/bootstrap.php';
echo lithium\action\Dispatcher::run(new lithium\action\Request(array(           
   'url' => parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH)                     
))); 
@nateabele
Union of RAD member

Ah, good point. Yeah, I guess we'll have to keep playing with it.

@gwoo
Union of RAD member
gwoo commented Feb 29, 2012 edited

could we do

if ($uri = $this->env('REQUEST_URI') && empty($_GET['url'])) {
@gwoo gwoo closed this Feb 29, 2012
@gwoo gwoo reopened this Feb 29, 2012
@nateabele
Union of RAD member

Now that I think about it, I don't think we could check $_GET['url'] at all, since the whole point is to eliminate the use of it because it conflicts with other things.

@greut

Once you remove it, the only way to have the actual behaviour is this. As badly backward incompatible as it can be. But it's for good imho.

<?php
require dirname(__DIR__) . '/config/bootstrap.php';
echo lithium\action\Dispatcher::run(new lithium\action\Request(array(           
   'url' => !empty($_GET['url']) ? $_GET['url'] : '/'
)));
@greut

There is one thing we had to fix after that switch is urlencode/urldecode. URI were urlencoded more than they should have… I cannot point any places where it could come from, just that you know it didn't go without any pains ;-)

@davidpersson davidpersson modified the milestone: future Sep 30, 2014
@greut greut removed this from the future milestone Sep 30, 2014
@davidpersson davidpersson added this to the 2.0 milestone May 26, 2016
@davidpersson davidpersson added structure and removed rfc labels May 26, 2016
@davidpersson
Union of RAD member
davidpersson commented May 26, 2016 edited

The only way I see is removing checking for $_GET['url'] in Request and suggest the workaround @greut detailed for those who still need it:

<?php
require dirname(__DIR__) . '/config/bootstrap.php';
echo lithium\action\Dispatcher::run(new lithium\action\Request(array(           
   'url' => !empty($_GET['url']) ? $_GET['url'] : '/'
)));

The .htaccess file don't need to be changed as they already do not use url. IIS's web.config needs change. As well as manual docs for lighty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment