fix(npm): reject a non-semver publish version with 422

Summary

A publish whose versions map key is not valid semver is accepted with 201, and every packument GET for that package then answers 500, forever. The row is created and permanently unreadable.

The publish path never called npmrules.ValidateVersion. The packument renderer does, fail-closed, so the write side created a row the read side refuses to project. S11 requires 422 version_invalid with no rows written (docs/specs/S11-npm-hosted.md:1794-1796, and the Error Cases row at :1960).

What a reviewer should not have to infer:

  • The gate sits in streampub at versions-map key capture, not in the handler pre-check, so a bad key never tokenizes the per-version object and never reaches the attachment gate that opens the upload session.
  • processEnvelope switches on a bare string, and its default: arm answers 400 bad_request silently. The new case is what makes this a 422.
  • A non-semver key decides the envelope. The gate wins over every rule the walk would have reached later: of the fourteen envelope shapes measured, six move from 400 publish_envelope_invalid to 422 version_invalid (the empty version key among them) and four keep 422 with a changed code. A bad name written before versions still wins, which is the npm CLI's key order, and both orders have a test.
  • npm.CodeVersionInvalid is now bound to streampub.CodeVersionInvalid, in place, so no deletion hunk reaches the neighbouring CodeUnauthorized doc and the comment-caps gate stays quiet.
  • The read path stays fail-closed, deliberately. A row published before this fix still 500s that package's documents until it is removed through DELETE .../npm/packages/{package_id}/versions/{version_id}, which resolves by UUID, and the 500's log line names the row. Skipping it on read would serve a document missing a version its publisher created.
  • docs/specs/S11-npm-hosted.md:231-232 still says 400 for this rule, against the spec's own Error Cases row. Out of scope here, and worth a spec MR.

Governing ADRs

None. ADR-004 sets npm's tarball limit and no version grammar, ADR-007 puts no format constraint on npm_versions.version, and ADR-009 defers client API behavior to each protocol's specification, which for npm is S11.

