From edabc99c610a752649a9de0eee1949d6daaf645d Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Mon, 29 Jun 2026 21:58:19 -0400 Subject: [PATCH 1/2] fix(rest): set Vary: Accept-Encoding on GzipMiddleware responses GzipMiddleware negotiates compression on Accept-Encoding but never told a shared cache the body varied by it. A cache keyed on the URL alone could store one variant and hand it to a client that negotiated the other: a gzipped body to a client that cannot decode it, or the reverse. Set Vary: Accept-Encoding unconditionally at the top of the middleware, so both branches (gzip and identity) and both surfaces it fronts (/api and the docs handler) carry it, including the bodyless 204/304 finals. Add, not Set, so a Vary set upstream survives. The golden corpus stays unchanged and Vary stays off the contractHeaders allowlist: the goldens replay against the old stack, which sent no Vary, so recording it would pin its absence. The header is pinned by rest/vary_test.go instead and documented in openapi.yaml next to the Cache-Control entries. > *This was generated by AI* --- contract/spec_test.go | 8 ++ openapi/openapi.yaml | 136 +++++++++++++++++++++++++++++++++ rest/gzip.go | 9 +++ rest/vary_test.go | 173 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 326 insertions(+) create mode 100644 rest/vary_test.go diff --git a/contract/spec_test.go b/contract/spec_test.go index 5f48d4b..7a4487f 100644 --- a/contract/spec_test.go +++ b/contract/spec_test.go @@ -739,6 +739,14 @@ func TestSpec_MutationCanary(t *testing.T) { const ranks200Block = ` "200": description: Rank reference list. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index cc33139..345f5fa 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -68,6 +68,14 @@ paths: "200": description: The full milpac profile. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -115,6 +123,14 @@ paths: "200": description: The full milpac profile. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -150,6 +166,14 @@ paths: "200": description: The full milpac profile. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -185,6 +209,14 @@ paths: "200": description: The full milpac profile. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -216,6 +248,14 @@ paths: "200": description: Profiles keyed by milpac relation id. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -251,6 +291,14 @@ paths: "200": description: Lite profiles keyed by milpac relation id. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -284,6 +332,14 @@ paths: "200": description: S1 uniforms profiles keyed by milpac relation id. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -336,6 +392,14 @@ paths: Matching lite profiles keyed by milpac relation id; {} when nothing matches (200, never 404). headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -365,6 +429,14 @@ paths: "200": description: Rank reference list. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -394,6 +466,14 @@ paths: "200": description: Position group reference tree. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -421,6 +501,14 @@ paths: "200": description: AWOL list. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -453,6 +541,14 @@ paths: "200": description: Forum group directory. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: data may be up to 10 minutes stale. @@ -576,6 +672,14 @@ paths: "200": description: One page of tickets. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: always served live. Responses are @@ -617,6 +721,14 @@ paths: "200": description: The ticket with its first messages. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: always served live. Responses are @@ -655,6 +767,14 @@ paths: "200": description: The ticket with its first messages. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: always served live. Responses are @@ -715,6 +835,14 @@ paths: "200": description: One page of the message thread. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: always served live. Responses are @@ -745,6 +873,14 @@ paths: "200": description: Category reference tree. headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding Cache-Control: description: >- Freshness signal: always served live. Responses are diff --git a/rest/gzip.go b/rest/gzip.go index 35bcd29..ff1fab2 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -17,6 +17,15 @@ import ( // closed. func GzipMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // The response body is selected by proactive content negotiation on + // Accept-Encoding, so declare that to any shared cache: it must key each + // encoding variant separately rather than serve one body for both. Set + // unconditionally, before the branch, so every response inherits it — + // the gzip branch and the identity pass-through alike, and every status, + // including the bodyless 204/304 finals (Vary is a cache-keying hint, so + // it is correct to keep there, unlike the Content-Encoding stripped off + // them below). Add, not Set, so a handler's own Vary value survives. + w.Header().Add("Vary", "Accept-Encoding") if strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { w.Header().Set("Content-Encoding", "gzip") gz := gzip.NewWriter(w) diff --git a/rest/vary_test.go b/rest/vary_test.go new file mode 100644 index 0000000..a0112d2 --- /dev/null +++ b/rest/vary_test.go @@ -0,0 +1,173 @@ +package rest_test + +// Vary: Accept-Encoding (#228). GzipMiddleware selects the response encoding +// from the request's Accept-Encoding: one URL yields a gzipped body for a gzip +// client and an identity body for everyone else. The header declares that the +// origin chose the representation by content negotiation, so a shared cache +// keyed on the URL alone keys each encoding variant separately instead of +// handing a gzipped body to a client that cannot decode it (or the reverse). +// +// The middleware sets it unconditionally at the top of the handler, so every +// response inherits it — both the gzip branch and the identity pass-through, +// across both surfaces the middleware fronts (/api and the docs handler). +// +// Like the Cache-Control pins next door (cachecontrol_test.go), these are +// deliberately new-stack-only: the golden corpus replays against the old +// stack, which sent no Vary, so the header stays off the contractHeaders +// allowlist and is pinned here instead. + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/7cav/api/contract" + "github.com/7cav/api/rest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// An /api request that does not negotiate gzip is served identity, and still +// carries Vary: Accept-Encoding — the identity variant must declare it too, or +// a cache could store it without the header and mis-serve it to a gzip client. +func TestNewStack_VaryOnIdentityAPIResponse(t *testing.T) { + h := newStack(t) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/milpacs/ranks", nil) + req.Header.Set("Authorization", "Bearer cav7_readkey") + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + require.Empty(t, rr.Result().Header.Get("Content-Encoding"), + "a request that does not advertise gzip is served identity") + assert.Equal(t, "Accept-Encoding", rr.Result().Header.Get("Vary")) +} + +// The gzip branch carries it too: a gzip-negotiated /api response is compressed +// (Content-Encoding: gzip) and still advertises Vary: Accept-Encoding. +func TestNewStack_VaryOnGzipAPIResponse(t *testing.T) { + h := newStack(t) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/milpacs/ranks", nil) + req.Header.Set("Authorization", "Bearer cav7_readkey") + req.Header.Set("Accept-Encoding", "gzip") + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + require.Equal(t, "gzip", rr.Result().Header.Get("Content-Encoding")) + assert.Equal(t, "Accept-Encoding", rr.Result().Header.Get("Vary")) +} + +// Across the whole implemented /api battery, every response that passes through +// GzipMiddleware advertises Vary: Accept-Encoding — the 200s and every error +// status that reaches the handlers (scope 403s, binding 400s, not-found 404s, +// injected-outage 500s). The auth 401 tier is the one exception: AuthMiddleware +// sits OUTSIDE the gzip layer (the same boundary that keeps 401s un-gzipped), so +// it short-circuits before the header is set and carries no Vary. Riding +// implementedCases means a future route's battery cases are covered the moment +// they land. Observed through the wire-faithful commitSnapshot helper, so a +// header stamped after commit would not pass. +func TestNewStack_VaryAcceptEncodingAcrossBattery(t *testing.T) { + var last http.Header + h := captureHeader(newStack(t), &last) + + known := map[string]contract.Case{} + for _, c := range contract.Cases() { + known[c.Name] = c + } + + sawGzipPath, sawAuthShortCircuit := false, false + for _, name := range implementedCases { + c, ok := known[name] + require.True(t, ok, "implementedCases entry %q names no battery case", name) + t.Run(name, func(t *testing.T) { + g, _, err := contract.RunCase(h, c) + require.NoError(t, err) + + if g.Status == http.StatusUnauthorized { + assert.Empty(t, last.Get("Vary"), + "the auth 401 tier short-circuits before GzipMiddleware, so it carries no Vary") + sawAuthShortCircuit = true + return + } + assert.Equal(t, "Accept-Encoding", last.Get("Vary"), + "every response through GzipMiddleware carries Vary (status %d on %s)", g.Status, c.Path) + sawGzipPath = true + }) + } + + // Non-vacuousness: the battery must witness both arms — at least one + // response through the gzip layer and at least one auth short-circuit — + // otherwise either assertion could pass by absence. + assert.True(t, sawGzipPath, "no battery response witnessed a path through GzipMiddleware") + assert.True(t, sawAuthShortCircuit, "no battery response witnessed an auth 401 short-circuit") +} + +// The docs surface inherits the header from the same GzipMiddleware, on both +// negotiations: the identity pass-through and the gzip branch. +func TestDocsHandler_VaryOnBothNegotiations(t *testing.T) { + h := rest.DocsHandler("dev") + + t.Run("identity", func(t *testing.T) { + rr := docsGet(t, h, "/openapi.yaml") + require.Equal(t, http.StatusOK, rr.Code) + require.Empty(t, rr.Result().Header.Get("Content-Encoding"), + "an un-negotiated docs response is served verbatim") + assert.Equal(t, "Accept-Encoding", rr.Result().Header.Get("Vary")) + }) + + t.Run("gzip", func(t *testing.T) { + rr := docsGetGzip(t, h, "/openapi.yaml") + require.Equal(t, http.StatusOK, rr.Code) + require.Equal(t, "gzip", rr.Result().Header.Get("Content-Encoding"), + "a gzip-negotiated docs response is compressed") + assert.Equal(t, "Accept-Encoding", rr.Result().Header.Get("Vary")) + }) +} + +// The header is appended, not written over. An upstream layer that already set +// its own Vary keeps it: GzipMiddleware uses Header().Add, so both values reach +// the wire. A Set would silently drop the upstream value. +func TestGzipMiddleware_VaryAppendsToExistingValue(t *testing.T) { + inner := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + // An outer layer that set Vary before GzipMiddleware runs (e.g. a future + // middleware varying on Origin). The chain has none today; the Add contract + // is what keeps it safe if one is ever mounted outside the gzip layer. + outer := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Vary", "Origin") + inner.ServeHTTP(w, r) + }) + + rr := httptest.NewRecorder() + outer.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, "/", nil)) + + require.Equal(t, http.StatusOK, rr.Code) + got := rr.Result().Header.Values("Vary") + assert.Contains(t, got, "Origin", "an upstream Vary value must survive (Add, not Set)") + assert.Contains(t, got, "Accept-Encoding", "GzipMiddleware appends its own Vary value") +} + +// Vary is a cache-keying hint, so it is correct to keep on bodyless final +// statuses (204/304) — unlike Content-Encoding, which the middleware strips off +// them because there is no body to encode. A gzip-negotiated 204 demonstrates +// both: Content-Encoding gone, Vary kept. +func TestGzipMiddleware_VaryKeptOnBodylessStatus(t *testing.T) { + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("Accept-Encoding", "gzip") + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + require.Equal(t, http.StatusNoContent, rr.Code) + assert.Empty(t, rr.Result().Header.Get("Content-Encoding"), + "a bodyless final carries no Content-Encoding — there is no body to encode") + assert.Equal(t, "Accept-Encoding", rr.Result().Header.Get("Vary"), + "Vary is a cache-keying hint, kept on bodyless finals on purpose") +} From befcad869bdb4017511002aaf02173d691964f1d Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Mon, 29 Jun 2026 22:11:04 -0400 Subject: [PATCH 2/2] fix(rest): correct Vary comment direction and document it on error responses The GzipMiddleware Vary comment described the wrong direction. Its Add runs at the top of the middleware, before next.ServeHTTP, so it can only preserve a Vary set by an outer layer, not protect a downstream route handler that later calls Set. Reword it to match the companion vary_test.go. Also fix a word collision where "Set unconditionally" used Set colloquially right next to the literal "Add, not Set," and scope "every response inherits it" to responses that reach this layer, since the outer auth 401 short-circuits first. GzipMiddleware adds Vary: Accept-Encoding to every response that reaches the handlers, including the JSON errors (400/403/404/500) every operation surfaces through the shared Error response. The spec declared Vary only on the 200 blocks, so it under-documented the header. Add the same Vary definition to the Error response component. Unauthorized (401) stays untouched: auth short-circuits before the gzip layer, so 401s carry no Vary. > *This was generated by AI* --- openapi/openapi.yaml | 9 +++++++++ rest/gzip.go | 14 ++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index 345f5fa..75029d3 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -962,6 +962,15 @@ components: JSON error body. `code` is a numeric status code (3 InvalidArgument → 400, 5 NotFound → 404, 7 PermissionDenied → 403, 13 Internal → 500). `message` is a human-readable description of the failure. + headers: + Vary: + description: >- + Cache-keying hint: the server selects the body encoding from + Accept-Encoding, so a shared cache must store the gzip and + identity variants under separate keys. + schema: + type: string + const: Accept-Encoding content: application/json: schema: diff --git a/rest/gzip.go b/rest/gzip.go index ff1fab2..83f1912 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -19,12 +19,14 @@ func GzipMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // The response body is selected by proactive content negotiation on // Accept-Encoding, so declare that to any shared cache: it must key each - // encoding variant separately rather than serve one body for both. Set - // unconditionally, before the branch, so every response inherits it — - // the gzip branch and the identity pass-through alike, and every status, - // including the bodyless 204/304 finals (Vary is a cache-keying hint, so - // it is correct to keep there, unlike the Content-Encoding stripped off - // them below). Add, not Set, so a handler's own Vary value survives. + // encoding variant separately rather than serve one body for both. + // Applied unconditionally, before the branch, so every response that + // reaches this layer inherits it (the outer auth 401 short-circuits + // before it) — the gzip branch and the identity pass-through alike, and + // every status, including the bodyless 204/304 finals (Vary is a + // cache-keying hint, so it is correct to keep there, unlike the + // Content-Encoding stripped off them below). Add, not Set, so a Vary + // value already set by an outer layer survives — Set would drop it. w.Header().Add("Vary", "Accept-Encoding") if strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { w.Header().Set("Content-Encoding", "gzip")