Repository: coderedcorp/wagtail-cache

Issue:
Ignore tracking querystrings
## Status quo
Django full page(s) cache will cache multiple times the same page if different GET parameters are passed.
Example:
- `foo.com/` -> new cache entry
- `foo.com/?do=something` -> new cache entry
- `foo.com/?do=something_else` -> new cache entry

## Why can this be a problem?
First of all, I feel like it's relatively rare these that GET params are changing anything in a page. And if it does, it's something that the developer has under control.
Also, more and more these days, many 3rd parties are polluting our urls and therefore makes it impossible to hit the cached version of a page, even though nothing changes in term of content in the page. There is **many** reasons why random GET params will be appended to your urls!

Example from [here](https://www.wordstream.com/gclid#:~:text=gclid%20tracking%20parameter%20to%20your,of%20a%20particular%20user%20session.):
_A default feature of any Google Ads (formerly known as Google AdWords) account is URL auto-tagging. When this is turned on, Google automatically appends a unique ?gclid tracking parameter to your destination URL whenever someone clicks on one of your ads. Essentially, this is a way for Google Ads to tell Analytics about all the minute details of a particular user session._

You can imagine how big of a problem this can be:
1. The the cache is not hit, so a new cache entry is added.
1. This cache is most likely useless as it is unique to the visitor

## Suggestion(s)
I personally think that we should control this more. Here are a few ideas, up for discussion (please refer to their number when discussing them 🙂)
1. Remove GET parameters from factors that make the cache unique, always return the page as if there was nothing special.
1. Whitelist a few GET parameters keys. If I decide that the key `do_something` should cache a different version of the page, then I have to add it to a certain place (environment variable for instance, like `WAGTAIL_CACHE_GET_PARAMS_WHITELIST = ['key_one', 'key_two']`)
1. Maybe not any page of your website could be affected by these GET parameters, so maybe having a way to whitelist keys directly in a Page Type class definition instead?
1. It could be pushed even further by not only declaring the whitelisted keys, but also a regular expression for the possible values!

## Conclusion
I'm not a Django expert and I can be mistaking somewhere, but so far I'm curious to know what others think about this idea. If not relevant, feel free to close this ticket but please explain why ;)

Discussion:

Patch:
diff --git a/docs/getting_started/django_settings.rst b/docs/getting_started/django_settings.rst
index f50a719..651c8ac 100644
--- a/docs/getting_started/django_settings.rst
+++ b/docs/getting_started/django_settings.rst
@@ -3,21 +3,54 @@ Django Settings
 
 WAGTAIL_CACHE
 -------------
-Boolean toggling whether or not to load the page caching machinery and enable cache settings in
-the wagtail admin. Defaults to ``True`` which is on. Most developers will want to set this to
-``False`` in the development environment.
+
+Boolean toggling whether or not to load the page caching machinery and enable
+cache settings in the wagtail admin. Defaults to ``True`` which is on. Most
+developers will want to set this to ``False`` in the development environment.
 
 WAGTAIL_CACHE_BACKEND
 ---------------------
-The name of the Django cache alias/backend to use for the page cache; one of the keys in the
-``CACHES`` Django setting. Defaults to ``'default'`` which is required by Django when using
-the cache. Complex projects would likely want to use a separate cache for the page cache to
-easily purge as needed without affecting other caches. Clearing the cache through the wagtail
-admin will purge this entire cache.
+
+The name of the Django cache alias/backend to use for the page cache; one of the
+keys in the ``CACHES`` Django setting. Defaults to ``'default'`` which is
+required by Django when using the cache. Complex projects would likely want to
+use a separate cache for the page cache to easily purge as needed without
+affecting other caches. Clearing the cache through the wagtail admin will purge
+this entire cache.
 
 WAGTAIL_CACHE_HEADER
 --------------------
