[simonchrz]

npm tarballs were cached without being checked against the dist.integrity in their packument. Whatever bytes the first fetch got were stored and then served to every client.

Changes

• Existing checks: the packument's dist.integrity goes to the proxy's integrity checks (newIntegrityChecks / integrity.Result.Verify). No second verification path is added.
  • Stored fetches: the tarball is checked while it is stored. On a mismatch the blob is deleted, nothing is cached, the failure is counted in proxy_integrity_failures_total, and the client gets a 502.
  • Streamed fetches (stream_artifacts): the check runs at end of stream through wrapFailOnMismatch, and the response is aborted on a mismatch.
• Cache hits: the integrity is recorded in versions.integrity, so readStored re-checks cache hits against it with the check that already exists.
• Matching rule: any digest of the strongest algorithm listed may match, as SRI specifies.
• Fallback: if dist.integrity is missing or can't be parsed, or the packument can't be fetched, the tarball is fetched unverified as before.
• GetOrFetchArtifactFromURLWithIntegrity is the entry point. The declared integrity is part of the fetch-coalescing key.

Not in this PR

npm's dist.shasum fallback and Composer's dist.shasum check need SHA-1. Those are in git-pkgs/integrity#3, which implements integrity#1, and draft #479, which builds on it.

Tests

npm_integrity_test.go, on both the stored and the streamed path:

• Mismatch: a mismatched sha512 returns 502, or aborts the stream, and nothing is cached. Before this change it returned 200.
• Matches: matching sha512 and sha256 are served and cached. With two digests of the strongest algorithm, either one may match.
• Strongest algorithm decides: a weaker mismatching digest doesn't count when the strongest matches.
• No usable integrity: a missing or unparseable integrity, or only a dist.shasum, is served unverified.
• Recorded: the verified integrity is stored with the cached version.

I also broke the stored check and the streamed check in turn, and the tests failed both times.

go test -race ./... and golangci-lint v2.13.1 pass.

[andrew]

Verifying npm and Composer archives against their metadata is the right thing to do, but checksum.go builds a second verification path next to the one the proxy already has.

internal/handler/integrity.go wraps github.com/git-pkgs/integrity, and newIntegrityChecks(contentHash, native) already verifies sha256/sha384/sha512 SRI on both the stored path and the streamed path (wrapFailOnMismatch). checkCache also re-verifies versions.integrity on cache hits through the same function. Recording the declared checksum there and reusing integrityChecks removes the need for checksumHasher, checksumVerifyingReader, upstreamHashForChecksum and the isDeclaredChecksum exception in artifactHashMatches, and it closes the limitation in the description, since cached artifacts would be re-checked against the metadata checksum too.

The one thing the integrity module can't do yet is SHA-1. That belongs in the module rather than in the proxy: git-pkgs/integrity#1 covers verification-only SHA-1 and MD5, hex checksum parsing, an exact-digest verify path and an explicit strength ranking (the current enum-order comparison in strongestAlgorithm becomes unsafe once weaker algorithms exist).

So the change that can land now is npm dist.integrity through the existing integrityChecks. The dist.shasum fallback for npm and the Composer dist.shasum check need integrity#1 and a dependency bump first, and should come in a separate PR after that.

Reusing integrity.Result.Verify also fixes a bug in npmVersionChecksum: it takes the first digest of the strongest algorithm, while SRI allows several digests of that algorithm and any of them may match, so a valid tarball listing two sha512 entries would currently fail as a mismatch.

[simonchrz]

Thanks, agreed on all points. I've narrowed this PR to npm dist.integrity through the existing integrityChecks, rebuilt on current main (2d258dd):

• Removed: checksum.go and its tests, checksumHasher, checksumVerifyingReader, upstreamHashForChecksum and the isDeclaredChecksum exception.
• Stored path: storeArtifact checks the declared integrity with newIntegrityChecks while storing. On a mismatch it deletes the blob, records the integrity failure and returns ErrArtifactDigestMismatch.
• Streamed path: passes the declared integrity to newIntegrityChecks(hash, declared) and wrapFailOnMismatch.
• Cache hits: the integrity is recorded in versions.integrity, so readStored re-checks hits against the metadata checksum. That closes the limitation the old description listed.
• Multiple digests: checking goes through integrity.Result.Verify, which fixes the bug you found. "any digest of the strongest algorithm" is now a test case that passes with two sha512 digests where only the second matches.

For the SHA-1 part:

• Verify registry checksums, including MD5 and SHA-1 integrity#3 implements integrity#1. It adds verification-only MD5 and SHA-1, ParseChecksum/Digest.Checksum for algorithm-prefixed hex, Result.VerifyDigest for exact digests, and an explicit strength ranking in place of enum order. It also keeps weak digests out of ParseSRI, FormatSRI and Digest.SRI.
• Draft Verify npm and Composer archives against their SHA-1 shasum #479 adds the npm dist.shasum fallback and the Composer dist.shasum check on top of this PR and integrity#3, through the same integrityChecks. It uses a temporary replace until integrity#3 is released.

[andrew]

@simonchrz i just merged the integrity pr and tagged v0.2.0: https://github.com/git-pkgs/integrity/releases/tag/v0.2.0

[simonchrz]

@andrew thanks for the quick merge and release!

• Verify npm and Composer archives against their SHA-1 shasum #479 now builds against v0.2.0. The temporary replace is gone, it's out of draft, and CI is green. It adds the npm dist.shasum fallback and the Composer dist.shasum check through the same integrityChecks.
• This PR (Verify npm tarballs against dist.integrity #473) is ready for another look whenever you have time. It's the narrowed version from my comment above (2d258dd): npm dist.integrity only, verified with integrity.Result.Verify. The "changes requested" status is still from the earlier version.

#479 sits on top of #473, so #473 should go in first. After that I'll rebase #479 onto main so it carries only the v0.2.0 bump and the shasum commit.