From a524fa95fb26ced7e77a3d741dccf777da3ea20e Mon Sep 17 00:00:00 2001 From: Mohamed Labiadh <47183211+medlabib@users.noreply.github.com> Date: Sun, 18 Oct 2026 01:08:58 +0100 Subject: [PATCH] Flag stale npm metadata and relay definitive npm answers npm metadata served through the rewrite cache lost the stale Warning header, so a cached packument looked fresh after an upstream failure. serveRewrittenMetadata now sets Warning 110 when the body came from the stale fallback. The npm handler also relays a definitive registry answer instead of serving the cached package: 404 for an unpublished package and 401/403 for rejected credentials, carried in an upstreamStatusError. The no-fallback rule is scoped to the npm ecosystem; other ecosystems keep their documented stale fallback (covered by a regression test through the generic metadata handler), and npm still falls back for transport errors and retryable statuses (429, 5xx). Routed packages keep answering 502, since a route's credentials are the proxy's concern and a relayed 401 would send npm off authenticating to the proxy. Fixes #499 Signed-off-by: Mohamed Labiadh <47183211+medlabib@users.noreply.github.com> --- internal/handler/handler.go | 27 +++ internal/handler/npm.go | 36 +++- internal/handler/npm_metadata_stale_test.go | 199 ++++++++++++++++++++ internal/handler/rewrite_cache.go | 7 +- 4 files changed, 259 insertions(+), 10 deletions(-) create mode 100644 internal/handler/npm_metadata_stale_test.go diff --git a/internal/handler/handler.go b/internal/handler/handler.go index d29508eb..4f88770d 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -1204,6 +1204,17 @@ var ErrUpstreamNotFound = fmt.Errorf("upstream: %w", fetch.ErrNotFound) // upstreamNotFoundError. const upstreamNotFoundBodyLimit = 4 << 10 +// upstreamStatusError is a definitive answer from upstream, not a transport +// failure or a retryable status. npm relays it instead of serving the cached +// copy. +type upstreamStatusError struct { + status int +} + +func (e *upstreamStatusError) Error() string { + return fmt.Sprintf("upstream returned %d", e.status) +} + // upstreamNotFoundError is ErrUpstreamNotFound with the start of the // upstream's 404 body, for handlers that relay the reason to clients. type upstreamNotFoundError struct { @@ -1391,6 +1402,16 @@ func (p *Proxy) fetchMetadataFromUpstream(ctx context.Context, ecosystem, cacheK return metadataResult{body: meta.body, contentType: meta.contentType, contentEncoding: meta.contentEncoding, digest: digest} } + // npm relays a definitive answer from the registry: serving the cached + // copy would hide an unpublished package (404) or credentials it rejected + // (401/403). Other ecosystems keep their documented stale fallback, and + // transport failures and retryable statuses (429, 5xx) fall back for npm + // too. + var upstreamStatus *upstreamStatusError + if ecosystem == "npm" && (errors.Is(err, ErrUpstreamNotFound) || errors.As(err, &upstreamStatus)) { + 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)} @@ -1607,6 +1628,12 @@ func (p *Proxy) fetchUpstreamMetadata(ctx context.Context, upstreamURL string, e return nil, &upstreamNotFoundError{body: body} } if resp.StatusCode != http.StatusOK { + // 401 and 403 are the upstream answering, not failing: record them + // as definitive so npm can relay them instead of serving the cached + // copy. + if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { + return nil, &upstreamStatusError{status: resp.StatusCode} + } return nil, fmt.Errorf("upstream returned %d", resp.StatusCode) } diff --git a/internal/handler/npm.go b/internal/handler/npm.go index a874c61c..b85ae51a 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -201,6 +201,7 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques h.proxy.Logger.Info("npm metadata request", "package", packageName, "path", packagePath) registryURL, cacheKey := h.sourceFor(packageName) + _, routed := h.routes.match(packageName) // A version or dist-tag is its own path segment. Escaping the whole path // turns "lodash/4.17.21" into "lodash%2F4.17.21", which the registry // rejects with 405. @@ -226,15 +227,7 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques upstream := h.proxy.fetchMetadataDocument(r.Context(), "npm", metadataCacheKey, upstreamURL, accept) if err := upstream.err; err != nil { - if errors.Is(err, ErrUpstreamNotFound) { - if versionLookup { - JSONError(w, http.StatusNotFound, ErrVersionNotFound.Error()) - } else { - JSONError(w, http.StatusNotFound, ErrPackageNotFound.Error()) - } - return - } - if writeUpstreamHeld(w, err) { + if h.writeNPMMetadataError(w, err, versionLookup, routed) { return } h.proxy.Logger.Error("failed to fetch npm metadata", "error", err) @@ -269,9 +262,34 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques return } + rewritten.stale = upstream.stale serveRewrittenMetadata(w, r, rewritten) } +// writeNPMMetadataError writes the response for a failed npm metadata lookup +// and reports whether it did. When it reports false the caller answers the +// upstream it could not reach with 502. +func (h *NPMHandler) writeNPMMetadataError(w http.ResponseWriter, err error, versionLookup, routed bool) bool { + if errors.Is(err, ErrUpstreamNotFound) { + if versionLookup { + JSONError(w, http.StatusNotFound, ErrVersionNotFound.Error()) + } else { + JSONError(w, http.StatusNotFound, ErrPackageNotFound.Error()) + } + return true + } + // A route's own credentials are the proxy's business, not the client's: + // relaying its 401 would send npm off authenticating to the proxy. The + // default registry's answer is relayed instead, so broken upstream + // credentials fail loudly rather than serving a stale 200. + var upstreamStatus *upstreamStatusError + if errors.As(err, &upstreamStatus) && !routed { + JSONError(w, upstreamStatus.status, "upstream rejected the request") + return true + } + return writeUpstreamHeld(w, err) +} + // packumentAccept returns the Accept header packuments are requested with. // It prefers the smaller abbreviated packument format but includes // application/json as a fallback so upstreams that reject the abbreviated type diff --git a/internal/handler/npm_metadata_stale_test.go b/internal/handler/npm_metadata_stale_test.go new file mode 100644 index 00000000..fb48adde --- /dev/null +++ b/internal/handler/npm_metadata_stale_test.go @@ -0,0 +1,199 @@ +package handler + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" +) + +// testPackument is a minimal npm packument the proxy can rewrite. +const testPackument = `{"name":"testpkg","versions":{"1.0.0":{"name":"testpkg","version":"1.0.0","dist":{"tarball":"https://registry.npmjs.org/testpkg/-/testpkg-1.0.0.tgz"}}}}` + +// newStaleMetadataUpstream serves testPackument until status holds a non-zero +// HTTP status, which it answers instead. +func newStaleMetadataUpstream(t *testing.T) (*httptest.Server, *atomic.Int32) { + t.Helper() + var status atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if code := status.Load(); code != 0 { + w.WriteHeader(int(code)) + return + } + w.Header().Set("Content-Type", contentTypeJSON) + _, _ = io.WriteString(w, testPackument) + })) + t.Cleanup(server.Close) + return server, &status +} + +// newStaleMetadataHandler returns an npm handler over an expiring metadata +// cache, with the rewrite cache enabled when rewriteCache is true. +func newStaleMetadataHandler(t *testing.T, upstreamURL string, rewriteCache bool) *NPMHandler { + t.Helper() + proxy, _, _, _ := setupTestProxy(t) + proxy.CacheMetadata = true + proxy.MetadataTTL = time.Millisecond + proxy.HTTPClient = http.DefaultClient + if rewriteCache { + proxy.SetMetadataRewriteCacheSize(1 << 20) + } + return NewNPMHandler(proxy, "http://proxy.test", upstreamURL) +} + +// primeStaleMetadataCache fetches the packument once and lets the entry expire, +// so the next request goes upstream again. +func primeStaleMetadataCache(t *testing.T, h *NPMHandler) string { + t.Helper() + w := serveNPM(t, h, http.MethodGet, "/testpkg", nil, nil) + if w.Code != http.StatusOK { + t.Fatalf("priming request: status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + time.Sleep(5 * time.Millisecond) + return w.Body.String() +} + +func TestNPMMetadataStaleWarningAfterUpstreamFailure(t *testing.T) { + for _, rewriteCache := range []bool{false, true} { + t.Run(fmt.Sprintf("rewrite cache=%v", rewriteCache), func(t *testing.T) { + upstream, status := newStaleMetadataUpstream(t) + h := newStaleMetadataHandler(t, upstream.URL, rewriteCache) + primed := primeStaleMetadataCache(t, h) + + status.Store(http.StatusBadGateway) + w := serveNPM(t, h, http.MethodGet, "/testpkg", nil, nil) + + if w.Code != http.StatusOK { + t.Fatalf("stale request: status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + if w.Body.String() != primed { + t.Errorf("stale body = %q, want the cached %q", w.Body.String(), primed) + } + if got := w.Header().Get("Warning"); got != `110 - "Response is Stale"` { + t.Errorf("Warning = %q, want the stale warning", got) + } + }) + } +} + +func TestNPMMetadataFreshResponseHasNoStaleWarning(t *testing.T) { + upstream, _ := newStaleMetadataUpstream(t) + h := newStaleMetadataHandler(t, upstream.URL, true) + primed := primeStaleMetadataCache(t, h) + + w := serveNPM(t, h, http.MethodGet, "/testpkg", nil, nil) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + if w.Body.String() != primed { + t.Errorf("body = %q, want %q", w.Body.String(), primed) + } + if got := w.Header().Get("Warning"); got != "" { + t.Errorf("Warning = %q, want none on a fresh response", got) + } +} + +// TestNPMMetadataPassesDefinitiveUpstreamAnswersThrough checks that an +// unpublished package or rejected credentials reach the client instead of the +// cached copy. +func TestNPMMetadataPassesDefinitiveUpstreamAnswersThrough(t *testing.T) { + for _, code := range []int{http.StatusNotFound, http.StatusUnauthorized, http.StatusForbidden} { + t.Run(http.StatusText(code), func(t *testing.T) { + upstream, status := newStaleMetadataUpstream(t) + h := newStaleMetadataHandler(t, upstream.URL, true) + primed := primeStaleMetadataCache(t, h) + + status.Store(int32(code)) + w := serveNPM(t, h, http.MethodGet, "/testpkg", nil, nil) + + if w.Code != code { + t.Fatalf("status = %d, want %d; body: %s", w.Code, code, w.Body.String()) + } + if w.Body.String() == primed { + t.Error("the cached packument was served for a definitive upstream answer") + } + if got := w.Header().Get("Warning"); got != "" { + t.Errorf("Warning = %q, want none", got) + } + var body map[string]any + if err := json.NewDecoder(strings.NewReader(w.Body.String())).Decode(&body); err != nil { + t.Fatalf("response body is not JSON: %v", err) + } + if _, ok := body["error"]; !ok { + t.Errorf("response body %v missing \"error\" key", body) + } + }) + } +} + +// TestNPMMetadataServesStaleOnRetryableStatus guards the offline fallback for +// rate limits and outages, which must still serve the cached copy. +func TestNPMMetadataServesStaleOnRetryableStatus(t *testing.T) { + upstream, status := newStaleMetadataUpstream(t) + h := newStaleMetadataHandler(t, upstream.URL, true) + primed := primeStaleMetadataCache(t, h) + + status.Store(http.StatusServiceUnavailable) + w := serveNPM(t, h, http.MethodGet, "/testpkg", nil, nil) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + if w.Body.String() != primed { + t.Errorf("body = %q, want the cached %q", w.Body.String(), primed) + } + if got := w.Header().Get("Warning"); got != `110 - "Response is Stale"` { + t.Errorf("Warning = %q, want the stale warning", got) + } +} + +// TestGenericMetadataKeepsStaleFallback guards the documented fallback for +// ecosystems other than npm (#486): a cached document is still served when +// upstream answers 404/401/403. Only npm relays those answers. +func TestGenericMetadataKeepsStaleFallback(t *testing.T) { + for _, code := range []int{http.StatusNotFound, http.StatusUnauthorized, http.StatusForbidden} { + t.Run(http.StatusText(code), func(t *testing.T) { + upstream, status := newStaleMetadataUpstream(t) + + proxy, _, _, _ := setupTestProxy(t) + proxy.CacheMetadata = true + proxy.MetadataTTL = time.Millisecond + proxy.HTTPClient = http.DefaultClient + + upstreamURL := upstream.URL + "/testpkg" + primed := primeGenericMetadataCache(t, proxy, upstreamURL) + time.Sleep(5 * time.Millisecond) + + status.Store(int32(code)) + w := httptest.NewRecorder() + proxy.ProxyCached(w, httptest.NewRequest(http.MethodGet, "/testpkg", nil), upstreamURL, "composer", "testpkg") + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 from the cached copy; body: %s", w.Code, w.Body.String()) + } + if w.Body.String() != primed { + t.Errorf("body = %q, want the cached %q", w.Body.String(), primed) + } + if got := w.Header().Get("Warning"); got != `110 - "Response is Stale"` { + t.Errorf("Warning = %q, want the stale warning", got) + } + }) + } +} + +// primeGenericMetadataCache fetches a metadata document once and returns the +// body, so a later request can be compared against it. +func primeGenericMetadataCache(t *testing.T, proxy *Proxy, upstreamURL string) string { + t.Helper() + w := httptest.NewRecorder() + proxy.ProxyCached(w, httptest.NewRequest(http.MethodGet, "/testpkg", nil), upstreamURL, "composer", "testpkg") + if w.Code != http.StatusOK { + t.Fatalf("priming request: status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + return w.Body.String() +} diff --git a/internal/handler/rewrite_cache.go b/internal/handler/rewrite_cache.go index 83ce2a10..8225b5f1 100644 --- a/internal/handler/rewrite_cache.go +++ b/internal/handler/rewrite_cache.go @@ -81,11 +81,13 @@ type metadataValidators struct { // rewrittenMetadata is a rewritten metadata document with its validators. // gzipped is the document gzip-compressed, or nil when there is no -// compressed copy to serve. +// compressed copy to serve. stale is set when the body is the cached copy +// served because upstream failed. type rewrittenMetadata struct { body []byte metadataValidators gzipped []byte + stale bool } // metadataETag returns the strong ETag of a rewritten document. @@ -351,6 +353,9 @@ func serveRewrittenMetadata(w http.ResponseWriter, r *http.Request, m rewrittenM w.WriteHeader(http.StatusNotModified) return } + if m.stale { + w.Header().Set("Warning", `110 - "Response is Stale"`) + } w.Header().Set(headerContentType, contentTypeJSON) if compressed { w.Header().Set(headerContentEncoding, "gzip")