-By default, an HTTP header named ``X-Wagtail-Cache`` is added to the response to indicate
-a cache hit or miss. To turn off this header, set ``WAGTAIL_CACHE_HEADER = False``,
-or to customize the header set to a string. Note that other HTTP headers may also added by
-the Django cache middleware.
+
+By default, an HTTP header named ``X-Wagtail-Cache`` is added to the response to
+indicate a cache hit or miss. To turn off this header, set
+``WAGTAIL_CACHE_HEADER = False``, or to customize the header set to a string.
+Note that other HTTP headers may also be added by the Django cache middleware.
+
+WAGTAIL_CACHE_IGNORE_QS
+-----------------------
+
+.. versionadded:: 2.0
+
+   This setting will ignore tracking/advertising URL parameters, and is ON by
+   default. To restore the old behavior, set to ``None``.
+
+A list of strings (regular expressions) to ignore from the URL querystring when
+determining the caching decision. Any querystrings in this list **MUST NOT**
+have any effect on how your pages/views are served.
+
+By default this is set to a list of well-known tracking and advertising tags
+such used by Google Analytics, Facebook, HubSpot, etc. These tracking codes are
+a sysadmin's worst nightmare as they effectively bust any semblance of a cache
+and senselessly spike server load. `The defaults are defined here
+<https://github.com/coderedcorp/wagtail-cache/blob/main/wagtailcache/settings.py>`_.
+
+If you use these querystring parameters for server-side logic, or if you find
+that Wagtail Cache is serving incorrect page contents, you may need to customize
+or disable this setting.
+
+To restore the old behavior, and treat each combination of querystrings as its
+own unique page in the cache, set this value to ``None`` or ``[]``.
+
+If you feel as though the spammers have won, and want the nuclear option, you
+can set this to ``[r".*"]`` which will ignore all querystrings. This is surely
+a terrible idea, but it can be done.
diff --git a/docs/getting_started/hooks.rst b/docs/getting_started/hooks.rst
index df653ce..b67173f 100644
--- a/docs/getting_started/hooks.rst
+++ b/docs/getting_started/hooks.rst
@@ -1,15 +1,27 @@
 Wagtail Hooks
 =============
 
+.. warning::
+
+   Returning ``True`` from any of the wagtail hooks below can be very dangerous,
+   as it will completely override the relatively safe and predictable default
+   behavior. Returning ``True`` could for example cause ``POST`` requests, or
+   requests from logged in users, to be cached and served publicly.
+
+   Be sure to read the docs below carefully before implementing these hooks.
+
+
 is_request_cacheable
 --------------------
-The callable passed into this hook should take an ``HttpRequest`` and a ``bool`` argument, and returns a
-``bool`` indicating whether or not the response to this request should be cached
-(served from the cache if it is already cached, or added to the cache if it is not already
-cached). Not returning, or returning anything other than a bool will not affect the caching
-decision. The ``bool`` passed in represents the current caching decision. So if there were multiple
-``is_request_cacheable`` hooks called, each one would receive the result of the previous. Use this
-variable as you see fit to help with your own logic.
+
+The callable passed into this hook should take an ``HttpRequest`` and a ``bool``
+argument, and returns a ``bool`` indicating whether or not the response to this
+request should be cached (served from the cache if it is already cached, or
+added to the cache if it is not already cached). Not returning, or returning
+anything other than a bool will not affect the caching decision. The ``bool``
+passed in represents the current caching decision. So if there were multiple
+``is_request_cacheable`` hooks called, each one would receive the result of the
+previous. Use this variable as you see fit to help with your own logic.
 
 For example:
 
@@ -17,23 +29,26 @@ For example:
 
     from wagtail.core import hooks
 
-    @hooks.register('is_request_cacheable')
+    @hooks.register("is_request_cacheable")
     def nocache_in_query(request, curr_cache_decision):
         # if the querystring contains a "nocache" key, return False to
         # forcibly not cache. Otherwise, do not return to let wagtail-cache
         # decide how to cache.
-        if 'nocache' in request.GET:
+        if "nocache" in request.GET:
             return False
 
 
 is_response_cacheable
 ---------------------
-The callable passed into this hook should take an ``HttpResponse`` and a ``bool`` argument, and returns a
-``bool`` indicating whether or not this response should be cached (added to the cache if it is not already
-cached). Not returning, or returning anything other than a bool will not affect the caching
-decision. The ``bool`` passed in represents the current caching decision. So if there were multiple
-``is_response_cacheable`` hooks called, each one would receive the result of the previous. Use this
-variable as you see fit to help with your own logic.
+
+The callable passed into this hook should take an ``HttpResponse`` and a
+``bool`` argument, and returns a ``bool`` indicating whether or not this
+response should be cached (added to the cache if it is not already cached). Not
+returning, or returning anything other than a bool will not affect the caching
+decision. The ``bool`` passed in represents the current caching decision. So if
+there were multiple ``is_response_cacheable`` hooks called, each one would
+receive the result of the previous. Use this variable as you see fit to help
+with your own logic.
 
 For example:
 
