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
In BulkTracking output, return the index of each invalid request. #8644
Conversation
… lines of logs that were considered invalid by the Piwik tracker.
I reckon it's okay to merge. Wondering if one could misuse it? Eg can I send a bulk request containing 1000 requests with different siteIds and see how many are in use and how many sites this website has? Also could I try 1000 different tokens at once etc and see if maybe one is valid? I presume one gets the same information as well when sending 1000 individual requests so should not be a problem. Another "downside" would be that we send even more data back when people are using bulk requests via |
This sounds like a possible exploit. We could only send the indices if the request is authenticated... I'll make this change. We should perhaps make it a policy not to return information about tracking from tracker requests, since it is open to the world and is expected to be used a lot (ie, will never have rate limiting).
For bulk tracking I think the token is supplied differently (ie, not for each request), and if the token auth is wrong, tracking still goes ahead (unless there is a auth required config set...).
After limiting the response to only authenticated requests, I think this will be less of an issue. |
… lines of logs that were considered invalid by the Piwik tracker.
… lines of logs that were considered invalid by the Piwik tracker.
Modifying to require authentication is a bit non-trivial, making this a WIP for now. Btw, I noticed that in BulkTracking, authentication is done for every request instead of just once before hand. @tsteur Do you know if this is intentional? See: https://github.com/piwik/piwik/blob/master/plugins/BulkTracking/Tracker/Requests.php#L35 and https://github.com/piwik/piwik/blob/master/plugins/BulkTracking/Tracker/Requests.php#L85. Maybe I am looking at the code wrong. |
I think it is only if https://github.com/piwik/piwik/blob/master/plugins/BulkTracking/Tracker/Requests.php#L35 It shouldn't be really slow as we cache that kinda part during one request so we're not authenticating the same token twice in one request. |
43c7ac1
to
1748ed4
Compare
…ddition to count of invalid (count remains in output for BC).
… lines of logs that were considered invalid by the Piwik tracker.
e11ebfc
to
f5ce32a
Compare
@tsteur Ready for another review. |
return false; | ||
} | ||
|
||
Piwik::postEvent('Request.initAuthenticationObject'); |
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.
Have you tested this case re performance?
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.
Sorry just noticed this is only called once, so shouldn't be a big problem I presume
Looks good to me, I'm just not sure if any user with |
Made some changes. |
Refs matomo-org/matomo#8644, use bulk tracking indices to display the lines of logs that were considered invalid by the Piwik tracker.
@tsteur Merging, let me know if you still have more comments. |
Fixes #8296, in BulkTracking output, return the index of each invalid request.
As title. Ready to merge, but following todo should be done either before or after:
Fixes #8296
@tsteur Can you review when you have time?
Merge Order
This PR has two other PRs in other repos that need to be merged w/ this one. The merge order should probably be: