fix(cache): never store responses that belong to one caller - #305
Merged
Merged
Conversation
@cache wrote a response to the shared backend even when the handler marked it private or no-store, when it set a cookie, or when the request carried Authorization, and then replaced the handler's Cache-Control with the decorator's. One user's response was replayed to everyone. A request with Authorization now bypasses the backend like private=True (RFC 9111 section 3.5), unless the route is public=True or opts in with the new cache_authorized=True for identity-aware key builders. A rendered response with private/no-store in its own Cache-Control, or with Set-Cookie, is served but not written; an existing entry is left alone, and a private/no-store header from the handler is kept. Each skip is logged at DEBUG. Closes #296
A response left unstored because it sets a cookie, or because it answers an Authorization request the backend was bypassed for, still went out with the decorator's Cache-Control, so a CDN or proxy could store it (public explicitly allowed it, and must-revalidate alone lets a shared cache reuse an Authorization response under RFC 9111 3.5). Such responses, and their 304s, now carry private in place of public with the other directives kept (private, no-cache on no_cache routes). The variant is built once at decoration time. A handler's own private/no-store header and no_store=True still win. must_revalidate=True still does not lift the Authorization bypass; only public=True or cache_authorized=True do. The docstring and docs say so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
@cache(ttl=...)wrote a response to the shared backend even when it belonged to one caller, and then replaced the handler'sCache-Controlwith the decorator's (#296):Cache-Control: privateorno-store;Set-Cookiewas stripped from the entry, but the body it came with was stored and replayed to everyone);Authorization(RFC 9111 §3.5 forbids a shared cache from reusing such a response unless it allows it).Repro from the issue: a handler that sets
private, no-storeand returns theAuthorizationheader served{"user": "Bearer alice"}to bob, withCache-Control: max-age=60.Fix
All in
fastapi_cachex/cache.py:Authorizationrequests bypass the backend. No read and no write, through the existingprivate/no-ttlbypass path, soIf-None-Matchstill gets a 304 when it matches the fresh render. Two exceptions:public=True(as RFC 9111 §3.5 allows), and the new keyword-only opt-incache_authorized: bool = False. Usecache_authorizedfor routes whosekey_builderincludes the verified caller identity.must_revalidate=Truedoes not lift the bypass: RFC 9111 would allow reuse undermust-revalidate, but the library requires an explicit opt-in. The docstring and docs say so.privateorno-store. The check matches whole directive tokens, case-insensitively, across everyCache-Controlfield. The response is served but not written. The handler's header is sent unchanged:_with_cache_controlno longer replaces it, and 304s on the bypass andno_cachepaths repeat it too.no_store=Trueon the decorator still sendsno-storeunconditionally, because it is stricter than anything the handler can send.Set-Cookie. It is served normally but not written.privatefor a downstream shared cache. A response that sets a cookie, and the answer to anAuthorizationrequest that bypassed the backend, are sent withprivatein place ofpublic(200 and 304 alike). The decorator's other directives are kept (max-age,must-revalidate,stale-*,immutable). On ano_cacheroute the header becomesprivate, no-cache(plusmust-revalidatewhen set).publicis never sent on these responses, so a CDN or proxy does not store them either. Without this,must_revalidate=Truealone would let a shared cache reuse anAuthorizationresponse. The private variant is built once at decoration time, likecache_control. A handler's ownprivate/no-storeheader still wins, andno_store=Truestill sends onlyno-store.DEBUG.In cases 2 and 3, an entry that is already stored under the key is left alone. This matches how a non-2xx render is handled: the stored entry came from a shareable response. A request that finds a valid entry is still answered from it before the handler runs. The handler only runs with a live entry present on
no_cacheroutes, and those never serve the entry's body without revalidation. Routes that trigger none of these rules behave as before.Docs:
docs/HTTP_CACHING.md(directive tablepublicrow, the storage rules, "Authenticated endpoints": the per-user example now passescache_authorized=True, and a note on when the key builder runs),docs/CACHE_FLOW.md(flow diagram, decision pseudo-code, a new note, the scenarios table), the README warning, and the zh-TW mirrors (i18n/zh-TW/docs/HTTP_CACHING.md,CACHE_FLOW.md,index.md). Nothing inexamples/caches a route that receivesAuthorization, so no example needed changes, andtests/test_examples.pypasses.Tests
New file
tests/test_cache_unshareable.py(28 tests):private, no-store);private/no-store/ mixed-case / multi-directive values are not stored, and the handler's header is kept;max-age,no-cacheandx-private-hint, publicare still stored (whole-token match only);Set-Cookieresponse is not stored and each request gets its own cookie;Authorizationrequest neither reads an existing anonymous entry nor writes;Authorizationrequest still revalidates with a 304;no_cachepath repeats the handler'sprivateheader;public=TruecachesAuthorizationrequests;cache_authorized=Truewith a per-user key builder: an alice hit, and bob gets his own entry;no_cacheroute leaves the existing entry alone;no_store=Trueoverrides the handler's header;DEBUG;privateheader:Set-Cookieon apublic=Trueroute with every directive getsprivate, max-age=60, must-revalidate, stale-while-revalidate=30, immutableand is not stored;Set-Cookie304 on a bypassed (ttl-less,public) route getsprivate;Set-Cookieon ano_cacheroute getsprivate, no-cache, must-revalidateon the 200 and the 304;Authorizationwithmust_revalidate=Truegetsprivate, max-age=60, must-revalidateon the 200 and the 304 and is not stored;Authorizationon ano_cacheroute getsprivate, no-cache;Authorizationroutes keep the decorator's header (public, max-age=60/max-age=60), andno_store=Truestill sendsno-storeoverSet-Cookie.The first commit's cookie and
Authorizationtests (setting_a_cookie_is_not_stored,authorization_request_bypasses_the_backend,authorization_request_still_revalidates) expectedmax-age=60. The second commit updates them to expectprivate, max-age=60.Changed test:
tests/test_cache_status_headers.py::test_set_cookie_is_never_replayed. It encoded the buggy behaviour: it expected the second request to be a cache hit of the cookie-setting response, without the cookie. It now asserts that the response is not stored at all: the handler runs twice, both responses carry the cookie andX-Safe, and the backend has no entry. The test's intent, that a cookie is never replayed, is kept and made stronger.Mutation check. With
cache.pyreverted to master, 14 of the first commit's new or changed tests fail. The ones that pass on master are the guard tests (other directives stored, 304 forAuthorization,public=True,no_storeoverride), which pass by design. Each targeted mutation of the fix fails at least one test:Authorizationbypasspublicexceptioncache_authorizedexceptionno_cache304 uses the decorator headerno_storekeeps the handler headerFor the second commit, with
cache.pyat the first commit, 8 of the 28 tests fail. These are the 5 newprivate-header tests plus the 3 updated ones. The 3 guards pass by design. The targeted mutations below were run on the full suite:Authorizationbypass sends the decorator headerSet-Cookiekeeps the decorator headerno_cachevariant without theprivateprefixpublicmust-revalidateAuthorization304 sends the decorator headermust_revalidate=Truelifts the bypassLocal results:
uv run pytest: 885 passed, 191 skipped (live Redis/Memcached tests skipped), 93.70% coverage,cache.pyat 100%.ruff check,ruff format --check,mypy --strictandpre-commit run --all-filesare clean.Compatibility
This changes behaviour under an unchanged API. A route that relied on sharing cached responses across
Authorizationrequests now needspublic=Trueorcache_authorized=True. Cookie-setting responses, and bypassedAuthorizationresponses, now sendprivateinstead ofpublic(or no scope).CHANGELOG
The entry is in
changelog.d/296.security.mdand is merged intoCHANGELOG.mdat release time.Closes #296