Removing default behaviour relying on `?url=`

[greut]

trafficstars

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:

• http://www.nateabele.com/about?url=1
• http://www.totsy.com/pages/contact?url=register

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]

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]

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

[gwoo]

could we do

if ($uri = $this->env('REQUEST_URI') && empty($_GET['url'])) {

[nateabele]

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 ;-)

[mariuswilms]

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.

[nateabele]

Resolved in bd82155402a44dfaf47c09a10270fea3e8577015.