Don't fetch npm tarballs whose packument can't be read

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)
 }
 

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)
 	}
 }
 

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",

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)

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