From c4f34b07b4942da0e83cd80b54ec3754437f9236 Mon Sep 17 00:00:00 2001 From: wickedOne Date: Wed, 30 Sep 2026 18:45:08 +0200 Subject: [PATCH 1/2] fix pnpm audit --- README.md | 7 + internal/handler/npm.go | 128 ++++++++++++++ internal/handler/npm_test.go | 311 +++++++++++++++++++++++++++++++++++ 3 files changed, 446 insertions(+) diff --git a/README.md b/README.md index 81024a26..3638c93a 100644 --- a/README.md +++ b/README.md @@ -154,6 +154,12 @@ Or use environment variable: npm_config_registry=http://localhost:8080/npm/ npm install ``` +`npm audit`, `pnpm audit` and `yarn npm audit` work through the proxy: the audit +endpoints are passed through to the configured upstream registry, with upstream +authentication applied. Advisories therefore come from upstream's database, not +from the proxy's own vulnerability data, and versions withheld by +[cooldown](#version-cooldown) are not excluded from the report. + ### Cargo Create or edit `~/.cargo/config.toml`: @@ -883,6 +889,7 @@ Recently cached: | `GET /stats` | Cache statistics (JSON) | | `GET /metrics` | Prometheus metrics | | `GET /npm/*` | npm registry protocol | +| `POST /npm/-/npm/v1/security/*` | npm/pnpm/Yarn audit endpoints, passed through to upstream | | `GET /cargo/*` | Cargo sparse index protocol | | `GET /gem/*` | RubyGems protocol | | `GET /go/*` | Go module proxy protocol | diff --git a/internal/handler/npm.go b/internal/handler/npm.go index a9f2a55a..1c210dcb 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -1,9 +1,11 @@ package handler import ( + "bytes" "encoding/json" "errors" "fmt" + "io" "net/http" "net/url" "sort" @@ -15,6 +17,21 @@ const ( npmUpstream = "https://registry.npmjs.org" npmAcceptDefault = "application/vnd.npm.install-v1+json;q=1.0, application/json;q=0.8" scopedParts = 2 // scope + name in scoped packages + + // npmSecurityPrefix is the base path npm, pnpm and Yarn use for the audit + // and bulk advisory endpoints: /-/npm/v1/security/audits, + // /-/npm/v1/security/audits/quick and /-/npm/v1/security/advisories/bulk. + npmSecurityPrefix = "/-/npm/v1/security/" + + // npmSecurityMaxBody caps the audit payload read from the client. The body + // is a name-to-version map of the whole dependency tree, so even a large + // monorepo lockfile stays well under this. + // + // The body is buffered rather than streamed upstream so an oversized + // payload can be answered with a 413 before the upstream request starts; + // streaming would surface the cap as a write failure mid-request. That + // bounds memory at this cap per in-flight audit request. + npmSecurityMaxBody = 16 << 20 ) // NPMHandler handles npm registry protocol requests. @@ -41,6 +58,13 @@ func NewNPMHandler(proxy *Proxy, proxyURL, upstreamURL string) *NPMHandler { // Mount this at /npm on your router. func (h *NPMHandler) Routes() http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Audit endpoints are POSTs and share the /-/ prefix with tarball + // paths, so they have to be routed before the GET-only gate below. + if strings.HasPrefix(r.URL.Path, npmSecurityPrefix) { + h.handleSecurity(w, r) + return + } + if r.Method != http.MethodGet { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return @@ -59,6 +83,110 @@ func (h *NPMHandler) Routes() http.Handler { }) } +// npmSecurityForwardHeaders are the request headers the audit passthrough +// carries upstream. The body is opaque to the proxy and clients may send it +// gzipped, so the headers describing how to read it have to travel with it. +var npmSecurityForwardHeaders = []string{ //nolint:gochecknoglobals // fixed header list shared across audit requests + headerContentType, + headerContentEncoding, + "Accept", + headerAcceptEncoding, +} + +// handleSecurity relays the npm security endpoints (`npm audit`, `pnpm audit`, +// `yarn npm audit`) to upstream verbatim. +// +// There is nothing for the proxy to do to these beyond passing them along: the +// request body is the dependency tree being audited, so no two requests share a +// cache key, and the response is an advisory report that carries no tarball +// URLs to rewrite. Without this, the handler answered the POST with a 405 and a +// plain-text body, which clients report as a malformed audit response +// (ERR_PNPM_AUDIT_BAD_RESPONSE). +// +// Note that advisories come from upstream's database, not from the proxy's own +// vulnerability data, and cooldown-filtered versions are not excluded from the +// report. +func (h *NPMHandler) handleSecurity(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + JSONError(w, http.StatusMethodNotAllowed, "method not allowed") + return + } + + if containsPathTraversal(r.URL.Path) { + JSONError(w, http.StatusBadRequest, "invalid path") + return + } + + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, npmSecurityMaxBody)) + if err != nil { + var maxBytesErr *http.MaxBytesError + if errors.As(err, &maxBytesErr) { + h.proxy.Logger.Warn("npm audit request over size cap", + "path", r.URL.Path, "limit", npmSecurityMaxBody) + JSONError(w, http.StatusRequestEntityTooLarge, "audit request too large") + return + } + // A client that aborts mid-upload lands here. Reporting it as 413 + // would send operators looking for a size limit that was never hit. + h.proxy.Logger.Warn("npm audit request body unreadable", "path", r.URL.Path, "error", err) + JSONError(w, http.StatusBadRequest, "could not read audit request") + return + } + + // EscapedPath keeps a percent-encoded "?" or "#" in the request path + // encoded. The decoded r.URL.Path would let either character turn the rest + // of the path into a query or fragment on the upstream request. + upstreamURL := h.upstreamURL + r.URL.EscapedPath() + if r.URL.RawQuery != "" { + upstreamURL += "?" + r.URL.RawQuery + } + + h.proxy.Logger.Info("npm audit request", "path", r.URL.Path, "bytes", len(body)) + + req, err := http.NewRequestWithContext(r.Context(), http.MethodPost, upstreamURL, bytes.NewReader(body)) + if err != nil { + JSONError(w, http.StatusInternalServerError, "failed to create request") + return + } + + for _, header := range npmSecurityForwardHeaders { + if v := r.Header.Get(header); v != "" { + req.Header.Set(header, v) + } + } + if req.Header.Get(headerContentType) == "" { + req.Header.Set(headerContentType, contentTypeJSON) + } + req.ContentLength = int64(len(body)) + h.proxy.applyUpstreamAuth(req) + + resp, err := h.proxy.HTTPClient.Do(req) + if err != nil { + h.proxy.Logger.Error("npm audit request failed", "path", r.URL.Path, "error", err) + JSONError(w, http.StatusBadGateway, "failed to reach upstream registry") + return + } + defer func() { _ = resp.Body.Close() }() + + // Content-Length is deliberately not copied. If the upstream connection + // breaks mid-report, letting Go pick the framing makes that a transport + // error the client can detect, rather than a 200 with a short body, which + // is the malformed-audit symptom this passthrough exists to avoid. + for key, values := range resp.Header { + if http.CanonicalHeaderKey(key) == headerContentLength { + continue + } + for _, v := range values { + w.Header().Add(key, v) + } + } + + w.WriteHeader(resp.StatusCode) + if _, err := io.Copy(w, resp.Body); err != nil { + h.proxy.Logger.Warn("npm audit response truncated", "path", r.URL.Path, "error", err) + } +} + // handlePackageMetadata proxies package metadata from upstream and rewrites tarball URLs. func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Request) { packageName := h.extractPackageName(r) diff --git a/internal/handler/npm_test.go b/internal/handler/npm_test.go index c4a5f7ec..fc45f078 100644 --- a/internal/handler/npm_test.go +++ b/internal/handler/npm_test.go @@ -1,6 +1,8 @@ package handler import ( + "bytes" + "compress/gzip" "encoding/json" "errors" "io" @@ -10,6 +12,7 @@ import ( "strings" "sync/atomic" "testing" + "testing/iotest" "time" "github.com/git-pkgs/cooldown" @@ -709,3 +712,311 @@ func TestNPMDownloadErrorResponsesAreJSON(t *testing.T) { }) } } + +// newNPMAuditUpstream starts a stub registry for the npm security endpoints and +// returns a handler pointed at it plus a pointer to the last request it saw. +func newNPMAuditUpstream(t *testing.T, respond http.HandlerFunc) (*NPMHandler, *npmAuditCapture) { + t.Helper() + + capture := &npmAuditCapture{} + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capture.method = r.Method + capture.path = r.URL.Path + capture.query = r.URL.RawQuery + capture.contentType = r.Header.Get("Content-Type") + capture.contentEncoding = r.Header.Get("Content-Encoding") + capture.authorization = r.Header.Get("Authorization") + capture.body, _ = io.ReadAll(r.Body) + respond(w, r) + })) + t.Cleanup(upstream.Close) + + proxy, _, _, _ := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + return NewNPMHandler(proxy, "http://proxy.test", upstream.URL), capture +} + +type npmAuditCapture struct { + method string + path string + query string + contentType string + contentEncoding string + authorization string + body []byte +} + +func npmAuditJSON(body string) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, body) + } +} + +func serveNPMAudit(t *testing.T, h *NPMHandler, method, target string, body io.Reader, + headers map[string]string, +) *httptest.ResponseRecorder { + t.Helper() + + req := httptest.NewRequest(method, target, body) + for name, value := range headers { + req.Header.Set(name, value) + } + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + return w +} + +// TestNPMAuditRelaysRequestAndResponse checks that a `pnpm audit` POST reaches +// upstream unchanged and its report comes back to the client unchanged. +func TestNPMAuditRelaysRequestAndResponse(t *testing.T) { + const report = `{"actions":[],"advisories":{},"metadata":{"vulnerabilities":{"total":0}}}` + const payload = `{"name":"app","requires":{"lodash":"^4.17.21"},"dependencies":{}}` + + h, got := newNPMAuditUpstream(t, npmAuditJSON(report)) + w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits?foo=bar", + strings.NewReader(payload), map[string]string{"Content-Type": "application/json"}) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + if w.Body.String() != report { + t.Errorf("body = %q, want %q", w.Body.String(), report) + } + if ct := w.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("response Content-Type = %q, want application/json", ct) + } + if got.method != http.MethodPost { + t.Errorf("upstream method = %q, want POST", got.method) + } + if got.path != "/-/npm/v1/security/audits" { + t.Errorf("upstream path = %q, want /-/npm/v1/security/audits", got.path) + } + if got.query != "foo=bar" { + t.Errorf("upstream query = %q, want foo=bar", got.query) + } + if string(got.body) != payload { + t.Errorf("upstream body = %q, want %q", got.body, payload) + } + if got.contentType != "application/json" { + t.Errorf("upstream Content-Type = %q, want application/json", got.contentType) + } +} + +// TestNPMAuditForwardsGzippedBody covers clients that compress the audit +// payload: the bytes and the header describing them must travel together. +func TestNPMAuditForwardsGzippedBody(t *testing.T) { + var gzipped bytes.Buffer + zw := gzip.NewWriter(&gzipped) + if _, err := io.WriteString(zw, `{"name":"app"}`); err != nil { + t.Fatalf("writing gzip body: %v", err) + } + if err := zw.Close(); err != nil { + t.Fatalf("closing gzip writer: %v", err) + } + want := gzipped.Bytes() + + h, got := newNPMAuditUpstream(t, npmAuditJSON(`{}`)) + w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", + bytes.NewReader(want), map[string]string{ + "Content-Type": "application/json", + "Content-Encoding": "gzip", + }) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + if got.contentEncoding != "gzip" { + t.Errorf("upstream Content-Encoding = %q, want gzip", got.contentEncoding) + } + if !bytes.Equal(got.body, want) { + t.Errorf("upstream body was altered: got %d bytes, want %d", len(got.body), len(want)) + } +} + +// TestNPMAuditCoversAllSecurityEndpoints checks the paths used by pnpm, npm and +// Yarn all reach upstream. +func TestNPMAuditCoversAllSecurityEndpoints(t *testing.T) { + paths := []string{ + "/-/npm/v1/security/audits", // pnpm audit, npm audit (full) + "/-/npm/v1/security/audits/quick", // yarn npm audit, npm audit fallback + "/-/npm/v1/security/advisories/bulk", // npm audit (npm 7+) + } + + for _, path := range paths { + t.Run(path, func(t *testing.T) { + h, got := newNPMAuditUpstream(t, npmAuditJSON(`{}`)) + w := serveNPMAudit(t, h, http.MethodPost, path, strings.NewReader(`{}`), nil) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + if got.path != path { + t.Errorf("upstream path = %q, want %q", got.path, path) + } + }) + } +} + +// TestNPMAuditAppliesUpstreamAuth checks audits against a private registry are +// authenticated like every other upstream request. +func TestNPMAuditAppliesUpstreamAuth(t *testing.T) { + h, got := newNPMAuditUpstream(t, npmAuditJSON(`{}`)) + h.proxy.AuthForURL = func(string) (string, string) { + return "Authorization", "Bearer npm-token" + } + + serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) + + if got.authorization != "Bearer npm-token" { + t.Errorf("Authorization = %q, want %q", got.authorization, "Bearer npm-token") + } +} + +// TestNPMAuditRelaysUpstreamError checks an upstream rejection reaches the +// client as-is rather than being reshaped into a proxy error. +func TestNPMAuditRelaysUpstreamError(t *testing.T) { + const upstreamBody = `{"error":"unauthorized"}` + + h, _ := newNPMAuditUpstream(t, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = io.WriteString(w, upstreamBody) + }) + w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) + + if w.Code != http.StatusUnauthorized { + t.Errorf("status = %d, want %d", w.Code, http.StatusUnauthorized) + } + if w.Body.String() != upstreamBody { + t.Errorf("body = %q, want %q", w.Body.String(), upstreamBody) + } +} + +// TestNPMAuditUpstreamUnreachable checks the proxy still answers with JSON when +// it cannot reach the registry, so clients report a transport failure rather +// than a malformed audit response. +func TestNPMAuditUpstreamUnreachable(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + upstreamURL := upstream.URL + upstream.Close() // nothing is listening now + + proxy, _, _, _ := setupTestProxy(t) + h := NewNPMHandler(proxy, "http://proxy.test", upstreamURL) + + w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) + + if w.Code != http.StatusBadGateway { + t.Errorf("status = %d, want %d", w.Code, http.StatusBadGateway) + } + if ct := w.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("Content-Type = %q, want application/json", ct) + } + if !json.Valid(w.Body.Bytes()) { + t.Errorf("body is not valid JSON: %q", w.Body.String()) + } +} + +// TestNPMAuditRejectsBadRequests covers the guards on the passthrough. +func TestNPMAuditRejectsBadRequests(t *testing.T) { + t.Run("non-POST method", func(t *testing.T) { + proxy, _, _, _ := setupTestProxy(t) + h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test") + + w := serveNPMAudit(t, h, http.MethodGet, "/-/npm/v1/security/audits", nil, nil) + + if w.Code != http.StatusMethodNotAllowed { + t.Errorf("status = %d, want %d", w.Code, http.StatusMethodNotAllowed) + } + if !json.Valid(w.Body.Bytes()) { + t.Errorf("body is not valid JSON: %q", w.Body.String()) + } + }) + + t.Run("body over the size cap", func(t *testing.T) { + proxy, _, _, _ := setupTestProxy(t) + h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test") + + body := strings.NewReader(strings.Repeat("a", npmSecurityMaxBody+1)) + w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", body, nil) + + if w.Code != http.StatusRequestEntityTooLarge { + t.Errorf("status = %d, want %d", w.Code, http.StatusRequestEntityTooLarge) + } + }) + + // A client that aborts mid-upload must not be told its payload was too + // large, or the operator goes looking for a cap that was never reached. + t.Run("unreadable body is not reported as too large", func(t *testing.T) { + proxy, _, _, _ := setupTestProxy(t) + h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test") + + req := httptest.NewRequest(http.MethodPost, "/-/npm/v1/security/audits", + iotest.TimeoutReader(strings.NewReader("{}"))) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want %d", w.Code, http.StatusBadRequest) + } + if !json.Valid(w.Body.Bytes()) { + t.Errorf("body is not valid JSON: %q", w.Body.String()) + } + }) +} + +// TestNPMAuditDoesNotInjectUpstreamQuery checks a percent-encoded "?" in the +// request path stays part of the path upstream instead of becoming a query +// separator. +func TestNPMAuditDoesNotInjectUpstreamQuery(t *testing.T) { + h, got := newNPMAuditUpstream(t, npmAuditJSON(`{}`)) + + // httptest.NewRequest parses the target, so RawPath keeps the encoding. + serveNPMAudit(t, h, http.MethodPost, + "/-/npm/v1/security/audits%3Fevil=1", strings.NewReader(`{}`), nil) + + if got.query != "" { + t.Errorf("upstream query = %q, want empty: encoded ? leaked into the query", got.query) + } + if got.path != "/-/npm/v1/security/audits?evil=1" { + t.Errorf("upstream path = %q, want the encoded ? kept in the path", got.path) + } +} + +// TestNPMAuditDropsUpstreamContentLength checks the proxy reframes the response +// rather than promising a length it may not be able to fill. +func TestNPMAuditDropsUpstreamContentLength(t *testing.T) { + const report = `{"metadata":{"vulnerabilities":{"total":0}}}` + + h, _ := newNPMAuditUpstream(t, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Length", "99999") // longer than the body sent + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, report) + }) + w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) + + if got := w.Header().Get("Content-Length"); got == "99999" { + t.Errorf("Content-Length = %q, want upstream's value dropped", got) + } + if w.Body.String() != report { + t.Errorf("body = %q, want %q", w.Body.String(), report) + } +} + +// TestNPMTarballStillRoutesToDownload guards the dispatch order: tarball paths +// share the /-/ prefix with the security endpoints. +func TestNPMTarballStillRoutesToDownload(t *testing.T) { + proxy, _, _, artifactFetcher := setupTestProxy(t) + artifactFetcher.artifact = &fetch.Artifact{ + Body: io.NopCloser(strings.NewReader("package")), + ContentType: "application/gzip", + } + h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test") + + w := serveNPMAudit(t, h, http.MethodGet, "/lodash/-/lodash-4.17.21.tgz", nil, nil) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } +} From d8a811148c49c0ecfb47ef420a27a8573ae3b1da Mon Sep 17 00:00:00 2001 From: wickedOne Date: Thu, 1 Oct 2026 10:09:33 +0200 Subject: [PATCH 2/2] apply review changes --- README.md | 12 +-- internal/handler/handler.go | 1 + internal/handler/npm.go | 88 +++++++-------------- internal/handler/npm_test.go | 137 +++++++++++++++++++++------------ internal/handler/relay_test.go | 2 + 5 files changed, 125 insertions(+), 115 deletions(-) diff --git a/README.md b/README.md index 3638c93a..eb265244 100644 --- a/README.md +++ b/README.md @@ -154,11 +154,12 @@ Or use environment variable: npm_config_registry=http://localhost:8080/npm/ npm install ``` -`npm audit`, `pnpm audit` and `yarn npm audit` work through the proxy: the audit -endpoints are passed through to the configured upstream registry, with upstream -authentication applied. Advisories therefore come from upstream's database, not -from the proxy's own vulnerability data, and versions withheld by -[cooldown](#version-cooldown) are not excluded from the report. +`npm audit`, `pnpm audit`, `yarn npm audit` and `npm audit signatures` work +through the proxy: the audit and signing-key endpoints are passed through to the +configured upstream registry, with upstream authentication applied. Advisories +therefore come from upstream's database, not from the proxy's own vulnerability +data, and versions withheld by [cooldown](#version-cooldown) are not excluded +from the report. ### Cargo @@ -890,6 +891,7 @@ Recently cached: | `GET /metrics` | Prometheus metrics | | `GET /npm/*` | npm registry protocol | | `POST /npm/-/npm/v1/security/*` | npm/pnpm/Yarn audit endpoints, passed through to upstream | +| `GET /npm/-/npm/v1/keys` | npm registry signing keys, passed through to upstream | | `GET /cargo/*` | Cargo sparse index protocol | | `GET /gem/*` | RubyGems protocol | | `GET /go/*` | Go module proxy protocol | diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 584b9728..b9ecc625 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -96,6 +96,7 @@ func packagePURLStrings(ecosystem, name, version string) (string, string, error) const contentTypeJSON = "application/json" const ( + headerAccept = "Accept" headerAcceptEncoding = "Accept-Encoding" headerContentType = "Content-Type" headerContentLength = "Content-Length" diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 1c210dcb..27cf2513 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -18,19 +18,15 @@ const ( npmAcceptDefault = "application/vnd.npm.install-v1+json;q=1.0, application/json;q=0.8" scopedParts = 2 // scope + name in scoped packages - // npmSecurityPrefix is the base path npm, pnpm and Yarn use for the audit - // and bulk advisory endpoints: /-/npm/v1/security/audits, - // /-/npm/v1/security/audits/quick and /-/npm/v1/security/advisories/bulk. + // npmSecurityPrefix covers the audit endpoints: /-/npm/v1/security/audits, + // .../audits/quick and .../advisories/bulk. npmSecurityPrefix = "/-/npm/v1/security/" - // npmSecurityMaxBody caps the audit payload read from the client. The body - // is a name-to-version map of the whole dependency tree, so even a large - // monorepo lockfile stays well under this. - // - // The body is buffered rather than streamed upstream so an oversized - // payload can be answered with a 413 before the upstream request starts; - // streaming would surface the cap as a write failure mid-request. That - // bounds memory at this cap per in-flight audit request. + // npmKeysPath serves the registry signing keys `npm audit signatures` reads. + npmKeysPath = "/-/npm/v1/keys" + + // npmSecurityMaxBody caps the audit payload. Buffering it lets an oversized + // body be refused before the upstream request starts. npmSecurityMaxBody = 16 << 20 ) @@ -58,8 +54,9 @@ func NewNPMHandler(proxy *Proxy, proxyURL, upstreamURL string) *NPMHandler { // Mount this at /npm on your router. func (h *NPMHandler) Routes() http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Audit endpoints are POSTs and share the /-/ prefix with tarball - // paths, so they have to be routed before the GET-only gate below. + // The /-/npm/v1 endpoints share the /-/ prefix with tarball paths, so + // they are routed before the tarball dispatch below. Audits are POSTs, + // so they also precede the GET-only gate. if strings.HasPrefix(r.URL.Path, npmSecurityPrefix) { h.handleSecurity(w, r) return @@ -70,6 +67,12 @@ func (h *NPMHandler) Routes() http.Handler { return } + if r.URL.Path == npmKeysPath { + h.proxy.ProxyUpstream(w, r, h.upstreamURL+npmKeysPath, + []string{headerAccept, headerAcceptEncoding}) + return + } + path := strings.TrimPrefix(r.URL.Path, "/") // Check if this is a tarball download (contains /-/) @@ -83,29 +86,12 @@ func (h *NPMHandler) Routes() http.Handler { }) } -// npmSecurityForwardHeaders are the request headers the audit passthrough -// carries upstream. The body is opaque to the proxy and clients may send it -// gzipped, so the headers describing how to read it have to travel with it. -var npmSecurityForwardHeaders = []string{ //nolint:gochecknoglobals // fixed header list shared across audit requests - headerContentType, - headerContentEncoding, - "Accept", - headerAcceptEncoding, -} - -// handleSecurity relays the npm security endpoints (`npm audit`, `pnpm audit`, -// `yarn npm audit`) to upstream verbatim. -// -// There is nothing for the proxy to do to these beyond passing them along: the -// request body is the dependency tree being audited, so no two requests share a -// cache key, and the response is an advisory report that carries no tarball -// URLs to rewrite. Without this, the handler answered the POST with a 405 and a -// plain-text body, which clients report as a malformed audit response -// (ERR_PNPM_AUDIT_BAD_RESPONSE). +// handleSecurity relays the npm audit endpoints to upstream. The request body +// is the dependency tree being audited, so no two requests share a cache key, +// and the response carries no tarball URLs to rewrite. // -// Note that advisories come from upstream's database, not from the proxy's own -// vulnerability data, and cooldown-filtered versions are not excluded from the -// report. +// Advisories come from upstream's database, not the proxy's own vulnerability +// data, and versions withheld by cooldown are not excluded from the report. func (h *NPMHandler) handleSecurity(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { JSONError(w, http.StatusMethodNotAllowed, "method not allowed") @@ -121,21 +107,17 @@ func (h *NPMHandler) handleSecurity(w http.ResponseWriter, r *http.Request) { if err != nil { var maxBytesErr *http.MaxBytesError if errors.As(err, &maxBytesErr) { - h.proxy.Logger.Warn("npm audit request over size cap", - "path", r.URL.Path, "limit", npmSecurityMaxBody) JSONError(w, http.StatusRequestEntityTooLarge, "audit request too large") return } - // A client that aborts mid-upload lands here. Reporting it as 413 - // would send operators looking for a size limit that was never hit. + // A client aborting mid-upload must not be told its payload was too big. h.proxy.Logger.Warn("npm audit request body unreadable", "path", r.URL.Path, "error", err) JSONError(w, http.StatusBadRequest, "could not read audit request") return } - // EscapedPath keeps a percent-encoded "?" or "#" in the request path - // encoded. The decoded r.URL.Path would let either character turn the rest - // of the path into a query or fragment on the upstream request. + // EscapedPath keeps an encoded "?" or "#" from turning the rest of the path + // into a query or fragment upstream. upstreamURL := h.upstreamURL + r.URL.EscapedPath() if r.URL.RawQuery != "" { upstreamURL += "?" + r.URL.RawQuery @@ -149,7 +131,8 @@ func (h *NPMHandler) handleSecurity(w http.ResponseWriter, r *http.Request) { return } - for _, header := range npmSecurityForwardHeaders { + // Clients may gzip the body, so the headers describing it travel with it. + for _, header := range []string{headerContentType, headerContentEncoding, headerAccept, headerAcceptEncoding} { if v := r.Header.Get(header); v != "" { req.Header.Set(header, v) } @@ -157,7 +140,6 @@ func (h *NPMHandler) handleSecurity(w http.ResponseWriter, r *http.Request) { if req.Header.Get(headerContentType) == "" { req.Header.Set(headerContentType, contentTypeJSON) } - req.ContentLength = int64(len(body)) h.proxy.applyUpstreamAuth(req) resp, err := h.proxy.HTTPClient.Do(req) @@ -168,23 +150,7 @@ func (h *NPMHandler) handleSecurity(w http.ResponseWriter, r *http.Request) { } defer func() { _ = resp.Body.Close() }() - // Content-Length is deliberately not copied. If the upstream connection - // breaks mid-report, letting Go pick the framing makes that a transport - // error the client can detect, rather than a 200 with a short body, which - // is the malformed-audit symptom this passthrough exists to avoid. - for key, values := range resp.Header { - if http.CanonicalHeaderKey(key) == headerContentLength { - continue - } - for _, v := range values { - w.Header().Add(key, v) - } - } - - w.WriteHeader(resp.StatusCode) - if _, err := io.Copy(w, resp.Body); err != nil { - h.proxy.Logger.Warn("npm audit response truncated", "path", r.URL.Path, "error", err) - } + h.proxy.relayResponse(w, r, resp, nil) } // handlePackageMetadata proxies package metadata from upstream and rewrites tarball URLs. diff --git a/internal/handler/npm_test.go b/internal/handler/npm_test.go index fc45f078..d0c38890 100644 --- a/internal/handler/npm_test.go +++ b/internal/handler/npm_test.go @@ -5,6 +5,7 @@ import ( "compress/gzip" "encoding/json" "errors" + "fmt" "io" "log/slog" "net/http" @@ -713,8 +714,8 @@ func TestNPMDownloadErrorResponsesAreJSON(t *testing.T) { } } -// newNPMAuditUpstream starts a stub registry for the npm security endpoints and -// returns a handler pointed at it plus a pointer to the last request it saw. +// newNPMAuditUpstream returns a handler pointed at a stub registry, plus the +// last request that registry saw. func newNPMAuditUpstream(t *testing.T, respond http.HandlerFunc) (*NPMHandler, *npmAuditCapture) { t.Helper() @@ -753,7 +754,7 @@ func npmAuditJSON(body string) http.HandlerFunc { } } -func serveNPMAudit(t *testing.T, h *NPMHandler, method, target string, body io.Reader, +func serveNPM(t *testing.T, h *NPMHandler, method, target string, body io.Reader, headers map[string]string, ) *httptest.ResponseRecorder { t.Helper() @@ -767,14 +768,12 @@ func serveNPMAudit(t *testing.T, h *NPMHandler, method, target string, body io.R return w } -// TestNPMAuditRelaysRequestAndResponse checks that a `pnpm audit` POST reaches -// upstream unchanged and its report comes back to the client unchanged. func TestNPMAuditRelaysRequestAndResponse(t *testing.T) { const report = `{"actions":[],"advisories":{},"metadata":{"vulnerabilities":{"total":0}}}` const payload = `{"name":"app","requires":{"lodash":"^4.17.21"},"dependencies":{}}` h, got := newNPMAuditUpstream(t, npmAuditJSON(report)) - w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits?foo=bar", + w := serveNPM(t, h, http.MethodPost, "/-/npm/v1/security/audits?foo=bar", strings.NewReader(payload), map[string]string{"Content-Type": "application/json"}) if w.Code != http.StatusOK { @@ -803,8 +802,8 @@ func TestNPMAuditRelaysRequestAndResponse(t *testing.T) { } } -// TestNPMAuditForwardsGzippedBody covers clients that compress the audit -// payload: the bytes and the header describing them must travel together. +// npm gzips its audit payload, so the bytes and the header describing them +// must travel together. func TestNPMAuditForwardsGzippedBody(t *testing.T) { var gzipped bytes.Buffer zw := gzip.NewWriter(&gzipped) @@ -817,7 +816,7 @@ func TestNPMAuditForwardsGzippedBody(t *testing.T) { want := gzipped.Bytes() h, got := newNPMAuditUpstream(t, npmAuditJSON(`{}`)) - w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", + w := serveNPM(t, h, http.MethodPost, "/-/npm/v1/security/audits", bytes.NewReader(want), map[string]string{ "Content-Type": "application/json", "Content-Encoding": "gzip", @@ -834,8 +833,6 @@ func TestNPMAuditForwardsGzippedBody(t *testing.T) { } } -// TestNPMAuditCoversAllSecurityEndpoints checks the paths used by pnpm, npm and -// Yarn all reach upstream. func TestNPMAuditCoversAllSecurityEndpoints(t *testing.T) { paths := []string{ "/-/npm/v1/security/audits", // pnpm audit, npm audit (full) @@ -846,7 +843,7 @@ func TestNPMAuditCoversAllSecurityEndpoints(t *testing.T) { for _, path := range paths { t.Run(path, func(t *testing.T) { h, got := newNPMAuditUpstream(t, npmAuditJSON(`{}`)) - w := serveNPMAudit(t, h, http.MethodPost, path, strings.NewReader(`{}`), nil) + w := serveNPM(t, h, http.MethodPost, path, strings.NewReader(`{}`), nil) if w.Code != http.StatusOK { t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) @@ -858,23 +855,19 @@ func TestNPMAuditCoversAllSecurityEndpoints(t *testing.T) { } } -// TestNPMAuditAppliesUpstreamAuth checks audits against a private registry are -// authenticated like every other upstream request. func TestNPMAuditAppliesUpstreamAuth(t *testing.T) { h, got := newNPMAuditUpstream(t, npmAuditJSON(`{}`)) h.proxy.AuthForURL = func(string) (string, string) { return "Authorization", "Bearer npm-token" } - serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) + serveNPM(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) if got.authorization != "Bearer npm-token" { t.Errorf("Authorization = %q, want %q", got.authorization, "Bearer npm-token") } } -// TestNPMAuditRelaysUpstreamError checks an upstream rejection reaches the -// client as-is rather than being reshaped into a proxy error. func TestNPMAuditRelaysUpstreamError(t *testing.T) { const upstreamBody = `{"error":"unauthorized"}` @@ -883,7 +876,7 @@ func TestNPMAuditRelaysUpstreamError(t *testing.T) { w.WriteHeader(http.StatusUnauthorized) _, _ = io.WriteString(w, upstreamBody) }) - w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) + w := serveNPM(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) if w.Code != http.StatusUnauthorized { t.Errorf("status = %d, want %d", w.Code, http.StatusUnauthorized) @@ -893,9 +886,8 @@ func TestNPMAuditRelaysUpstreamError(t *testing.T) { } } -// TestNPMAuditUpstreamUnreachable checks the proxy still answers with JSON when -// it cannot reach the registry, so clients report a transport failure rather -// than a malformed audit response. +// A proxy-side failure must still be JSON, or the client reports it as a +// malformed audit response. func TestNPMAuditUpstreamUnreachable(t *testing.T) { upstream := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) upstreamURL := upstream.URL @@ -904,7 +896,7 @@ func TestNPMAuditUpstreamUnreachable(t *testing.T) { proxy, _, _, _ := setupTestProxy(t) h := NewNPMHandler(proxy, "http://proxy.test", upstreamURL) - w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) + w := serveNPM(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) if w.Code != http.StatusBadGateway { t.Errorf("status = %d, want %d", w.Code, http.StatusBadGateway) @@ -917,13 +909,12 @@ func TestNPMAuditUpstreamUnreachable(t *testing.T) { } } -// TestNPMAuditRejectsBadRequests covers the guards on the passthrough. func TestNPMAuditRejectsBadRequests(t *testing.T) { t.Run("non-POST method", func(t *testing.T) { proxy, _, _, _ := setupTestProxy(t) h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test") - w := serveNPMAudit(t, h, http.MethodGet, "/-/npm/v1/security/audits", nil, nil) + w := serveNPM(t, h, http.MethodGet, "/-/npm/v1/security/audits", nil, nil) if w.Code != http.StatusMethodNotAllowed { t.Errorf("status = %d, want %d", w.Code, http.StatusMethodNotAllowed) @@ -938,15 +929,13 @@ func TestNPMAuditRejectsBadRequests(t *testing.T) { h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test") body := strings.NewReader(strings.Repeat("a", npmSecurityMaxBody+1)) - w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", body, nil) + w := serveNPM(t, h, http.MethodPost, "/-/npm/v1/security/audits", body, nil) if w.Code != http.StatusRequestEntityTooLarge { t.Errorf("status = %d, want %d", w.Code, http.StatusRequestEntityTooLarge) } }) - // A client that aborts mid-upload must not be told its payload was too - // large, or the operator goes looking for a cap that was never reached. t.Run("unreadable body is not reported as too large", func(t *testing.T) { proxy, _, _, _ := setupTestProxy(t) h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test") @@ -965,14 +954,10 @@ func TestNPMAuditRejectsBadRequests(t *testing.T) { }) } -// TestNPMAuditDoesNotInjectUpstreamQuery checks a percent-encoded "?" in the -// request path stays part of the path upstream instead of becoming a query -// separator. func TestNPMAuditDoesNotInjectUpstreamQuery(t *testing.T) { h, got := newNPMAuditUpstream(t, npmAuditJSON(`{}`)) - // httptest.NewRequest parses the target, so RawPath keeps the encoding. - serveNPMAudit(t, h, http.MethodPost, + serveNPM(t, h, http.MethodPost, "/-/npm/v1/security/audits%3Fevil=1", strings.NewReader(`{}`), nil) if got.query != "" { @@ -983,29 +968,83 @@ func TestNPMAuditDoesNotInjectUpstreamQuery(t *testing.T) { } } -// TestNPMAuditDropsUpstreamContentLength checks the proxy reframes the response -// rather than promising a length it may not be able to fill. -func TestNPMAuditDropsUpstreamContentLength(t *testing.T) { - const report = `{"metadata":{"vulnerabilities":{"total":0}}}` +// The audit POST must go through relayResponse, not copy upstream headers +// wholesale: a relayed Connection header would be honoured downstream. +// TestRelayRoutes covers the GET paths; this covers the POST. +func TestNPMAuditStripsHopByHopHeaders(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + conn, rw, err := w.(http.Hijacker).Hijack() + if err != nil { + t.Error(err) + return + } + defer func() { _ = conn.Close() }() + _, _ = fmt.Fprint(rw, "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n"+ + "Connection: X-Private\r\nX-Private: secret\r\nContent-Length: 2\r\n\r\n{}") + _ = rw.Flush() + })) + defer upstream.Close() - h, _ := newNPMAuditUpstream(t, func(w http.ResponseWriter, _ *http.Request) { + proxy, _, _, _ := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + downstream := httptest.NewServer(NewNPMHandler(proxy, "http://proxy.test", upstream.URL).Routes()) + defer downstream.Close() + + resp, err := downstream.Client().Post( + downstream.URL+"/-/npm/v1/security/audits", contentTypeJSON, strings.NewReader(`{}`)) + if err != nil { + t.Fatal(err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.Header.Get("Connection") != "" || resp.Header.Get("X-Private") != "" { + t.Errorf("connection-scoped headers leaked: %v", resp.Header) + } +} + +// `npm audit signatures` reads the registry signing keys. The path used to fall +// through to the package dispatch and be escaped into a package name. +func TestNPMKeysProxiesUpstream(t *testing.T) { + const keys = `{"keys":[{"keyid":"SHA256:jl3bwswu","keytype":"ecdsa-sha2-nistp256"}]}` + + var gotPath, gotAuth string + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // EscapedPath, not Path: the bug escaped the slashes into a package + // name, and the server decodes %2F back into Path either way. + gotPath, gotAuth = r.URL.EscapedPath(), r.Header.Get("Authorization") + if gotPath != npmKeysPath { + w.WriteHeader(http.StatusMethodNotAllowed) // what registry.npmjs.org answers + return + } w.Header().Set("Content-Type", "application/json") - w.Header().Set("Content-Length", "99999") // longer than the body sent - w.WriteHeader(http.StatusOK) - _, _ = io.WriteString(w, report) - }) - w := serveNPMAudit(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil) + _, _ = io.WriteString(w, keys) + })) + defer upstream.Close() - if got := w.Header().Get("Content-Length"); got == "99999" { - t.Errorf("Content-Length = %q, want upstream's value dropped", got) + proxy, _, _, _ := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + proxy.AuthForURL = func(string) (string, string) { + return "Authorization", "Bearer npm-token" } - if w.Body.String() != report { - t.Errorf("body = %q, want %q", w.Body.String(), report) + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL) + + w := serveNPM(t, h, http.MethodGet, npmKeysPath, nil, nil) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + if w.Body.String() != keys { + t.Errorf("body = %q, want %q", w.Body.String(), keys) + } + if gotPath != npmKeysPath { + t.Errorf("upstream path = %q, want %q", gotPath, npmKeysPath) + } + if gotAuth != "Bearer npm-token" { + t.Errorf("Authorization = %q, want it applied", gotAuth) } } -// TestNPMTarballStillRoutesToDownload guards the dispatch order: tarball paths -// share the /-/ prefix with the security endpoints. +// Tarball paths share the /-/ prefix with the /-/npm/v1 endpoints. func TestNPMTarballStillRoutesToDownload(t *testing.T) { proxy, _, _, artifactFetcher := setupTestProxy(t) artifactFetcher.artifact = &fetch.Artifact{ @@ -1014,7 +1053,7 @@ func TestNPMTarballStillRoutesToDownload(t *testing.T) { } h := NewNPMHandler(proxy, "http://proxy.test", "https://npm.example.test") - w := serveNPMAudit(t, h, http.MethodGet, "/lodash/-/lodash-4.17.21.tgz", nil, nil) + w := serveNPM(t, h, http.MethodGet, "/lodash/-/lodash-4.17.21.tgz", nil, nil) if w.Code != http.StatusOK { t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) diff --git a/internal/handler/relay_test.go b/internal/handler/relay_test.go index 23f2cba0..5200ffba 100644 --- a/internal/handler/relay_test.go +++ b/internal/handler/relay_test.go @@ -42,6 +42,7 @@ func relayTestRoutes(proxy *Proxy, upstream string) http.Handler { mount("/hex", NewHexHandlerWithUpstreams(proxy, proxyURL, upstream, upstream).Routes()) mount("/swift", NewSwiftHandler(proxy, proxyURL, upstream).Routes()) mount("/v2", NewContainerHandlerWithRegistry(proxy, proxyURL, upstream).Routes()) + mount("/npm", NewNPMHandler(proxy, proxyURL, upstream).Routes()) // Match the production recovery middleware: it must not swallow the abort. return middleware.Recoverer(mux) } @@ -54,6 +55,7 @@ func TestRelayRoutes(t *testing.T) { "/gem/info/demo", "/conda/conda-forge/noarch/repodata.json", "/hex/packages/demo", "/swift/scope/demo/1.0.0/Package.swift", "/v2/library/demo/manifests/latest", "/v2/library/demo/tags/list", + "/npm/-/npm/v1/keys", } { t.Run(route, func(t *testing.T) { for _, truncated := range []bool{false, true} {