fix(npm): close hosted-format DoS gaps found in AppSec review

Summary

Fixes three findings from the AppSec review on the npm hosted format (work item 241): each one lets a single client push the shared multi-tenant process toward OOM, two of them without authentication.

  1. Inline-build concurrency across distinct packages (High). Packument and dist-tags cache misses build the document inline; singleflight only collapses concurrent misses for the same package. Nothing bounded concurrent builds across different packages, so a burst of misses against several large packages could grow live heap without limit. Added a process-wide semaphore (cap 64, mirroring the existing rebuildSem/bufferedUpdateSem pattern) that sheds excess builds with a new 503 inline_build_capacity_exceeded instead of running them. The shed response carries Cache-Control: no-store and a Retry-After, and is logged at Warn with the package coordinates. Scope note: the cap's unit is the singleflight key (namespace, package, document kind), not the package, so one package's full, abbreviated, and dist-tags builds occupy separate slots. It bounds concurrency, not memory — at the version-count and package.json ceilings the service permits, 64 concurrent builds still reach tens of GB, so this reduces the blast radius rather than closing the OOM outright.

  2. Unpublish body-size cap reused from publish (High). The single-version unpublish PUT (.../-rev/{rev}) capped its body at npm.max_publish_envelope_size (6.7GB, sized for a base64 tarball this route never carries), and only the JSON versions map's keys are ever read. Added a purpose-sized, configurable npm.max_unpublish_envelope_size (default 100MB) and switched the endpoint to it. Sizing note: real npm clients (via libnpmpublish) resend the full remaining packument, not just keys, so the cap can't be tiny; 100MB comfortably covers the largest real packument I could find (npm itself, 604 versions, 25.5MB) with headroom, while cutting the DoS blast radius ~67x from the previous cap. It does not cover every package npm.max_versions_per_package admits: at the max_package_json_size ceiling the default is reached around 5,000 versions against that setting's default of 25,000, so a package past that point cannot be unpublished a version at a time until an operator raises the value. The trade is deliberate and docs/dev/configuration-reference.md states it.

    The cap is what bounds this route's memory. An earlier revision of this description said the destination type made peak heap track the key count rather than the body size; that was wrong, as @jdrpereira's review showed — json.Decoder.Decode buffers the whole top-level value before unmarshaling, so decoding cost O(body size) whatever the values decoded to. The parser now walks the document instead: keys are read with json.Decoder.Token, and every value is consumed through a decode into an empty struct, which discards an object's fields and rejects a scalar only after the value has been consumed, so nothing is materialized at any size. Measured on a 50MB packument of 1,000 versions (sampled peak heap, default GC pacing): 118MB decoding into map[string]struct{}, 149MB into the original map[string]json.RawMessage, 0.2MB for the walk. Peak now scales with the largest single value in the document, not the document — so a body that is one 50MB string still costs 117MB, and the cap remains the bound for that shape.

  3. Per-package version/tag caps enforced only against a buffered counter (Low, amplifies finding 1). npm.max_versions_per_package and npm.max_tags_per_package were pre-checked against npm_packages.versions_count/tags_count, which are eventually-consistent buffered counters that can lag under contention or drop increments entirely when the buffered-update in-flight cap saturates — so the caps were effectively advisory. Added an authoritative COUNT(*) re-check inside the write transaction (publish's version insert, and every npm_tags insert — dist-tag PUT and publish's envelope dist-tags alike), under the npm_packages row lock the transaction already holds, so the check is race-free. A rebind of an existing tag is exempt, matching the pre-check's own rule.

All three fixes update docs/specs/S11-npm-hosted.md (Error Cases, Security Considerations, and the relevant endpoint/config tables) to match.

From review

Beyond the per-finding fixes above, the review rounds changed these:

  • The authoritative version-count re-check in finding 3 ran before the npm_versions insert, so a re-publish of an existing version at exactly the cap answered 422 version_count_exceeded where the duplicate owes the client 409 version_exists. It now runs after the insert, at count > limit, matching the tag path.
  • The shed 503's code was missing from the npm request-metric code closed set (internal/metrics/cardinality.go), which AuditCardinality would have failed on first emission, and the shed completion-logged at Info because isServerErrorCode did not list it. Both fixed.
  • Cache-Control is no longer staged before the packument cache lookup. Staging it put a directive that is cacheable on a public repository onto every response written downstream, leaving each error path to remember an override; the success paths set it now, so an unhandled path is uncacheable by omission. The dist-tags handler already worked this way.
  • The publish path's dist-tag COUNT(*) is taken on the first genuine insert rather than eagerly, so a publish whose only dist-tag rebinds latest — nearly every publish after the first — no longer pays a count under the npm_packages row lock.
  • The two new count queries no longer bake namespace and package UUIDs into their error strings (docs/dev/database-query-patterns.md's identifier-free rule).
  • The collapse-metering exclusion moved into a countsAsCollapse predicate with a table test: reached only through collapseInlineBuild, the shared-shed case was observable in well under 1% of runs, so the guard did not hold.
  • The JSON object walk the unpublish parser needs is now shared with the publish/deprecate route sniffer (internal/format/npm/json_walk.go) instead of duplicated, and returns errors so the unpublish route keeps its 413-vs-400 split on *http.MaxBytesError.

Testing

  • Unit tests for the new semaphore, the 503/422 error-code mappings, config parsing/defaulting, the countsAsCollapse predicate over every (shared, err) pair, and parseSubmittedVersions over the document shapes it accepts, the shapes it rejects, and the *http.MaxBytesError propagation the 413 rests on.
  • Integration tests against a real Postgres database for the authoritative count guards (including that a tag-cap rejection rolls back an already-inserted version row in the same transaction) and the dist-tag/unpublish write paths.
  • Full npm e2e conformance suite (real npm CLI round-trip) passes, including the exact unpublish flow finding 2 changed.
  • golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 reports no findings attributable to this diff. The touched files do still carry pre-existing contextcheck findings (~92), all of them the shared seedNamespace/seedNpmRepository/seedNpmPackage test helpers wanting a context parameter. That backlog is repo-wide rather than local to this MR — the same linter reports 2,574 findings across internal/format/npm, internal/datastore, and internal/config alone, and closing it means changing helper signatures used at 812 call sites in 86 files. Left for a separate refactor(test): MR.

e2e scenario catalog

No docs/testing/e2e/npm.md catalog is merged yet (only a draft exists on an unmerged branch), and the catalog's own stated scope is user-journeys, not protocol/endpoint-level or capacity/concurrency assertions — no catalog (merged or draft) has any rate-limit/capacity/concurrency rows today. No entry added.

Related to #241

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading