fix: cache responses without public directive

Author: fiam

v2/pkg/caching/cachecontrol.go

@@ -17,14 +17,13 @@ func TTL(headers http.Header, defaultTTL time.Duration) (time.Duration, bool) {
 		return 0, false
 	}
 
-	// We for now consider no cache and private as dont cache even
+	// We currently treat no-cache and private as not reusable by this shared cache.
 	if cc.NoCache != nil || cc.Private != nil {
 		return 0, false
 	}
-	if !cc.Public {
-		return 0, false
-	}
 
+	// An explicit freshness lifetime takes precedence over the configured
+	// fallback TTL.
 	switch {
 	case cc.SMaxAge != nil:
 		if *cc.SMaxAge <= 0 {
@@ -39,6 +38,13 @@ func TTL(headers http.Header, defaultTTL time.Duration) (time.Duration, bool) {
 		return cc.MaxAge.AsDuration(), true
 	}
 
+	// Without an explicit freshness lifetime, a configured fallback applies only
+	// when the response supplied caching intent. A missing, empty, or
+	// extension-only Cache-Control header does not opt the response into caching.
+	if !cc.HasCachingDirectives() {
+		return 0, false
+	}
+
 	if defaultTTL <= 0 {
 		return 0, false
 	}

v2/pkg/caching/cachecontrol_test.go

@@ -16,32 +16,57 @@ func TestTTL(t *testing.T) {
 		wantTTL      time.Duration
 		wantOK       bool
 	}{
-		// public is the opt-in, and on its own it takes the configured default.
-		{name: "public alone", cacheControl: "public", defaultTTL: defaultTTL, wantTTL: defaultTTL, wantOK: true},
+		// A positive freshness lifetime is sufficient to store a response. Public
+		// permits storage in cases that would otherwise forbid it; it is not a
+		// general prerequisite for shared caching.
+		{name: "max-age without public", cacheControl: "max-age=60", defaultTTL: defaultTTL, wantTTL: time.Minute, wantOK: true},
+		{name: "s-maxage without public", cacheControl: "s-maxage=60", defaultTTL: defaultTTL, wantTTL: time.Minute, wantOK: true},
 		{name: "public with max-age", cacheControl: "public, max-age=60", defaultTTL: defaultTTL, wantTTL: time.Minute, wantOK: true},
 		{name: "public with s-maxage", cacheControl: "public, s-maxage=60", defaultTTL: defaultTTL, wantTTL: time.Minute, wantOK: true},
+		{name: "max-age does not need a usable default", cacheControl: "max-age=60", wantTTL: time.Minute, wantOK: true},
 
 		// s-maxage is the directive addressed to shared caches, so it outranks
 		// max-age here however the two are ordered in the header.
-		{name: "s-maxage outranks max-age", cacheControl: "public, s-maxage=60, max-age=3600", defaultTTL: defaultTTL, wantTTL: time.Minute, wantOK: true},
-		{name: "s-maxage outranks max-age reversed", cacheControl: "public, max-age=3600, s-maxage=60", defaultTTL: defaultTTL, wantTTL: time.Minute, wantOK: true},
+		{name: "s-maxage outranks max-age", cacheControl: "s-maxage=60, max-age=3600", defaultTTL: defaultTTL, wantTTL: time.Minute, wantOK: true},
+		{name: "s-maxage outranks max-age reversed", cacheControl: "max-age=3600, s-maxage=60", defaultTTL: defaultTTL, wantTTL: time.Minute, wantOK: true},
+		{name: "zero s-maxage overrides positive max-age", cacheControl: "max-age=60, s-maxage=0", defaultTTL: defaultTTL, wantOK: false},
+		{name: "positive s-maxage overrides zero max-age", cacheControl: "max-age=0, s-maxage=60", defaultTTL: defaultTTL, wantTTL: time.Minute, wantOK: true},
+
+		// A recognized caching directive without a freshness lifetime opts into
+		// the configured fallback. Public is one such directive, not a requirement.
+		{name: "public alone", cacheControl: "public", defaultTTL: defaultTTL, wantTTL: defaultTTL, wantOK: true},
+		{name: "must-revalidate without public", cacheControl: "must-revalidate", defaultTTL: defaultTTL, wantTTL: defaultTTL, wantOK: true},
+		{name: "proxy-revalidate without public", cacheControl: "proxy-revalidate", defaultTTL: defaultTTL, wantTTL: defaultTTL, wantOK: true},
+		{name: "bare stale-if-error without public", cacheControl: "stale-if-error", defaultTTL: defaultTTL, wantTTL: defaultTTL, wantOK: true},
+		{name: "stale-if-error without public", cacheControl: "stale-if-error=60", defaultTTL: defaultTTL, wantTTL: defaultTTL, wantOK: true},
+		{name: "stale-while-revalidate without public", cacheControl: "stale-while-revalidate=60", defaultTTL: defaultTTL, wantTTL: defaultTTL, wantOK: true},
+
+		// No header or a header without recognized caching intent does not use the
+		// configured fallback.
+		{name: "no cache-control header", defaultTTL: defaultTTL, wantOK: false},
+		{name: "extension directive without public", cacheControl: "cdn-cache-control=60", defaultTTL: defaultTTL, wantOK: false},
+		{name: "immutable without public", cacheControl: "immutable", defaultTTL: defaultTTL, wantOK: false},
+		{name: "no-transform without public", cacheControl: "no-transform", defaultTTL: defaultTTL, wantOK: false},
+		{name: "must-understand without public", cacheControl: "must-understand", defaultTTL: defaultTTL, wantOK: false},
 
-		// Without public the response is not offered to a shared cache, whatever
-		// lifetime it names.
-		{name: "max-age without public", cacheControl: "max-age=60", defaultTTL: defaultTTL, wantOK: false},
-		{name: "s-maxage without public", cacheControl: "s-maxage=60", defaultTTL: defaultTTL, wantOK: false},
-		{name: "no cache-control header", cacheControl: "", defaultTTL: defaultTTL, wantOK: false},
+		// Explicit refusals win whether public is present or not.
+		{name: "no-store", cacheControl: "max-age=60, no-store", defaultTTL: defaultTTL, wantOK: false},
+		{name: "no-cache", cacheControl: "max-age=60, no-cache", defaultTTL: defaultTTL, wantOK: false},
+		{name: "field-specific no-cache", cacheControl: `max-age=60, no-cache="Set-Cookie"`, defaultTTL: defaultTTL, wantOK: false},
+		{name: "private", cacheControl: "max-age=60, private", defaultTTL: defaultTTL, wantOK: false},
+		{name: "private wins over public", cacheControl: "public, max-age=60, private", defaultTTL: defaultTTL, wantOK: false},
 
-		// Explicit refusals win over public.
-		{name: "no-store", cacheControl: "public, no-store", defaultTTL: defaultTTL, wantOK: false},
-		{name: "no-cache", cacheControl: "public, no-cache", defaultTTL: defaultTTL, wantOK: false},
-		{name: "private", cacheControl: "public, private", defaultTTL: defaultTTL, wantOK: false},
+		// A lifetime of zero is a refusal, and malformed lifetimes cannot opt a
+		// response into caching.
+		{name: "zero max-age", cacheControl: "max-age=0", defaultTTL: defaultTTL, wantOK: false},
+		{name: "zero s-maxage", cacheControl: "s-maxage=0", defaultTTL: defaultTTL, wantOK: false},
+		{name: "public does not override zero max-age", cacheControl: "public, max-age=0", defaultTTL: defaultTTL, wantOK: false},
+		{name: "invalid max-age", cacheControl: "max-age=abc", defaultTTL: defaultTTL, wantOK: false},
+		{name: "invalid stale-while-revalidate", cacheControl: "stale-while-revalidate=abc", defaultTTL: defaultTTL, wantOK: false},
 
-		// A lifetime of zero is a refusal, and so is a default that was never
-		// usable in the first place.
-		{name: "zero max-age", cacheControl: "public, max-age=0", defaultTTL: defaultTTL, wantOK: false},
-		{name: "zero s-maxage", cacheControl: "public, s-maxage=0", defaultTTL: defaultTTL, wantOK: false},
-		{name: "public with non-positive default", cacheControl: "public", defaultTTL: 0, wantOK: false},
+		// The fallback must itself be usable.
+		{name: "caching intent with zero default", cacheControl: "must-revalidate", wantOK: false},
+		{name: "caching intent with negative default", cacheControl: "must-revalidate", defaultTTL: -time.Second, wantOK: false},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			headers := http.Header{}

v2/pkg/engine/cache/cache_control.go

@@ -226,6 +226,19 @@ type CacheControlResponse struct {
 	// store the specified field-names(s), whereas it MAY store the
 	// remainder of the response message.
 	Private *FieldNames
+
+	// hasAdditionalCachingDirective records recognized response caching
+	// directives whose behavior is not otherwise modeled by this type. It lets
+	// callers distinguish a response that supplied caching intent from a missing,
+	// empty, or extension-only Cache-Control header.
+	hasAdditionalCachingDirective bool
+}
+
+// HasCachingDirectives reports whether the response contained a recognized
+// directive that affects caching.
+func (c *CacheControlResponse) HasCachingDirectives() bool {
+	return c.MaxAge != nil || c.SMaxAge != nil || c.NoStore || c.NoCache != nil ||
+		c.Public || c.Private != nil || c.hasAdditionalCachingDirective
 }
 
 // ToHeaderString converts a CacheControlResponse to a Cache-Control header string.
@@ -412,6 +425,25 @@ func parseIdent(name token, l *lexer, cc *CacheControlResponse) error {
 		}
 		fieldNamesArgument(arg, cc.Private)
 
+	case "must-revalidate", "proxy-revalidate":
+		cc.hasAdditionalCachingDirective = true
+
+	case "stale-if-error":
+		// Retain compatibility with senders that use the bare form. With a value,
+		// only a valid delta-seconds carries caching intent.
+		if !arg.present {
+			cc.hasAdditionalCachingDirective = true
+			break
+		}
+		if _, err := deltaSecondsArgument("stale-if-error", arg); err == nil {
+			cc.hasAdditionalCachingDirective = true
+		}
+
+	case "stale-while-revalidate":
+		if _, err := deltaSecondsArgument("stale-while-revalidate", arg); err == nil {
+			cc.hasAdditionalCachingDirective = true
+		}
+
 	default:
 		// We ignore directives that we don't specify here.
 	}

v2/pkg/engine/cache/cache_control_test.go

@@ -37,21 +37,25 @@ import (
 //
 //	D1  "No directives" is never an error, however it arises. An empty field, a
 //	    whitespace-only field, a field of nothing but commas, and a field of
-//	    nothing but directives we do not model all parse to a non-nil, zero
+//	    nothing but unknown extension directives all parse to a non-nil, zero
 //	    CacheControlResponse. ParseCacheControlResponse never returns nil on
-//	    success either, so callers never have to nil-check.
+//	    success either, so callers never have to nil-check. Standard caching
+//	    directives that are not otherwise modeled can still contribute to
+//	    HasCachingDirectives — see D3.
 //
 //	    This was three inconsistent rules before: "" and "   " were errors,
 //	    "," was an error, but `must-revalidate` produced a non-nil zero value —
 //	    three inputs carrying zero directives, three different outcomes. Under
 //	    D0 none of them is a syntax problem, so none of them errors. See D13.
 //	D2  Directive names are case-insensitive (RFC 9110 tokens).
-//	D3  Directives that the CacheControlResponse struct does not model
-//	    (must-revalidate, no-transform, proxy-revalidate, immutable,
-//	    stale-while-revalidate, arbitrary cache extensions) are ignored, not
-//	    rejected. RFC 9111 §5.2: unknown directives MUST be ignored. A header
-//	    consisting only of such directives still parses to a non-nil, zero
-//	    CacheControlResponse.
+//	D3  Directives that the CacheControlResponse struct does not otherwise model
+//	    are not rejected. Recognized directives that carry caching intent
+//	    (must-revalidate, proxy-revalidate, stale-if-error and
+//	    stale-while-revalidate) contribute to HasCachingDirectives so a caller
+//	    can decide whether to apply a configured freshness lifetime. Directives
+//	    with no storage or freshness effect here (no-transform, immutable and
+//	    must-understand) and arbitrary extensions are ignored. RFC 9111 §5.2:
+//	    unknown directives MUST be ignored.
 //	D4  A boolean directive (no-store, public) that carries an argument keeps
 //	    its meaning; the stray argument is discarded. `no-store=true` still
 //	    sets NoStore, since the argument tells us nothing the directive name
@@ -245,14 +249,14 @@ func TestParse(t *testing.T) {
 		requireParses(t, "private", &CacheControlResponse{Private: fieldNames()})
 	})
 
-	// --- D3: unmodelled directives are ignored ------------------------------
+	// --- D3: directives without modeled fields -------------------------------
 
-	t.Run("must-revalidate alone is ignored but still parses", func(t *testing.T) {
+	t.Run("must-revalidate has no modeled field", func(t *testing.T) {
 		t.Parallel()
 		requireParses(t, "must-revalidate", &CacheControlResponse{})
 	})
 
-	t.Run("stale-while-revalidate is ignored", func(t *testing.T) {
+	t.Run("stale-while-revalidate has no modeled field", func(t *testing.T) {
 		t.Parallel()
 		requireParses(t, "max-age=60, stale-while-revalidate=30", &CacheControlResponse{MaxAge: seconds(60)})
 	})
@@ -982,10 +986,8 @@ func TestParse(t *testing.T) {
 func TestParseCacheControlResponse(t *testing.T) {
 	t.Parallel()
 
-	// An absent header and a header carrying no directives both yield an empty
-	// response rather than nil, so callers never have to nil-check. Nothing is
-	// lost: every field is already optional, so "absent" and "present but
-	// modelled nothing" are the same answer to every question a caller asks.
+	// An absent header and a header carrying no recognized directives both yield
+	// an empty response rather than nil, so callers never have to nil-check.
 
 	t.Run("nil headers", func(t *testing.T) {
 		t.Parallel()
@@ -1002,14 +1004,14 @@ func TestParseCacheControlResponse(t *testing.T) {
 		requireHeaderParses(t, http.Header{"Cache-Control": []string{""}}, &CacheControlResponse{})
 	})
 
-	// D1 + D3 — a field we understood but modelled none of still parsed, so it
-	// is not the "absent" case and must not be nil.
-	t.Run("Cache-Control with only unmodelled directives is non-nil", func(t *testing.T) {
+	// D1 + D3 — a recognized caching directive can carry intent even when its
+	// individual behavior is not otherwise modeled.
+	t.Run("Cache-Control with an additional caching directive retains intent", func(t *testing.T) {
 		t.Parallel()
-		requireHeaderParses(t,
-			http.Header{"Cache-Control": []string{"must-revalidate"}},
-			&CacheControlResponse{},
-		)
+
+		got, err := ParseCacheControlResponse(http.Header{"Cache-Control": []string{"must-revalidate"}})
+		require.NoError(t, err)
+		require.True(t, got.HasCachingDirectives())
 	})
 
 	t.Run("valid Cache-Control", func(t *testing.T) {

v2/pkg/engine/resolve/response_cache_test.go

@@ -406,7 +406,8 @@ func TestResponseCacheCollectTags(t *testing.T) {
 			statusCode: http.StatusOK,
 			httpResponseContext: &httpclient.ResponseContext{
 				Response: &http.Response{
-					Header: http.Header{"Cache-Control": []string{"public, max-age=60"}},
+					// An explicit freshness lifetime is cacheable without public.
+					Header: http.Header{"Cache-Control": []string{"max-age=60"}},
 				},
 			},
 		}
@@ -500,7 +501,7 @@ func TestResponseCacheCollectTags(t *testing.T) {
 		require.Equal(t, []string{"declared:accounts:user-7"}, items[1].Tags)
 	})
 
-	t.Run("a response with no tags is cached exactly as it was before", func(t *testing.T) {
+	t.Run("max-age without public is cached when the response has no tags", func(t *testing.T) {
 		body := `{"data": {"_entities": [{"id": 42}]}}`
 		items := collect(t, body, []string{"k-42"}, declaredOnly)