encode: ignore request no-transform, vary identity responses on Accept-Encoding (#8301)

Co-authored-by: elee1766 <a@tuxpa.in>

modules/caddyhttp/encode/encode.go

@@ -170,32 +170,35 @@ func hasNonEmptyValue(h http.Header, key string) bool {
 }
 
 func (enc *Encode) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
-	if !httpguts.HeaderValuesContainsToken(r.Header["Cache-Control"], "no-transform") {
-		for _, encName := range AcceptedEncodings(r, enc.Prefer) {
-			if _, ok := enc.writerPools[encName]; !ok {
-				continue // encoding not offered
-			}
-			w = enc.openResponseWriter(encName, w, r.Method == http.MethodConnect)
-			defer w.(*responseWriter).Close()
-
-			// to comply with RFC 9110 section 8.8.3(.3), we modify the Etag when encoding
-			// by appending a hyphen and the encoder name; the problem is, the client will
-			// send back that Etag in an If-None-Match header, but upstream handlers that set
-			// the Etag in the first place don't know that we appended to their Etag! so here
-			// we have to strip our addition so the upstream handlers can still honor client
-			// caches without knowing about our changes...
-			if etags := r.Header["If-None-Match"]; len(etags) > 0 {
-				etag := strings.Join(etags, ", ")
-				stripped := strings.ReplaceAll(etag, "-"+encName+`"`, `"`)
-				if stripped != etag || len(etags) > 1 {
-					r.Header["If-None-Match"] = []string{stripped}
-				}
-			}
-
-			break
+	var encodingName string
+	for _, encName := range AcceptedEncodings(r, enc.Prefer) {
+		if _, ok := enc.writerPools[encName]; !ok {
+			continue // encoding not offered
 		}
+		encodingName = encName
+
+		// to comply with RFC 9110 section 8.8.3(.3), we modify the Etag when encoding
+		// by appending a hyphen and the encoder name; the problem is, the client will
+		// send back that Etag in an If-None-Match header, but upstream handlers that set
+		// the Etag in the first place don't know that we appended to their Etag! so here
+		// we have to strip our addition so the upstream handlers can still honor client
+		// caches without knowing about our changes...
+		if etags := r.Header["If-None-Match"]; len(etags) > 0 {
+			etag := strings.Join(etags, ", ")
+			stripped := strings.ReplaceAll(etag, "-"+encName+`"`, `"`)
+			if stripped != etag || len(etags) > 1 {
+				r.Header["If-None-Match"] = []string{stripped}
+			}
+		}
+
+		break
 	}
 
+	// wrap even when no encoding was negotiated so eligible identity responses
+	// still carry Vary: Accept-Encoding for shared caches
+	w = enc.openResponseWriter(encodingName, w, r.Method == http.MethodConnect)
+	defer w.(*responseWriter).Close()
+
 	err := next.ServeHTTP(w, r)
 	// If there was an error, disable encoding completely
 	// This prevents corruption when handle_errors processes the response
@@ -255,7 +258,7 @@ func (enc *Encode) initResponseWriter(rw *responseWriter, encodingName string, w
 // configured by config.
 type responseWriter struct {
 	http.ResponseWriter
-	encodingName string
+	encodingName string // empty when the client accepts no offered encoding
 	w            Encoder
 	config       *Encode
 	statusCode   int
@@ -534,16 +537,20 @@ func (rw *responseWriter) init() {
 
 	hdr := rw.Header()
 
+	// Only the response no-transform binds us; the request one addresses intermediaries.
 	if !hasNonEmptyValue(hdr, "Content-Encoding") &&
 		!httpguts.HeaderValuesContainsToken(hdr["Cache-Control"], "no-transform") &&
 		rw.config.Match(rw) {
+		if !hasVaryValue(hdr, "Accept-Encoding") {
+			hdr["Vary"] = append(hdr["Vary"], "Accept-Encoding")
+		}
+		if rw.encodingName == "" {
+			return
+		}
 		rw.w = rw.config.writerPools[rw.encodingName].Get().(Encoder)
 		rw.w.Reset(rw.ResponseWriter)
 		delete(hdr, "Content-Length") // https://github.com/golang/go/issues/14975
 		hdr["Content-Encoding"] = []string{rw.encodingName}
-		if !hasVaryValue(hdr, "Accept-Encoding") {
-			hdr["Vary"] = append(hdr["Vary"], "Accept-Encoding")
-		}
 		delete(hdr, "Accept-Ranges") // we don't know ranges for dynamically-encoded content
 
 		// strong ETags need to be distinct depending on the encoding ("selected representation")

modules/caddyhttp/encode/encode_conformance_test.go

@@ -234,7 +234,7 @@ var encodeScenarios = []encodeScenario{
 		checkResponse: checkStripsEncodedETagSuffix,
 	},
 	{
-		name:      "request cache-control no-transform prevents encoding",
+		name:      "request cache-control no-transform does not prevent encoding",
 		method:    http.MethodGet,
 		minLength: 1,
 		reqHeaders: func(encoderCase) http.Header {
@@ -243,7 +243,7 @@ var encodeScenarios = []encodeScenario{
 		next: func(encoderCase) caddyhttp.Handler {
 			return conformanceLargeBodyHandler(conformanceContentType)
 		},
-		checkResponse: checkBypassesEncoding,
+		checkResponse: checkEncodesConformanceBody,
 	},
 	{
 		name:      "response cache-control no-transform prevents encoding",
@@ -259,18 +259,6 @@ var encodeScenarios = []encodeScenario{
 		},
 		checkResponse: checkBypassesEncoding,
 	},
-	{
-		name:      "request cache-control no-transform on a later line prevents encoding",
-		method:    http.MethodGet,
-		minLength: 1,
-		reqHeaders: func(encoderCase) http.Header {
-			return http.Header{"Cache-Control": {"no-cache", "no-transform"}}
-		},
-		next: func(encoderCase) caddyhttp.Handler {
-			return conformanceLargeBodyHandler(conformanceContentType)
-		},
-		checkResponse: checkBypassesEncoding,
-	},
 	{
 		name:      "response cache-control no-transform on a later line prevents encoding",
 		method:    http.MethodGet,
@@ -308,6 +296,42 @@ var encodeScenarios = []encodeScenario{
 		},
 		checkResponse: checkBypassesEncoding,
 	},
+	{
+		name:      "missing accept-encoding keeps identity and varies",
+		method:    http.MethodGet,
+		minLength: 1,
+		reqHeaders: func(encoderCase) http.Header {
+			return http.Header{"Accept-Encoding": nil}
+		},
+		next: func(encoderCase) caddyhttp.Handler {
+			return conformanceLargeBodyHandler(conformanceContentType)
+		},
+		checkResponse: checkIdentityVaries,
+	},
+	{
+		name:      "identity accept-encoding keeps identity and varies",
+		method:    http.MethodGet,
+		minLength: 1,
+		reqHeaders: func(encoderCase) http.Header {
+			return http.Header{"Accept-Encoding": {"identity"}}
+		},
+		next: func(encoderCase) caddyhttp.Handler {
+			return conformanceLargeBodyHandler(conformanceContentType)
+		},
+		checkResponse: checkIdentityVaries,
+	},
+	{
+		name:      "missing accept-encoding with content type matcher rejection does not vary",
+		method:    http.MethodGet,
+		minLength: 1,
+		reqHeaders: func(encoderCase) http.Header {
+			return http.Header{"Accept-Encoding": nil}
+		},
+		next: func(encoderCase) caddyhttp.Handler {
+			return conformanceLargeBodyHandler("image/png")
+		},
+		checkResponse: checkBypassesEncoding,
+	},
 }
 
 // TestEncodeResponseSemantics verifies HTTP edge cases (304, HEAD, range, WebSocket,
@@ -486,6 +510,16 @@ func conformanceLargeBodyHandler(contentType string) caddyhttp.Handler {
 	})
 }
 
+func checkEncodesConformanceBody(t *testing.T, w *httptest.ResponseRecorder, encCase encoderCase) {
+	t.Helper()
+
+	encName := encCase.encoding.AcceptEncoding()
+	if got := w.Header().Get("Content-Encoding"); got != encName {
+		t.Fatalf("Content-Encoding = %q, want %q", got, encName)
+	}
+	assertDecompresses(t, encCase, w.Body.Bytes(), conformanceLargeBody())
+}
+
 func checkBypassesEncoding(t *testing.T, w *httptest.ResponseRecorder, encCase encoderCase) {
 	t.Helper()
 
@@ -496,6 +530,24 @@ func checkBypassesEncoding(t *testing.T, w *httptest.ResponseRecorder, encCase e
 	if !bytes.Equal(w.Body.Bytes(), want) {
 		t.Fatalf("body len = %d, want len = %d", w.Body.Len(), len(want))
 	}
+	if encode.HasVaryValue(w.Header(), "Accept-Encoding") {
+		t.Fatalf("Vary = %q, want no Accept-Encoding", w.Header().Values("Vary"))
+	}
+}
+
+func checkIdentityVaries(t *testing.T, w *httptest.ResponseRecorder, encCase encoderCase) {
+	t.Helper()
+
+	want := conformanceLargeBody()
+	if got := w.Header().Get("Content-Encoding"); got != "" {
+		t.Fatalf("Content-Encoding = %q, want empty", got)
+	}
+	if !bytes.Equal(w.Body.Bytes(), want) {
+		t.Fatalf("body len = %d, want len = %d", w.Body.Len(), len(want))
+	}
+	if !encode.HasVaryValue(w.Header(), "Accept-Encoding") {
+		t.Fatalf("Vary = %q, want Accept-Encoding", w.Header().Values("Vary"))
+	}
 }
 
 func checkKeepsUpstreamContentEncoding(t *testing.T, w *httptest.ResponseRecorder, encCase encoderCase) {