@@ -41,13 +56,13 @@ For example:
 
     from wagtail.core import hooks
 
-    @hooks.register('is_response_cacheable')
+    @hooks.register("is_response_cacheable")
     def nocache_secrets(request, curr_cache_decision):
         # if the response contains a custom header, return False to
         # forcibly not cache. Otherwise, do not return to let wagtail-cache
         # decide how to cache.
-        if response.has_header('X-My-Header'):
-            if response['X-My-Header'] == 'secret':
+        if response.has_header("X-My-Header"):
+            if response["X-My-Header"] == "secret":
                 return False
 
 
@@ -56,12 +71,18 @@ Notes about the request/response cycle
 
 During a request/response cycle, wagtail-cache makes its caching decision as so:
 
-#. If the request is not a preview, does not have a logged in user, and is GET or HEAD, then try to cache.
-#. Run ``is_request_cacheable`` hooks.
-#. If the result of the hooks is ``True``, then check to see if this request is already in the cache.
+#. If the request is not a preview, does not have a logged in user, and is GET
+   or HEAD, then try to cache.
+#. Run ``is_request_cacheable`` hooks, and continue only if the result is
+   ``True``.
+#. Strip querystrings from the request URL which match
+   ``WAGTAIL_CACHE_IGNORE_QS``.
 #. If the request is already in the cache, serve directly from the cache.
-#. If the request is not in the cache, then run the view.
-#. Check if the view's response contains a ``Cache-Control`` header containing ``private`` or ``no-cache``.
-#. Run ``is_response_cacheable`` hooks.
-#. If the result of the hooks is ``True``, then save the response into the cache for next time.
+#. If the request is not in the cache, then run the view. The original request,
+   including all querystrings, is passed to the view.
+#. Check if the view's response contains a ``Cache-Control`` header containing
+   ``private`` or ``no-cache``.
+#. Run ``is_response_cacheable`` hooks, and continue only if the result is
+   ``True``.
+#. Save the response into the cache for next time.
 #. Finally, return the response.
diff --git a/pyproject.toml b/pyproject.toml
index 24b9307..44fae44 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,6 +1,6 @@
 [tool.black]
 line-length = 80
