fix(npm): accept yarn's _attachments-first publish envelope

Hayley Swimelarrequested to merge
1020/yarn-publish-envelope-key-order into main

Summary

Every yarn npm publish gets 400 publish_envelope_invalid. yarn's makePublishBody serializes the envelope as _id, _attachments, name, access, dist-tags, versions, readme. The streampub walk depended on that order: after the attachment's data string it byte-scanned the rest, so a name or versions after _attachments was never read, and the handler's gate rejected the incomplete identity as non_canonical before a byte streamed. npm puts name and versions first, so npm published and yarn never did.

What is not obvious

  • The walker resumes the same decoder. After data streams, jsontext.Decoder.Reset runs over a synthetic prefix that reopens the three enclosing containers, carrying the per-object key counts and an absolute abort offset. Every bound the tokenizer already enforced now reaches late keys, and the byte scanner is gone.
  • The deferred path. When name or the version key trails _attachments, the gate checks quota, opens the session, streams the tarball, and runs the identity checks before commit. A rejection cancels the session. So a yarn-order Step 2 rejection streams the tarball first, and quota outranks version-exists and dist-tag-shape rejections in that order (429 where npm's order answers 409 or 422). npm's order still fails fast before any byte.
  • This MR retires non_canonical from metrics.md and internal/metrics/cardinality.go, so that publish_gate_rejections_total series stops at rollout. Nothing in this repo alerts on it. Check the runbooks before merging.
  • The two cmd/artifact-registry boot probes send a data-less attachment, because scanForAttachmentsKey routes on the key's presence.
  • S11 ### Publish flow now reads in condition-and-event form: Step 2 runs before any byte when the identity precedes _attachments, and after the stream when it follows.
  • One 2xx to 4xx tightening: a two-entry _attachments map is 400 (AC68). main silently dropped the second entry. The spec always forbade it. The clients that send one are npm publish --provenance and yarn npm publish --provenance, whose second entry is the {name}-{version}.sigstore attestation bundle: main admitted those publishes and discarded the bundle, this MR refuses them, and npm hosted: decide how a provenance publish's .... (#1156 - closed) • Hayley Swimelar holds the product decision.
  • A client hangup books 499 at Warn. writeStorageError hands a request whose context ended to writeIfClientClosed, under every 500 writer in the handler and in both key orders, with upload_id on the line once a session is open. The tarball arm returns the context error too, so a cut inside the base64 data is a 499 rather than the inspector's 422, and the Warn line keeps the read error: an i/o timeout when npm.publish_read_timeout fired, an unexpected EOF when the client left. main booked 500 at Error before _attachments and 400 after it.
  • The _attachments key is bounded at streampub.MaxAttachmentKeyLength, 475 bytes: a 214-byte name, -, node-semver's 256-byte version, and .tgz. The check runs before the key is stored or copied into the resume prefix, so a key no npm-compatible client can send streams no tarball. AR's ValidateVersion has no length cap, so a version past 256 bytes is refused only through its key.
  • Every rejection raised inside the tokenizing pass carries AbortOffset: bounds, syntax errors, shape and name rejections, and the data string's opening scan. The gate, the tarball pipeline, and validateShape leave it zero. A repeat of a key the resume prefix re-declares (_attachments, data) is caught by the decoder's unique-name rule rather than by dispatchTopLevel, and both record where they stopped.

Size

3798 reviewable LOC across 37 files (guardrail 18), at a1b9157a. Split: streampub production 463, other production (npm handler, metrics, cmd) 450, streampub tests 1259, npm and cmd tests 1461, and docs 165. Production is net +138 lines of code and -200 lines of comments. Three pieces could ship apart, and each rides here for a reason:

  • The streampub resume with its tests (about 1150 LOC) is not behavior-neutral on its own: it reads the keys main's byte scanner skipped, so an npm-order envelope with dist-tags after _attachments would carry tags the gate never shape-checked into the commit stage. The handler's deferred re-check is what closes that, so the two halves land together.
  • The non_canonical retirement (49 LOC across metrics.go, metrics.md, and cardinality.go) deletes a sentinel that has no caller once the handler defers. Shipped apart, it leaves dead code with a live metric label in between.
  • The one-entry _attachments rule is forced by the resume: once the decoder reads a second entry, the walker has to admit, skip, or reject it, and the spec never allowed the silent drop main performed.

Plan file

docs/plans/2026-05-11-npm-hosted.md:1245 describes the Step 15 gate rejecting an unknown identity with errNonCanonicalEnvelope, a symbol this MR deletes. The plan stays as written: it records what Step 15 built, and plan files are not edited from implementation MRs (guardrail 4).

Rollback

Revert the merge as a whole. A production-only revert leaves the test-first commit's edits to eight existing test files red, and four later commits carry test and code together.

Governing ADRs

ADR-008 (two-phase upload): the deferred path reuses the one session and cancels it on rejection, no second staging path. ADR-011 (reconciliation): a canceled session's staging object is the best-effort-deleted class the ADR accepts. ADR-023 (code structure): streampub stays free of DB and HTTP, and the handler keeps the DB-backed checks. ADR-004, ADR-009, and ADR-025 read, no conflict. No amendment needed.

Merge order and overlap

Six open MRs touch files this MR touches, measured before the current rebase onto main. Re-check at merge time, since that rebase may have merged or reordered some. Merge-second reconciles in every case. Merge-tree is clean against the first two, and the other four already conflict with main on their own.

The three e2e MRs mark the publish-yarn row blocked on the issue this MR closes, so whichever lands second updates that row. The last MR's Go files merge clean against this head and only its npm.md hunk conflicts.

Testing

Unit and fuzz suites (FuzzProcess carries a yarn-ordered seed) and the TestPublish* integration suites pass at a1b9157a, the latter against a local Postgres. The full integration run under testcontainers and scripts/conformance/npm-e2e.sh on the npm CLI last passed at 4abcdde9's pre-rebase tree, apart from four middleware and remote tests that fail locally on the Postgres container's out of shared memory in files this branch does not touch. The yarn CLI leg ran against a local instance built from the branch before the current rebase onto main (round 4 changes the publish log lines and the log-field bound and tests only, no publish wire behavior): yarn 4.18.0 yarn npm publish answered 201, yarn npm info and npm view read the version back, and a second yarn npm publish of the same version answered 409 after the stream with the session canceled. docs/testing/e2e/npm.md gains the yarn-order rows (guardrail 12): the publish-yarn outcome names the key order, attachments-before-metadata pins order independence for other clients, attachments-first-over-quota pins the order-dependent precedence (blocked on the publish quota checker, #260), and reject-two-attachments pins the one-entry rule and names the provenance flow it refuses.

Checks against a running instance:

  • A yarn-order PUT answers 201 with one session and one blob row.
  • The same body for an existing version answers 409 after the stream, with the session canceled and zero blob rows.
  • dist-tags after _attachments bound to another version answers 422 dist_tag_version_mismatch.
  • A per-version identity divergence answers 400 and increments publish_gate_rejections_total{rejection_reason="identity_diverges"}.
  • Two _attachments entries answer 400.
  • npm's order is unchanged and fails fast before any byte.

Spec coverage

Spec: docs/specs/S11-npm-hosted.md. The table from the test-first commit, refreshed with the tests later commits added.

Acceptance criteria

# Criterion Tests
AC2 An existing version answers 409 version_exists and writes no rows TestPublish_AttachmentsFirst_VersionExists_RejectsAfterStream, TestPublishStreamIntegration_AttachmentsFirst_VersionExists_CancelsSession. npm order: existing TestPublishStreamIntegration_VersionExists_NoSessionOpened
AC37 Invalid JSON: 400 bad_request, no upload session opened TestProcess_TruncatedAfterAttachment (the two container-boundary cuts after the attachment), TestProcess_TruncatedAtVersionsBoundary (both orders). When the cut follows _attachments, the deferred Cancel every post-session streampub error takes closes the session, asserted by existing TestPublishStreamIntegration_MalformedTarball and by TestPublishStreamIntegration_AttachmentsFirst_VersionExists_CancelsSession
AC38 Shape-invalid envelope: 400 publish_envelope_invalid, no rows TestProcess_EnvelopeShapeInvalid (new case: the attachment key mismatches a name read after it, cause pinned), TestProcess_AttachmentsFirst_ReadsLateIdentity, TestProcess_AttachmentGate_DefersEmptyIdentity, TestProcess_AttachmentKeyTooLong. Handler level, after the stream: TestPublishStreamIntegration_AttachmentsFirst_ShapeRejection_CancelsSession (mismatched key row)
AC39 name fails the npm regex: 422 package_name_invalid, no upload session opened TestProcess_PackageNameInvalidAfterAttachment, and at the handler level after the stream TestPublishStreamIntegration_AttachmentsFirst_ShapeRejection_CancelsSession (invalid name row: session canceled, no row). npm order: existing TestProcess_PackageNameInvalid
AC43 The tokenizer aborts at the first byte past npm.max_envelope_package_json_size, with AbortOffset at most the cap plus one byte TestProcess_AbortOffsetAbsoluteAfterAttachment (absolute offset for a cap tripped after the attachment, over whole-body, one-byte, and half readers). npm order: existing TestProcess_EnvelopePackageJSONTooLarge
AC44 Nesting past the depth limit: 422 envelope_too_deep TestProcess_TrailingFieldBoundBypass (after _attachments), existing TestProcess_EnvelopeTooDeep
AC45 More keys than the per-object limit in any one object: 422 envelope_too_many_keys TestProcess_ObjectKeyCapSpansAttachment (keys on both sides of _attachments count as one object, top level and attachment object), TestProcess_TrailingObjectKeyBound, TestProcess_ObjectKeyCapDoesNotLeakIntoVersions (no false positive at the cap), existing TestProcess_EnvelopeTooManyKeys
AC46 dist-tags over npm.max_envelope_dist_tags: 422 envelope_dist_tags_too_many TestProcess_AttachmentGate_DistTagsAfterAttachment (subtest: the cap applies after the attachment), existing TestProcess_EnvelopeDistTagsTooMany
AC47 versions with two entries: 400 publish_envelope_invalid, abort before the second value TestProcess_TwoVersionsAfterAttachmentAbortsEarly, existing TestProcess_TwoVersionsAbortsEarly and TestProcess_VersionsShapeInvalid
AC51 A successful publish leaves no resident coherence-sink goroutine TestProcess_NoGoroutineLeakOnSuccess_AttachmentsFirst, existing TestProcess_NoGoroutineLeakOnSuccess and TestPublishStreamIntegration_SuccessPath_NoCASGet_NoGoroutineLeak
AC54 Storage quota exceeded: 429 quota_exceeded, no upload session opened Yarn order: TestPublish_AttachmentsFirst_OverQuota_OpensNoSession (the gate runs quota before the session opens), TestPublish_QuotaReadOncePerPublish (the deferred pre-check skips the quota step), TestPublish_QuotaAndVersionExists_PrecedenceByKeyOrder (429 outranks 409 when the identity trails), TestPublish_AttachmentsFirst_QuotaCheckFails_Returns500. npm order: existing TestPublishStreamIntegration_OverQuota_NoSessionOpened
AC66 Publish dist-tags: a semver-shaped name is 422 tag_name_invalid, a value other than the published version is 422 dist_tag_version_mismatch, both before any rows TestPublish_DistTagsAfterAttachment_RechecksPrecheck (mismatch read after _attachments, nothing reaches the committer), TestPublish_QuotaAndDistTagShape_PrecedenceByKeyOrder (a semver-shaped tag before _attachments outranks version-exists and quota with no quota read, and after it the gate's quota verdict answers first, before any byte), existing TestDistTagShapeVerdict, TestPublishCommitIntegration_SemverTagName_Rejected, and TestPublishCommitIntegration_MismatchedDistTagValue_Rejected
AC68 (new) _attachments with two entries: 400 publish_envelope_invalid, the tokenizer aborts on the second key before reading its value TestProcess_TwoAttachmentsAbortsEarly, TestProcess_TruncatedAfterAttachment (subtest: inside a second attachment key)
AC69 (new) name, versions, or dist-tags after _attachments are admitted or rejected under the same rules and codes. A Step 2 rejection in that order arrives after the stream, cancels the session, and writes no rows. An admitted publish opens exactly one session TestProcess_AttachmentsFirst_ReadsLateIdentity (three body chunkings), TestProcess_AttachmentGate_DefersEmptyIdentity, TestPublish_AttachmentsFirst_Commits, TestPublish_AttachmentsFirst_VersionExists_RejectsAfterStream, TestPublish_AttachmentsFirst_PerVersionIdentityDiverges, TestPublishStreamIntegration_AttachmentsFirst_Commits, TestPublishStreamIntegration_AttachmentsFirst_VersionExists_CancelsSession, FuzzProcess (yarn-ordered seed). Precedence over three orders: TestPublish_QuotaAndVersionExists_PrecedenceByKeyOrder (identity first 409 with no session and no quota read, identity after 429 with one quota read and no session, only dist-tags trailing 409 before any byte). Deferral trigger: TestStagingStateIdentityChanged. Zero-value Identity{}: TestPerVersionIdentityDiverges. Infrastructure failures: TestPublish_GateInfrastructureFailure_LogsPreCheckFailed (a gate-side 500 logs "pre-check failed"), TestPublish_AttachmentsFirst_PackageReadFails_Returns500, TestPublishStreamIntegration_AttachmentsFirst_PackageReadFails_CancelsSession

Error cases

# Condition Tests
E-1 Invalid JSON: 400 bad_request TestProcess_TruncatedAfterAttachment (cuts after the attachment value object and after _attachments), TestProcess_TruncatedAtVersionsBoundary, TestProcess_DuplicateKeysAcrossAttachment (a repeated name, versions, dist-tags, or _attachments across the attachment, and a repeated data inside it, with a counting gate proving one invocation per row and an AbortOffset before each repeated value)
E-2 Envelope shape invalid (missing or mismatched name, versions, _attachments): 400 publish_envelope_invalid TestProcess_EnvelopeShapeInvalid (late-name key mismatch), TestProcess_TwoAttachmentsAbortsEarly, TestProcess_TwoVersionsAfterAttachmentAbortsEarly, TestProcess_TruncatedAfterAttachment (inside a second attachment key), TestProcess_AttachmentKeyTooLong, TestProcess_TokenizerRejectionsCarryAbortOffset (the in-pass shape and name rejections record an absolute offset past the member they rejected), TestPublish_AttachmentsFirst_PerVersionIdentityDiverges, TestPublishStreamIntegration_AttachmentsFirst_ShapeRejection_CancelsSession
E-3 Package name fails the npm regex: 422 package_name_invalid TestProcess_PackageNameInvalidAfterAttachment, TestPublishStreamIntegration_AttachmentsFirst_ShapeRejection_CancelsSession
E-4 dist-tags binds a tag to a version other than the one published: 422 dist_tag_version_mismatch TestPublish_DistTagsAfterAttachment_RechecksPrecheck
E-5 versions["{ver}"] raw span exceeds npm.max_envelope_package_json_size: 422 envelope_package_json_too_large TestProcess_AbortOffsetAbsoluteAfterAttachment
E-6 Nesting depth exceeds the platform limit: 422 envelope_too_deep TestProcess_TrailingFieldBoundBypass
E-7 A single object exceeds the per-object key limit: 422 envelope_too_many_keys TestProcess_ObjectKeyCapSpansAttachment, TestProcess_TrailingObjectKeyBound, TestProcess_ObjectKeyCapDoesNotLeakIntoVersions
E-8 dist-tags exceeds npm.max_envelope_dist_tags: 422 envelope_dist_tags_too_many TestProcess_AttachmentGate_DistTagsAfterAttachment
E-9 Version already published: 409 version_exists TestPublish_AttachmentsFirst_VersionExists_RejectsAfterStream, TestPublishStreamIntegration_AttachmentsFirst_VersionExists_CancelsSession, TestPublish_QuotaAndVersionExists_PrecedenceByKeyOrder

Security considerations

# Concern Tests
S-1 Envelope memory budget (T-04 vector 1): nesting depth after _attachments TestProcess_TrailingFieldBoundBypass
S-2 Per-object key count after _attachments, exact across the two decoder segments TestProcess_ObjectKeyCapSpansAttachment, TestProcess_TrailingObjectKeyBound, TestProcess_ObjectKeyCapDoesNotLeakIntoVersions
S-3 versions exactly one entry after _attachments, second value never read TestProcess_TwoVersionsAfterAttachmentAbortsEarly
S-4 Per-version byte span after _attachments, absolute abort offset TestProcess_AbortOffsetAbsoluteAfterAttachment
S-5 dist-tags entries after _attachments TestProcess_AttachmentGate_DistTagsAfterAttachment
S-6 A second _attachments entry aborts before its base64 value is materialized TestProcess_TwoAttachmentsAbortsEarly
S-7 Identity confusion through duplicate names across the attachment (RFC 8259 unique names, the walker's identity-key guard, and the prefix's data guard) TestProcess_DuplicateKeysAcrossAttachment (rejected: name, versions, dist-tags, _attachments, and data. Admitted: an unknown key on both sides, and an unknown attachment field on both sides of data. The gate runs once per row)
S-8 Truncated body after the attachment: no hang, no panic, a typed rejection TestProcess_TruncatedAfterAttachment, TestProcess_TruncatedAtVersionsBoundary, FuzzProcess (yarn-ordered seed)
S-9 A single oversized top-level scalar after _attachments (yarn's readme) Not tested. Bounded only by npm.max_publish_envelope_size, the exposure a scalar before _attachments already had. Tracked in the threat-model issue named under Non-goals
S-10 Coherence-sink goroutine lifecycle (T-04 vector 3) after the tokenizer resumes TestProcess_NoGoroutineLeakOnSuccess_AttachmentsFirst, existing TestProcess_DrainOnError_BoundedTime and TestProcess_ContextCancellation
S-11 The decoded tarball streams byte-exact through the gate's sink, never buffered, at any chunking TestProcess_AttachmentGate_DefersEmptyIdentity (sink bytes equal the tarball), TestProcess_AttachmentsFirst_ReadsLateIdentity (digests over whole-body, one-byte, and half readers), existing TestProcess_TarballSizeInvalid
S-12 A rejected yarn-order publish streams its tarball into a staging object before the 4xx or 5xx: the session is canceled and no blob row is committed TestPublishStreamIntegration_AttachmentsFirst_VersionExists_CancelsSession (canceledAny, blobRowCount == 0), TestPublishStreamIntegration_AttachmentsFirst_PackageReadFails_CancelsSession, TestPublish_AttachmentsFirst_VersionExists_RejectsAfterStream (session opened, nothing committed), TestPublishStreamIntegration_AttachmentsFirst_ShapeRejection_CancelsSession (a shape rejection after the stream cancels and commits nothing)
S-13 The _attachments key is bounded before it is stored or copied into the resume prefix, so a key no npm-compatible client can send streams no tarball TestProcess_AttachmentKeyTooLong (a 214-byte name with a 256-byte version composes a key at the bound and publishes, one more version byte is refused at the key with no gate call and AbortOffset before the value, and a key at the bound matching no identity streams, then fails the shape check)
S-14 A client hangup before the attachment, inside its base64 data, or after it books 499 at Warn with the read error on the line, no Error line, and no commit. Once a session is open its Cancel is attempted once and returns the ended context's error, so the row is left for upload purging TestPublish_ClientHangup_Answers499AtWarn (three cuts, Cancel attempts counted, the cancel's Warn line asserted), TestProcess_ContextEndedInsideAttachment_ReturnsContextError (the same cut is 422 on a live context), TestPublishStreamIntegration_ClientHangup_LeavesSessionRowForPurging (one attempt, zero blob rows, one upload_sessions row against the real store)

The remaining S11 rows are unaffected and keep their existing tests.

Context for LLM agents

Rationale

  • A streampub-owned post-walk gate (Identity.Complete plus a WithPostWalkGate option). Rejected: exported API growth and a second error-wrapping path, to buy a decision the handler makes with three field equalities.
  • Open the session before the walk, check everything after. Rejected: every client loses fail-fast-before-bytes, and every rejected publish opens a session.
  • Spool the tarball while the identity is unknown. Rejected: a second staging area and doubled I/O, against ADR-008's single two-phase path and S11's no-buffering rule.
  • Keep the rejection with a better message. Rejected: yarn renders the body as [object Object], and it is not a fix.
  • Chosen: the walker resumes the same decoder after the attachment, and the handler defers the identity checks when the gate saw an incomplete identity or the walk's identity differs from the one the gate checked.

Consequences: two decoder segments count one JSON object, kept exact by two carried offsets. runPrecheck runs twice when only dist-tags trail _attachments.

Non-goals

  • A hangup anywhere in the publish books 499 at Warn through writeIfClientClosed, where main booked 500 at Error before _attachments and 400 after it. The deferred Cancel runs on the canceled request context and returns before the discard, so the staging object and its upload_sessions row wait for ADR-011 reconciliation, as a mid-tarball hangup always has. The request metric books the request under success, the rule the download and remote read paths follow for a hangup. Detaching Session.Cancel onto a bounded context is a storage.Session contract change for every format, tracked in storage: Session.Cancel returns on an ended con... (#1155) • Unassigned. No publish QuotaChecker is wired in production (#260), so the row counts against no session cap today.
  • Provenance publishes carry a second _attachments entry, the .sigstore bundle, and answer 400 under the one-entry rule where main admitted them and discarded the bundle. Skipping or storing the attestation is npm hosted: decide how a provenance publish's .... (#1156 - closed) • Hayley Swimelar.
  • Six bound classes now apply after _attachments (depth, per-object keys, version span, versions count, dist-tags count, and the second attachment), and main's silent drop of trailing dist-tags is fixed. Both are the spec's existing rules reaching keys they never reached.
  • The fix commit carries publish_stream_internal_test.go, because that test names stagingState.identityChanged, a symbol absent at the test-first commit.
  • A single oversized top-level scalar after _attachments (yarn's readme) is bounded only by npm.max_publish_envelope_size, as a scalar before it always was. A per-scalar cap belongs in skipValue and is tracked in gitlab-org/ops/artifact-registry#82+s.

Closes #1020 (closed)

Edited by Hayley Swimelar

Merge request reports