Fix response-cache key collisions on pagination and request host

api/cache.py

@@ -16,7 +16,7 @@
 import hashlib
 from collections.abc import Iterable
 from enum import StrEnum
-from typing import Any
+from typing import Any, Protocol
 
 from django.core.cache import cache
 
@@ -40,10 +40,50 @@ class ModelLabel(StrEnum):
     UNIVERSE = "universe"
 
 
+class _CacheKeyRequest(Protocol):
+    """The subset of DRF's Request that cache-key builders need -- narrowed
+    so cache.py doesn't have to import rest_framework.request.Request just
+    to type-hint it."""
+
+    scheme: str
+
+    def get_host(self) -> str: ...
+
+    @property
+    def query_params(self) -> Any: ...
+
+
+def _request_digest(request: _CacheKeyRequest) -> str:
+    """Hash of everything about the request, beyond the object/model being
+    served, that the cached response's content depends on:
+
+    * origin (`{scheme}://{host}`) -- every cached serializer embeds a
+      `resource_url` built via `request.build_absolute_uri()`, so a
+      response cached while serving one scheme/host must not be served
+      back to a request against another.
+    * query params -- required for any paginated endpoint (list, or a
+      detail-scoped action like issue_list), otherwise `?page=1` and
+      `?page=2` compute the same digest and whichever page was cached
+      first gets served back for both. Harmless to include unconditionally
+      for unpaginated endpoints too, since those are never requested with
+      params that change the response.
+    """
+    normalized = f"origin={request.scheme}://{request.get_host()}"
+    query = request.query_params.lists()
+    normalized += "&" + "&".join(f"{k}={v}" for k, v in sorted(query))
+    return hashlib.sha256(normalized.encode()).hexdigest()[:16]
+
+
 def detail_cache_key(
-    model_label: str, action: str, pk: Any, modified, *dependent_labels: str
+    model_label: str,
+    action: str,
+    pk: Any,
+    modified,
+    *dependent_labels: str,
+    request: _CacheKeyRequest,
 ) -> str:
-    """Cache key for a single object's serialized detail response.
+    """Cache key for a single object's serialized detail response, or a
+    paginated detail-scoped action response (e.g. issue_list).
 
     Self-invalidating: a change to `modified` produces a new key, so old
     entries are simply orphaned and expire via TTL.
