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, 429 quota_exceeded, 413 request_entity_too_large, 422 version_count_exceeded, tag_count_exceeded, tag_name_invalid, or dist_tag_version_mismatch, or 503 upload_session_capacity_exceeded, now answer 400 publish_envelope_invalid.
  • A reversed map, .sigstore first, moves from 422 manifest_coherence_failed to 400 publish_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_invalid to bad_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.
  • readObjectKey now materializes the second key before MaxAttestationKeyLength applies, 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 what main already allows. npm.max_publish_envelope_size does not bound it meaningfully, because its default is 6,700,000,000 bytes.
  • A wrong-suffix first key no longer stamps package_version on 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_AttachmentKeyRejectionsAbortAtTheKey pins 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_request rather than publish_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, is automatable and not 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/ names publish_envelope_invalid, _attachments, or manifest_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_invalid from 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_AttestationKeyTooLong catch an implementation reusing MaxAttachmentKeyLength at 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 into checkAttachmentKey, 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-1074 lists the one-entry aborts on versions and _attachments among what publish_envelope_invalid covers. That is now imprecise for a repeated second key and a truncated second key, both of which answer bad_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-attachment covers the second key, the third key, and the reversed map. A new row is a follow-up, outside this step's files.
  • validateShape keeps 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

Edited by Hayley Swimelar

Merge request reports

Loading
Loading