diff --git a/internal/cache/handlers.go b/internal/cache/handlers.go index 0e882e6..d352ddc 100644 --- a/internal/cache/handlers.go +++ b/internal/cache/handlers.go @@ -150,6 +150,14 @@ func New(enabled bool, cacheDir string) (*DB, error) { return db, nil } for i := range in { + if in[i].Entry != nil && !cacheableStatus(in[i].Entry.Status) { + // Skip conditional / no-content entries (1xx/204/205/304) persisted + // by an older proxy. cacheableStatus only gates new writes, so + // without this filter such entries would still be loaded here and + // replayed by OnRequest after upgrade, reintroducing the stale-304 + // problem this change is meant to fix. + continue + } db.cacheDB[in[i].Key] = in[i].Entry } // prevent successive runs from overwriting previous cache entries @@ -190,6 +198,27 @@ func bodyless(status int, method string) bool { } } +// cacheableStatus reports whether a response with the given status code is worth +// persisting in the cache. Informational (1xx), no-content (204/205) and +// conditional (304 Not Modified) responses carry no reusable body, so caching +// them saves nothing. Worse, caching a 304 is actively harmful for a dependency +// updater: replaying a stale "Not Modified" hides upstream package updates that +// the client would otherwise fetch, and a client whose local copy no longer +// matches (e.g. pnpm) fails with ERR_PNPM_CACHE_MISSING_AFTER_304. These +// responses are always passed straight through instead. +func cacheableStatus(status int) bool { + switch { + case status >= 100 && status < 200: + return false + case status == http.StatusNoContent, + status == http.StatusResetContent, + status == http.StatusNotModified: + return false + default: + return true + } +} + // OnRequest checks to see if the response is cached, if so responds with the cached data. func (d *DB) OnRequest(r *http.Request, proxyCtx *goproxy.ProxyCtx) (*http.Request, *http.Response) { if d == nil { @@ -303,12 +332,24 @@ func (d *DB) OnResponse(resp *http.Response, proxyCtx *goproxy.ProxyCtx) *http.R // restore http.NoBody so goproxy sees an unmodified empty body and frames // the response without a chunked terminator. Leaving a non-NoBody body in // place would make goproxy stamp a "0\r\n\r\n" the client never reads, - // desyncing the keep-alive MITM tunnel. ContentLength is left untouched so - // a HEAD still advertises the length its GET would return. + // desyncing the keep-alive MITM tunnel. This normalization runs whether + // or not we go on to cache the response. if resp.Body != nil && resp.Body != http.NoBody { _ = resp.Body.Close() resp.Body = http.NoBody } + + if !cacheableStatus(resp.StatusCode) { + // Never cache conditional / no-content responses (1xx/204/205/304). + // A cached 304 would replay a stale "Not Modified" and hide upstream + // package updates from the updater; there is no body to reuse anyway. + // Pass it straight through, correctly framed. + return resp + } + + // A HEAD response for an otherwise cacheable status (e.g. 200) has no + // body but carries useful headers (notably Content-Length). Cache it + // headers-only so a later HEAD hit reproduces the same metadata. d.cacheDB[key] = &Entry{ Status: resp.StatusCode, ResponseHeaders: resp.Header, diff --git a/internal/cache/handlers_test.go b/internal/cache/handlers_test.go index 74d5b28..f8c510c 100644 --- a/internal/cache/handlers_test.go +++ b/internal/cache/handlers_test.go @@ -18,6 +18,7 @@ import ( "github.com/elazarl/goproxy" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) // None of these tests should make network calls @@ -148,28 +149,53 @@ func Test_bodyless(t *testing.T) { } } +func Test_cacheableStatus(t *testing.T) { + cases := []struct { + name string + status int + want bool + }{ + {"200 OK is cacheable", 200, true}, + {"203 Non-Authoritative is cacheable", 203, true}, + {"301 Moved Permanently is cacheable", 301, true}, + {"404 Not Found is cacheable", 404, true}, + {"100 Continue is not cacheable", 100, false}, + {"204 No Content is not cacheable", 204, false}, + {"205 Reset Content is not cacheable", 205, false}, + {"304 Not Modified is not cacheable", 304, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, cacheableStatus(tc.status)) + }) + } +} + // TestCache_BodylessResponses is a regression test for the keep-alive desync // introduced by goproxy v1.9.0. Bodyless responses (1xx/204/304/HEAD) must keep // Body == http.NoBody through both cache paths; otherwise goproxy stamps a // chunked terminator the client never reads, desyncing the reused MITM tunnel. +// Conditional / no-content statuses (1xx/204/205/304) are additionally never +// cached, so replaying them can't hide upstream updates; only a HEAD for a +// cacheable status is stored (headers-only). func TestCache_BodylessResponses(t *testing.T) { cases := []struct { name string method string status int contentLength string + wantCached bool wantHitContentLen int64 }{ - {"304 Not Modified", http.MethodGet, http.StatusNotModified, "", 0}, - {"204 No Content", http.MethodGet, http.StatusNoContent, "", 0}, - {"205 Reset Content", http.MethodGet, http.StatusResetContent, "", 0}, - {"HEAD 200", http.MethodHead, http.StatusOK, "", 0}, + {"304 Not Modified", http.MethodGet, http.StatusNotModified, "", false, 0}, + {"204 No Content", http.MethodGet, http.StatusNoContent, "", false, 0}, + {"205 Reset Content", http.MethodGet, http.StatusResetContent, "", false, 0}, + {"HEAD 200", http.MethodHead, http.StatusOK, "", true, 0}, // A HEAD advertises the length the equivalent GET would return; a cache // hit must report that same length, not force it to zero. - {"HEAD 200 with Content-Length", http.MethodHead, http.StatusOK, "5", 5}, - // A non-HEAD bodyless status must stay zero: a non-zero ContentLength - // with an empty body would fail resp.Write for GET responses. - {"304 with Content-Length", http.MethodGet, http.StatusNotModified, "5", 0}, + {"HEAD 200 with Content-Length", http.MethodHead, http.StatusOK, "5", true, 5}, + // A 304 is never cached even when it carries a Content-Length. + {"304 with Content-Length", http.MethodGet, http.StatusNotModified, "5", false, 0}, } for _, tc := range cases { @@ -198,7 +224,20 @@ func TestCache_BodylessResponses(t *testing.T) { resp = cacher.OnResponse(resp, missCtx) assert.True(t, resp.Body == http.NoBody, "OnResponse must leave bodyless Body as http.NoBody (not tee-wrapped)") - assert.Len(t, cacher.cacheDB, 1, "bodyless response should still be cached") + + if !tc.wantCached { + assert.Empty(t, cacher.cacheDB, + "conditional/no-content responses must not be cached") + + // --- A later identical request must be a miss (passed through). --- + hitReq := httptest.NewRequestWithContext(t.Context(), tc.method, URL, nil) + hitCtx := &goproxy.ProxyCtx{Req: hitReq} + _, hit := cacher.OnRequest(hitReq, hitCtx) + assert.Nil(t, hit, "uncached conditional response must not produce a cache hit") + return + } + + assert.Len(t, cacher.cacheDB, 1, "cacheable HEAD response should be cached") // --- Cache hit: OnRequest must serve a bodyless response. --- hitReq := httptest.NewRequestWithContext(t.Context(), tc.method, URL, nil) @@ -216,18 +255,21 @@ func TestCache_BodylessResponses(t *testing.T) { // TestCache_BodylessResponseWithWrappedBodyIsRestored verifies that when an // earlier response handler has replaced a bodyless response's http.NoBody with // another (empty) ReadCloser — as PythonIndexHandler does — OnResponse restores -// http.NoBody before caching. Otherwise goproxy sees resp.Body != http.NoBody, -// stamps a chunked terminator, and desyncs the keep-alive MITM tunnel. +// http.NoBody. Otherwise goproxy sees resp.Body != http.NoBody, stamps a chunked +// terminator, and desyncs the keep-alive MITM tunnel. This restoration happens +// whether or not the response is cached: conditional/no-content statuses are not +// cached, but their bodies must still be normalized before pass-through. func TestCache_BodylessResponseWithWrappedBodyIsRestored(t *testing.T) { cases := []struct { - name string - method string - status int + name string + method string + status int + wantCached bool }{ - {"HEAD 200", http.MethodHead, http.StatusOK}, - {"204 No Content", http.MethodGet, http.StatusNoContent}, - {"205 Reset Content", http.MethodGet, http.StatusResetContent}, - {"304 Not Modified", http.MethodGet, http.StatusNotModified}, + {"HEAD 200", http.MethodHead, http.StatusOK, true}, + {"204 No Content", http.MethodGet, http.StatusNoContent, false}, + {"205 Reset Content", http.MethodGet, http.StatusResetContent, false}, + {"304 Not Modified", http.MethodGet, http.StatusNotModified, false}, } for _, tc := range cases { @@ -257,7 +299,12 @@ func TestCache_BodylessResponseWithWrappedBodyIsRestored(t *testing.T) { assert.True(t, resp.Body == http.NoBody, "OnResponse must restore http.NoBody when a bodyless response was wrapped") assert.True(t, closed, "the replaced wrapper must be closed to avoid leaks") - assert.Len(t, cacher.cacheDB, 1, "bodyless response should still be cached") + if tc.wantCached { + assert.Len(t, cacher.cacheDB, 1, "cacheable HEAD response should be cached") + } else { + assert.Empty(t, cacher.cacheDB, + "conditional/no-content responses must not be cached") + } }) } } @@ -367,6 +414,42 @@ func TestCache_MissingCacheFileIsNotCountedAsHit(t *testing.T) { assert.False(t, WasResponseCached(hitCtx), "request must not be tagged as cached when the file is missing") } +// TestCache_PersistedNonCacheableEntriesAreNotLoaded verifies that conditional / +// no-content entries (1xx/204/205/304) persisted in a db.yaml by an older proxy +// are dropped on load, so they are never served by OnRequest after upgrade. +func TestCache_PersistedNonCacheableEntriesAreNotLoaded(t *testing.T) { + cacheDir := filepath.Join(os.TempDir(), strconv.Itoa(time.Now().Nanosecond())) + require.NoError(t, os.MkdirAll(cacheDir, 0o750)) + defer os.RemoveAll(cacheDir) + + // A backing file for the cacheable (200) entry. + bodyFile := filepath.Join(cacheDir, "000001-body") + require.NoError(t, os.WriteFile(bodyFile, []byte("hello"), 0o600)) + + persisted := []Out{ + {Key: Key{Method: http.MethodGet, URL: "https://example.test/ok"}, + Entry: &Entry{Status: http.StatusOK, FilePath: bodyFile}}, + {Key: Key{Method: http.MethodGet, URL: "https://example.test/not-modified"}, + Entry: &Entry{Status: http.StatusNotModified}}, + {Key: Key{Method: http.MethodGet, URL: "https://example.test/no-content"}, + Entry: &Entry{Status: http.StatusNoContent}}, + } + data, err := yaml.Marshal(persisted) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(cacheDir, "db.yaml"), data, 0o600)) + + cacher, err := New(true, cacheDir) + require.NoError(t, err) + + assert.Len(t, cacher.cacheDB, 1, "only the cacheable 200 entry should be loaded") + _, ok := cacher.cacheDB[Key{Method: http.MethodGet, URL: "https://example.test/ok"}] + assert.True(t, ok, "the 200 entry must be loaded") + _, ok = cacher.cacheDB[Key{Method: http.MethodGet, URL: "https://example.test/not-modified"}] + assert.False(t, ok, "a persisted 304 entry must not be loaded") + _, ok = cacher.cacheDB[Key{Method: http.MethodGet, URL: "https://example.test/no-content"}] + assert.False(t, ok, "a persisted 204 entry must not be loaded") +} + func Test_sanitize(t *testing.T) { var tests = []struct { Input, Expected string diff --git a/proxy_test.go b/proxy_test.go index 02034ae..63690fa 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -223,8 +223,11 @@ func TestProxyHTTPSMITMHeadCacheHitPreservesContentLength(t *testing.T) { } // TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse verifies that a -// 304 response served through the cache does not corrupt the tunnel, and that a -// later unconditional request still returns the original cached 200 body. +// 304 response passed through the cache does not corrupt the reused tunnel, and +// that a later unconditional request still returns the original cached 200 body. +// Conditional validators stay in the cache key, so a conditional request is +// never short-circuited to a cached 200 (which could serve content older than +// the client already holds). func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { const ( etag = `"v1"` @@ -265,6 +268,7 @@ func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { return resp } + // Unconditional request populates the cache with the full 200 body. resp := request("") responseBody, err := io.ReadAll(resp.Body) require.NoError(t, err) @@ -272,6 +276,8 @@ func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, body, string(responseBody)) + // Conditional request keeps its own key, misses the cache, and reaches + // upstream, which returns a bodyless 304 (never cached, correctly framed). resp = request(etag) responseBody, err = io.ReadAll(resp.Body) require.NoError(t, err) @@ -279,6 +285,8 @@ func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { assert.Equal(t, http.StatusNotModified, resp.StatusCode) assert.Empty(t, responseBody) + // The unconditional entry is untouched: a later unconditional request is + // still served the original cached 200 body over the same healthy tunnel. resp = request("") responseBody, err = io.ReadAll(resp.Body) require.NoError(t, err) @@ -291,6 +299,56 @@ func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { assert.Equal(t, int32(1), proxyDials.Load()) } +// TestProxyHTTPSConditionalNotModifiedIsNotCached verifies that a 304 Not +// Modified is never stored in the cache. Replaying a cached 304 would let a +// stale "Not Modified" mask upstream package updates from the dependency +// updater, so every conditional request must reach upstream. It also confirms +// the passthrough 304 keeps the reused MITM tunnel healthy across requests. +func TestProxyHTTPSConditionalNotModifiedIsNotCached(t *testing.T) { + const etag = `"v1"` + var conditionalRequests atomic.Int32 + upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", etag) + if r.Header.Get("If-None-Match") == etag { + conditionalRequests.Add(1) + w.WriteHeader(http.StatusNotModified) + return + } + _, err := io.WriteString(w, "body") + assert.NoError(t, err) + })) + defer upstream.Close() + + t.Setenv("PROXY_CACHE", "true") + client, proxy := testProxyServer(t, testProxyConfig, nil, upstream.Certificate()) + defer proxy.Close() + transport := client.Transport.(*http.Transport) + var proxyDials atomic.Int32 + dialer := &net.Dialer{} + transport.DialContext = func(ctx context.Context, network, address string) (net.Conn, error) { + proxyDials.Add(1) + return dialer.DialContext(ctx, network, address) + } + + for i := 0; i < 3; i++ { + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, upstream.URL, nil) + require.NoError(t, err) + req.Header.Set("If-None-Match", etag) + resp, err := client.Do(req) + require.NoError(t, err) + responseBody, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusNotModified, resp.StatusCode) + assert.Empty(t, responseBody) + } + + // Every conditional request must reach upstream: a cached 304 is never + // replayed. A single dial proves the reused tunnel stayed in sync. + assert.Equal(t, int32(3), conditionalRequests.Load()) + assert.Equal(t, int32(1), proxyDials.Load()) +} + // TestProxyUpstreamCloseIsNotCachedAsBodylessResponse verifies that an upstream // connection failure (goproxy returns no response, synthesizes a 500) is logged // distinctly and NOT cached as if it were a valid bodyless response, so a later