@c960657 opened this Pull Request on April 16th 2018 Contributor

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 commented on April 23rd 2018 Member

Thanks for the PR @c960657 :+1:

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?

@c960657 commented on April 27th 2018 Contributor

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 <a class='mention' href='https://github.com/deprecated'>@deprecated</a>. I hope this makes sense.

@c960657 commented on April 27th 2018 Contributor

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

@diosmosis commented on June 10th 2018 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.)

This Pull Request was closed on June 18th 2018
Powered by GitHub Issue Mirror