/index.php/.html?... URLs can be interpreted by old browsers using the extension instead of the Content-Type header. This redirection should prevent that.
The part that does the redirection is in the
FrontController (just before the dispatching). If there is a better place for this (to avoid crowding up the front controller) let me know.
I have also added a simple PHPUnit test for this. I ended up using
curl manually because the
Http class doesn't return the HTTP response headers. If I missed how to do that properly with
Http, let me know too.
@mattab @tsteur @diosmosis I've pushed a new version where the URL parsing is done in a separate class (whose name could be better but I had trouble coming up with a better one). There are 2 tests: - unit tests for the class that filters URLs - system test for the whole redirection
What's left is to test the XSS with IE7 (had some troubles with the VM, will try later).
While doing that refactoring, I noticed that
Url::getCurrentUrl() doesn't always return the current URL.
For example, if the URL is
http://localhost:8000/index.php/.html?module=API, then it will return
http://localhost:8000/index.php?module=API (note the missing
/.html, which is what PHP calls PATH_INFO). There are no tests for this method. I've got a local branch with some tests I wrote, but the whole global state thing is getting on my nerves. Tests set values in
$_SERVER and other global variables, and that means I get inconsistent tests results from run to run… Arghhhh. I'll try again later.
Feel free to review this PR in the meantime.
@mnapoli Could name the class Router, then later we can move more FrontController logic to it. I would also suggest moving the url creating code to Url.php (like getCurrentUrlWithPathInfo() or something).
@diosmosis I've renamed the class to
However I haven't created a new
getCurrentUrlWithPathInfo() method, it's actually
getCurrentUrl() that is bugged (doesn't really return the current url), and it needs to be fixed. Creating a new method would be a bit like when PHP added
mysql_real_escape_string() to replace
mysql_escape_string() instead of fixing it :p
I will create a pull request to fix
getCurrentUrl() and then replace the ugly line from the front controller. But since we want to push this fix quickly for the upcoming release I'll leave it as is and refactor it in that new PR (to be merged later).
By the way, I've tested and confirm that the XSS described in #6053 is fixed by this change.
I just have to fix the tests, the redirections are different on Travis I need to understand why.
I saw an explicit removal of PATH_INFO in getCurrentUrl which is why I suggested a new method. Of course w/ the changes in this pull request, you could just remove the code since Piwik should always redirect to the URL w/o the path info.
Yes but the removal of PATH_INFO is in
getCurrentScriptName(), which makes sense there (because we only want the script name). In
getCurrentUrl() however, you want the current URL.
I think we should try to keep this Url class working outside of Piwik's scope too. Sure with this PR PATH_INFO should always be empty on Piwik, but will that always be the case. And what if we put that utility class in a separate project so that it could be reused elsewhere. It's not much more effort and it's much more futureproof.
OK I've fixed getCurrentUrl() and getCurrentUrlWithoutQueryString() in that PR, that's simpler (https://github.com/piwik/piwik/commit/17a9c8f651b0308188e44ecc8640f11e6a5a19c9).
Url::getCurrentScheme() . '://' . $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI']; has been replaced with
Waiting for the tests now, and then all is good to merge.
@mattab The code looks good to me, though the new tests seem to be failing on travis.
@diosmosis good point, master was broken when I created the branch. Master is broken now too, we'll have to wait till master is green and I'll update this PR so that tests pass here too.
Actually there are some nginx problems in this PR -_- so still WIP, do not merge.