From 1c91ed13ec0a8762025290e278931f89bee0afa4 Mon Sep 17 00:00:00 2001 From: Alec Thomas Date: Mon, 22 Jun 2026 14:50:14 +1000 Subject: [PATCH] refactor(httputil): consolidate conditional-request serving ETag/If-Match/If-None-Match handling was duplicated across the apiv1 and git handlers and missing entirely from the generic caching handler builder. Add ServeCacheHit, ServeCacheStat and CheckConditionals to httputil and route apiv1, the handler builder, and git snapshots through them. This fixes a latent bug where builder-based strategies (github-releases, host, hermit, proxy, artifactory) ignored conditional requests on cache hits, and removes git's upward dependency on the strategy package for CheckConditionals. --- internal/httputil/conditional.go | 74 +++++++++++++ internal/httputil/conditional_test.go | 151 ++++++++++++++++++++++++++ internal/strategy/apiv1.go | 55 +--------- internal/strategy/git/snapshot.go | 6 +- internal/strategy/handler/handler.go | 7 +- 5 files changed, 232 insertions(+), 61 deletions(-) create mode 100644 internal/httputil/conditional.go create mode 100644 internal/httputil/conditional_test.go diff --git a/internal/httputil/conditional.go b/internal/httputil/conditional.go new file mode 100644 index 00000000..81947bc7 --- /dev/null +++ b/internal/httputil/conditional.go @@ -0,0 +1,74 @@ +package httputil + +import ( + "io" + "maps" + "net/http" + "strings" + + "github.com/alecthomas/errors" +) + +// ETagHeader is the HTTP header used to carry an object's entity tag. +const ETagHeader = "ETag" + +// CheckConditionals evaluates RFC 7232 If-Match and If-None-Match precondition +// headers against the stored ETag. It returns 0 when all preconditions pass, +// otherwise the HTTP status code the caller should send: 412 Precondition +// Failed for a failed If-Match, or 304 Not Modified for a satisfied +// If-None-Match. +func CheckConditionals(r *http.Request, etag string) int { + if ifMatch := r.Header.Get("If-Match"); ifMatch != "" { + if etag == "" || !etagListMatches(ifMatch, etag) { + return http.StatusPreconditionFailed + } + } + if ifNoneMatch := r.Header.Get("If-None-Match"); ifNoneMatch != "" { + if etag != "" && etagListMatches(ifNoneMatch, etag) { + return http.StatusNotModified + } + } + return 0 +} + +// etagListMatches reports whether etag matches an If-Match / If-None-Match +// header value, which may be a comma-separated list of ETags or the "*" +// wildcard. Stored ETags are always strong, so weak comparison is not required. +func etagListMatches(headerValue, etag string) bool { + for candidate := range strings.SplitSeq(headerValue, ",") { + candidate = strings.TrimSpace(candidate) + if candidate == "*" || candidate == etag { + return true + } + } + return false +} + +// ServeCacheHit serves a cache hit over HTTP. It copies the stored headers onto +// the response, evaluates conditional request preconditions against the stored +// ETag, and either short-circuits with a 304/412 status or streams the body. +// The body is always closed. +// +// This consolidates the validator-aware serving path shared by handlers that +// return a single cached object (e.g. the API and the generic caching handler). +func ServeCacheHit(w http.ResponseWriter, r *http.Request, headers http.Header, body io.ReadCloser) error { + maps.Copy(w.Header(), headers) + if status := CheckConditionals(r, headers.Get(ETagHeader)); status != 0 { + w.WriteHeader(status) + return errors.WithStack(body.Close()) + } + _, copyErr := io.Copy(w, body) + return errors.Wrap(errors.Join(copyErr, body.Close()), "serve cache hit") +} + +// ServeCacheStat answers a metadata-only (HEAD) request from stored headers. It +// copies the headers onto the response and writes the status determined by the +// conditional request preconditions, defaulting to 200 OK. +func ServeCacheStat(w http.ResponseWriter, r *http.Request, headers http.Header) { + maps.Copy(w.Header(), headers) + status := CheckConditionals(r, headers.Get(ETagHeader)) + if status == 0 { + status = http.StatusOK + } + w.WriteHeader(status) +} diff --git a/internal/httputil/conditional_test.go b/internal/httputil/conditional_test.go new file mode 100644 index 00000000..912c6253 --- /dev/null +++ b/internal/httputil/conditional_test.go @@ -0,0 +1,151 @@ +package httputil_test + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/alecthomas/assert/v2" + + "github.com/block/cachew/internal/httputil" +) + +const testETag = `"abc123"` + +func cacheHeaders(extra ...[2]string) http.Header { + h := http.Header{} + h.Set(httputil.ETagHeader, testETag) + for _, kv := range extra { + h.Set(kv[0], kv[1]) + } + return h +} + +func newRequest(t *testing.T, ifMatch, ifNoneMatch string) *http.Request { + t.Helper() + r := httptest.NewRequest(http.MethodGet, "/", nil) + if ifMatch != "" { + r.Header.Set("If-Match", ifMatch) + } + if ifNoneMatch != "" { + r.Header.Set("If-None-Match", ifNoneMatch) + } + return r +} + +func TestCheckConditionals(t *testing.T) { + tests := []struct { + name string + ifMatch string + ifNoneMatch string + etag string + want int + }{ + {name: "NoConditions", etag: testETag, want: 0}, + {name: "IfMatchExact", ifMatch: testETag, etag: testETag, want: 0}, + {name: "IfMatchWildcard", ifMatch: "*", etag: testETag, want: 0}, + {name: "IfMatchList", ifMatch: `"other", ` + testETag, etag: testETag, want: 0}, + {name: "IfMatchMismatch", ifMatch: `"other"`, etag: testETag, want: http.StatusPreconditionFailed}, + {name: "IfMatchNoETag", ifMatch: "*", etag: "", want: http.StatusPreconditionFailed}, + {name: "IfNoneMatchExact", ifNoneMatch: testETag, etag: testETag, want: http.StatusNotModified}, + {name: "IfNoneMatchWildcard", ifNoneMatch: "*", etag: testETag, want: http.StatusNotModified}, + {name: "IfNoneMatchList", ifNoneMatch: `"other", ` + testETag, etag: testETag, want: http.StatusNotModified}, + {name: "IfNoneMatchMismatch", ifNoneMatch: `"other"`, etag: testETag, want: 0}, + {name: "IfNoneMatchNoETag", ifNoneMatch: "*", etag: "", want: 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := httputil.CheckConditionals(newRequest(t, tt.ifMatch, tt.ifNoneMatch), tt.etag) + assert.Equal(t, tt.want, got) + }) + } +} + +// trackingReader records whether Close was called, so tests can assert the body +// is always released. +type trackingReader struct { + io.Reader + closed bool +} + +func (t *trackingReader) Close() error { + t.closed = true + return nil +} + +func TestServeCacheHit(t *testing.T) { + t.Run("StreamsBodyWhenFresh", func(t *testing.T) { + body := &trackingReader{Reader: strings.NewReader("payload")} + headers := cacheHeaders([2]string{"Content-Type", "text/plain"}) + w := httptest.NewRecorder() + + err := httputil.ServeCacheHit(w, newRequest(t, "", ""), headers, body) + assert.NoError(t, err) + assert.True(t, body.closed) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, testETag, resp.Header.Get("ETag")) + assert.Equal(t, "text/plain", resp.Header.Get("Content-Type")) + data, _ := io.ReadAll(resp.Body) + assert.Equal(t, "payload", string(data)) + }) + + t.Run("NotModifiedSkipsBody", func(t *testing.T) { + body := &trackingReader{Reader: strings.NewReader("payload")} + headers := cacheHeaders() + w := httptest.NewRecorder() + + err := httputil.ServeCacheHit(w, newRequest(t, "", testETag), headers, body) + assert.NoError(t, err) + assert.True(t, body.closed) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusNotModified, resp.StatusCode) + data, _ := io.ReadAll(resp.Body) + assert.Equal(t, "", string(data)) + }) + + t.Run("PreconditionFailedSkipsBody", func(t *testing.T) { + body := &trackingReader{Reader: strings.NewReader("payload")} + headers := cacheHeaders() + w := httptest.NewRecorder() + + err := httputil.ServeCacheHit(w, newRequest(t, `"other"`, ""), headers, body) + assert.NoError(t, err) + assert.True(t, body.closed) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusPreconditionFailed, resp.StatusCode) + }) +} + +func TestServeCacheStat(t *testing.T) { + t.Run("OKWhenFresh", func(t *testing.T) { + headers := cacheHeaders() + w := httptest.NewRecorder() + + httputil.ServeCacheStat(w, newRequest(t, "", ""), headers) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, testETag, resp.Header.Get("ETag")) + }) + + t.Run("NotModified", func(t *testing.T) { + headers := cacheHeaders() + w := httptest.NewRecorder() + + httputil.ServeCacheStat(w, newRequest(t, "", testETag), headers) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusNotModified, resp.StatusCode) + }) +} diff --git a/internal/strategy/apiv1.go b/internal/strategy/apiv1.go index a6f18f22..75510eea 100644 --- a/internal/strategy/apiv1.go +++ b/internal/strategy/apiv1.go @@ -5,10 +5,8 @@ import ( "encoding/json" "io" "log/slog" - "maps" "net/http" "os" - "strings" "time" "github.com/alecthomas/errors" @@ -69,12 +67,7 @@ func (d *APIV1) statObject(w http.ResponseWriter, r *http.Request) { return } - maps.Copy(w.Header(), headers) - if status := CheckConditionals(r, headers.Get("ETag")); status != 0 { - w.WriteHeader(status) - return - } - w.WriteHeader(http.StatusOK) + httputil.ServeCacheStat(w, r, headers) } func (d *APIV1) getObject(w http.ResponseWriter, r *http.Request) { @@ -100,19 +93,8 @@ func (d *APIV1) getObject(w http.ResponseWriter, r *http.Request) { return } - maps.Copy(w.Header(), headers) - if status := CheckConditionals(r, headers.Get("ETag")); status != 0 { - _ = cr.Close() - w.WriteHeader(status) - return - } - - _, err = io.Copy(w, cr) - if err != nil { - d.logger.Error("Failed to copy cache object to response", "error", err, "key", key) - } - if cerr := cr.Close(); cerr != nil { - d.logger.Error("Failed to close cache reader", "error", cerr, "key", key) + if err := httputil.ServeCacheHit(w, r, headers, cr); err != nil { + d.logger.Error("Failed to serve cache object", "error", err, "key", key) } } @@ -218,34 +200,3 @@ func (d *APIV1) httpError(w http.ResponseWriter, code int, err error, message st d.logger.Error(message, args...) http.Error(w, message, code) } - -// CheckConditionals evaluates If-Match and If-None-Match precondition headers -// against the stored ETag for GET/HEAD requests. Returns 0 if all -// preconditions pass, otherwise the HTTP status code to send (304 or 412). -func CheckConditionals(r *http.Request, etag string) int { - if ifMatch := r.Header.Get("If-Match"); ifMatch != "" { - if etag == "" || !etagListMatches(ifMatch, etag) { - return http.StatusPreconditionFailed - } - } - if ifNoneMatch := r.Header.Get("If-None-Match"); ifNoneMatch != "" { - if etag != "" && etagListMatches(ifNoneMatch, etag) { - return http.StatusNotModified - } - } - return 0 -} - -// etagListMatches reports whether etag matches an RFC 7232 If-Match / -// If-None-Match header value, which may be a comma-separated list of ETags or -// the "*" wildcard. Stored ETags are always strong, so weak comparison is not -// required. -func etagListMatches(headerValue, etag string) bool { - for candidate := range strings.SplitSeq(headerValue, ",") { - candidate = strings.TrimSpace(candidate) - if candidate == "*" || candidate == etag { - return true - } - } - return false -} diff --git a/internal/strategy/git/snapshot.go b/internal/strategy/git/snapshot.go index 0c13ca62..7cd5c689 100644 --- a/internal/strategy/git/snapshot.go +++ b/internal/strategy/git/snapshot.go @@ -20,9 +20,9 @@ import ( "github.com/block/cachew/internal/cache" "github.com/block/cachew/internal/gitclone" + "github.com/block/cachew/internal/httputil" "github.com/block/cachew/internal/logging" "github.com/block/cachew/internal/snapshot" - "github.com/block/cachew/internal/strategy" ) const lfsFetchTimeout = 25 * time.Minute @@ -367,7 +367,7 @@ func (s *Strategy) serveSnapshotHead(ctx context.Context, w http.ResponseWriter, } status := http.StatusOK - if conditional := strategy.CheckConditionals(r, headers.Get(cache.ETagKey)); conditional != 0 { + if conditional := httputil.CheckConditionals(r, headers.Get(cache.ETagKey)); conditional != 0 { status = conditional } w.WriteHeader(status) @@ -557,7 +557,7 @@ func (s *Strategy) serveSnapshotWithBundle(ctx context.Context, w http.ResponseW // avoid streaming the full snapshot when the client already has it. var n int64 var err error - if status := strategy.CheckConditionals(r, headers.Get(cache.ETagKey)); status != 0 { + if status := httputil.CheckConditionals(r, headers.Get(cache.ETagKey)); status != 0 { w.WriteHeader(status) } else { n, err = serveReaderFast(w, r, reader) diff --git a/internal/strategy/handler/handler.go b/internal/strategy/handler/handler.go index 7079876c..0353463e 100644 --- a/internal/strategy/handler/handler.go +++ b/internal/strategy/handler/handler.go @@ -171,12 +171,7 @@ func (h *Handler) serveCached(w http.ResponseWriter, r *http.Request, key cache. } logging.FromContext(r.Context()).DebugContext(r.Context(), "Cache hit") - defer cr.Close() - maps.Copy(w.Header(), headers) - if _, err := io.Copy(w, cr); err != nil { - return true, errors.Wrap(err, "stream from cache") - } - return true, nil + return true, errors.WithStack(httputil.ServeCacheHit(w, r, headers, cr)) } func (h *Handler) fetchAndCache(w http.ResponseWriter, r *http.Request, key cache.Key) error {