chore(npm): apply the _attachments key rules at the key (S11 Provenance Attestations plan: 8/20)
Stacked on chore(npm): bounded JSON string reader for stre... (!2805 - closed) • Hayley Swimelar • 19.5, merges after it. The dependency is chain-only: the key rules use nothing Step 4 lands.
Why
The spec's Step 1 selects each _attachments key's reader from that key's
suffix, at the key, before the key's value is read. The walker judges the key
only after the whole walk, in validateShape, so a first key such as
foo-1.0.0.tar.gz streams its whole tarball into an upload session and is
refused afterwards. After this step no session opens and no tarball byte
streams for it.
The answers a client sees change, and the plan sentence that is wrong
The merged plan says at
docs/plans/2026-09-11-npm-provenance-attestations.md:903 that "the status and
code a client sees do not change", and rests the chore type on that at
:469-471. That is inaccurate. A step MR cannot edit the plan, so this
description is the only place to record it.
- A wrong-suffix first key is refused before the gate, so the Step 2 pre-check
never runs. Envelopes that answered 409
version_exists, 429quota_exceeded, 413request_entity_too_large, 422version_count_exceeded,tag_count_exceeded,tag_name_invalid, ordist_tag_version_mismatch, or 503upload_session_capacity_exceeded, now answer 400publish_envelope_invalid. - A reversed map,
.sigstorefirst, moves from 422manifest_coherence_failedto 400publish_envelope_invalid. So does a wrong first suffix whose value does not decode. - A second key repeating the first key's name, and a body cut inside a second
key, move from
publish_envelope_invalidtobad_request, both still 400.
This is the spec's ordering rather than a regression. Step 1, parse and
validate, runs ahead of Step 2, quota and version-exists, so the old order was
the out-of-spec one. No publish that used to succeed regresses, because
validateShape already required the key to be {name}-{version}.tgz, which
always ends .tgz. Five answer classes move. Four stay inside 4xx. The fifth
crosses status classes: an envelope that would have answered 503
upload_session_capacity_exceeded now answers 400 publish_envelope_invalid.
The spec orders that one too, and it carries a client consequence: a client
that would have retried the 503 does not retry the 400.
What
- Position 3 and beyond stays unreachable while position 2 always refuses, so a three-entry map is refused at its second key. Step 11 makes position 3 reachable and owes it a bound.
readObjectKeynow materializes the second key beforeMaxAttestationKeyLengthapplies, where the previous pass peeked. A suffix cannot be judged without reading the key, so that is inherent to the rule. It is not a new class of exposure: the first key, every top-level key, and every dist-tags key already read then bound, and reaching position 2 needs a valid first key plus a cleanly-streamed tarball, which is strictly harder to drive than whatmainalready allows.npm.max_publish_envelope_sizedoes not bound it meaningfully, because its default is 6,700,000,000 bytes.- A wrong-suffix first key no longer stamps
package_versionon the completion log line and no longer feeds the envelope-identity-rejected counter, because both live inside the gate. That matches every other pre-gate refusal.
Size
520 reviewable LOC, over guardrail 18's 500. attachment.go is 93 (67 added,
26 removed), and the tests are 427: key_order_test.go 262, bounds_test.go
95, abort_offset_test.go 58, and streampub_test.go 12. Tests are 86% of the
added lines.
Splitting does not help. Guardrail 6 forbids landing the rule without its tests, and the overshoot is two coverage holes closed during review, each pinning a rule that would otherwise be droppable without a red test.
streampub_test.go is a fifth file beyond the plan's four-file list, at +12/-0
carrying only the five new FuzzProcess seeds. FuzzProcess lives in that
file, so the corpus extension has nowhere else to go. fuzz:npm already names
FuzzProcess at .gitlab-ci.yml:2786, so CI needs no change.
Test plan
mise exec -- go build ./...
mise exec -- go vet ./...
mise exec -- go test ./internal/format/npm/... ./internal/jsonsafe/... -count=1
mise exec -- golangci-lint run ./internal/format/npm/streampub/ --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false
mise exec -- bash scripts/ci/fuzz.sh -run='^$' -fuzz=FuzzProcess -fuzztime=30s ./internal/format/npm/streampub/All green. Lint reports 0 issues. FuzzProcess ran 6,292,165 executions with
no reproducer. The six tests carrying the key rules hold 63 subtests, every
resume-sensitive one crossed with the three body chunkings the suite uses
(whole body, one byte per read, and two halves).
Acceptance criteria
| # | Criterion | Tests |
|---|---|---|
| AC-1 | A first key not ending in .tgz answers publish_envelope_invalid at that key, its value unread |
TestProcess_AttachmentKeySuffixRules (first-key rows), TestProcess_AttachmentKeyRejectionsAbortAtTheKey, TestProcess_AttachmentKeyTooLong |
| AC-2 | That key opens no upload session and streams no tarball byte | TestProcess_AttachmentKeySuffixRules, TestProcess_AttachmentKeyTooLong (gate-call count 0) |
| AC-3 | A second key of any name answers publish_envelope_invalid at the key, its value unread |
TestProcess_AttachmentKeySuffixRules (second-key rows), TestProcess_AttestationKeyTooLong, TestProcess_AttachmentKeyRejectionsAbortAtTheKey |
| AC-4 | A third key answers publish_envelope_invalid at the offending key |
TestProcess_AttachmentKeySuffixRules (both three-key rows, refused at the second, which precedes the third) |
| AC-5 | The second key must end in .sigstore, bounded by MaxAttestationKeyLength |
TestProcess_AttestationKeyTooLong (the arithmetic pin, plus at, one under, and one over the bound) |
| AC-6 | A reversed map is refused at its first key, and its bundle never reaches the base64 reader | TestProcess_AttachmentKeySuffixRules ("a reversed map is refused at its .sigstore first key") |
| AC-7 | Only the {name}-{version} prefix waits for the walk to finish |
TestProcess_AttachmentKeySuffixRules (bare .tgz row: streams, gate 1, no abort offset), TestProcess_AttachmentKeyTooLong (.tgz key at the bound matching no identity) |
| AC-8 | A second key under yarn's order, with the identity still unread, is refused the same way | TestProcess_AttachmentKeyRejectionsAbortAtTheKey (yarn-order row) |
| AC-9 | TestProcess_TwoAttachmentsAbortsEarly still passes |
unchanged, passing |
| AC-10 | Every other streampub test passes unchanged | whole package, go test ./internal/format/npm/streampub/ |
Error cases
| # | Condition | Tests |
|---|---|---|
| E-1 | A key over its position's bound: publish_envelope_invalid, and the cause names that position's bound |
TestProcess_AttachmentKeyTooLong, TestProcess_AttestationKeyTooLong (cause text, the only observable telling the attestation bound from the tarball key's) |
| E-2 | A key at or under the bound whose suffix does not match its position | TestProcess_AttachmentKeySuffixRules (.tar.gz first, .tgz second), TestProcess_AttachmentKeyTooLong (a key at the bound not ending .tgz) |
| E-3 | A key that is exactly .tgz or exactly .sigstore, with no prefix |
TestProcess_AttachmentKeySuffixRules (bare-suffix rows) |
| E-4 | A body cut inside a second _attachments key: bad_request |
TestProcess_TruncatedAfterAttachment ("inside a second attachment key") |
| E-5 | A body cut inside a third key: publish_envelope_invalid, because the second key's entry-count refusal lands first |
TestProcess_TruncatedAfterAttachment ("inside a third attachment key") |
| E-6 | A second key repeating the first key's name: bad_request from the decoder's unique-name rule |
TestProcess_DuplicateKeysAcrossAttachment ("attachment entry key repeated inside _attachments") |
| E-7 | An empty _attachments map |
TestProcess_EnvelopeShapeInvalid (pre-existing, unchanged) |
| E-8 | Arbitrary bytes must not panic the walker | FuzzProcess, corpus extended with five key-rule seeds |
Security considerations
| # | Concern | Tests |
|---|---|---|
| S-1 | An over-long key never reaches Result.AttachmentKey or the resume prefix |
TestProcess_AttachmentKeyTooLong, TestProcess_AttestationKeyTooLong |
| S-2 | A refused key's value is never read | TestProcess_AttachmentKeyRejectionsAbortAtTheKey (the exact offset), assertKeyRuleAbort in the suffix and bound suites (the loose form) |
| S-3 | A refused first key stages nothing: no session, no CAS sink, no tarball byte | TestProcess_AttachmentKeySuffixRules, TestProcess_AttachmentKeyTooLong (gate-call counts) |
| S-4 | The attachment object's carried key count reaches neither the _attachments map nor a later depth-3 object |
TestProcess_SecondAttachmentKeyDoesNotInheritTheEntryKeyCount, TestProcess_ObjectKeyCapDoesNotLeakIntoVersions |
| S-5 | The cause text carries the key length, never the key | TestProcessError_LeakPreventionContract (pre-existing) |
Spec ambiguities resolved
- The memory budget's "before reading that key's value" reads as: the pass
consumes the offending key and stops on its closing quote, leaving the colon
and the value unread.
TestProcess_AttachmentKeyRejectionsAbortAtTheKeypins that offset exactly. - Step 1 and the memory budget are both silent on a truncated second key.
Reading the key to judge it makes a cut inside one a decode failure, so it
answers
bad_requestrather thanpublish_envelope_invalid. Both are 400, and the walker never received a key to judge.
Guardrails
- 12.
docs/testing/e2e/npm.md:143,e2e.npm.publish.reject-wrong-second-attachment, isautomatableandnot started, and its text already describes the refusal at the key, the reversed map included. It stays unchanged, and this step is what makes its expected outcome hold at the key. - 22. ADRs 004, 009, and 023 were read in full text. No conflict.
- 24. No page under
docs/user/namespublish_envelope_invalid,_attachments, ormanifest_coherence_failed, so no user documentation is affected. The codes a client sees do move, as the section above records.
Context for LLM agents
Rationale
- A peek guard at position 3, refusing a third key before position 2's entry-count
rule reaches it. Declined: position 2 always refuses today, so the guard is dead
code no test can drive red, and guardrail 6 has no test to pair with it. The
truncation row "inside a third attachment key" records the gap instead. It
answers
publish_envelope_invalidfrom the second key today, and turns into a real third-key assertion under a Step 11 stand-in that admits the second value. - Two separate causes for the suffix rule and the bound at each position, versus
one cause per rule shared across positions. Chosen: shared per rule, with the
bound's cause carrying its own number, because that is what lets
TestProcess_AttestationKeyTooLongcatch an implementation reusingMaxAttachmentKeyLengthat position 2 while both rows answer one code. - Chosen loop: one pass over the keys with a per-position switch in
readAttachmentEntry, and the bound plus the suffix check factored intocheckAttachmentKey, so the two positions differ only by their two constants.
Consequences: every position past the first answers the shared cause
_attachments map has more than one entry. Once Step 11 admits the second
entry's value, that cause is stale for a third key and needs one of its own.
Non-goals
internal/format/npm/metrics.md:1073-1074lists the one-entry aborts onversionsand_attachmentsamong whatpublish_envelope_invalidcovers. That is now imprecise for a repeated second key and a truncated second key, both of which answerbad_request. The file is outside this step's bounds, so it is a follow-up.- No e2e row covers a wrong-suffix first key or the zero-session property
this step's gate-call assertions pin.
e2e.npm.publish.reject-wrong-second-attachmentcovers the second key, the third key, and the reversed map. A new row is a follow-up, outside this step's files. validateShapekeeps the{name}-{version}prefix check. Moving it to the key needs the envelope identity, which yarn's order does not supply until after_attachments.
Related to #1273