amadan
Merge branch 'edge-s-maxage' into main
2 files changed,
+130
−10
internal/edge/edge.go+46 −10internal/edge/edge_test.go+84 −0
CI passed — run details
diff --git a/internal/edge/edge.go b/internal/edge/edge.go| index 16b7751a3..96345a4d9 100644 |
| --- a/internal/edge/edge.go |
| +++ b/internal/edge/edge.go |
| @@ -68,7 +68,7 @@ const ( |
| ) |
| // DefaultCacheMaxTTL is the ceiling every cached entry's TTL is clamped to, |
| -// however long the app's own max-age asks for. It is what makes spec decision |
| +// however long the app's own s-maxage or max-age asks for. It is what makes spec decision |
| // 4 true — "a lost purge self-heals within a bounded window": without a |
| // ceiling, an app setting max-age=31536000 turns one dropped purge call into a |
| // permanently wrong page. Five minutes is short enough that a missed purge is |
| @@ -1533,8 +1533,9 @@ func (e *Edge) cacheMaxBody() int { |
| } |
| // maybeStore inspects a recorded response and caches it if it qualifies: |
| -// 2xx status, a Cache-Control that names a positive max-age, and none of the |
| -// per-visitor markers below. This cache is shared across every visitor of a |
| +// 2xx status, a Cache-Control that names a positive shared-cache lifetime |
| +// (s-maxage, else max-age — see parseMaxAge), and none of the per-visitor |
| +// markers below. This cache is shared across every visitor of a |
| // host and caching is unconditional per route (not opt-in per app), so each |
| // refusal here is the only thing standing between one visitor's response and |
| // everyone else's: |
| @@ -1698,17 +1699,43 @@ func varyValue(r *http.Request, vary string) string { |
| return b.String() |
| } |
| -// parseMaxAge extracts max-age from a Cache-Control header. Returns |
| -// ok=false for "no-store", "no-cache", "private", or a missing/malformed |
| -// max-age — any of which mean "don't cache this in a shared cache." |
| +// parseMaxAge extracts the freshness lifetime a SHARED cache should use from a |
| +// Cache-Control header. Returns ok=false for "no-store", "no-cache", "private", |
| +// or a missing/malformed lifetime — any of which mean "don't cache this in a |
| +// shared cache." |
| // |
| -// The whole header is scanned before the max-age is honored, so a |
| +// s-maxage wins over max-age when both are present (RFC 9111 §5.2.2.10: it |
| +// overrides max-age for a shared cache, and a private cache ignores it). That |
| +// is the only directive pair an origin has for telling the two caches apart, |
| +// and it is what makes a purge effective end to end: "s-maxage=120, max-age=0" |
| +// asks the edge to hold the response for two minutes and every browser to |
| +// revalidate, so a purge here is not left with a client-side tail it cannot |
| +// reach. Reading only max-age left an origin one number for both caches — a |
| +// long one bought edge caching at the cost of stale browsers after a purge, a |
| +// short one freshened browsers by giving up the traffic the cache exists to |
| +// absorb, and the correct pair above read as max-age=0 and turned edge caching |
| +// off entirely (issue #403; tito PRO-4758/PRO-4759). |
| +// |
| +// max-age alone still means exactly what it did. Nothing can have depended on |
| +// s-maxage being ignored, because ignoring it is what made it unusable. |
| +// |
| +// The whole header is scanned before any lifetime is honored, so a |
| // "max-age=600, private" (directives in either order, and "private" in its |
| // field-name form private="Set-Cookie") is refused rather than cached because |
| -// max-age happened to come first. |
| +// max-age happened to come first — and, for the same reason, the two spellings |
| +// "s-maxage=120, max-age=0" and "max-age=0, s-maxage=120" mean the same thing. |
| +// A malformed argument on EITHER directive refuses the response whichever order |
| +// it arrives in, rather than falling back to the other one: an origin that |
| +// cannot state its lifetime is not asking for a guess. |
| +// |
| +// s-maxage is deliberately NOT read as an override of the Authorization |
| +// refusal in maybeStore, though RFC 9111 §3.5 permits that: making a response |
| +// to an authenticated request shareable stays an explicit "public", because |
| +// declining to cache is the safe direction and the origin already has the word |
| +// for it. |
| func parseMaxAge(cc string) (time.Duration, bool) { |
| - var ttl time.Duration |
| - found := false |
| + var ttl, shared time.Duration |
| + found, sharedFound := false, false |
| for name, arg := range ccDirectives(cc) { |
| switch name { |
| case "no-store", "no-cache", "private": |
| @@ -1719,8 +1746,17 @@ func parseMaxAge(cc string) (time.Duration, bool) { |
| return 0, false |
| } |
| ttl, found = time.Duration(secs)*time.Second, true |
| + case "s-maxage": |
| + secs, err := strconv.Atoi(strings.TrimSpace(arg)) |
| + if err != nil { |
| + return 0, false |
| + } |
| + shared, sharedFound = time.Duration(secs)*time.Second, true |
| } |
| } |
| + if sharedFound { |
| + return shared, true |
| + } |
| if !found { |
| return 0, false |
| } |
diff --git a/internal/edge/edge_test.go b/internal/edge/edge_test.go| index 157fc7d71..137d41149 100644 |
| --- a/internal/edge/edge_test.go |
| +++ b/internal/edge/edge_test.go |
| @@ -818,6 +818,90 @@ func TestServeHTTPClampsMaxAgeToCeiling(t *testing.T) { |
| } |
| } |
| +// RFC 9111 §5.2.2.10: s-maxage overrides max-age for a SHARED cache, and a |
| +// private cache ignores it. That pair is the only way an origin can say "hold |
| +// this at the edge for two minutes, and have every browser revalidate" — which |
| +// is what makes a purge effective end to end rather than leaving a client-side |
| +// tail the purge channel cannot reach. Reading max-age alone turned exactly |
| +// that header ("s-maxage=120, max-age=0") into "don't cache at all", so the |
| +// origins trying hardest to stay purgeable were the ones that silently lost |
| +// edge caching (issue #403; tito PRO-4758/PRO-4759). |
| +func TestServeHTTPPrefersSMaxageOverMaxAge(t *testing.T) { |
| + cases := []struct { |
| + path string |
| + cc []string // Cache-Control, one entry per header line |
| + ttl time.Duration // 0 = must not be cached at all |
| + }{ |
| + {"/pair", []string{"s-maxage=120, max-age=0"}, 120 * time.Second}, |
| + {"/pair-reversed", []string{"max-age=0, s-maxage=120"}, 120 * time.Second}, |
| + {"/pair-split", []string{"s-maxage=120", "max-age=0"}, 120 * time.Second}, |
| + {"/pair-split-reversed", []string{"max-age=0", "s-maxage=120"}, 120 * time.Second}, |
| + {"/shared-only", []string{"s-maxage=120"}, 120 * time.Second}, |
| + {"/mixed-case", []string{"S-MaxAge=120, Max-Age=0"}, 120 * time.Second}, |
| + {"/spaced", []string{"s-maxage = 120 , max-age=0"}, 120 * time.Second}, |
| + {"/shared-shorter", []string{"s-maxage=30, max-age=600"}, 30 * time.Second}, |
| + {"/max-age-only", []string{"max-age=60"}, 60 * time.Second}, |
| + {"/clamped", []string{"s-maxage=31536000, max-age=0"}, DefaultCacheMaxTTL}, |
| + {"/shared-zero", []string{"s-maxage=0, max-age=600"}, 0}, |
| + {"/private", []string{"private, s-maxage=120"}, 0}, |
| + {"/no-store", []string{"no-store, s-maxage=120"}, 0}, |
| + {"/no-cache", []string{"s-maxage=120, no-cache"}, 0}, |
| + {"/malformed-shared", []string{"s-maxage=soon, max-age=600"}, 0}, |
| + {"/malformed-max-age", []string{"max-age=soon, s-maxage=120"}, 0}, |
| + } |
| + byPath := map[string][]string{} |
| + for _, c := range cases { |
| + byPath[c.path] = c.cc |
| + } |
| + e := cacheProbeEdge(t, "smaxage.example.com", func(path string, h http.Header) { |
| + for _, line := range byPath[path] { |
| + h.Add("Cache-Control", line) |
| + } |
| + }) |
| + for _, c := range cases { |
| + t.Run(c.path, func(t *testing.T) { |
| + e.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "http://smaxage.example.com"+c.path, nil)) |
| + resp, ok := e.cache.get(CacheKey{Host: "smaxage.example.com", Method: "GET", Path: c.path}) |
| + if c.ttl == 0 { |
| + if ok { |
| + t.Fatalf("Cache-Control %q was cached; it must be refused", c.cc) |
| + } |
| + return |
| + } |
| + if !ok { |
| + t.Fatalf("Cache-Control %q was not cached; the edge should hold it for %s", c.cc, c.ttl) |
| + } |
| + if resp.TTL != c.ttl { |
| + t.Fatalf("Cache-Control %q cached with TTL %s, want %s", c.cc, resp.TTL, c.ttl) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// The stored response is replayed to later visitors verbatim, so the browser |
| +// half of the pair has to survive the edge: an origin that said max-age=0 |
| +// still gets a revalidating browser on a cache HIT, not the edge's own idea of |
| +// the client lifetime. Nothing rewrites Cache-Control on the way downstream — |
| +// which is the point of reading s-maxage rather than inventing a header. |
| +func TestServeHTTPCacheHitReplaysOriginCacheControl(t *testing.T) { |
| + e := cacheProbeEdge(t, "replay.example.com", func(path string, h http.Header) { |
| + h.Set("Cache-Control", "s-maxage=120, max-age=0") |
| + }) |
| + get := func(label string) { |
| + t.Helper() |
| + rec := httptest.NewRecorder() |
| + e.ServeHTTP(rec, httptest.NewRequest("GET", "http://replay.example.com/page", nil)) |
| + if got := rec.Header().Get("Cache-Control"); got != "s-maxage=120, max-age=0" { |
| + t.Fatalf("%s: Cache-Control = %q, want the origin's own header", label, got) |
| + } |
| + } |
| + get("miss") |
| + if !cachedAt(e, "replay.example.com", "/page") { |
| + t.Fatal("s-maxage=120 should have been stored at the edge") |
| + } |
| + get("hit") |
| +} |
| + |
| func TestServeHTTPRefusesVaryCookie(t *testing.T) { |
| backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| w.Header().Set("Cache-Control", "max-age=60") |