Skip to content
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

Merged
merged 22 commits into from Oct 20, 2014

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Oct 8, 2014

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.

/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(
Copy link
Member

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?

@mnapoli mnapoli added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Oct 10, 2014
@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 10, 2014

@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
Copy link
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
Copy link
Contributor Author

mnapoli commented Oct 13, 2014

@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
Copy link
Contributor Author

mnapoli commented Oct 13, 2014

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
Copy link
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
Copy link
Contributor Author

mnapoli commented Oct 13, 2014

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
Copy link
Contributor Author

mnapoli commented Oct 13, 2014

OK I've fixed getCurrentUrl() and getCurrentUrlWithoutQueryString() in that PR, that's simpler (17a9c8f). 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.

@mattab mattab modified the milestones: Piwik 2.9.0, Piwik 2.8.0 Oct 13, 2014
@diosmosis
Copy link
Member

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

@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 15, 2014

@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
Copy link
Contributor Author

mnapoli commented Oct 15, 2014

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

@mnapoli mnapoli modified the milestones: Piwik 2.8.1, Piwik 2.9.0 Oct 19, 2014
mnapoli added a commit that referenced this pull request Oct 20, 2014
Fixes #6156 Redirect /index.php/.whatever?... to /index.php?...
@mnapoli mnapoli merged commit 0efb54a into master Oct 20, 2014
@mnapoli mnapoli deleted the bugfix/6156 branch October 20, 2014 04:38
@mattab mattab added the duplicate For issues that already existed in our issue tracker and were reported previously. label Oct 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate For issues that already existed in our issue tracker and were reported previously. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants