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