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
Fixes #6156 Redirect /index.php/.whatever?... to /index.php?... #6404
Conversation
/index.php/.html?... URLs can be interpreted by old browsers using the extension instead of the Content-Type header. This redirection should prevent that.
'index.php/.html', | ||
'index.php', | ||
), | ||
array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add couple test cases for correctly formed and un-redirected URLs?
@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:
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 For example, if the URL is 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 I will create a pull request to fix |
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 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. |
Fixed getCurrentUrl() and getCurrentUrlWithoutQueryString() + tests
OK I've fixed getCurrentUrl() and getCurrentUrlWithoutQueryString() in that PR, that's simpler (17a9c8f). 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. |
…ache (for testing wrong urls in the tests)
Fixes #6156 Redirect /index.php/.whatever?... to /index.php?...
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 theHttp
class doesn't return the HTTP response headers. If I missed how to do that properly withHttp
, let me know too.