@diosmosis opened this Pull Request on August 25th 2015 Member

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:

@tsteur commented on August 27th 2015 Owner

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 piwik.js as done eg in content tracking etc but it should be rather a minor problem and can be ignored I reckon. Especially as requests should be usually valid.

@diosmosis commented on August 27th 2015 Member

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?

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

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.

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

Another "downside" would be that we send even more data back when people are using bulk requests via piwik.js as done eg in content tracking etc but it should be rather a minor problem and can be ignored I reckon.

After limiting the response to only authenticated requests, I think this will be less of an issue.

@diosmosis commented on August 27th 2015 Member

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.

@tsteur commented on August 28th 2015 Owner

I think it is only if bulk_requests_require_authentication is enabled and it is disabled by default.

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.

@diosmosis commented on September 1st 2015 Member

@tsteur Ready for another review.

@diosmosis commented on September 4th 2015 Member

@mattab @tsteur ping-ing for review

@tsteur commented on September 7th 2015 Owner

Looks good to me, I'm just not sure if any user with view permission should be allowed to see the result indices. Maybe we could limit it to super users or so.

@diosmosis commented on September 7th 2015 Member

Made some changes.

@diosmosis commented on September 8th 2015 Member

@tsteur Merging, let me know if you still have more comments.

This Pull Request was closed on September 8th 2015
Powered by GitHub Issue Mirror