From 93a8fddf0ebc6db9dba32d078a0a0d021c4bc76d Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:01:43 +0300 Subject: [PATCH 1/2] Answer a revisit 304 whether its ETag comes back weak or strong nginx's gzip turns the service's strong ETags weak on the way out (W/"..."), and the browser sends back what it was given. The boards tree, the wizard, the builds explorer and httpx.WriteJSON all compared If-None-Match with their ETag as strings, so through the front door every revisit got the whole body again -- while the same request straight to the service got its 304, which is why only the conformance suite against openipc.org noticed (TestTheBoardTreeIsJSONRevalidatedByETagAndSetsNothing). httpx.ETagMatches does the weak comparison RFC 9110 prescribes for If-None-Match -- W/ ignored on both sides, a list of tags, or * -- and the four handlers use it. --- service/internal/boards/api.go | 3 ++- service/internal/builds/explorer.go | 4 ++- service/internal/httpx/etag_test.go | 37 ++++++++++++++++++++++++++ service/internal/httpx/httpx.go | 41 ++++++++++++++++++++++++++++- service/internal/wizard/handler.go | 3 ++- 5 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 service/internal/httpx/etag_test.go diff --git a/service/internal/boards/api.go b/service/internal/boards/api.go index dbe0f124..9e121c2f 100644 --- a/service/internal/boards/api.go +++ b/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.ETagMatches(r.Header.Get("If-None-Match"), etag) { w.Header().Set("ETag", etag) w.Header().Set("Cache-Control", fmt.Sprintf("public, max-age=%d", maxAge)) w.WriteHeader(http.StatusNotModified) diff --git a/service/internal/builds/explorer.go b/service/internal/builds/explorer.go index 32bd36f6..635730ef 100644 --- a/service/internal/builds/explorer.go +++ b/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.ETagMatches(r.Header.Get("If-None-Match"), etag) { w.WriteHeader(http.StatusNotModified) return } diff --git a/service/internal/httpx/etag_test.go b/service/internal/httpx/etag_test.go new file mode 100644 index 00000000..472148ee --- /dev/null +++ b/service/internal/httpx/etag_test.go @@ -0,0 +1,37 @@ +package httpx + +import "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}, + {`*`, true}, + {`"13c177f64ce8057e992861fc"`, false}, + {`W/"13c177f64ce8057e"`, false}, + {``, false}, + {`13c177f64ce8057e992861fb`, false}, // not an entity-tag + {`"13c177f64ce8057e992861fb`, false}, // unterminated + {`"a,b"`, false}, // a comma inside a tag is part of it + } { + 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"`, ``) { + t.Error("no ETag matched something") + } +} diff --git a/service/internal/httpx/httpx.go b/service/internal/httpx/httpx.go index 0e009cba..22c94831 100644 --- a/service/internal/httpx/httpx.go +++ b/service/internal/httpx/httpx.go @@ -162,6 +162,45 @@ func Log(log *slog.Logger, next http.Handler) http.Handler { }) } +// ETagMatches reports whether an If-None-Match header names etag, by the weak +// comparison RFC 9110 (13.1.2) prescribes for it: a W/ prefix is ignored on +// either side, the header may list several tags, and "*" matches anything. +// +// 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. +func ETagMatches(header, etag string) bool { + want := strings.TrimPrefix(etag, "W/") + if want == "" { + return false + } + for s := strings.TrimSpace(header); s != ""; { + s = strings.TrimLeft(s, " \t,") + if s == "" { + break + } + if s[0] == '*' { + return true + } + s = strings.TrimPrefix(s, "W/") + if s == "" || s[0] != '"' { + return false // not an entity-tag: nothing after it can be trusted + } + end := strings.IndexByte(s[1:], '"') + if end < 0 { + return false + } + if s[:end+2] == want { + return true + } + s = s[end+2:] + } + return false +} + // 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 +209,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 ETagMatches(r.Header.Get("If-None-Match"), etag) { w.WriteHeader(http.StatusNotModified) return } diff --git a/service/internal/wizard/handler.go b/service/internal/wizard/handler.go index 9fa0a622..4f72ee6c 100644 --- a/service/internal/wizard/handler.go +++ b/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.ETagMatches(r.Header.Get("If-None-Match"), c.etag) { w.WriteHeader(http.StatusNotModified) return } From 0d0cdc7d2099ab7a9ea1a93fc81f01b6552a6eb9 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:09:28 +0300 Subject: [PATCH 2/2] Read every If-None-Match field, refuse malformed ones, and never 304 on * - A request may carry If-None-Match more than once; httpx.Revisited reads every field as one list, where Header.Get saw only the first. - A value that is not a well-formed list of entity-tags matches nothing, even when it holds the current tag: saying no costs only a full response. - "*" matches nothing. Every handler decides 304 before it has looked the resource up, so it answered 304 for a board or platform that does not exist; no browser sends it on a GET. --- service/internal/boards/api.go | 2 +- service/internal/builds/explorer.go | 2 +- service/internal/httpx/etag_test.go | 43 +++++++++++++++++++++++++--- service/internal/httpx/httpx.go | 44 +++++++++++++++++++++-------- service/internal/wizard/handler.go | 2 +- 5 files changed, 74 insertions(+), 19 deletions(-) diff --git a/service/internal/boards/api.go b/service/internal/boards/api.go index 9e121c2f..3fbfc3a7 100644 --- a/service/internal/boards/api.go +++ b/service/internal/boards/api.go @@ -776,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 httpx.ETagMatches(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) diff --git a/service/internal/builds/explorer.go b/service/internal/builds/explorer.go index 635730ef..09c97031 100644 --- a/service/internal/builds/explorer.go +++ b/service/internal/builds/explorer.go @@ -75,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 httpx.ETagMatches(r.Header.Get("If-None-Match"), etag) { + if httpx.Revisited(r, etag) { w.WriteHeader(http.StatusNotModified) return } diff --git a/service/internal/httpx/etag_test.go b/service/internal/httpx/etag_test.go index 472148ee..b1fc8183 100644 --- a/service/internal/httpx/etag_test.go +++ b/service/internal/httpx/etag_test.go @@ -1,6 +1,9 @@ package httpx -import "testing" +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 @@ -15,13 +18,23 @@ func TestARevisitIsRecognisedWeakOrStrong(t *testing.T) { {`W/"13c177f64ce8057e992861fb"`, true}, // what came back through nginx {`"other", W/"13c177f64ce8057e992861fb"`, true}, {` "other" , "13c177f64ce8057e992861fb" `, true}, - {`*`, 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) @@ -31,7 +44,29 @@ func TestARevisitIsRecognisedWeakOrStrong(t *testing.T) { if !ETagMatches(`"abc"`, `W/"abc"`) || !ETagMatches(`W/"abc"`, `W/"abc"`) { t.Error("a weak ETag of ours did not match") } - if ETagMatches(`"abc"`, ``) { - t.Error("no ETag matched something") + 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") } } diff --git a/service/internal/httpx/httpx.go b/service/internal/httpx/httpx.go index 22c94831..2bdcf042 100644 --- a/service/internal/httpx/httpx.go +++ b/service/internal/httpx/httpx.go @@ -162,9 +162,9 @@ func Log(log *slog.Logger, next http.Handler) http.Handler { }) } -// ETagMatches reports whether an If-None-Match header names etag, by the weak +// 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, the header may list several tags, and "*" matches anything. +// 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, @@ -172,33 +172,53 @@ func Log(log *slog.Logger, next http.Handler) http.Handler { // 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 want == "" { + if len(want) < 2 || want[0] != '"' || want[len(want)-1] != '"' { return false } - for s := strings.TrimSpace(header); s != ""; { - s = strings.TrimLeft(s, " \t,") + matched := false + s := strings.TrimLeft(strings.TrimSpace(header), " \t,") + for first := true; ; first = false { + s = strings.TrimLeft(s, " \t") if s == "" { - break + return matched && !first } - if s[0] == '*' { - return true + 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 // not an entity-tag: nothing after it can be trusted + return false // "*", or not an entity-tag at all } end := strings.IndexByte(s[1:], '"') if end < 0 { return false } if s[:end+2] == want { - return true + matched = true } s = s[end+2:] } - return false +} + +// 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 @@ -209,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 ETagMatches(r.Header.Get("If-None-Match"), etag) { + if Revisited(r, etag) { w.WriteHeader(http.StatusNotModified) return } diff --git a/service/internal/wizard/handler.go b/service/internal/wizard/handler.go index 4f72ee6c..242b4cbb 100644 --- a/service/internal/wizard/handler.go +++ b/service/internal/wizard/handler.go @@ -68,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 httpx.ETagMatches(r.Header.Get("If-None-Match"), c.etag) { + if httpx.Revisited(r, c.etag) { w.WriteHeader(http.StatusNotModified) return }