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
streampubat 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. processEnvelopeswitches on a barestring, and itsdefault:arm answers 400bad_requestsilently. The newcaseis 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_invalidto 422version_invalid(the empty version key among them) and four keep 422 with a changed code. A badnamewritten beforeversionsstill wins, which is the npm CLI's key order, and both orders have a test. npm.CodeVersionInvalidis now bound tostreampub.CodeVersionInvalid, in place, so no deletion hunk reaches the neighbouringCodeUnauthorizeddoc 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-232still 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 newTestProcessEnvelope_Mappingrow.go test -tags=integration ./internal/format/npm/against a real Postgres.TestPublishCommitIntegration_VersionInvalidis 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 onorigin/main.)golangci-linton./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 findingsorigin/mainreports../scripts/ci/check-comment-caps.sh --base origin/main: OK.docs/testing/e2e/npm.md: markdownlint and Vale clean.scripts/conformance/npm-e2e.shis 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.distTagShapeVerdictis 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_foundon an unknown version.S11-npm-hosted.md:1955names all three explicitly, so a 422 there needs a spec amendment first. - The npm remote and virtual write paths cannot reach this gate.
go listconfirmsinternal/format/npmis the only importer ofinternal/format/npm/streampub, and the kind dispatch inhandler.goanswers 405unsupportedfor both kinds without constructing aPublishHandler. - 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.ParseVersionrejects. - No metrics, config, OpenAPI, or Bruno change.
internal/format/npm/metrics.mdenumerates no 422 denial codes, the code string is already inrequestCodeLabels(),s11ErrorCaseCodes(), andinternal/metrics/cardinality.go, andapi/openapi/andapi/bruno/describe only the management API for npm. AbortOffsetdrops to 0 on the shapes that previously aborted mid-token.token.go:15is 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.versionandnpm_files.file_nameeach carry achar_length(...) <= 255CHECK (internal/datastore/migrations/sql/20260603120200_create_npm_versions.sql:28and.../20260612120300_create_npm_files.sql:33). A valid semver whose derived{name}-{version}.tgzexceeds 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
_attachmentsbeforenameandversions, 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 reconcilesinternal/format/npm/streampub/streampub_test.go. - The e2e catalog overlaps an open merge request. The whole Publish table of
docs/testing/e2e/npm.mdhas 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.