From b6d7575b88960355de4c6e34a6197336025f1364 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 24 Oct 2026 08:06:10 +0200 Subject: [PATCH] Don't fetch npm tarballs whose packument can't be read An uncached tarball is checked against the integrity its packument declares. When the packument couldn't be fetched or was refused, for example because it is larger than metadata_max_size or the registry answered 503, the download went ahead with no integrity at all, and the unverified tarball was cached and served to every later client. Without the packument the download now fails like the Composer one does: 502, or 404 when the registry says the package doesn't exist. Nothing is fetched or cached. Tarballs already in the cache are still served, since they were verified when they were stored, and a packument that lists the version without a usable checksum still falls back to an unverified fetch as before. --- internal/handler/npm.go | 21 ++++--- internal/handler/npm_routes_test.go | 11 +++- internal/handler/npm_tarball_test.go | 67 ++++++++++++++++++++--- internal/handler/npm_test.go | 4 ++ internal/handler/stream_artifacts_test.go | 2 +- 5 files changed, 83 insertions(+), 22 deletions(-) diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 0e2d8f26..ba1e2b1b 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -955,18 +955,21 @@ func (h *NPMHandler) getTarball(r *http.Request, registryURL, packageName, versi if cached, err := h.proxy.GetCachedArtifact(r.Context(), "npm", packageName, version, cacheFilename); err != nil || cached != nil { return cached, err } - downloadURL := fmt.Sprintf("%s/%s/-/%s", registryURL, escapeNPMDownloadPackage(packageName), url.PathEscape(filename)) - var sri string + // The packument says where the tarball is and what it must hash to. + // Without it the tarball could not be verified, and an unverified copy + // would be cached and served to everyone, so the download fails instead. body, err := metadata() - if err == nil { - if tarball := npmVersionTarball(body, version); tarball != "" { - downloadURL, err = validateTarballURL(tarball, registryURL) - if err != nil { - return nil, err - } + if err != nil { + return nil, fmt.Errorf("fetching npm metadata to verify tarball: %w", err) + } + downloadURL := fmt.Sprintf("%s/%s/-/%s", registryURL, escapeNPMDownloadPackage(packageName), url.PathEscape(filename)) + if tarball := npmVersionTarball(body, version); tarball != "" { + downloadURL, err = validateTarballURL(tarball, registryURL) + if err != nil { + return nil, err } - sri = npmVersionIntegrity(body, version) } + sri := npmVersionIntegrity(body, version) return h.proxy.GetOrFetchArtifactFromURLWithIntegrity(r.Context(), "npm", packageName, version, cacheFilename, downloadURL, sri) } diff --git a/internal/handler/npm_routes_test.go b/internal/handler/npm_routes_test.go index a2ffb38e..56fc952b 100644 --- a/internal/handler/npm_routes_test.go +++ b/internal/handler/npm_routes_test.go @@ -190,10 +190,15 @@ func TestNPMRoutedDownloadWithoutMetadataStaysOnRoute(t *testing.T) { } h := newRoutedNPMHandler(proxy, public, private) - serveNPM(t, h, http.MethodGet, "/@example/widgets/-/widgets-1.0.0.tgz", nil, nil) + w := serveNPM(t, h, http.MethodGet, "/@example/widgets/-/widgets-1.0.0.tgz", nil, nil) - if !strings.HasPrefix(artifactFetcher.fetchedURL, private.URL+"/registry/") { - t.Errorf("fetched URL = %q, want one on the route %q", artifactFetcher.fetchedURL, private.URL+"/registry/") + // Without the route's metadata the tarball can't be verified, so it is + // not fetched at all, and in particular not from the default registry. + if w.Code != http.StatusBadGateway { + t.Errorf("status = %d, want 502", w.Code) + } + if artifactFetcher.fetchedURL != "" { + t.Errorf("fetched URL = %q, want none", artifactFetcher.fetchedURL) } } diff --git a/internal/handler/npm_tarball_test.go b/internal/handler/npm_tarball_test.go index 303f2184..fe1508d8 100644 --- a/internal/handler/npm_tarball_test.go +++ b/internal/handler/npm_tarball_test.go @@ -92,12 +92,9 @@ func npmContentAddressedRegistry(t *testing.T, metadataCalls, tarballCalls *atom } func TestNPMTarballMetadataFallback(t *testing.T) { - for _, body := range []string{`{"versions":{}}`, `{"versions":{"1.0.0":{"dist":{}}}}`, `not json`, "unavailable"} { + for _, body := range []string{`{"versions":{}}`, `{"versions":{"1.0.0":{"dist":{}}}}`} { t.Run(body, func(t *testing.T) { upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - if body == "unavailable" { - w.WriteHeader(http.StatusServiceUnavailable) - } _, _ = io.WriteString(w, body) })) t.Cleanup(upstream.Close) @@ -106,12 +103,10 @@ func TestNPMTarballMetadataFallback(t *testing.T) { p.MetadataTTL = time.Hour fetcher.artifact = &fetch.Artifact{Body: io.NopCloser(strings.NewReader("package"))} h := NewNPMHandler(p, "http://proxy.test", upstream.URL) - if body != "unavailable" { - if _, _, err := p.FetchOrCacheMetadata(t.Context(), "npm", "@example/widget", upstream.URL); err != nil { - t.Fatal(err) - } - upstream.Close() + if _, _, err := p.FetchOrCacheMetadata(t.Context(), "npm", "@example/widget", upstream.URL); err != nil { + t.Fatal(err) } + upstream.Close() response := httptest.NewRecorder() h.Routes().ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/@example/widget/-/widget-1.0.0.tgz", nil)) if response.Code != http.StatusOK { @@ -124,6 +119,60 @@ func TestNPMTarballMetadataFallback(t *testing.T) { } } +// TestNPMTarballWithoutMetadataIsNotFetched checks that a tarball whose +// packument can't be read, so its integrity is unknown, is neither fetched +// nor cached. +func TestNPMTarballWithoutMetadataIsNotFetched(t *testing.T) { + for _, tc := range []struct { + name string + status int + body string + limit int64 + }{ + {name: "unavailable", status: http.StatusServiceUnavailable, body: "unavailable"}, + {name: "over metadata_max_size", status: http.StatusOK, body: `{"versions":{"1.0.0":{"dist":{"integrity":"` + sha512SRI("package") + `"}}}}`, limit: 16}, + } { + t.Run(tc.name, func(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tc.status) + _, _ = io.WriteString(w, tc.body) + })) + t.Cleanup(upstream.Close) + p, _, _, fetcher := setupTestProxy(t) + if tc.limit > 0 { + p.MetadataMaxSize = tc.limit + } + fetcher.artifact = &fetch.Artifact{Body: io.NopCloser(strings.NewReader("tampered"))} + h := NewNPMHandler(p, "http://proxy.test", upstream.URL) + + response := httptest.NewRecorder() + h.Routes().ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/@example/widget/-/widget-1.0.0.tgz", nil)) + if response.Code != http.StatusBadGateway { + t.Fatalf("status = %d, want 502: %s", response.Code, response.Body.String()) + } + if fetcher.fetchedURL != "" { + t.Errorf("fetched %q without the metadata to verify it", fetcher.fetchedURL) + } + cached, err := p.GetCachedArtifact(t.Context(), "npm", "@example/widget", "1.0.0", "widget-1.0.0.tgz") + if err != nil { + t.Fatal(err) + } + if cached != nil { + _ = cached.Reader.Close() + t.Error("tarball was cached without verification") + } + }) + } +} + +// serveEmptyPackuments answers every metadata request with a packument that +// lists no versions, for tests about the tarball download itself. +func serveEmptyPackuments(p *Proxy) { + p.HTTPClient = &http.Client{Transport: pypiRoundTripFunc(func(r *http.Request) (*http.Response, error) { + return pypiHTTPResponse(r, "application/json", `{"versions":{}}`), nil + })} +} + func TestNPMTarballRejectsUnsafeMetadataURL(t *testing.T) { for _, target := range []string{ "https://outside.invalid/package.tgz", "//outside.invalid/package.tgz", "/registry/package.tgz", diff --git a/internal/handler/npm_test.go b/internal/handler/npm_test.go index 23e223fb..bbec4c65 100644 --- a/internal/handler/npm_test.go +++ b/internal/handler/npm_test.go @@ -96,6 +96,7 @@ func TestNPMHandlerUsesConfiguredUpstream(t *testing.T) { Body: io.NopCloser(strings.NewReader("package")), ContentType: "application/gzip", } + serveEmptyPackuments(proxy) h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test/root/") req := httptest.NewRequest(http.MethodGet, "/testpkg/-/testpkg-1.0.0.tgz", nil) @@ -117,6 +118,7 @@ func TestNPMHandlerUsesConfiguredUpstream(t *testing.T) { Body: io.NopCloser(strings.NewReader("package")), ContentType: "application/gzip", } + serveEmptyPackuments(proxy) h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test/root/") req := httptest.NewRequest(http.MethodGet, "/@scope/name/-/name-1.0.0.tgz", nil) @@ -693,6 +695,7 @@ func TestNPMDownloadErrorResponsesAreJSON(t *testing.T) { ContentType: "application/octet-stream", } + serveEmptyPackuments(proxy) h := NewNPMHandler(proxy, "http://proxy.test", "http://upstream.invalid") srv := httptest.NewServer(h.Routes()) defer srv.Close() @@ -1053,6 +1056,7 @@ func TestNPMTarballStillRoutesToDownload(t *testing.T) { Body: io.NopCloser(strings.NewReader("package")), ContentType: "application/gzip", } + serveEmptyPackuments(proxy) h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test") w := serveNPM(t, h, http.MethodGet, "/lodash/-/lodash-4.17.21.tgz", nil, nil) diff --git a/internal/handler/stream_artifacts_test.go b/internal/handler/stream_artifacts_test.go index 3a5929b7..af0e3f16 100644 --- a/internal/handler/stream_artifacts_test.go +++ b/internal/handler/stream_artifacts_test.go @@ -317,7 +317,7 @@ func TestStreamArtifactsNPMTarballWithoutFinalChunkIsIncomplete(t *testing.T) { upstream := truncatingUpstream(t, func(r *http.Request) bool { return strings.HasSuffix(r.URL.Path, ".tgz") }, "HTTP/1.1 200 OK\r\nContent-Type: application/octet-stream\r\nTransfer-Encoding: chunked\r\n\r\n5\r\nshort\r\n", - http.NotFound) + func(w http.ResponseWriter, _ *http.Request) { _, _ = io.WriteString(w, `{"versions":{}}`) }) proxy, _, store, _ := setupTestProxy(t) proxy.StreamArtifacts = true