diff --git a/service/internal/boards/api.go b/service/internal/boards/api.go index dbe0f124..3fbfc3a7 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.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 32bd36f6..09c97031 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.Revisited(r, 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..b1fc8183 --- /dev/null +++ b/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") + } +} diff --git a/service/internal/httpx/httpx.go b/service/internal/httpx/httpx.go index 0e009cba..2bdcf042 100644 --- a/service/internal/httpx/httpx.go +++ b/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 } diff --git a/service/internal/wizard/handler.go b/service/internal/wizard/handler.go index 9fa0a622..242b4cbb 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.Revisited(r, c.etag) { w.WriteHeader(http.StatusNotModified) return }