shelley: fix ETag caching: remove immutable, support weak validators and multiple ETags

Philip Zeyliger created 4 months ago

Prompt: can you reset to origin/main and double-check that etag caching
is working correctly. I notice the etags have quotes around them in the
browser console, which is a bit weird. I cherry-picked a change and
rebuilt recently, and it didn't get picked up right away until i cleared
cache

The ETag-based caching wasn't working correctly because:

1. The Cache-Control header had 'immutable' which tells browsers to NEVER
   revalidate during the max-age period, even if ETags are present.
   Changed to 'max-age=0, must-revalidate' to force ETag checking.

2. The If-None-Match comparison was too strict - it only checked exact
   string match. Per RFC 7232:
   - Weak validators (W/"...") should match strong validators
   - If-None-Match can contain multiple comma-separated ETags
   - Any matching ETag should trigger a 304 Not Modified

Added etagMatches() helper function with comprehensive tests to handle
all these cases correctly.

Note: The quotes around ETags in the browser console are correct per HTTP
spec - ETags are required to be quoted strings.

Change summary

server/handlers.go      | 34 ++++++++++++++++++++++++++++++--
server/handlers_test.go | 44 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+), 3 deletions(-)

Detailed changes

server/handlers.go

@@ -193,6 +193,33 @@ func acceptsGzip(r *http.Request) bool {
 	return strings.Contains(r.Header.Get("Accept-Encoding"), "gzip")
 }
 
+// etagMatches checks if the client's If-None-Match header matches the given ETag.
+// Per RFC 7232, If-None-Match can contain multiple ETags (comma-separated)
+// and may use weak validators (W/"..."). For GET/HEAD, weak comparison is used.
+func etagMatches(ifNoneMatch, etag string) bool {
+	if ifNoneMatch == "" {
+		return false
+	}
+	// Normalize our ETag by stripping W/ prefix if present
+	normEtag := strings.TrimPrefix(etag, `W/`)
+
+	// If-None-Match can be "*" which matches any
+	if ifNoneMatch == "*" {
+		return true
+	}
+
+	// Split by comma and check each tag
+	for _, tag := range strings.Split(ifNoneMatch, ",") {
+		tag = strings.TrimSpace(tag)
+		// Strip W/ prefix for weak comparison
+		tag = strings.TrimPrefix(tag, `W/`)
+		if tag == normEtag {
+			return true
+		}
+	}
+	return false
+}
+
 func (s *Server) staticHandler(fsys http.FileSystem) http.Handler {
 	fileServer := http.FileServer(fsys)
 
@@ -235,7 +262,7 @@ func (s *Server) staticHandler(fsys http.FileSystem) http.Handler {
 				if hash, ok := checksums[filename]; ok {
 					etag := `"` + hash + `"`
 					w.Header().Set("ETag", etag)
-					if r.Header.Get("If-None-Match") == etag {
+					if etagMatches(r.Header.Get("If-None-Match"), etag) {
 						w.WriteHeader(http.StatusNotModified)
 						return
 					}
@@ -244,8 +271,9 @@ func (s *Server) staticHandler(fsys http.FileSystem) http.Handler {
 
 			w.Header().Set("Content-Type", mime.TypeByExtension(filepath.Ext(r.URL.Path)))
 			w.Header().Set("Vary", "Accept-Encoding")
-			// Cache for 1 year - ETag ensures revalidation works
-			w.Header().Set("Cache-Control", "public, max-age=31536000, immutable")
+			// Use must-revalidate so browsers check ETag on each request.
+			// We can't use immutable since we don't have content-hashed filenames.
+			w.Header().Set("Cache-Control", "public, max-age=0, must-revalidate")
 
 			if acceptsGzip(r) {
 				// Client accepts gzip - serve compressed directly

server/handlers_test.go

@@ -0,0 +1,44 @@
+package server
+
+import (
+	"testing"
+)
+
+func TestEtagMatches(t *testing.T) {
+	tests := []struct {
+		name        string
+		ifNoneMatch string
+		etag        string
+		want        bool
+	}{
+		// Basic matching
+		{"exact match", `"abc123"`, `"abc123"`, true},
+		{"no match", `"abc123"`, `"xyz789"`, false},
+		{"empty if-none-match", "", `"abc123"`, false},
+
+		// Weak validators (W/ prefix)
+		{"weak validator match", `W/"abc123"`, `"abc123"`, true},
+		{"weak etag match", `"abc123"`, `W/"abc123"`, true},
+		{"both weak match", `W/"abc123"`, `W/"abc123"`, true},
+		{"weak no match", `W/"abc123"`, `"xyz789"`, false},
+
+		// Multiple ETags
+		{"multiple first", `"abc123", "def456"`, `"abc123"`, true},
+		{"multiple second", `"abc123", "def456"`, `"def456"`, true},
+		{"multiple none", `"abc123", "def456"`, `"xyz789"`, false},
+		{"multiple with spaces", `"a" , "b" , "c"`, `"b"`, true},
+		{"multiple with weak", `"a", W/"b", "c"`, `"b"`, true},
+
+		// Wildcard
+		{"wildcard", "*", `"anything"`, true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := etagMatches(tt.ifNoneMatch, tt.etag)
+			if got != tt.want {
+				t.Errorf("etagMatches(%q, %q) = %v, want %v", tt.ifNoneMatch, tt.etag, got, tt.want)
+			}
+		})
+	}
+}