From 8266cbfd4834f4ff0c7a9dd54875e159568976e6 Mon Sep 17 00:00:00 2001 From: Sivamuthu Kumar Date: Mon, 10 Aug 2026 13:29:00 -0400 Subject: [PATCH 1/2] fix(npm): enhance GitHub packages tarball handling and add tests --- internal/handler/npm.go | 116 ++++++++++++++++--- internal/handler/npm_test.go | 215 +++++++++++++++++++++++++++++++++-- 2 files changed, 303 insertions(+), 28 deletions(-) diff --git a/internal/handler/npm.go b/internal/handler/npm.go index b7d96a3..2e920f9 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -202,10 +202,7 @@ func (h *NPMHandler) rewriteTarballURLs(versions map[string]any, packageName str continue } - filename := tarball - if idx := strings.LastIndex(tarball, "/"); idx >= 0 { - filename = tarball[idx+1:] - } + filename := h.proxyTarballFilename(packageName, version, tarball) escapedName := url.PathEscape(packageName) newTarball := fmt.Sprintf("%s/npm/%s/-/%s", h.proxyURL, escapedName, filename) @@ -217,6 +214,30 @@ func (h *NPMHandler) rewriteTarballURLs(versions map[string]any, packageName str } } +func (h *NPMHandler) proxyTarballFilename(packageName, version, tarball string) string { + filename := tarball + if idx := strings.LastIndex(tarball, "/"); idx >= 0 { + filename = tarball[idx+1:] + } + if h.extractVersionFromFilename(packageName, filename) != "" { + return filename + } + + return npmTarballFilename(packageName, version) +} + +func npmTarballFilename(packageName, version string) string { + return npmPackageShortName(packageName) + "-" + version + ".tgz" +} + +func npmPackageShortName(packageName string) string { + parts := strings.SplitN(packageName, "/", scopedParts) + if len(parts) == scopedParts { + return parts[1] + } + return packageName +} + // findNewestVersion returns the version string with the most recent timestamp // from the remaining versions, using the time map. func (h *NPMHandler) findNewestVersion(versions map[string]any, timeMap map[string]any) string { @@ -275,12 +296,12 @@ func (h *NPMHandler) handleDownload(w http.ResponseWriter, r *http.Request) { return } - downloadURL := fmt.Sprintf( - "%s/%s/-/%s", - h.upstreamURL, - escapeNPMDownloadPackage(packageName), - url.PathEscape(filename), - ) + downloadURL, err := h.downloadURL(r, packageName, version, filename) + if err != nil { + h.proxy.Logger.Error("failed to resolve npm tarball URL", "error", err) + JSONError(w, http.StatusBadRequest, "invalid tarball request") + return + } result, err := h.proxy.GetOrFetchArtifactFromURL( r.Context(), "npm", packageName, version, filename, downloadURL, ) @@ -341,6 +362,73 @@ func (h *NPMHandler) versionInCooldown(r *http.Request, packageName, version str return !h.proxy.Cooldown.IsAllowed("npm", canonicalPackagePURL("npm", packageName), publishedAt) } +func (h *NPMHandler) downloadURL(r *http.Request, packageName, version, filename string) (string, error) { + metadataURL := fmt.Sprintf("%s/%s", h.upstreamURL, url.PathEscape(packageName)) + body, _, err := h.proxy.FetchOrCacheMetadata(r.Context(), "npm", packageName, metadataURL, contentTypeJSON) + if err != nil { + h.proxy.Logger.Warn("could not fetch npm metadata for tarball resolution; using constructed URL", + "package", packageName, "version", version, "error", err) + return h.constructDownloadURL(packageName, filename), nil + } + + var metadata map[string]any + if err := json.Unmarshal(body, &metadata); err != nil { + return "", fmt.Errorf("parsing npm metadata: %w", err) + } + + versions, ok := metadata["versions"].(map[string]any) + if !ok { + return "", errors.New("npm metadata has no versions") + } + vdata, ok := versions[version].(map[string]any) + if !ok { + return "", fmt.Errorf("npm metadata has no version %q", version) + } + dist, ok := vdata["dist"].(map[string]any) + if !ok { + return "", fmt.Errorf("npm metadata version %q has no dist", version) + } + tarball, ok := dist["tarball"].(string) + if !ok { + return "", fmt.Errorf("npm metadata version %q has no tarball", version) + } + + return h.validateUpstreamTarballURL(tarball) +} + +func (h *NPMHandler) constructDownloadURL(packageName, filename string) string { + return fmt.Sprintf( + "%s/%s/-/%s", + h.upstreamURL, + escapeNPMDownloadPackage(packageName), + url.PathEscape(filename), + ) +} + +func (h *NPMHandler) validateUpstreamTarballURL(tarball string) (string, error) { + tarballURL, err := url.Parse(tarball) + if err != nil { + return "", fmt.Errorf("parsing tarball URL: %w", err) + } + upstreamURL, err := url.Parse(h.upstreamURL) + if err != nil { + return "", fmt.Errorf("parsing upstream URL: %w", err) + } + if tarballURL.User != nil || tarballURL.Scheme != upstreamURL.Scheme || + !strings.EqualFold(tarballURL.Host, upstreamURL.Host) { + return "", errors.New("npm tarball URL does not match upstream registry") + } + + basePath := strings.TrimSuffix(upstreamURL.Path, "/") + if basePath != "" && basePath != "/" { + if tarballURL.Path != basePath && !strings.HasPrefix(tarballURL.Path, basePath+"/") { + return "", errors.New("npm tarball URL is outside upstream base path") + } + } + + return tarballURL.String(), nil +} + func escapeNPMDownloadPackage(packageName string) string { scope, name, scoped := strings.Cut(packageName, "/") if scoped && strings.HasPrefix(scope, "@") && len(scope) > 1 && name != "" && !strings.Contains(name, "/") { @@ -399,12 +487,8 @@ func (h *NPMHandler) extractVersionFromFilename(packageName, filename string) st } base := strings.TrimSuffix(filename, ".tgz") - // For scoped packages, the filename uses the short name - shortName := packageName - if strings.Contains(packageName, "/") { - parts := strings.SplitN(packageName, "/", scopedParts) - shortName = parts[1] - } + // For scoped packages, the filename uses the short name. + shortName := npmPackageShortName(packageName) // Expected format: {shortName}-{version} prefix := shortName + "-" diff --git a/internal/handler/npm_test.go b/internal/handler/npm_test.go index 07da9c3..8d87be3 100644 --- a/internal/handler/npm_test.go +++ b/internal/handler/npm_test.go @@ -210,6 +210,195 @@ func TestNPMRewriteMetadataScopedPackage(t *testing.T) { } } +func TestNPMRewriteMetadataGitHubPackagesTarball(t *testing.T) { + h := &NPMHandler{ + proxy: testProxy(), + proxyURL: "http://localhost:8080", + } + + input := `{ + "name": "@example/private-package", + "versions": { + "1.0.0": { + "dist": { + "shasum": "e053d091c6ae91793f6333f5fe0a55633cf3c584", + "tarball": "https://npm.pkg.github.com/download/@example/private-package/1.0.0/e053d091c6ae91793f6333f5fe0a55633cf3c584" + } + } + } + }` + + output, err := h.rewriteMetadata("@example/private-package", []byte(input)) + if err != nil { + t.Fatalf("rewriteMetadata failed: %v", err) + } + + var result map[string]any + if err := json.Unmarshal(output, &result); err != nil { + t.Fatalf("failed to parse output: %v", err) + } + + versions := result["versions"].(map[string]any) + v := versions[testVersion100].(map[string]any) + dist := v["dist"].(map[string]any) + tarball := dist["tarball"].(string) + + expected := "http://localhost:8080/npm/@example%2Fprivate-package/-/private-package-1.0.0.tgz" + if tarball != expected { + t.Errorf("tarball = %q, want %q", tarball, expected) + } +} + +func TestNPMHandlerDownloadsGitHubPackagesTarball(t *testing.T) { + const shasum = "e053d091c6ae91793f6333f5fe0a55633cf3c584" + const tarballPath = "/download/@example/private-package/1.0.0/" + shasum + + var upstream *httptest.Server + upstream = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/@example/private-package" { + t.Errorf("metadata path = %q, want scoped package path", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", contentTypeJSON) + _, _ = io.WriteString(w, `{"versions":{"1.0.0":{"dist":{"tarball":"`+upstream.URL+tarballPath+`"}}}}`) + })) + defer upstream.Close() + + proxy, _, _, artifactFetcher := setupTestProxy(t) + 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) + + req := httptest.NewRequest( + http.MethodGet, + "/@example/private-package/-/private-package-1.0.0.tgz", + nil, + ) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + if artifactFetcher.fetchedURL != upstream.URL+tarballPath { + t.Errorf("fetched URL = %q, want %q", artifactFetcher.fetchedURL, upstream.URL+tarballPath) + } +} + +func TestNPMHandlerRejectsMissingMetadataVersion(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", contentTypeJSON) + _, _ = io.WriteString(w, `{"versions":{"2.0.0":{}}}`) + })) + defer upstream.Close() + + proxy, _, _, artifactFetcher := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL) + + req := httptest.NewRequest( + http.MethodGet, + "/pkg/-/pkg-1.0.0.tgz", + nil, + ) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusBadRequest, w.Body.String()) + } + if artifactFetcher.fetchedURL != "" { + t.Errorf("artifact fetcher should not be called, fetched URL = %q", artifactFetcher.fetchedURL) + } +} + +func TestNPMHandlerRejectsTarballFromDifferentHost(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", contentTypeJSON) + _, _ = io.WriteString(w, `{"versions":{"1.0.0":{"dist":{"tarball":"https://example.invalid/package.tgz"}}}}`) + })) + defer upstream.Close() + + proxy, _, _, artifactFetcher := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL) + + req := httptest.NewRequest( + http.MethodGet, + "/pkg/-/pkg-1.0.0.tgz", + nil, + ) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusBadRequest, w.Body.String()) + } + if artifactFetcher.fetchedURL != "" { + t.Errorf("artifact fetcher should not be called, fetched URL = %q", artifactFetcher.fetchedURL) + } +} + +func TestNPMHandlerRejectsTarballOutsideUpstreamBasePath(t *testing.T) { + var upstream *httptest.Server + upstream = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/root/pkg" { + t.Errorf("metadata path = %q, want %q", r.URL.Path, "/root/pkg") + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", contentTypeJSON) + _, _ = io.WriteString(w, `{"versions":{"1.0.0":{"dist":{"tarball":"`+upstream.URL+`/outside/pkg-1.0.0.tgz"}}}}`) + })) + defer upstream.Close() + + proxy, _, _, artifactFetcher := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL+"/root/") + + req := httptest.NewRequest(http.MethodGet, "/pkg/-/pkg-1.0.0.tgz", nil) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusBadRequest, w.Body.String()) + } + if artifactFetcher.fetchedURL != "" { + t.Errorf("artifact fetcher should not be called, fetched URL = %q", artifactFetcher.fetchedURL) + } +} + +func TestNPMHandlerFallsBackWhenMetadataIsUnavailable(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + })) + defer upstream.Close() + + proxy, _, _, artifactFetcher := setupTestProxy(t) + 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) + + req := httptest.NewRequest(http.MethodGet, "/pkg/-/pkg-1.0.0.tgz", nil) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + want := upstream.URL + "/pkg/-/pkg-1.0.0.tgz" + if artifactFetcher.fetchedURL != want { + t.Errorf("fetched URL = %q, want %q", artifactFetcher.fetchedURL, want) + } +} + func TestNPMHandlerMetadataProxy(t *testing.T) { // Create a mock upstream server upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -457,19 +646,21 @@ func TestNPMHandlerMetadataNotFound(t *testing.T) { func TestNPMDownloadCooldown(t *testing.T) { now := time.Now() - packument := `{ - "name": "leftpad", - "dist-tags": {"latest": "2.0.0"}, - "time": { - "1.0.0": "` + now.Add(-30*24*time.Hour).Format(time.RFC3339) + `", - "2.0.0": "` + now.Add(-1*time.Hour).Format(time.RFC3339) + `" - }, - "versions": {"1.0.0": {}, "2.0.0": {}} - }` - - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + var upstream *httptest.Server + upstream = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", contentTypeJSON) - _, _ = io.WriteString(w, packument) + _, _ = io.WriteString(w, `{ + "name": "leftpad", + "dist-tags": {"latest": "2.0.0"}, + "time": { + "1.0.0": "`+now.Add(-30*24*time.Hour).Format(time.RFC3339)+`", + "2.0.0": "`+now.Add(-1*time.Hour).Format(time.RFC3339)+`" + }, + "versions": { + "1.0.0": {"dist": {"tarball": "`+upstream.URL+`/leftpad/-/leftpad-1.0.0.tgz"}}, + "2.0.0": {"dist": {"tarball": "`+upstream.URL+`/leftpad/-/leftpad-2.0.0.tgz"}} + } + }`) })) defer upstream.Close() From c2016c728e83924f31d5db09d6fd254233178d63 Mon Sep 17 00:00:00 2001 From: Sivamuthu Kumar Date: Mon, 10 Aug 2026 14:09:59 -0400 Subject: [PATCH 2/2] fix(npm): improve tarball retrieval from npm metadata --- internal/handler/npm.go | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 2e920f9..5eab4c6 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -371,29 +371,36 @@ func (h *NPMHandler) downloadURL(r *http.Request, packageName, version, filename return h.constructDownloadURL(packageName, filename), nil } - var metadata map[string]any + tarball, err := npmVersionTarball(body, version) + if err != nil { + return "", err + } + + return h.validateUpstreamTarballURL(tarball) +} + +func npmVersionTarball(body []byte, version string) (string, error) { + var metadata struct { + Versions map[string]struct { + Dist struct { + Tarball string `json:"tarball"` + } `json:"dist"` + } `json:"versions"` + } + if err := json.Unmarshal(body, &metadata); err != nil { return "", fmt.Errorf("parsing npm metadata: %w", err) } - versions, ok := metadata["versions"].(map[string]any) - if !ok { - return "", errors.New("npm metadata has no versions") - } - vdata, ok := versions[version].(map[string]any) + versionData, ok := metadata.Versions[version] if !ok { return "", fmt.Errorf("npm metadata has no version %q", version) } - dist, ok := vdata["dist"].(map[string]any) - if !ok { - return "", fmt.Errorf("npm metadata version %q has no dist", version) - } - tarball, ok := dist["tarball"].(string) - if !ok { + if versionData.Dist.Tarball == "" { return "", fmt.Errorf("npm metadata version %q has no tarball", version) } - return h.validateUpstreamTarballURL(tarball) + return versionData.Dist.Tarball, nil } func (h *NPMHandler) constructDownloadURL(packageName, filename string) string {