@@ -60,13 +100,16 @@ def detail_cache_key(
     cascade a `modified` bump onto this one (e.g. a Series response embeds
     its Publisher's name, but renaming the Publisher doesn't touch the
     Series row).
+
+    `request` supplies origin and query params -- see `_request_digest()`.
     """
     key = f"api:detail:{model_label}:{action}:{pk}:{modified.timestamp()}"
     if dependent_labels:
         version_map = get_model_versions(dependent_labels)
         versions = "-".join(str(version_map[lbl]) for lbl in dependent_labels)
         key = f"{key}:{versions}"
-    return key
+    digest = _request_digest(request)
+    return f"{key}:{digest}"
 
 
 def get_model_version(model_label: str) -> int:
@@ -111,20 +154,18 @@ def bump_model_version(model_label: str) -> None:
 def list_cache_key(
     model_label: str,
     *dependent_labels: str,
-    query: Iterable[tuple[str, list[str]]],
+    request: _CacheKeyRequest,
     scope: str = "",
 ) -> str:
     """Cache key for a list-type response: one or more model versions plus a
-    normalized hash of the request's query params.
-
-    `query` should come from `request.query_params.lists()` (multi-value),
+    normalized hash of the request's origin and query params -- see
+    `_request_digest()`. Uses `request.query_params.lists()` (multi-value),
     not `.dict()` -- `.dict()` silently drops all-but-the-last value for
     repeated params (e.g. IssueFilter's `role_id`), which would let distinct
     multi-value requests collide on the same key.
     """
     labels = (model_label, *dependent_labels)
     version_map = get_model_versions(labels)
     versions = "-".join(str(version_map[lbl]) for lbl in labels)
-    normalized = "&".join(f"{k}={v}" for k, v in sorted(query))
-    digest = hashlib.sha256(normalized.encode()).hexdigest()[:16]
+    digest = _request_digest(request)
     return f"api:list:{model_label}:{scope}:{versions}:{digest}"

api/views.py

@@ -224,7 +224,12 @@ def _cached_retrieve(self, request, *args, **kwargs):
             return mixins.RetrieveModelMixin.retrieve(self, request, *args, **kwargs)
 
         key = detail_cache_key(
-            self.cache_model_label, "retrieve", pk, modified, *self.cache_detail_dependent_labels
+            self.cache_model_label,
+            "retrieve",
+            pk,
+            modified,
+            *self.cache_detail_dependent_labels,
+            request=self.request,
         )
         cached = cache.get(key)
         if cached is not None:
@@ -261,7 +266,7 @@ def list(self, request, *args, **kwargs):
         key = list_cache_key(
             self.cache_model_label,
             *self.cache_dependent_labels,
-            query=request.query_params.lists(),
+            request=request,
         )
         cached = cache.get(key)
         if cached is not None:
@@ -299,6 +304,7 @@ def _cached_paginated_action(self, *, build_queryset, serializer_class):
                 pk,
                 modified,
                 *self.cache_action_dependent_labels,
+                request=self.request,
             )
             cached = cache.get(key)
             if cached is not None:
@@ -697,7 +703,7 @@ def series_list(self, request, pk=None):
         key = list_cache_key(
             ModelLabel.SERIES,
             ModelLabel.ISSUE,
-            query=self.request.query_params.lists(),
+            request=request,
             scope=f"publisher:{pk}:series_list",
         )
         cached = cache.get(key)

tests/comicsdb/test_api_response_caching.py

@@ -8,6 +8,7 @@
 from django.urls import reverse
 from django.utils import timezone
 from rest_framework import status
+from rest_framework.pagination import PageNumberPagination
 
 from api.views import CollectionViewSet, PullListViewSet, WishListViewSet
 from comicsdb.models import Credits, Issue, Variant
@@ -488,6 +489,59 @@ def test_arc_retrieve_does_not_poison_issue_list_cache(
     assert body["results"][0]["number"] == "1"
 
 
+def test_arc_issue_list_pagination_is_not_poisoned_across_pages(
+    api_client_with_credentials, issue_with_arc, fc_arc, fc_series, monkeypatch, local_cache
+):
+    """issue_list's cache key must include the request's query string, since
+    ?page=1 and ?page=2 return different data for the same (pk, modified)."""
+    # PageNumberPagination.page_size is read from api_settings.PAGE_SIZE at
+    # class-definition time, not per-request, so overriding
+    # settings.REST_FRAMEWORK here wouldn't affect it -- patch the class
+    # attribute directly instead.
+    monkeypatch.setattr(PageNumberPagination, "page_size", 1)
+
+    second_issue = Issue.objects.create(
+        series=fc_series,
+        number="2",
+        slug="final-crisis-2",
+        cover_date=timezone.now().date(),
+        edited_by=issue_with_arc.edited_by,
+        created_by=issue_with_arc.created_by,
+    )
+    second_issue.arcs.add(fc_arc)
+
+    url = reverse("api:arc-issue-list", kwargs={"pk": fc_arc.pk})
+    page_one = api_client_with_credentials.get(url, {"page": 1})
+    assert page_one.status_code == status.HTTP_200_OK
+    assert [r["number"] for r in page_one.json()["results"]] == ["1"]
+
+    page_two = api_client_with_credentials.get(url, {"page": 2})
+    assert page_two.status_code == status.HTTP_200_OK
+    assert [r["number"] for r in page_two.json()["results"]] == ["2"]
+
+
+def test_arc_retrieve_resource_url_is_not_poisoned_across_hosts(
+    api_client_with_credentials, fc_arc, settings, local_cache
+):
+    """resource_url is built from request.build_absolute_uri(), so the
+    cache key must include the request's scheme/host -- otherwise a
+    response cached while serving one hostname gets that hostname baked
+    into resource_url for every other hostname the API is also reachable
+    under."""
+    settings.ALLOWED_HOSTS = [*settings.ALLOWED_HOSTS, "other.example"]
+    url = reverse("api:arc-detail", kwargs={"pk": fc_arc.pk})
+
+    resp = api_client_with_credentials.get(url, HTTP_HOST="testserver")
+    assert resp.status_code == status.HTTP_200_OK
+    assert resp["X-Cache"] == "MISS"
+    assert resp.json()["resource_url"].startswith("http://testserver/")
+
+    resp = api_client_with_credentials.get(url, HTTP_HOST="other.example")
+    assert resp.status_code == status.HTTP_200_OK
+    assert resp["X-Cache"] == "MISS"
+    assert resp.json()["resource_url"].startswith("http://other.example/")
+
+
 def test_character_retrieve_does_not_poison_issue_list_cache(
     api_client_with_credentials, issue_with_arc, superman, local_cache
 ):