feat(npm): streampub streaming attachment integration (part 6/8)
📦 What this MR does
Part 6/8 of the stack splitting the streampub streaming publish-envelope library (S11 Step 13). See Part 1/8 (!432) for the stack overview and the byte-identity contract against the reference, !420 (closed).
- Targets
10io/npm-local-step-13-part5of8(Part 5/8). GitLab retargets tomainas the parent merges. - Depends on Part 5/8 (set as a merge-request dependency).
This is the streaming integration — it wires the leaf primitives into the walk, completing the single-pass design. After this, only the checked-in fixture corpus and the fuzz target remain (Part 8/8).
pipeline.go(new) —pipeAttachmentowns theio.Pipelifecycle: base64 decode → multi-hash + size-limit → bounded buffer → inspector goroutine, with the drain-on-error /CloseWithErrorcontract so neither side blocks until the request read deadline.resolveAttachmentcombines the producer and inspector outcomes into one typed result.attachment.go— thedatafield is now streamed (streamData) rather than skipped: it splices the decoder'sUnreadBufferahead of the reader, hands the base64 body to the pipeline, then consumes the envelope remainder withconsumeToEnvelopeEnd— a string-aware tail scanner that enforcesMaxEnvelopeDepthinline, so a depth bomb after_attachmentscannot bypass the tokenizer's guards. This replaces the Part 4/8 structural stub.inspector.go(new) —runInspectorgzip-decompresses, walks the tar, capturespackage/package.json(bounded byMaxPackageJSONSize), validates entry paths (rejecting absolute,.., and backslash traversal), and always drains the pipe.walker.go— gains thesrcreader and thedoneflag the streaming hand-off needs.error.go— the coherence / tarball / package-json codes and sentinels, referenced for the first time here.- Deletes
primitives_test.go— the Part 5/8 throwaway. The primitives are now exercised black-box throughProcess.
🧪 Tests
TestProcess_HappyPath— the full result: multi-hash digests (SHA-256/1/512) over the decoded tarball, capturedpackage/package.json, tarball size, and the envelope identity fields.TarballSizeInvalid,Base64Corruption,DecoderBoundaryClean,TarBackslashTraversalRejected,ErrorDoesNotLeakEntryName,TrailingFieldBoundBypass(the AC-44 envelope-tail depth bound via the streaming tail scanner).
The fixture-corpus coherence cases (TestProcess_CoherenceFailures) and the fuzz target land with the checked-in corpus in Part 8/8.
golangci-lint(repo-pinned v2.12) → 0 issuesGOEXPERIMENT=jsonv2 go test -race -count=1 ./internal/format/npm/streampub/...→ pass (17 tests)- Diff is 466 net lines (633 added / 167 removed — the removed lines are the deleted throwaway test and the Part 4/8 attachment stub now superseded by
streamData). Within the 500-LOC reviewability guideline; the production half is one interlocking mechanism (the single-pass hand-off), reviewed as a unit.
🚩 Deviations from the reference (!420 (closed))
With this part, attachment.go, walker.go, token.go, versions.go, name.go, base64reader.go, buffer.go are byte-identical to the reference. The remaining deviations are the previously-documented reviewed changes, the stale-comment fixes, plus two review fixes to pipeline.go/inspector.go here:
error.go/streampub.go— theProcessErrorAPI + six-capvalidate(Parts 1/2).sink.go— thesizeLimitWriterbyte-count fix (Part 5).pipeline.go—pipeAttachmentnow closes the pipe writer via a deferredCloseWithErroron the named return, per the S11 coherence-sink lifecycle contract (the reference closes inline only; a producer panic could leave the inspector blocked). The inline close is retained before the join so the inspector unblocks on the normal path (a defer-only form would deadlock).inspector.go—inspectTarballskips a non-regular tar entry atpackage/package.jsonrather than capturing it as the manifest, perdocs/dev/go-secure-coding.md("skip non-regular files"). Path-traversal validation still runs for every entry.- Stale comment fixes (this part):
TestProcess_TarBackslashTraversalRejected,TestProcess_ErrorDoesNotLeakEntryName, andTestProcess_TrailingFieldBoundBypasscarried test-author-phase "this test FAILS today" notes (andErrorDoesNotLeakEntryNamedescribed the pre-Part-1Error()format that embedded the cause). The implementations now satisfy these tests, so the comments are corrected to describe current behavior. - Added test coverage (
6c6e17c):TestProcessError_LeakPreventionContractasserts both halves of the*ProcessErrormodel —Error()returns only the spec code, and the wrapped cause stays reachable viaUnwrap. This takeserror.goUnwrapfrom 0% → 100%; it was unexercised becauseerrors.Asin the helpers matches*ProcessErrorat the first level without unwrapping past it. The reference leavesUnwrapat 0% too; this is extra coverage in the permanentstreampub_test.go, not a functional change.
Security / spec-compliance deviations (deep review, 2ff91d6). Two gaps that exist in the reference itself; both run on untrusted input once streampub is wired into the publish handler (a later step), so closed before that:
- Decompression-bomb cap (
inspector.go/streampub.go): the reference caps only the compressed.tgz(MaxTarballSize) and the manifest body, leaving the decompressed tar walk unbounded — ago-secure-coding.mdviolation (a small payload can inflate ~1000:1). Adds a dedicatedMaxUncompressedTarballSizeLimitscap, wraps the gzip stream inio.LimitReader, and fails closed invalidate(); exceeding it ismanifest_coherence_failed. Open item: the production default for this cap needs config/spec-author sign-off at the publish-handler wiring step (this part only adds the injected bound + enforcement). - Inspector panic recovery (
inspector.go): S11 Step 3 requires "the reader goroutine MUST NOT panic", but the reference has norecover()— a panic in the off-request inspector goroutine (beyond LabKit'sPanicRecoveryMiddleware) would crash the process.runInspectornow recovers into a coherence failure, still draining and sending once ondone. Follow-up (09f4555): the recover path wraps a distincterrInspectorPanicsentinel (noterrCoherence) soerrors.Isseparates a recovered panic from routine corruption (client code unchanged:manifest_coherence_failed); the panic metric/export is deferred to the wiring step. - Control-byte tar-path rejection (
inspector.go,c43746c):validateEntryPathnow rejects NUL and ASCII control bytes (go-secure-coding.md), the same path-poisoning class as the existing backslash guard. The reference lacks this. NUL can't round-trip througharchive/tar's writer, so it's tested white-box againstvalidateEntryPath; a surviving control byte is tested black-box throughProcess. - Docstring scope (
streampub.go): theProcesscancellation claim is scoped to the tokenizing phase; the streaming phase's backstop is the request read deadline (S11 Step 3 forbids actx.Done()side goroutine). Comment-only. - Tests:
TestProcess_UncompressedTarballTooLarge,TestRunInspector_RecoversPanic(white-box, panicking reader), andTestProcess_ManifestTooLarge(AC 41package_json_too_large, inline — the reference covers it only via the Part 8/8 fixture corpus).
🔗 References
- Reference MR (read-only): !420 (closed)
- Related work item: #131 (closed) (not closed by this MR)