From db6d1d672d52bedf2d67ada4a50216c96652d78b Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 17 Oct 2026 14:10:55 +0200 Subject: [PATCH] Request npm packuments in one format for downloads too The metadata route caches a package's packument in the abbreviated install format, unless cooldown or npm_full_metadata needs the full one. Tarball downloads read the packument from the same cache row but asked for application/json. Once the row was due for revalidation, a tarball the proxy had not cached yet fetched the full packument, which did not match the abbreviated document's ETag, and stored it in its place. Until the next refresh every client was then served the full document, typically several times the size. Both now use packumentAccept. Downloads read only dist.tarball, dist.integrity and dist.shasum, which the abbreviated format carries; the publish times the cooldown needs come with the full format it selects. --- internal/handler/npm.go | 34 +++++--- internal/handler/npm_packument_accept_test.go | 78 +++++++++++++++++++ 2 files changed, 100 insertions(+), 12 deletions(-) create mode 100644 internal/handler/npm_packument_accept_test.go diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 832a8601..c92bf2ad 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -207,17 +207,7 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques upstreamURL := registryURL + "/" + registryPath metadataCacheKey := npmMetadataCacheKey(cacheKey, packagePath, packageName) - // Prefer the smaller abbreviated packument format but include application/json - // as a fallback so upstreams that reject the abbreviated type (e.g. JFrog - // Artifactory, which returns 406) can still respond with full metadata. - // When cooldown is enabled we must use full metadata exclusively because the - // abbreviated format omits the "time" map required for version age filtering. - // Operators can also force full metadata so clients that gate on publish - // age (for example Yarn's npmMinimalAgeGate) keep working through the proxy. - accept := npmAcceptDefault - if h.proxy.NPMFullMetadata || (h.proxy.Cooldown != nil && h.proxy.Cooldown.Enabled()) { - accept = contentTypeJSON - } + accept := h.packumentAccept() specifier, versionLookup := npmVersionSpecifier(packagePath, packageName) if h.serveLocalMetadata(w, r, npmMetadataRequest{packageName, specifier, versionLookup, upstreamURL, accept, metadataCacheKey}) { @@ -282,6 +272,26 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques serveRewrittenMetadata(w, r, rewritten) } +// 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 +// (e.g. JFrog Artifactory, which returns 406) can still respond with full +// metadata. When cooldown is enabled we must use full metadata exclusively +// because the abbreviated format omits the "time" map required for version age +// filtering. Operators can also force full metadata so clients that gate on +// publish age (for example Yarn's npmMinimalAgeGate) keep working through the +// proxy. +// +// Every request for a packument uses it, since they share one cache row: a +// request with another Accept would replace the document the others are +// served with one in the other format. +func (h *NPMHandler) packumentAccept() string { + if h.proxy.NPMFullMetadata || h.proxy.cooldownEnabled() { + return contentTypeJSON + } + return npmAcceptDefault +} + // handleFilteredNPMVersionMetadata serves /pkg/ and /pkg/ from the // 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) { @@ -841,7 +851,7 @@ func (h *NPMHandler) handleDownload(w http.ResponseWriter, r *http.Request) { registryURL, cacheKey := h.sourceFor(packageName) metadata := sync.OnceValues(func() ([]byte, error) { upstreamURL := registryURL + "/" + url.PathEscape(packageName) - body, _, err := h.proxy.FetchOrCacheMetadata(r.Context(), "npm", cacheKey, upstreamURL, contentTypeJSON) + body, _, err := h.proxy.FetchOrCacheMetadata(r.Context(), "npm", cacheKey, upstreamURL, h.packumentAccept()) return body, err }) if h.versionInCooldown(packageName, version, metadata) { diff --git a/internal/handler/npm_packument_accept_test.go b/internal/handler/npm_packument_accept_test.go new file mode 100644 index 00000000..7158cefa --- /dev/null +++ b/internal/handler/npm_packument_accept_test.go @@ -0,0 +1,78 @@ +package handler + +import ( + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/git-pkgs/registries/fetch" +) + +// TestNPMTarballKeepsCachedPackumentFormat checks that a tarball download +// revalidates the packument in the format the metadata route cached it in, +// rather than replacing the abbreviated document every client is served with +// the full one. +func TestNPMTarballKeepsCachedPackumentFormat(t *testing.T) { + const abbreviated = "application/vnd.npm.install-v1+json" + var ( + mu sync.Mutex + accepts []string + ) + var upstream *httptest.Server + upstream = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + accept := r.Header.Get("Accept") + mu.Lock() + accepts = append(accepts, accept) + mu.Unlock() + contentType, etag := contentTypeJSON, `"full"` + if strings.HasPrefix(accept, abbreviated) { + contentType, etag = abbreviated, `"abbreviated"` + } + w.Header().Set(headerETag, etag) + if r.Header.Get("If-None-Match") == etag { + w.WriteHeader(http.StatusNotModified) + return + } + w.Header().Set(headerContentType, contentType) + _, _ = fmt.Fprintf(w, `{"name":"demo","versions":{"1.0.0":{"name":"demo","version":"1.0.0","dist":{"tarball":%q}}}}`, + upstream.URL+"/demo/-/demo-1.0.0.tgz") + })) + t.Cleanup(upstream.Close) + + proxy, db, _, artifactFetcher := setupTestProxy(t) + proxy.CacheMetadata = true + proxy.MetadataTTL = time.Nanosecond // the tarball request revalidates + proxy.HTTPClient = upstream.Client() + artifactFetcher.artifact = &fetch.Artifact{Body: io.NopCloser(strings.NewReader("package")), ContentType: "application/gzip"} + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL) + + if w := serveNPM(t, h, http.MethodGet, "/demo", nil, nil); w.Code != http.StatusOK { + t.Fatalf("metadata: status %d: %s", w.Code, w.Body.String()) + } + if w := serveNPM(t, h, http.MethodGet, "/demo/-/demo-1.0.0.tgz", nil, nil); w.Code != http.StatusOK { + t.Fatalf("tarball: status %d: %s", w.Code, w.Body.String()) + } + + mu.Lock() + defer mu.Unlock() + if len(accepts) != 2 { + t.Fatalf("upstream packument requests = %d (%q), want 2", len(accepts), accepts) + } + for _, accept := range accepts { + if accept != npmAcceptDefault { + t.Errorf("packument requested with Accept %q, want %q", accept, npmAcceptDefault) + } + } + entry, err := db.GetMetadataCache("npm", "demo") + if err != nil || entry == nil { + t.Fatalf("cache row: %v, %v", entry, err) + } + if entry.ContentType.String != abbreviated { + t.Errorf("cached packument Content-Type = %q, want %q", entry.ContentType.String, abbreviated) + } +}