From f77d299b31922e974dfb9340807dd44f29c66b29 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Tue, 20 Oct 2026 21:33:08 +0200 Subject: [PATCH 1/4] Don't hide upstream 404 and 401/403 behind stale metadata With cache_metadata on, a failed upstream metadata fetch fell back to the cached copy for any error. A 404 (package removed) and a 401/403 (the proxy's credentials rejected) are definite answers, though, and serving the old version list in their place kept them from reaching the client. They now pass through: 404 as 404, 401/403 as 502 like other upstream failures. Timeouts, connection errors, 5xx and 429 still fall back. The npm, Composer and Chef handlers serve metadata through the rewrite cache, which dropped the stale flag, so a fallback looked like a fresh 200. They now set the same Warning: 110 header as other metadata. Fixes #499 --- docs/configuration.md | 2 +- internal/handler/chef.go | 1 + internal/handler/composer.go | 2 + internal/handler/composer_hosted.go | 1 + internal/handler/handler.go | 29 +++++- internal/handler/npm.go | 2 + internal/handler/npm_hosted.go | 1 + internal/handler/stale_metadata_test.go | 114 ++++++++++++++++++++++++ 8 files changed, 148 insertions(+), 4 deletions(-) create mode 100644 internal/handler/stale_metadata_test.go diff --git a/docs/configuration.md b/docs/configuration.md index dccdf265..3bffadb6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -987,7 +987,7 @@ Or via environment variable: `PROXY_METADATA_TTL=10m`. Set to `"0"` to always revalidate with upstream (ETag-based conditional requests still avoid re-downloading unchanged content). -When upstream is unreachable and the cached entry is past its TTL, the proxy serves the stale cached copy with a `Warning: 110 - "Response is Stale"` header so clients can tell the data may be outdated. +When upstream is unreachable and the cached entry is past its TTL, the proxy serves the stale cached copy with a `Warning: 110 - "Response is Stale"` header so clients can tell the data may be outdated. This covers timeouts, connection errors, 5xx and 429 responses. A 404 or a 401/403 from the upstream is an answer rather than an outage, so it is not covered: a removed package answers 404, and credentials the upstream rejects answer 502, instead of the client getting a cached version list that no longer exists or can no longer be read. ### Metadata size limit diff --git a/internal/handler/chef.go b/internal/handler/chef.go index 72839920..764d8a36 100644 --- a/internal/handler/chef.go +++ b/internal/handler/chef.go @@ -162,6 +162,7 @@ func (h *ChefHandler) serveDocument(w http.ResponseWriter, r *http.Request, docu http.Error(w, "invalid Chef Supermarket response", http.StatusBadGateway) return } + markStale(w, upstream.stale) serveRewrittenMetadata(w, r, rewritten) } diff --git a/internal/handler/composer.go b/internal/handler/composer.go index 5bc2586f..643b287f 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -189,11 +189,13 @@ func (h *ComposerHandler) handlePackageMetadata(w http.ResponseWriter, r *http.R return // the client left while waiting on a shared rewrite } h.proxy.Logger.Warn("failed to rewrite metadata, proxying original", "error", err) + markStale(w, upstream.stale) w.Header().Set(headerContentType, "application/json") _, _ = w.Write(upstream.body) return } + markStale(w, upstream.stale) serveRewrittenMetadata(w, r, rewritten) } diff --git a/internal/handler/composer_hosted.go b/internal/handler/composer_hosted.go index 9a27e6fc..689cf8f9 100644 --- a/internal/handler/composer_hosted.go +++ b/internal/handler/composer_hosted.go @@ -99,6 +99,7 @@ func (h *ComposerHandler) serveMetadataWithLocal(w http.ResponseWriter, r *http. http.Error(w, "failed to rewrite package metadata", http.StatusBadGateway) return } + markStale(w, upstream.stale) serveRewrittenMetadata(w, r, h.proxy.uncachedRewrite("composer", h.proxyURL, cacheKey, out)) } diff --git a/internal/handler/handler.go b/internal/handler/handler.go index d29508eb..31451d38 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -1197,6 +1197,21 @@ func JSONError(w http.ResponseWriter, status int, message string) { _, _ = fmt.Fprintf(w, `{"error":%q}`, message) } +// ErrUpstreamRefused indicates the upstream answered 401 or 403: the +// credentials the proxy sends are missing, wrong or no longer allowed. +var ErrUpstreamRefused = errors.New("upstream refused the request") + +// staleWarning marks a response served from the metadata cache because the +// upstream could not be reached. +const staleWarning = `110 - "Response is Stale"` + +// markStale sets the stale warning on a metadata response when stale is set. +func markStale(w http.ResponseWriter, stale bool) { + if stale { + w.Header().Set("Warning", staleWarning) + } +} + // ErrUpstreamNotFound indicates the upstream returned 404. var ErrUpstreamNotFound = fmt.Errorf("upstream: %w", fetch.ErrNotFound) @@ -1391,6 +1406,13 @@ func (p *Proxy) fetchMetadataFromUpstream(ctx context.Context, ecosystem, cacheK return metadataResult{body: meta.body, contentType: meta.contentType, contentEncoding: meta.contentEncoding, digest: digest} } + // A 404 or 401/403 is the upstream's answer, not an outage: the package + // is gone, or the proxy may no longer read it. Serving the cached copy + // would hide that from the client, so only other failures fall back. + if errors.Is(err, ErrUpstreamNotFound) || errors.Is(err, ErrUpstreamRefused) { + return metadataResult{err: err} + } + // Upstream failed -- fall back to cache if available if !p.CacheMetadata || entry == nil { return metadataResult{err: fmt.Errorf("upstream failed and no cached metadata: %w", err)} @@ -1606,6 +1628,9 @@ func (p *Proxy) fetchUpstreamMetadata(ctx context.Context, upstreamURL string, e body, _ := io.ReadAll(io.LimitReader(resp.Body, upstreamNotFoundBodyLimit)) return nil, &upstreamNotFoundError{body: body} } + if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { + return nil, fmt.Errorf("%w: upstream returned %d", ErrUpstreamRefused, resp.StatusCode) + } if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("upstream returned %d", resp.StatusCode) } @@ -1816,9 +1841,7 @@ func (p *Proxy) writeMetadataCachedResponseWithEncoding(w http.ResponseWriter, r if contentEncoding != "" { w.Header().Set(headerContentEncoding, contentEncoding) } - if cm.stale { - w.Header().Set("Warning", `110 - "Response is Stale"`) - } + markStale(w, cm.stale) w.WriteHeader(http.StatusOK) if r.Method != http.MethodHead { _, _ = w.Write(body) diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 6530f5a1..4e008514 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -263,12 +263,14 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques } // If rewriting fails, just proxy the original h.proxy.Logger.Warn("failed to rewrite metadata, proxying original", "error", err) + markStale(w, upstream.stale) w.Header().Set(headerContentType, contentTypeJSON) w.WriteHeader(http.StatusOK) _, _ = w.Write(upstream.body) return } + markStale(w, upstream.stale) serveRewrittenMetadata(w, r, rewritten) } diff --git a/internal/handler/npm_hosted.go b/internal/handler/npm_hosted.go index 65b59ba3..7532cbdd 100644 --- a/internal/handler/npm_hosted.go +++ b/internal/handler/npm_hosted.go @@ -123,6 +123,7 @@ func (h *NPMHandler) serveMetadataWithLocal(w http.ResponseWriter, r *http.Reque JSONError(w, http.StatusBadGateway, "failed to rewrite package metadata") return } + markStale(w, upstream.stale) serveRewrittenMetadata(w, r, h.proxy.uncachedRewrite("npm", h.proxyURL, metadataCacheKey, out)) } diff --git a/internal/handler/stale_metadata_test.go b/internal/handler/stale_metadata_test.go new file mode 100644 index 00000000..a5c2aac5 --- /dev/null +++ b/internal/handler/stale_metadata_test.go @@ -0,0 +1,114 @@ +package handler + +import ( + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" +) + +const stalePackument = `{"name":"left-pad","versions":{"1.0.0":{"name":"left-pad","version":"1.0.0",` + + `"dist":{"tarball":"https://registry.npmjs.org/left-pad/-/left-pad-1.0.0.tgz"}}}}` + +// npmUpstreamThen serves the packument once, with an ETag, and answers every +// later request with then. +func npmUpstreamThen(t *testing.T, then http.HandlerFunc) *httptest.Server { + t.Helper() + var requests atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if requests.Add(1) == 1 { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("ETag", `"v1"`) + _, _ = w.Write([]byte(stalePackument)) + return + } + then(w, r) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestNPMMetadataAfterUpstreamAnswer(t *testing.T) { + tests := []struct { + name string + then http.HandlerFunc + wantStatus int + wantStale bool + }{ + {name: "upstream error", then: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadGateway) + }, wantStatus: http.StatusOK, wantStale: true}, + {name: "timeout", then: func(w http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + case <-time.After(2 * time.Second): + } + }, wantStatus: http.StatusOK, wantStale: true}, + {name: "not modified", then: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotModified) + }, wantStatus: http.StatusOK}, + {name: "unpublished", then: func(w http.ResponseWriter, _ *http.Request) { + http.NotFound(w, nil) + }, wantStatus: http.StatusNotFound}, + {name: "credentials rejected", then: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + }, wantStatus: http.StatusBadGateway}, + {name: "access forbidden", then: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + }, wantStatus: http.StatusBadGateway}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + upstream := npmUpstreamThen(t, tt.then) + proxy, _, _, _ := setupTestProxy(t) + proxy.CacheMetadata = true + proxy.MetadataTTL = time.Millisecond + proxy.HTTPClient = &http.Client{Timeout: 200 * time.Millisecond} + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL+"/").Routes() + + first := httptest.NewRecorder() + h.ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/left-pad", nil)) + if first.Code != http.StatusOK { + t.Fatalf("first request: status = %d", first.Code) + } + time.Sleep(5 * time.Millisecond) + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/left-pad", nil)) + if w.Code != tt.wantStatus { + t.Fatalf("status = %d, want %d, body %s", w.Code, tt.wantStatus, w.Body) + } + if stale := w.Header().Get("Warning") == staleWarning; stale != tt.wantStale { + t.Errorf("Warning = %q, want stale %v", w.Header().Get("Warning"), tt.wantStale) + } + servedCopy := strings.Contains(w.Body.String(), "left-pad-1.0.0.tgz") + if wantCopy := tt.wantStatus == http.StatusOK; servedCopy != wantCopy { + t.Errorf("served the cached packument = %v, want %v", servedCopy, wantCopy) + } + }) + } +} + +func TestProxyCachedPassesDefiniteAnswersThrough(t *testing.T) { + for _, status := range []int{http.StatusNotFound, http.StatusUnauthorized, http.StatusForbidden} { + t.Run(http.StatusText(status), func(t *testing.T) { + upstream := npmUpstreamThen(t, func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(status) }) + proxy, _, _, _ := setupTestProxy(t) + proxy.CacheMetadata = true + proxy.MetadataTTL = time.Millisecond + proxy.HTTPClient = upstream.Client() + + w := httptest.NewRecorder() + proxy.ProxyCached(w, httptest.NewRequest(http.MethodGet, "/doc", nil), upstream.URL+"/doc", "test-eco", "doc") + time.Sleep(5 * time.Millisecond) + w = httptest.NewRecorder() + proxy.ProxyCached(w, httptest.NewRequest(http.MethodGet, "/doc", nil), upstream.URL+"/doc", "test-eco", "doc") + + if w.Code == http.StatusOK || strings.Contains(w.Body.String(), "left-pad") { + t.Errorf("status = %d, body %q, want the upstream's answer instead of the cached copy", w.Code, w.Body) + } + }) + } +} From 844e2ae4f34993de30361bc94fde50f7fb4becce Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Tue, 20 Oct 2026 21:34:07 +0200 Subject: [PATCH 2/4] Test that Composer metadata doesn't hide upstream 404 and 401/403 --- internal/handler/stale_metadata_test.go | 52 +++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/internal/handler/stale_metadata_test.go b/internal/handler/stale_metadata_test.go index a5c2aac5..5cd8d6dd 100644 --- a/internal/handler/stale_metadata_test.go +++ b/internal/handler/stale_metadata_test.go @@ -112,3 +112,55 @@ func TestProxyCachedPassesDefiniteAnswersThrough(t *testing.T) { }) } } + +const staleComposerDocument = `{"packages":{"acme/widget":[{"name":"acme/widget","version":"1.0.0",` + + `"dist":{"type":"zip","url":"https://repo.packagist.org/files/acme/widget/1.0.0/widget.zip"}}]},"minified":"composer/2.0"}` + +func TestComposerMetadataAfterUpstreamAnswer(t *testing.T) { + tests := []struct { + name string + status int + wantStatus int + wantStale bool + }{ + {name: "upstream error", status: http.StatusBadGateway, wantStatus: http.StatusOK, wantStale: true}, + {name: "removed", status: http.StatusNotFound, wantStatus: http.StatusNotFound}, + {name: "credentials rejected", status: http.StatusUnauthorized, wantStatus: http.StatusBadGateway}, + {name: "access forbidden", status: http.StatusForbidden, wantStatus: http.StatusBadGateway}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var requests atomic.Int32 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if requests.Add(1) == 1 { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(staleComposerDocument)) + return + } + w.WriteHeader(tt.status) + })) + t.Cleanup(upstream.Close) + proxy, _, _, _ := setupTestProxy(t) + proxy.CacheMetadata = true + proxy.MetadataTTL = time.Millisecond + proxy.HTTPClient = upstream.Client() + h := NewComposerHandlerWithUpstreams(proxy, "http://proxy.test", upstream.URL, upstream.URL).Routes() + + first := httptest.NewRecorder() + h.ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/p2/acme/widget.json", nil)) + if first.Code != http.StatusOK { + t.Fatalf("first request: status = %d, body %s", first.Code, first.Body) + } + time.Sleep(5 * time.Millisecond) + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/p2/acme/widget.json", nil)) + if w.Code != tt.wantStatus { + t.Fatalf("status = %d, want %d, body %s", w.Code, tt.wantStatus, w.Body) + } + if stale := w.Header().Get("Warning") == staleWarning; stale != tt.wantStale { + t.Errorf("Warning = %q, want stale %v", w.Header().Get("Warning"), tt.wantStale) + } + }) + } +} From 994a9368d1086283314fbd09f653f26774de62dd Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Wed, 21 Oct 2026 12:23:33 +0200 Subject: [PATCH 3/4] Keep the stale warning on filtered npm version metadata With cooldown or a version denylist, /pkg/ reads the filtered packument through handleFilteredNPMVersionMetadata, which dropped the stale flag. --- internal/handler/npm.go | 7 +++-- internal/handler/stale_metadata_test.go | 42 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 4e008514..af7bc2e6 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -298,8 +298,8 @@ func (h *NPMHandler) packumentAccept() string { // filtered packument so cooldown and dist-tags match the root metadata response. func (h *NPMHandler) handleFilteredNPMVersionMetadata(w http.ResponseWriter, r *http.Request, packageName, specifier, accept, registryURL, cacheKey, metadataCacheKey string) { _, packumentPath := npmMetadataPath(packageName) - packument, _, err := h.proxy.FetchOrCacheMetadata(r.Context(), "npm", cacheKey, registryURL+"/"+packumentPath, accept) - if err != nil { + upstream := h.proxy.fetchMetadataDocument(r.Context(), "npm", cacheKey, registryURL+"/"+packumentPath, accept) + if err := upstream.err; err != nil { if errors.Is(err, ErrUpstreamNotFound) { JSONError(w, http.StatusNotFound, ErrPackageNotFound.Error()) return @@ -312,7 +312,7 @@ func (h *NPMHandler) handleFilteredNPMVersionMetadata(w http.ResponseWriter, r * return } - rewritten, err := h.proxy.cachedRewrite(r.Context(), "npm", h.proxyURL, metadataCacheKey, packument, "", func(b []byte) ([]byte, []string, error) { + rewritten, err := h.proxy.cachedRewrite(r.Context(), "npm", h.proxyURL, metadataCacheKey, upstream.body, "", func(b []byte) ([]byte, []string, error) { out, err := h.rewriteNPMVersionFromPackument(packageName, specifier, b) return out, nil, err }) @@ -337,6 +337,7 @@ func (h *NPMHandler) handleFilteredNPMVersionMetadata(w http.ResponseWriter, r * return } + markStale(w, upstream.stale) w.Header().Set(headerContentType, contentTypeJSON) w.WriteHeader(http.StatusOK) _, _ = w.Write(rewritten.body) diff --git a/internal/handler/stale_metadata_test.go b/internal/handler/stale_metadata_test.go index 5cd8d6dd..1fd82386 100644 --- a/internal/handler/stale_metadata_test.go +++ b/internal/handler/stale_metadata_test.go @@ -91,6 +91,48 @@ func TestNPMMetadataAfterUpstreamAnswer(t *testing.T) { } } +// With a denylist entry for the package, version requests read the filtered +// packument through handleFilteredNPMVersionMetadata. +func TestNPMFilteredVersionMetadataAfterUpstreamAnswer(t *testing.T) { + tests := []struct { + name string + status int + wantStatus int + wantStale bool + }{ + {name: "upstream error", status: http.StatusServiceUnavailable, wantStatus: http.StatusOK, wantStale: true}, + {name: "unpublished", status: http.StatusNotFound, wantStatus: http.StatusNotFound}, + {name: "credentials rejected", status: http.StatusUnauthorized, wantStatus: http.StatusBadGateway}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + upstream := npmUpstreamThen(t, func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(tt.status) }) + proxy, _, _, _ := setupTestProxy(t) + proxy.CacheMetadata = true + proxy.MetadataTTL = time.Millisecond + proxy.HTTPClient = upstream.Client() + setTestDenylist(t, proxy, "pkg:npm/left-pad@2.0.0") + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL+"/").Routes() + + first := httptest.NewRecorder() + h.ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/left-pad", nil)) + if first.Code != http.StatusOK { + t.Fatalf("first request: status = %d", first.Code) + } + time.Sleep(5 * time.Millisecond) + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/left-pad/1.0.0", nil)) + if w.Code != tt.wantStatus { + t.Fatalf("status = %d, want %d, body %s", w.Code, tt.wantStatus, w.Body) + } + if stale := w.Header().Get("Warning") == staleWarning; stale != tt.wantStale { + t.Errorf("Warning = %q, want stale %v", w.Header().Get("Warning"), tt.wantStale) + } + }) + } +} + func TestProxyCachedPassesDefiniteAnswersThrough(t *testing.T) { for _, status := range []int{http.StatusNotFound, http.StatusUnauthorized, http.StatusForbidden} { t.Run(http.StatusText(status), func(t *testing.T) { From 0e25e46732ffecffee8cec7fd5938cad3d979ea3 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Thu, 22 Oct 2026 09:14:33 +0200 Subject: [PATCH 4/4] Keep the stale fallback for 404 and 401/403, leave npm to #503 Serving cached metadata when the upstream refuses is documented for generic upstreams, and GitHub rate limits are 403s, so the fallback stays as it is. #503 handles npm's definite answers. What is left here is the stale warning for Composer, Chef and hosted npm metadata, which went through the rewrite cache without it. --- docs/configuration.md | 2 +- internal/handler/handler.go | 14 -- internal/handler/npm.go | 9 +- internal/handler/stale_metadata_test.go | 220 ++++++------------------ 4 files changed, 57 insertions(+), 188 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 3bffadb6..dccdf265 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -987,7 +987,7 @@ Or via environment variable: `PROXY_METADATA_TTL=10m`. Set to `"0"` to always revalidate with upstream (ETag-based conditional requests still avoid re-downloading unchanged content). -When upstream is unreachable and the cached entry is past its TTL, the proxy serves the stale cached copy with a `Warning: 110 - "Response is Stale"` header so clients can tell the data may be outdated. This covers timeouts, connection errors, 5xx and 429 responses. A 404 or a 401/403 from the upstream is an answer rather than an outage, so it is not covered: a removed package answers 404, and credentials the upstream rejects answer 502, instead of the client getting a cached version list that no longer exists or can no longer be read. +When upstream is unreachable and the cached entry is past its TTL, the proxy serves the stale cached copy with a `Warning: 110 - "Response is Stale"` header so clients can tell the data may be outdated. ### Metadata size limit diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 31451d38..6419d17a 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -1197,10 +1197,6 @@ func JSONError(w http.ResponseWriter, status int, message string) { _, _ = fmt.Fprintf(w, `{"error":%q}`, message) } -// ErrUpstreamRefused indicates the upstream answered 401 or 403: the -// credentials the proxy sends are missing, wrong or no longer allowed. -var ErrUpstreamRefused = errors.New("upstream refused the request") - // staleWarning marks a response served from the metadata cache because the // upstream could not be reached. const staleWarning = `110 - "Response is Stale"` @@ -1406,13 +1402,6 @@ func (p *Proxy) fetchMetadataFromUpstream(ctx context.Context, ecosystem, cacheK return metadataResult{body: meta.body, contentType: meta.contentType, contentEncoding: meta.contentEncoding, digest: digest} } - // A 404 or 401/403 is the upstream's answer, not an outage: the package - // is gone, or the proxy may no longer read it. Serving the cached copy - // would hide that from the client, so only other failures fall back. - if errors.Is(err, ErrUpstreamNotFound) || errors.Is(err, ErrUpstreamRefused) { - return metadataResult{err: err} - } - // Upstream failed -- fall back to cache if available if !p.CacheMetadata || entry == nil { return metadataResult{err: fmt.Errorf("upstream failed and no cached metadata: %w", err)} @@ -1628,9 +1617,6 @@ func (p *Proxy) fetchUpstreamMetadata(ctx context.Context, upstreamURL string, e body, _ := io.ReadAll(io.LimitReader(resp.Body, upstreamNotFoundBodyLimit)) return nil, &upstreamNotFoundError{body: body} } - if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { - return nil, fmt.Errorf("%w: upstream returned %d", ErrUpstreamRefused, resp.StatusCode) - } if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("upstream returned %d", resp.StatusCode) } diff --git a/internal/handler/npm.go b/internal/handler/npm.go index af7bc2e6..6530f5a1 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -263,14 +263,12 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques } // If rewriting fails, just proxy the original h.proxy.Logger.Warn("failed to rewrite metadata, proxying original", "error", err) - markStale(w, upstream.stale) w.Header().Set(headerContentType, contentTypeJSON) w.WriteHeader(http.StatusOK) _, _ = w.Write(upstream.body) return } - markStale(w, upstream.stale) serveRewrittenMetadata(w, r, rewritten) } @@ -298,8 +296,8 @@ func (h *NPMHandler) packumentAccept() string { // filtered packument so cooldown and dist-tags match the root metadata response. func (h *NPMHandler) handleFilteredNPMVersionMetadata(w http.ResponseWriter, r *http.Request, packageName, specifier, accept, registryURL, cacheKey, metadataCacheKey string) { _, packumentPath := npmMetadataPath(packageName) - upstream := h.proxy.fetchMetadataDocument(r.Context(), "npm", cacheKey, registryURL+"/"+packumentPath, accept) - if err := upstream.err; err != nil { + packument, _, err := h.proxy.FetchOrCacheMetadata(r.Context(), "npm", cacheKey, registryURL+"/"+packumentPath, accept) + if err != nil { if errors.Is(err, ErrUpstreamNotFound) { JSONError(w, http.StatusNotFound, ErrPackageNotFound.Error()) return @@ -312,7 +310,7 @@ func (h *NPMHandler) handleFilteredNPMVersionMetadata(w http.ResponseWriter, r * return } - rewritten, err := h.proxy.cachedRewrite(r.Context(), "npm", h.proxyURL, metadataCacheKey, upstream.body, "", func(b []byte) ([]byte, []string, error) { + rewritten, err := h.proxy.cachedRewrite(r.Context(), "npm", h.proxyURL, metadataCacheKey, packument, "", func(b []byte) ([]byte, []string, error) { out, err := h.rewriteNPMVersionFromPackument(packageName, specifier, b) return out, nil, err }) @@ -337,7 +335,6 @@ func (h *NPMHandler) handleFilteredNPMVersionMetadata(w http.ResponseWriter, r * return } - markStale(w, upstream.stale) w.Header().Set(headerContentType, contentTypeJSON) w.WriteHeader(http.StatusOK) _, _ = w.Write(rewritten.body) diff --git a/internal/handler/stale_metadata_test.go b/internal/handler/stale_metadata_test.go index 1fd82386..9cd8e754 100644 --- a/internal/handler/stale_metadata_test.go +++ b/internal/handler/stale_metadata_test.go @@ -3,205 +3,91 @@ package handler import ( "net/http" "net/http/httptest" - "strings" "sync/atomic" "testing" "time" ) -const stalePackument = `{"name":"left-pad","versions":{"1.0.0":{"name":"left-pad","version":"1.0.0",` + - `"dist":{"tarball":"https://registry.npmjs.org/left-pad/-/left-pad-1.0.0.tgz"}}}}` - -// npmUpstreamThen serves the packument once, with an ETag, and answers every -// later request with then. -func npmUpstreamThen(t *testing.T, then http.HandlerFunc) *httptest.Server { +// upstreamThen serves body once and answers every later request with status. +func upstreamThen(t *testing.T, body string, status int) *httptest.Server { t.Helper() var requests atomic.Int32 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { if requests.Add(1) == 1 { w.Header().Set("Content-Type", "application/json") - w.Header().Set("ETag", `"v1"`) - _, _ = w.Write([]byte(stalePackument)) + _, _ = w.Write([]byte(body)) return } - then(w, r) + w.WriteHeader(status) })) t.Cleanup(srv.Close) return srv } -func TestNPMMetadataAfterUpstreamAnswer(t *testing.T) { - tests := []struct { - name string - then http.HandlerFunc - wantStatus int - wantStale bool - }{ - {name: "upstream error", then: func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusBadGateway) - }, wantStatus: http.StatusOK, wantStale: true}, - {name: "timeout", then: func(w http.ResponseWriter, r *http.Request) { - select { - case <-r.Context().Done(): - case <-time.After(2 * time.Second): - } - }, wantStatus: http.StatusOK, wantStale: true}, - {name: "not modified", then: func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotModified) - }, wantStatus: http.StatusOK}, - {name: "unpublished", then: func(w http.ResponseWriter, _ *http.Request) { - http.NotFound(w, nil) - }, wantStatus: http.StatusNotFound}, - {name: "credentials rejected", then: func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusUnauthorized) - }, wantStatus: http.StatusBadGateway}, - {name: "access forbidden", then: func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusForbidden) - }, wantStatus: http.StatusBadGateway}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - upstream := npmUpstreamThen(t, tt.then) - proxy, _, _, _ := setupTestProxy(t) - proxy.CacheMetadata = true - proxy.MetadataTTL = time.Millisecond - proxy.HTTPClient = &http.Client{Timeout: 200 * time.Millisecond} - h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL+"/").Routes() - - first := httptest.NewRecorder() - h.ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/left-pad", nil)) - if first.Code != http.StatusOK { - t.Fatalf("first request: status = %d", first.Code) - } - time.Sleep(5 * time.Millisecond) - - w := httptest.NewRecorder() - h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/left-pad", nil)) - if w.Code != tt.wantStatus { - t.Fatalf("status = %d, want %d, body %s", w.Code, tt.wantStatus, w.Body) - } - if stale := w.Header().Get("Warning") == staleWarning; stale != tt.wantStale { - t.Errorf("Warning = %q, want stale %v", w.Header().Get("Warning"), tt.wantStale) - } - servedCopy := strings.Contains(w.Body.String(), "left-pad-1.0.0.tgz") - if wantCopy := tt.wantStatus == http.StatusOK; servedCopy != wantCopy { - t.Errorf("served the cached packument = %v, want %v", servedCopy, wantCopy) - } - }) - } +// staleProxy returns a proxy whose cached metadata expires right away. +func staleProxy(t *testing.T, upstream *httptest.Server) *Proxy { + t.Helper() + proxy, _, _, _ := setupTestProxy(t) + proxy.CacheMetadata = true + proxy.MetadataTTL = time.Millisecond + proxy.HTTPClient = upstream.Client() + return proxy } -// With a denylist entry for the package, version requests read the filtered -// packument through handleFilteredNPMVersionMetadata. -func TestNPMFilteredVersionMetadataAfterUpstreamAnswer(t *testing.T) { - tests := []struct { - name string - status int - wantStatus int - wantStale bool - }{ - {name: "upstream error", status: http.StatusServiceUnavailable, wantStatus: http.StatusOK, wantStale: true}, - {name: "unpublished", status: http.StatusNotFound, wantStatus: http.StatusNotFound}, - {name: "credentials rejected", status: http.StatusUnauthorized, wantStatus: http.StatusBadGateway}, +// requestTwice requests path, lets the cached copy expire, and requests it +// again. +func requestTwice(t *testing.T, h http.Handler, path string) *httptest.ResponseRecorder { + t.Helper() + first := httptest.NewRecorder() + h.ServeHTTP(first, httptest.NewRequest(http.MethodGet, path, nil)) + if first.Code != http.StatusOK { + t.Fatalf("first request: status = %d, body %s", first.Code, first.Body) } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - upstream := npmUpstreamThen(t, func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(tt.status) }) - proxy, _, _, _ := setupTestProxy(t) - proxy.CacheMetadata = true - proxy.MetadataTTL = time.Millisecond - proxy.HTTPClient = upstream.Client() - setTestDenylist(t, proxy, "pkg:npm/left-pad@2.0.0") - h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL+"/").Routes() + time.Sleep(5 * time.Millisecond) + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, path, nil)) + return w +} - first := httptest.NewRecorder() - h.ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/left-pad", nil)) - if first.Code != http.StatusOK { - t.Fatalf("first request: status = %d", first.Code) - } - time.Sleep(5 * time.Millisecond) +// staleStatuses are upstream answers the metadata cache falls back on. +var staleStatuses = []int{http.StatusBadGateway, http.StatusNotFound, http.StatusUnauthorized, http.StatusForbidden} - w := httptest.NewRecorder() - h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/left-pad/1.0.0", nil)) - if w.Code != tt.wantStatus { - t.Fatalf("status = %d, want %d, body %s", w.Code, tt.wantStatus, w.Body) - } - if stale := w.Header().Get("Warning") == staleWarning; stale != tt.wantStale { - t.Errorf("Warning = %q, want stale %v", w.Header().Get("Warning"), tt.wantStale) - } - }) - } -} +const staleComposerDocument = `{"packages":{"acme/widget":[{"name":"acme/widget","version":"1.0.0",` + + `"dist":{"type":"zip","url":"https://repo.packagist.org/files/acme/widget/1.0.0/widget.zip"}}]},"minified":"composer/2.0"}` -func TestProxyCachedPassesDefiniteAnswersThrough(t *testing.T) { - for _, status := range []int{http.StatusNotFound, http.StatusUnauthorized, http.StatusForbidden} { +func TestComposerStaleMetadataIsMarked(t *testing.T) { + for _, status := range staleStatuses { t.Run(http.StatusText(status), func(t *testing.T) { - upstream := npmUpstreamThen(t, func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(status) }) - proxy, _, _, _ := setupTestProxy(t) - proxy.CacheMetadata = true - proxy.MetadataTTL = time.Millisecond - proxy.HTTPClient = upstream.Client() - - w := httptest.NewRecorder() - proxy.ProxyCached(w, httptest.NewRequest(http.MethodGet, "/doc", nil), upstream.URL+"/doc", "test-eco", "doc") - time.Sleep(5 * time.Millisecond) - w = httptest.NewRecorder() - proxy.ProxyCached(w, httptest.NewRequest(http.MethodGet, "/doc", nil), upstream.URL+"/doc", "test-eco", "doc") + upstream := upstreamThen(t, staleComposerDocument, status) + h := NewComposerHandlerWithUpstreams(staleProxy(t, upstream), "http://proxy.test", upstream.URL, upstream.URL).Routes() - if w.Code == http.StatusOK || strings.Contains(w.Body.String(), "left-pad") { - t.Errorf("status = %d, body %q, want the upstream's answer instead of the cached copy", w.Code, w.Body) + w := requestTwice(t, h, "/p2/acme/widget.json") + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 from the cache, body %s", w.Code, w.Body) + } + if got := w.Header().Get("Warning"); got != staleWarning { + t.Errorf("Warning = %q, want %q", got, staleWarning) } }) } } -const staleComposerDocument = `{"packages":{"acme/widget":[{"name":"acme/widget","version":"1.0.0",` + - `"dist":{"type":"zip","url":"https://repo.packagist.org/files/acme/widget/1.0.0/widget.zip"}}]},"minified":"composer/2.0"}` +const staleChefUniverse = `{"apt":{"7.5.0":{"location_type":"supermarket",` + + `"location_path":"https://supermarket.chef.io/api/v1","download_url":` + + `"https://supermarket.chef.io/api/v1/cookbooks/apt/versions/7.5.0/download","dependencies":{}}}}` -func TestComposerMetadataAfterUpstreamAnswer(t *testing.T) { - tests := []struct { - name string - status int - wantStatus int - wantStale bool - }{ - {name: "upstream error", status: http.StatusBadGateway, wantStatus: http.StatusOK, wantStale: true}, - {name: "removed", status: http.StatusNotFound, wantStatus: http.StatusNotFound}, - {name: "credentials rejected", status: http.StatusUnauthorized, wantStatus: http.StatusBadGateway}, - {name: "access forbidden", status: http.StatusForbidden, wantStatus: http.StatusBadGateway}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var requests atomic.Int32 - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - if requests.Add(1) == 1 { - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(staleComposerDocument)) - return - } - w.WriteHeader(tt.status) - })) - t.Cleanup(upstream.Close) - proxy, _, _, _ := setupTestProxy(t) - proxy.CacheMetadata = true - proxy.MetadataTTL = time.Millisecond - proxy.HTTPClient = upstream.Client() - h := NewComposerHandlerWithUpstreams(proxy, "http://proxy.test", upstream.URL, upstream.URL).Routes() - - first := httptest.NewRecorder() - h.ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/p2/acme/widget.json", nil)) - if first.Code != http.StatusOK { - t.Fatalf("first request: status = %d, body %s", first.Code, first.Body) - } - time.Sleep(5 * time.Millisecond) +func TestChefStaleMetadataIsMarked(t *testing.T) { + for _, status := range staleStatuses { + t.Run(http.StatusText(status), func(t *testing.T) { + upstream := upstreamThen(t, staleChefUniverse, status) + h := NewChefHandler(staleProxy(t, upstream), chefTestProxyURL, upstream.URL).Routes() - w := httptest.NewRecorder() - h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/p2/acme/widget.json", nil)) - if w.Code != tt.wantStatus { - t.Fatalf("status = %d, want %d, body %s", w.Code, tt.wantStatus, w.Body) + w := requestTwice(t, h, "/universe") + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 from the cache, body %s", w.Code, w.Body) } - if stale := w.Header().Get("Warning") == staleWarning; stale != tt.wantStale { - t.Errorf("Warning = %q, want stale %v", w.Header().Get("Warning"), tt.wantStale) + if got := w.Header().Get("Warning"); got != staleWarning { + t.Errorf("Warning = %q, want %q", got, staleWarning) } }) }