-target-version = ['py36', 'py37', 'py38']
+target-version = ['py37', 'py38', 'py39', 'py310']
 # Regular expression of files to exclude.
 exclude = '''
 /(
diff --git a/setup.cfg b/setup.cfg
index da10533..38a28d5 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -1,3 +1,6 @@
+[codespell]
+ignore-words-list = doubleclick
+
 [flake8]
 max-line-length = 100
 exclude = migrations
diff --git a/wagtailcache/cache.py b/wagtailcache/cache.py
index 2f2c5ae..01e31e7 100644
--- a/wagtailcache/cache.py
+++ b/wagtailcache/cache.py
@@ -7,6 +7,7 @@ from functools import wraps
 from typing import Callable, Optional, List
 from urllib.parse import unquote
 from django.core.cache import caches
+from django.core.cache.backends.base import BaseCache
 from django.core.handlers.wsgi import WSGIRequest
 from django.http.response import HttpResponse
 from django.template.response import SimpleTemplateResponse
@@ -51,6 +52,50 @@ def _patch_header(response: HttpResponse, status: Status) -> None:
         response[wagtailcache_settings.WAGTAIL_CACHE_HEADER] = status.value
 
 
+def _chop_querystring(r: WSGIRequest) -> WSGIRequest:
+    """
+    Given a request object, remove any of our ignored querystrings from it.
+    """
+    if len(r.GET) and wagtailcache_settings.WAGTAIL_CACHE_IGNORE_QS:
+        # Make a copy of querystrings, and delete any that should be ignored.
+        qs = r.GET.copy()
+        for q in r.GET:
+            for regex in wagtailcache_settings.WAGTAIL_CACHE_IGNORE_QS:
+                if re.match(regex, q):
+                    del qs[q]
+        # Mutate the request to include our chopped up querystrings. We must
+        # also mutate the raw QUERY_STRING as that is used within
+        # ``request.build_absolute_uri()`` which is used in Django cache
+        # middleware internals.
+        r.GET = qs
+        r.META["QUERY_STRING"] = qs.urlencode()
+    return r
+
+
+def _get_cache_key(r: WSGIRequest, c: BaseCache) -> str:
+    """
+    Wrapper for Django's get_cache_key which first strips specific
+    querystrings. Since the Django cache middleware is somewhat complicated and
+    RFC compliant, we are best off to chop and pass it along rather than
+    re-inventing the cache keying logic.
+    """
+    r = _chop_querystring(r)
+    return get_cache_key(r, None, r.method, c)
+
+
+def _learn_cache_key(
+    r: WSGIRequest, s: HttpResponse, t: int, c: BaseCache
+) -> str:
+    """
+    Wrapper for Django's learn_cache_key which first strips specific
+    querystrings. Since the Django cache middleware is somewhat complicated and
+    RFC compliant, we are best off to chop and pass it along rather than
+    re-inventing the cache keying logic.
+    """
+    r = _chop_querystring(r)
+    return learn_cache_key(r, s, t, None, c)
+
+
 class FetchFromCacheMiddleware(MiddlewareMixin):
     """
     Loads a request from the cache if it exists.
@@ -88,21 +133,14 @@ class FetchFromCacheMiddleware(MiddlewareMixin):
             setattr(request, "_wagtailcache_skip", True)
             return None  # Don't bother checking the cache.
 
-        # Try and get the cached GET response.
-        cache_key = get_cache_key(request, None, "GET", cache=self._wagcache)
+        # Try and get the cached response.
+        cache_key = _get_cache_key(request, self._wagcache)
         if cache_key is None:
             setattr(request, "_wagtailcache_update", True)
             return None  # No cache information available, need to rebuild.
 
         response = self._wagcache.get(cache_key)
 
-        # If it wasn't found and a HEAD was requested, try looking for that.
-        if response is None and request.method == "HEAD":
-            cache_key = get_cache_key(
-                request, None, "HEAD", cache=self._wagcache
-            )
-            response = self._wagcache.get(cache_key)
-
         if response is None:
             setattr(request, "_wagtailcache_update", True)
             return None  # No cache information available, need to rebuild.
@@ -180,12 +218,14 @@ class UpdateCacheMiddleware(MiddlewareMixin):
             timeout = self._wagcache.default_timeout
         patch_response_headers(response, timeout)
         if timeout:
-            cache_key = learn_cache_key(
-                request, response, timeout, None, cache=self._wagcache
+            cache_key = _learn_cache_key(
+                request, response, timeout, self._wagcache
             )
 
             # Track cache keys based on URI.
-            uri = unquote(request.build_absolute_uri())
+            # (of the chopped request, not the real one).
+            cr = _chop_querystring(request)
+            uri = unquote(cr.build_absolute_uri())
             keyring = self._wagcache.get("keyring", {})
             # Get current cache keys belonging to this URI.
             # This should be a list of keys.
@@ -228,9 +268,7 @@ def clear_cache(urls: List[str] = None) -> None:
         matched_urls = []
         for regex in urls:
             for key in keyring:
-                print(f"comparing '{regex}' to '{key}'")
                 if re.match(regex, key):
-                    print("matched!")
                     matched_urls.append(key)
 
         # If it matches, delete each entry from the cache,
diff --git a/wagtailcache/settings.py b/wagtailcache/settings.py
index c0d80fa..2ec92d0 100644
--- a/wagtailcache/settings.py
+++ b/wagtailcache/settings.py
@@ -11,6 +11,27 @@ class _DefaultSettings:
     WAGTAIL_CACHE = True
     WAGTAIL_CACHE_BACKEND = "default"
     WAGTAIL_CACHE_HEADER = "X-Wagtail-Cache"
+    WAGTAIL_CACHE_IGNORE_QS = [
+        r"^_bta_.*$",  # Bronto
+        r"^_ga$",  # Google Analytics
+        r"^affiliate$",  # Instagram affiliates
+        r"^ck_subscriber_id$",  # Instagram affiliates
+        r"^dm_i$",  # Dotdigital
+        r"^ef_.*$",  # Adobe Analytics
+        r"^epik$",  # Pinterest
+        r"^fb_source$",  # Facebook
+        r"^fbclid$",  # Facebook
+        r"^gclid$",  # Google AdWords
+        r"^gclsrc$",  # Google DoubleClick
+        r"^hsa_.*$",  # Hubspot
+        r"^matomo_.*$",  # Matomo
+        r"^mc_.*$",  # Mailchimp
+        r"^msclkid$",  # Microsoft Advertising
+        r"^mtm_.*$",  # Matomo
+        r"^s_kwcid$",  # Adobe Analytics
+        r"^trk_.*$",  # Listrak
+        r"^utm_.*$",  # Google Analytics
+    ]
 
     def __getattribute__(self, attr: Text):
         return getattr(settings, attr, super().__getattribute__(attr))


Test patch:
diff --git a/testproject/home/tests.py b/testproject/home/tests.py
index 3df403a..fad58da 100644
--- a/testproject/home/tests.py
+++ b/testproject/home/tests.py
@@ -172,6 +172,21 @@ class WagtailCacheTest(TestCase):
         )
         return response
 
+    def post_skip(self, url: str):
+        """
+        POSTS a page and tests that it was intentionally not served from
+        the cache.
+        """
+        response = self.client.post(url)
+        self.assertEqual(
+            response.get(self.header_name, None), Status.SKIP.value
+        )
+        self.assertTrue(
+            CacheControl.NOCACHE.value in response.get("Cache-Control", "")
+            or CacheControl.PRIVATE.value in response.get("Cache-Control", "")
+        )
+        return response
+
     # ---- TEST PAGES ----------------------------------------------------------
 
     def test_page_miss(self):
@@ -192,6 +207,27 @@ class WagtailCacheTest(TestCase):
             # Second get should continue to skip.
             self.get_skip(page.get_url())
 
+    def test_page_post(self):
+        # POSTs should never normally be cached, by default.
+        for page in self.should_cache_pages:
+            # First post should skip cache.
+            self.post_skip(page.get_url())
+            # Second post should skip cache.
+            self.post_skip(page.get_url())
+
+    def test_querystrings(self):
+        for page in self.should_cache_pages:
+            # First get should miss cache.
+            self.get_miss(page.get_url())
+            # Second get should hit cache.
+            self.get_hit(page.get_url())
+            # A get matching WAGTAIL_CACHE_IGNORE_QS should also hit the cache.
+            self.get_hit(page.get_url() + "?utm_code=0")
+            # A get with non-ignored querystrings should miss.
+            self.get_miss(page.get_url() + "?valid=0")
+            # A get with both should also hit, since it is the second request.
+            self.get_hit(page.get_url() + "?valid=0&utm_code=0")
+
     def test_page_restricted(self):
         auth_url = "/_util/authenticate_with_password/%d/%d/" % (
             self.view_restriction.id,
@@ -323,7 +359,7 @@ class WagtailCacheTest(TestCase):
         # Now the page should miss cache.
         self.get_miss(self.page_cachedpage.get_url())
 
-    # ---- PURGE SPECIFIC URLS & CLEAR ALL-------------------------------------------------
+    # ---- PURGE SPECIFIC URLS & CLEAR ALL--------------------------------------
 
     def test_cache_keyring(self):
         # Check if keyring is not present
@@ -414,20 +450,16 @@ class WagtailCacheTest(TestCase):
         hook_fns = hooks.get_hooks("is_request_cacheable")
         self.assertEqual(hook_fns, [hook_true])
 
-        # Setting `is_request_cacheale=True` does not really do much, because
+        # Setting `is_request_cachebale=True` does not really do much, because
         # the response still has the final say in whether or not the response is
-        # cached. The no-cache page will still not be cached due to the
-        # response. However a simple POST request will now be checked against
-        # the cache, although once again, it will probably not get cached due to
-        # the response.
+        # cached. However a simple POST request where the response does not
+        # forbid caching will in fact get cached!
         response = self.client.post(reverse("cached_view"))
         self.assertEqual(
             response.get(self.header_name, None), Status.MISS.value
         )
         response = self.client.post(reverse("cached_view"))
-        self.assertEqual(
-            response.get(self.header_name, None), Status.MISS.value
-        )
+        self.assertEqual(response.get(self.header_name, None), Status.HIT.value)
 
     def test_request_hook_false(self):
         # Register hook and assert it was actually registered.