Navigation Menu

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

Jump to bottom

Prevent caching of tracker in proxies #12730

Merged
merged 6 commits into from Jun 18, 2018
Merged

Prevent caching of tracker in proxies #12730

merged 6 commits into from Jun 18, 2018

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Apr 16, 2018 •
edited

When Piwik is running behind a reverse proxy (e.g. for load-balancing), hits to piwik.php may be cached in the proxy.

This has two problems:

  • Image tracking is unreliable. If the proxy is configured to cache resources for 10 minutes, only one hit is counted for during this period. This is not a problem with JavaScript tracking, because a timestamp and random number is added to the query string.
  • Unless explicitly configured to ignore piwik.php, the proxy server spends needless ressources caching and later garbage-collecting the tracker requests, possibly pushing other resources out of the cache.

This PR adds adds a Cache-Control directive that prevents any caching of the request. For POST requests and 204 requests, caching is already prohibited according to the HTTP spec (unless explicitly permitted by Cache-Control headers), so I do not send the header for these requests in order to save bytes over the wire.

Even though 204 responses are not cacheable without an explicit Cache-Control header, according to RFC 7231, section 6.1, the default Varnish configuration does not respect this, so Cache-Control: private is also sent for 204 responses.

@mattab
Copy link
Member

mattab commented Apr 23, 2018

Thanks for the PR @c960657 👍

Before we can merge, could you please add some test(s) in tests/PHPUnit/System/TrackerResponseTest.php to ensure we don't regress this in the future?

@mattab mattab added the Needs Review PRs that need a code review label Apr 24, 2018
@mattab mattab added this to the 3.6.0 milestone Apr 24, 2018
@c960657
Copy link
Contributor Author

c960657 commented Apr 27, 2018

I have added some tests.

Instead of using assertResponseCode() and assertHttpResponseText() that each make an HTTP requests, I used Http::sendHttpRequest() to make one request that is then used for several assertions using regular assertEquals() etc.

These methods assertResponseCode() and assertHttpResponseText() and the corresponding PHPUnit constraint classes are not used very much in Matomo core, and they don't seem to add much value over just asserting directly on the HTTP response object returned by Http::sendHttpRequest(), so I have marked them as @deprecated. I hope this makes sense.

@c960657
Copy link
Contributor Author

c960657 commented Apr 27, 2018

I changed it to use no-store instead of no-cache. no-store seems more appropriate, see e.g. this.

@c960657 c960657 force-pushed the tracker-no-cache branch 2 times, most recently from 37e7265 to 1d58c9f Compare May 29, 2018 14:10
@diosmosis
Copy link
Member

Code looks good to me.

FYI @tsteur this PR deprecates some custom asserts, requires your approval. (Don't think they're actually that useful, but just want to make sure no one else finds them useful.)

@diosmosis diosmosis merged commit ab2b3f3 into matomo-org:3.x-dev Jun 18, 2018
@c960657 c960657 deleted the tracker-no-cache branch June 19, 2018 05:12
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
@c960657 @InfinityVoid
Prevent caching of tracker in proxies (matomo-org#12730)
8bcb20a 
* Prevent caching of tracker in proxies

* Also send Cache-Control with 204 responses.

Varnish default config violates the HTTP spec in this respect.

* Test Cache-Control header

* Update changelog

* Use no-store instead of no-cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@c960657 @mattab @diosmosis