Don't cache conditional/bodyless responses; serve conditional requests from cached 200

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,

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

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,20 +268,25 @@ 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)
 	require.NoError(t, resp.Body.Close())
 	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)
 	require.NoError(t, resp.Body.Close())
 	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