fix pnpm audit

[wickedOne]

Add npm audit passthrough for pnpm, npm and Yarn

pnpm audit against the proxy fails with ERR_PNPM_AUDIT_BAD_RESPONSE.

The npm handler had a GET-only gate at the top of its router, so the audit
POST to {registry}/-/npm/v1/security/audits was answered with a 405 and a
plain-text body. The client tried to parse that as an audit report and reported
it as malformed. npm audit and yarn npm audit failed the same way.

These endpoints are relayed to the configured upstream registry untouched.
There is nothing for the proxy to add: 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.

Changes

• Route /-/npm/v1/security/* before the GET-only gate — audit paths share the
  /-/ prefix with tarball paths, so dispatch order matters
• Forward the request body byte-for-byte, along with Content-Type,
  Content-Encoding, Accept and Accept-Encoding; npm gzips its audit
  payload, so the body and the header describing it have to travel together
• Apply applyUpstreamAuth, so audits work against private registries
• Relay the upstream status, headers and body back as-is, so an upstream 401
  stays a 401 instead of becoming a proxy error

One prefix covers every client: audits (pnpm, npm full), audits/quick
(Yarn, npm fallback) and advisories/bulk (npm 7+).

Guards

• POST only; containsPathTraversal on the request path
• Request body capped at 16 MB. The cap returns 413, while an unreadable body
  (a client aborting mid-upload) returns 400 — reporting both as 413 would send
  operators hunting a limit that was never reached
• The upstream URL is built from EscapedPath(), so a percent-encoded ? or
  # in the request path cannot inject a query or truncate the URL upstream
• Upstream Content-Length is 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 same
  malformed-audit symptom this change exists to remove
• Failures answer with JSON, not plain text, for the same reason

The body is buffered rather than streamed so an oversized payload can be
rejected before the upstream request starts; that bounds memory at the cap per
in-flight audit request.

Caveats

Advisories come from upstream's database, not from the proxy's own vulnerability
data, and versions withheld by cooldown are not excluded from the report — so an
audit can flag a version the proxy will not serve. Both are documented in the
README and in the handler comment.

Testing

• 10 new tests covering verbatim relay (including the query string), gzipped
  bodies preserved byte-for-byte, all three endpoints, upstream auth, upstream
  errors relayed, unreachable upstream, each request guard, and a regression
  test that tarball GETs still reach the download path
• The three guard fixes were each verified to fail without the fix in place
• go build ./..., go vet ./..., go test ./... and golangci-lint run all
  clean

🤖 Generated with Claude Code

[andrew]

Routing the audit POSTs before the GET-only gate is the right fix, and the guards and tests look good. Three things before merge.

handleSecurity hand-rolls the upstream request and response relay in internal/handler/npm.go, which Proxy.ProxyUpstream (internal/handler/handler.go:772) already does apart from carrying a request body. The practical problem with the copy is the header loop: it forwards upstream hop-by-hop headers, so an upstream Connection: close is relayed and Go's server honours it on the client connection. That is the issue #326 class of bug, and #380 is centralizing a relay helper for exactly this. Either give ProxyUpstream an optional request body and call it, or, if #380 merges first, use the helper it adds in internal/handler/relay.go.

The comments are doing too much. The const block and the handleSecurity doc comment run to about 24 lines between them, including change history ("Without this, the handler answered the POST with a 405 and a plain-text body"). Keep the dispatch-order note and the reason Content-Encoding has to travel with the body, and drop the rest.

npmSecurityForwardHeaders is a package global with a //nolint:gochecknoglobals on it. As a local slice inside handleSecurity it needs no suppression.

[andrew]

One more finding, which should have been in the earlier review.

npm audit signatures still fails through the proxy with this change in place. It reads the registry's signing keys from GET /-/npm/v1/keys, which does not match the new security prefix and falls through to the package dispatch: extractPackageName returns -/npm/v1/keys, url.PathEscape turns it into -%2Fnpm%2Fv1%2Fkeys, registry.npmjs.org answers that 405, and the handler reports 502 {"error":"failed to fetch from upstream"}. I ran this against registry.npmjs.org to confirm the mechanism.

It belongs in this PR rather than a separate one for two reasons. The README section added here tells users npm audit works through the proxy, and npm audit signatures is part of that command. And /-/npm/v1/ is the namespace this PR starts routing, so the keys path is the other half of the same gap.

Routing /-/npm/v1/keys to ProxyUpstream ahead of the package and tarball dispatch covers it in about ten lines. The keys document is small and carries no tarball URLs to rewrite, so it needs no caching or rewriting, and ProxyUpstream already applies upstream auth and relays the response. The endpoint predates this PR.

[wickedOne]

Thanks — all four addressed. One thing worth flagging up front: #380 merge between your two reviews (05:35, review one was 05:16). I took the branch you offered for that case and used the helper it added, so this now needs a rebase onto current main (c3540fb) — internal/handler/relay.go does not exist on the base the PR was opened against. #380 does not touch npm.go, so the rebase is clean.

Relay duplication

handleSecurity now ends in h.proxy.relayResponse(w, r, resp, nil) and the hand-rolled header loop is gone.

You were right that it was a live bug, not just duplication. With the old loop an upstream Connection: X-Private and the X-Private header it names both reached the client. TestNPMAuditStripsHopByHopHeaders covers the POST path
with a raw upstream response, and I confirmed it fails against the old loop.

This also deleted something from my earlier round: I had been skipping Content-Length so a truncated report could not come back as a 200 with a short body. relayResponse handles that case properly by aborting the handler, so the
workaround went with the loop, and TestNPMAuditDropsUpstreamContentLength was dropped as obsolete.

The keys endpoint is a GET, so I registered the npm handler in relayTestRoutes and added /npm/-/npm/v1/keys to TestRelayRoutes rather than writing a parallel test — it picks up the hop-by-hop and truncation cases from the shared suite.

npm audit signatures

Added, routed to ProxyUpstream after the GET gate and ahead of the package and tarball dispatch. Agreed it belongs here: the README section this PR adds claims npm audit works, and /-/npm/v1/ is the namespace this PR starts routing.

Worth recording how close this came to shipping untested: my first version of TestNPMKeysProxiesUpstream passed without the fix. The stub registry answered every path, and Go's server decodes %2F back into r.URL.Path, so asserting on Path could not see the escaping. It now asserts on EscapedPath() and answers 405 for anything else, which reproduces your trace exactly — 502 {"error":"failed to fetch from upstream"} — and passes with the route in place.

README and the endpoint table now cover npm audit signatures and the keys path.

Comments

Net −32 comment lines (63 removed, 31 added). Dropped the change history, the ERR_PNPM_AUDIT_BAD_RESPONSE narration, the buffering rationale, and the test doc comments that only restated the function name. Kept the dispatch-order constraint, why Content-Encoding has to travel with the body, the EscapedPath reason, and the note that advisories come from upstream's database and ignore cooldown.

npmSecurityForwardHeaders

Now a local slice in handleSecurity; the //nolint:gochecknoglobals is gone.

One addition to call out

The two new slice literals pushed "Accept" to goconst's five-occurrence threshold, which failed lint on pre-existing lines in gem.go and hex.go as well as mine. I added headerAccept to the header constant block and used it in npm.go only, which drops the count below the threshold and clears all three. That is the one-line change in handler.go.

I left gem.go and hex.go on the literal to keep this focused — happy to convert those three slice-literal sites too if you would rather the constant be used consistently.

Verification

gofmt, go vet ./..., go test ./... and golangci-lint run are clean. Both new tests were verified to fail with their fix reverted.