@mnapoli opened this Pull Request on October 8th 2014 Contributor

See #6156. /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.

@mnapoli commented on October 10th 2014 Contributor

@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.

@diosmosis commented on October 10th 2014 Member

@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).

@mnapoli commented on October 13th 2014 Contributor

@diosmosis I've renamed the class to Router.

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

@mnapoli commented on October 13th 2014 Contributor

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.

@diosmosis commented on October 13th 2014 Member

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.

@mnapoli commented on October 13th 2014 Contributor

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.

@mnapoli commented on October 13th 2014 Contributor

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 Url::getCurrentUrl() \o/

Waiting for the tests now, and then all is good to merge.

@diosmosis commented on October 15th 2014 Member

@mattab The code looks good to me, though the new tests seem to be failing on travis.

@mnapoli commented on October 15th 2014 Contributor

@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.

@mnapoli commented on October 15th 2014 Contributor

Actually there are some nginx problems in this PR -_- so still WIP, do not merge.

This Pull Request was closed on October 20th 2014
Powered by GitHub Issue Mirror