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

Handle matrix URI parameters to ignore parameters like ";jsessionid=..." #3187

Closed
catchin opened this issue Jun 4, 2012 · 8 comments
Closed
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Milestone

Comments

@catchin
Copy link

catchin commented Jun 4, 2012

How to reproduce:
Log visits from a Java EE webserver which handles the session parameter through [http://www.w3.org/DesignIssues/MatrixURIs.html matrix parameters], i.e. a url like http://piwik.org;jsessionid=A3294FBE42?foo=bar.[[BR]]Then piwik logs the url including the ;jsessionid=A3294FBE42 part although "jsessionid" is a parameter excluded from the query parameters.

Suggestion how to fix it:
PHP's parse_url function is apparently not handling matrix parameters yet, so we have to do it on our own. I don't know where the best position in the code is and where the function would be used elsewhere. For now, I modified core/Tracker/Action.php's excludeQueryParametersFromUrl as in the attached patch.
Probably there are better ways to fix it. If you suggest one to me, I can work out and test other patches.

@catchin
Copy link
Author

catchin commented Jun 4, 2012

Attachment:
piwik-matrix-parameters.patch

@mattab
Copy link
Member

mattab commented Jun 19, 2012

Thanks for the patch. It would be nice to also fix: #3201 if you are able to submit a patch :) Also would be nice to add some tests in: /trunk/tests/core/Tracker/Action.test.php

@catchin
Copy link
Author

catchin commented Jul 4, 2012

Thanks for the hint. I added a fix for #3201 and also added some tests as you suggested.

Unfortunately, I cannot upload my patch, because I get the error "Submission rejected as potential spam (Maximum number of external links per post exceeded)". I now uploaded it here: http://textuploader.com/?p=6&id=F8aVM
Can someone who is allowed please attach the patch to this ticket?

@mattab
Copy link
Member

mattab commented Jul 19, 2012

Thanks for the patch!

Review note: We should check (add test) that when a parameter name is a XSS it is properly escaped in the report data UI (as we now do an additional urldecode() which could be dangerous)

@catchin
Copy link
Author

catchin commented Jul 19, 2012

I'm not so familiar with the urldecode behavior by means of XSS. I adapted the patch to remove the urldecode and replace it with a str_replace just for square brackets. Is this better/safer? http://textuploader.com/?p=6&id=RMDnM

@mattab
Copy link
Member

mattab commented Jul 20, 2012

Thanks for the patch, looks safer indeed.

Do your new tests test for all cases? Also why adding several times the same test at the bottom of the patch?

@catchin
Copy link
Author

catchin commented Jul 20, 2012

Thank you for reviewing!

The tests test for matrix parameters, matrix and normal parameters combined, and squared brackets. The last two tests are different indeed (the second one in addition uses two normal parameters), however with the same outcome (as matrix parameters are converted to normal ones).

@mattab
Copy link
Member

mattab commented Aug 3, 2012

(In [6659]) Fixes #3187 Handle java style matrix URLs
Fixes #3201 GET Parameter with square brackets can now be excluded properly

Thanks catchin for the patches!!
Please consider contributing further patches if you can :)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

No branches or pull requests

2 participants