Answer a revisit 304 whether its ETag comes back weak or strong

service/internal/boards/api.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"github.com/OpenIPC/website/service/internal/httpx"
 	"github.com/OpenIPC/website/service/internal/vendorfw"
 	"log/slog"
 	"net/http"
@@ -775,7 +776,7 @@ func (a *API) serve(w http.ResponseWriter, r *http.Request, maxAge int, load fun
 	}
 	sum := sha256.Sum256([]byte(fmt.Sprintf("%d|%d|%d|%s|%s", units, files, last.UnixNano(), about, r.URL.RequestURI())))
 	etag := `"` + hex.EncodeToString(sum[:12]) + `"`
-	if r.Header.Get("If-None-Match") == etag {
+	if httpx.Revisited(r, etag) {
 		w.Header().Set("ETag", etag)
 		w.Header().Set("Cache-Control", fmt.Sprintf("public, max-age=%d", maxAge))
 		w.WriteHeader(http.StatusNotModified)

service/internal/builds/explorer.go

@@ -13,6 +13,8 @@ import (
 
 	"github.com/jackc/pgx/v5"
 	"github.com/jackc/pgx/v5/pgxpool"
+
+	"github.com/OpenIPC/website/service/internal/httpx"
 )
 
 // Explorer is the firmware explorer's read API, answered from the builds
@@ -73,7 +75,7 @@ func (e *Explorer) serve(w http.ResponseWriter, r *http.Request, src string, loa
 	h := w.Header()
 	h.Set("Cache-Control", "public, max-age=300")
 	h.Set("ETag", etag)
-	if r.Header.Get("If-None-Match") == etag {
+	if httpx.Revisited(r, etag) {
 		w.WriteHeader(http.StatusNotModified)
 		return
 	}

service/internal/httpx/etag_test.go

@@ -0,0 +1,72 @@
+package httpx
+
+import (
+	"net/http"
+	"testing"
+)
+
+// A revisit must be recognised however the tag came back. The case that
+// failed in production: nginx's gzip weakened the service's strong ETag, the
+// client returned it weak, and an exact comparison answered 200 every time.
+func TestARevisitIsRecognisedWeakOrStrong(t *testing.T) {
+	strong := `"13c177f64ce8057e992861fb"`
+	for _, c := range []struct {
+		header string
+		want   bool
+	}{
+		{strong, true},
+		{`W/"13c177f64ce8057e992861fb"`, true}, // what came back through nginx
+		{`"other", W/"13c177f64ce8057e992861fb"`, true},
+		{` "other" ,  "13c177f64ce8057e992861fb" `, true},
+		{`, "13c177f64ce8057e992861fb",`, true}, // empty list elements are allowed
+		{`"13c177f64ce8057e992861fc"`, false},
+		{`W/"13c177f64ce8057e"`, false},
+		{``, false},
+		{`,`, false},
+		{`13c177f64ce8057e992861fb`, false},  // not an entity-tag
+		{`"13c177f64ce8057e992861fb`, false}, // unterminated
+		{`"a,b"`, false},                     // a comma inside a tag is part of it
+		// Malformed values match nothing, even when they hold the tag.
+		{`"13c177f64ce8057e992861fb"junk`, false},
+		{`"13c177f64ce8057e992861fb" "other"`, false},
+		{`"13c177f64ce8057e992861fb", junk`, false},
+		// "*" is decided before a handler has looked the resource up, so it
+		// would answer 304 for one that does not exist: it matches nothing.
+		{`*`, false},
+		{`*junk`, false},
+		{`*, "13c177f64ce8057e992861fb"`, false},
+	} {
+		if got := ETagMatches(c.header, strong); got != c.want {
+			t.Errorf("ETagMatches(%q) = %v, want %v", c.header, got, c.want)
+		}
+	}
+	// A weak ETag of our own matches its strong spelling too.
+	if !ETagMatches(`"abc"`, `W/"abc"`) || !ETagMatches(`W/"abc"`, `W/"abc"`) {
+		t.Error("a weak ETag of ours did not match")
+	}
+	if ETagMatches(`"abc"`, ``) || ETagMatches(`"abc"`, `abc`) {
+		t.Error("an ETag that is not one matched something")
+	}
+}
+
+// Every If-None-Match field counts, not only the first.
+func TestEveryIfNoneMatchFieldIsRead(t *testing.T) {
+	r, _ := http.NewRequest(http.MethodGet, "/", nil)
+	r.Header.Add("If-None-Match", `"old"`)
+	r.Header.Add("If-None-Match", `W/"current"`)
+	if !Revisited(r, `"current"`) {
+		t.Error("a tag in the second field was not recognised")
+	}
+	if Revisited(r, `"other"`) {
+		t.Error("a tag in no field matched")
+	}
+	empty, _ := http.NewRequest(http.MethodGet, "/", nil)
+	empty.Header.Add("If-None-Match", ``)
+	empty.Header.Add("If-None-Match", `"current"`)
+	if !Revisited(empty, `"current"`) {
+		t.Error("an empty first field hid the second")
+	}
+	if Revisited(&http.Request{Header: http.Header{}}, `"current"`) {
+		t.Error("no header matched")
+	}
+}

service/internal/httpx/httpx.go

@@ -162,6 +162,65 @@ func Log(log *slog.Logger, next http.Handler) http.Handler {
 	})
 }
 
+// ETagMatches reports whether an If-None-Match value names etag, by the weak
+// comparison RFC 9110 (13.1.2) prescribes for it: a W/ prefix is ignored on
+// either side, and the value may list several tags.
+//
+// The weak half is not a nicety. nginx's gzip turns a strong ETag into a weak
+// one on its way out (W/"..."), and the browser sends back what it was given,
+// so a handler comparing the header with its own ETag as strings answered
+// every revisit through nginx with the whole body -- while the same request
+// straight to the service got its 304, which is why nothing failed until a
+// check went through the front door.
+//
+// Strict where it can afford to be, because the cost of saying no is only a
+// full response: a value that is not a well-formed list of entity-tags matches
+// nothing, and "*" matches nothing either -- every handler here decides 304
+// before it has looked the resource up, so "*" would answer 304 for one that
+// does not exist, and no browser sends it on a GET.
+func ETagMatches(header, etag string) bool {
+	want := strings.TrimPrefix(etag, "W/")
+	if len(want) < 2 || want[0] != '"' || want[len(want)-1] != '"' {
+		return false
+	}
+	matched := false
+	s := strings.TrimLeft(strings.TrimSpace(header), " \t,")
+	for first := true; ; first = false {
+		s = strings.TrimLeft(s, " \t")
+		if s == "" {
+			return matched && !first
+		}
+		if !first {
+			// Tags are separated by commas; empty list elements are allowed.
+			if s[0] != ',' {
+				return false
+			}
+			s = strings.TrimLeft(s[1:], " \t,")
+			if s == "" {
+				return matched
+			}
+		}
+		s = strings.TrimPrefix(s, "W/")
+		if s == "" || s[0] != '"' {
+			return false // "*", or not an entity-tag at all
+		}
+		end := strings.IndexByte(s[1:], '"')
+		if end < 0 {
+			return false
+		}
+		if s[:end+2] == want {
+			matched = true
+		}
+		s = s[end+2:]
+	}
+}
+
+// Revisited reports whether the request already holds etag: every
+// If-None-Match field it carries, not only the first, read as one list.
+func Revisited(r *http.Request, etag string) bool {
+	return ETagMatches(strings.Join(r.Header.Values("If-None-Match"), ","), etag)
+}
+
 // WriteJSON sends a body with conditional-GET behaviour: a weak ETag
 // over the bytes, and 304 when the client already has them.
 func WriteJSON(w http.ResponseWriter, r *http.Request, body []byte) {
@@ -170,7 +229,7 @@ func WriteJSON(w http.ResponseWriter, r *http.Request, body []byte) {
 	h := w.Header()
 	h.Set("Content-Type", "application/json; charset=utf-8")
 	h.Set("Etag", etag)
-	if match := r.Header.Get("If-None-Match"); match != "" && match == etag {
+	if Revisited(r, etag) {
 		w.WriteHeader(http.StatusNotModified)
 		return
 	}

service/internal/wizard/handler.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/OpenIPC/website/service/internal/catalogue"
 	"github.com/OpenIPC/website/service/internal/firmware"
+	"github.com/OpenIPC/website/service/internal/httpx"
 )
 
 // Handler is GET /api/v1/wizard/{soc}.json: one SoC's installation data,
@@ -67,7 +68,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	hd.Set("Content-Type", "application/json")
 	hd.Set("Cache-Control", "public, max-age=300")
 	hd.Set("ETag", c.etag)
-	if r.Header.Get("If-None-Match") == c.etag {
+	if httpx.Revisited(r, c.etag) {
 		w.WriteHeader(http.StatusNotModified)
 		return
 	}