Testing

  • go test ./internal/format/npm/...: seven invalid version keys including the empty one, four valid boundary keys, the gate against an oversized per-version object and against two envelope-shape rules, both key orders, and a new TestProcessEnvelope_Mapping row.
  • go test -tags=integration ./internal/format/npm/ against a real Postgres. TestPublishCommitIntegration_VersionInvalid is the falsifiable test for "writes no rows": the pre-check harness wires no blob store, so its row assertions would pass with the gate deleted, and its doc comment says so. (Four remote-path tests fail in cleanup on this machine's Postgres lock table, identically on origin/main.)
  • golangci-lint on ./internal/format/npm/...: 0 findings by default and, with --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false, the same 66 findings origin/main reports.
  • ./scripts/ci/check-comment-caps.sh --base origin/main: OK.
  • docs/testing/e2e/npm.md: markdownlint and Vale clean.
  • scripts/conformance/npm-e2e.sh is green against isolated MinIO and Postgres and proves nothing about this fix: it carries no 422 assertion.

Spec coverage

Spec: docs/specs/S11-npm-hosted.md. Rows this change governs, plus the adjacent rows its precedence delta touches.

Acceptance criteria

# Criterion Tests
AC 40 Version key in versions is not a valid semver string: 422, code: "version_invalid", writes no rows (:1794-1796) TestProcess_VersionInvalid (7 subtests), TestProcessEnvelope_Mapping/version_invalid, TestPublishPrecheck_VersionInvalid (6 subtests), TestPublishCommitIntegration_VersionInvalid
AC 39 Envelope name fails the npm name regex: 422, code: "package_name_invalid" (:1787-1793) Existing TestProcess_PackageNameInvalid and TestPublishPrecheck_PackageNameInvalid, unedited. Precedence against the new gate is pinned by TestProcess_NameGateWinsInCanonicalOrder (2 subtests).
AC 38 Shape-invalid envelope: 400, code: "publish_envelope_invalid" (:1781-1786) Existing TestProcess_EnvelopeShapeInvalid and TestPublishPrecheck_EnvelopeShapeInvalid, unedited. Every subtest uses a valid version key, so the new gate does not reach them.
AC 47 versions map carries two entries: 400, code: "publish_envelope_invalid" (:1830) Existing TestProcess_VersionsShapeInvalid and TestPublishPrecheck_TwoVersionEntries, unedited. Both use valid version keys.
AC 42 Per-version object over npm.max_envelope_package_json_size: 422 (:1801) Existing TestPublishPrecheck_EnvelopePackageJSONTooLarge, unedited. Uses a valid version key, so the new gate does not reach it.

Error cases

Condition Behavior Tests
Version is not a valid semver (:1960) 422, code: "version_invalid" TestProcess_VersionInvalid (7 subtests), TestProcessEnvelope_Mapping/version_invalid, TestPublishPrecheck_VersionInvalid (6 subtests), TestPublishCommitIntegration_VersionInvalid
Package name fails the npm regex (:1953) 422, code: "package_name_invalid" TestProcess_NameGateWinsInCanonicalOrder/name_before_versions pins that the name gate still wins in the npm CLI's key order
Body envelope shape invalid (:1952) 400, code: "publish_envelope_invalid" Existing coverage unchanged. When the version key is also non-semver the version gate now wins. TestProcess_VersionGateWinsOverEnvelopeShapeRules pins that for the attachment-key mismatch and the second-versions-entry shapes, each run under both a valid and an invalid key. The Summary states the class the remaining shapes of the fourteen measured belong to.

Security considerations

Concern Tests
Envelope metadata memory budget (:2000-2018): the gate fires at versions-key capture, ahead of the tokenizer that enforces the 1 MB per-version byte span, so a rejected publish parses none of that object Not asserted. No seam reports bytes tokenized, and the gate's placement is what carries the property. The tests pin the code, not the work avoided.
Inline-base64 publish surface (:1994-1999): in the key orders the walk accepts today (name and versions before _attachments) the gate fires before _attachments, so no upload session opens and no tarball byte is staged TestPublishPrecheck_VersionInvalid asserts upload_sessions is 0 for the harness namespace, and TestPublishCommitIntegration_VersionInvalid asserts sessionsOpened() == 0 against the store-wired handler

Closes #364 (closed)

Context for LLM agents

Rejected alternatives

  • Gate in runPrecheck (publish_precheck.go). Order-independent precedence is its one real advantage. It costs a full envelope walk first: up to 1 MB of per-version object and up to 1000 dist-tags entries parsed before the same verdict, and it puts a publish-envelope code outside streampub. distTagShapeVerdict is a precedent for a stateless shape check in the pre-check, but the spec puts the dist-tag rule at publish-flow step 5 and the semver rule at step 1.
  • Gate in attachmentGate (publish_stream.go). Same timing as the pre-check, and no principled home in the walk.
  • Degrade the read path to skip-and-log. It would serve a document missing a version its publisher created and hide the data-integrity signal. The renderer has no diagnostic channel to report the omission.
  • Migrate or backfill the stored rows. The management API single-version delete already removes one, keeping the buffered counters and the cache force-expiry that raw SQL would skip.

Non-goals

  • Deprecate, single-version unpublish, and dist-tag PUT keep 404 version_not_found on an unknown version. S11-npm-hosted.md:1955 names all three explicitly, so a 422 there needs a spec amendment first.
  • The npm remote and virtual write paths cannot reach this gate. go list confirms internal/format/npm is the only importer of internal/format/npm/streampub, and the kind dispatch in handler.go answers 405 unsupported for both kinds without constructing a PublishHandler.
  • Whether closed-beta production holds a poisoned row is unmeasured. No production query was run. The scan is cheap, because the poisoned rows are exactly the ones npmrules.ParseVersion rejects.
  • No metrics, config, OpenAPI, or Bruno change. internal/format/npm/metrics.md enumerates no 422 denial codes, the code string is already in requestCodeLabels(), s11ErrorCaseCodes(), and internal/metrics/cardinality.go, and api/openapi/ and api/bruno/ describe only the management API for npm.
  • AbortOffset drops to 0 on the shapes that previously aborted mid-token. token.go:15 is its only writer and every reader is a test.

Known-open, not addressed here

  • A valid semver whose derived tarball name overflows the schema. npm_versions.version and npm_files.file_name each carry a char_length(...) <= 255 CHECK (internal/datastore/migrations/sql/20260603120200_create_npm_versions.sql:28 and .../20260612120300_create_npm_files.sql:33). A valid semver whose derived {name}-{version}.tgz exceeds 255 characters still returns 500 and strands a content-addressed blob. Pre-existing, unchanged by this MR, and no issue is filed for it yet.
  • The yarn key-order fix widens what the walk accepts. yarn writes _attachments before name and versions, so every yarn publish is refused today, which is yarn cannot publish: the envelope walk requires... (#1020 - closed) • Hayley Swimelar. The "gate fires before _attachments, so no upload session opens" property in the Spec coverage table holds for the key orders the envelope walk accepts today. That issue's fix widens them, and whichever of the two merge requests lands second reconciles internal/format/npm/streampub/streampub_test.go.
  • The e2e catalog overlaps an open merge request. The whole Publish table of docs/testing/e2e/npm.md has its Status cell rewritten in all eight rows by test(e2e): the end-to-end program in four profi... (!2052) • Suleimi Ahmed • 19.5, and this branch's new row lands directly below the last of them, so it is a keep-both conflict for whichever side lands second. The events-table row is clear of both that merge request and test(e2e): the npm HEAD-fill row, and two rows'... (!2219 - merged) • Suleimi Ahmed • 19.4, whose npm.md hunks stop about twenty lines short of it.

Size

+367/-12 lines, 340 of the additions in _test.go files. Under docs/dev/development-model.md's 500-LOC threshold, so the description carries no size justification.

Merge request reports

Loading
Loading