fix: cache responses without public directive

Author: fiam

v2/pkg/caching/cachecontrol.go

@@ -17,14 +17,14 @@ 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 makes a response cacheable without the
+	// public directive. Public is only needed below when the response relies on
+	// the configured fallback TTL.
 	switch {
 	case cc.SMaxAge != nil:
 		if *cc.SMaxAge <= 0 {
@@ -39,6 +39,10 @@ func TTL(headers http.Header, defaultTTL time.Duration) (time.Duration, bool) {
 		return cc.MaxAge.AsDuration(), true
 	}
 
+	if !cc.Public {
+		return 0, false
+	}
+
 	if defaultTTL <= 0 {
 		return 0, false
 	}

v2/pkg/caching/cachecontrol_test.go

@@ -16,32 +16,47 @@ 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},
+
+		// Public without a freshness lifetime opts into the configured fallback.
+		// No header or a header without storage intent does not.
+		{name: "public alone", cacheControl: "public", defaultTTL: defaultTTL, wantTTL: defaultTTL, wantOK: true},
+		{name: "public with another directive", cacheControl: "public, must-revalidate", defaultTTL: defaultTTL, wantTTL: defaultTTL, wantOK: true},
+		{name: "no cache-control header", defaultTTL: defaultTTL, wantOK: false},
+		{name: "unrelated directive without public", cacheControl: "must-revalidate", defaultTTL: defaultTTL, wantOK: false},
+		{name: "extension directive without public", cacheControl: "cdn-cache-control=60", 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},
 
-		// 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: "public with zero default", cacheControl: "public", wantOK: false},
+		{name: "public with negative default", cacheControl: "public", defaultTTL: -time.Second, wantOK: false},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			headers := http.Header{}

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)