Skip to content

Commit

Permalink
registry: verify integrity when loading manifest
Browse files Browse the repository at this point in the history
I ran into a weird issue where an options object was being reused
improperly between pacote.manifest() calls, leading to the tree builder
believing that it was safe to replace basically any node in the tree
with any new node it fetched.

While this is clearly a bug downstream, it would've been easier to catch
if pacote could flag this as a problem.

As of this change, if the provided integrity does not share any
algorithms in common with the integrity in the dist field (either
dist.integrity or dist.shasum) then it's concatenated onto the provided
integrity.  If it DOES share any algorithms in common, but the hashes
don't match, then an EINTEGRITY error is raised.

This also lets us more easily upgrade to newer integrity algos in the
future, and clients will transparently update their metadata when this
happens.
  • Loading branch information
isaacs committed Nov 27, 2019
1 parent f28888e commit e8c274c
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 5 deletions.
31 changes: 26 additions & 5 deletions lib/registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,32 @@ class RegistryFetcher extends Fetcher {
if (dist) {
this.resolved = mani._resolved = dist.tarball
mani._from = this.from
if (!this.integrity) {
if (dist.integrity)
this.integrity = ssri.parse(dist.integrity)
else if (dist.shasum)
this.integrity = ssri.fromHex(dist.shasum, 'sha1', this.opts)
const distIntegrity = dist.integrity ? ssri.parse(dist.integrity)
: dist.shasum ? ssri.fromHex(dist.shasum, 'sha1', this.opts)
: null
if (distIntegrity) {
if (!this.integrity)
this.integrity = distIntegrity
else if (!this.integrity.match(distIntegrity)) {
// only bork if they have algos in common.
// otherwise we end up breaking if we have saved a sha512
// previously for the tarball, but the manifest only
// provides a sha1, which is possible for older publishes.
// Otherwise, this is almost certainly a case of holding it
// wrong, and will result in weird or insecure behavior
// later on when building package tree.
for (const algo of Object.keys(this.integrity)) {
if (distIntegrity[algo]) {
throw Object.assign(new Error(
`Integrity checksum failed when using ${algo}: `+
`wanted ${this.integrity} but got ${distIntegrity}.`
), { code: 'EINTEGRITY' })
}
}
// made it this far, the integrity is worthwhile. accept it.
this.integrity = this.integrity.concat(distIntegrity)
this.opts.integrity = this.integrity
}
}
}
if (this.integrity)
Expand Down
38 changes: 38 additions & 0 deletions test/registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,44 @@ t.test('provide invalid integrity, fails to unpack', async t => {
})
})

t.test('provide invalid integrity, fails to manifest', async t => {
const f = new RegistryFetcher('@isaacs/namespace-test', {
registry,
cache,
integrity: 'sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=='
})
return t.rejects(f.manifest(), {
code: 'EINTEGRITY',
})
})

t.test('provide different type of integrity, concats', async t => {
const f = new RegistryFetcher('@isaacs/namespace-test', {
registry,
cache,
integrity: 'sha_a_le_beef-this_is_a_very_bad_joke_im_so_sorry',
})
return f.manifest().then(mani =>
t.equal(
mani._integrity,
'sha_a_le_beef-this_is_a_very_bad_joke_im_so_sorry ' +
'sha512-5ZYe1LgwHIaag0p9loMwsf5N/wJ4XAuHVNhSO+qulQOXWnyJVuco6IZjo+5u4ZLF/GimdHJcX+QK892ONfOCqQ=='
))
})

t.test('provide matching integrity, totes ok', async t => {
const f = new RegistryFetcher('@isaacs/namespace-test', {
registry,
cache,
integrity: 'sha512-5ZYe1LgwHIaag0p9loMwsf5N/wJ4XAuHVNhSO+qulQOXWnyJVuco6IZjo+5u4ZLF/GimdHJcX+QK892ONfOCqQ==',
})
return f.manifest().then(mani =>
t.equal(
mani._integrity,
'sha512-5ZYe1LgwHIaag0p9loMwsf5N/wJ4XAuHVNhSO+qulQOXWnyJVuco6IZjo+5u4ZLF/GimdHJcX+QK892ONfOCqQ=='
))
})

t.test('404 fails with E404', t => {
const f = new RegistryFetcher('thing-is-not-here', {registry, cache})
return t.rejects(f.resolve(), { code: 'E404' }).then(() =>
Expand Down

0 comments on commit e8c274c

Please sign in to comment.