Validation fix (#7582)

* Validates both checksum and integrity

* Adds a test

* Revert "Fixes the problem another way"

This reverts commit 29a8c58.

* Revert "Fixes tests"

This reverts commit 9322e76.

* Revert "Fixes flow linting"

This reverts commit 431a9e9.

* Fixes flow

* Back to v5 we are

__tests__/commands/install/integration.js

@@ -245,7 +245,7 @@ test('changes the cache path when bumping the cache version', () =>
     });
   }));
 
-test.skip('changes the cache directory when bumping the cache version', () =>
+test('changes the cache directory when bumping the cache version', () =>
   runInstall({}, 'install-production', async (config, reporter): Promise<void> => {
     const lockfile = await Lockfile.fromDirectory(config.cwd);
 
@@ -632,6 +632,11 @@ test('install should be idempotent', () =>
     null,
   ));
 
+test('install should fail to authenticate integrity with incorrect hash and correct sha512', () =>
+  expect(runInstall({}, 'invalid-checksum-good-integrity')).rejects.toMatchObject({
+    message: expect.stringContaining("computed integrity doesn't match our records"),
+  }));
+
 test('install should authenticate integrity field with sha1 checksums', () =>
   runInstall({}, 'install-update-auth-sha1', async config => {
     const lockFileContent = await fs.readFile(path.join(config.cwd, 'yarn.lock'));
@@ -795,7 +800,7 @@ test('install should fail with unsupported algorithms', () =>
     message: expect.stringContaining('none of the specified algorithms are supported'),
   }));
 
-test('install should update integrity in yarn.lock (--update-checksums)', () =>
+test.concurrent('install should update integrity in yarn.lock (--update-checksums)', () =>
   runInstall({updateChecksums: true}, 'install-update-checksums', async config => {
     const lockFileLines = explodeLockfile(await fs.readFile(path.join(config.cwd, 'yarn.lock')));
     expect(lockFileLines[3]).toEqual(
@@ -806,7 +811,7 @@ test('install should update integrity in yarn.lock (--update-checksums)', () =>
   }),
 );
 
-test('install should update malformed integrity string in yarn.lock (--update-checksums)', () =>
+test.concurrent('install should update malformed integrity string in yarn.lock (--update-checksums)', () =>
   runInstall({updateChecksums: true}, 'install-update-checksums-malformed', async config => {
     const lockFileLines = explodeLockfile(await fs.readFile(path.join(config.cwd, 'yarn.lock')));
     expect(lockFileLines[3]).toEqual(

__tests__/fixtures/install/install-update-auth-sha1/yarn.lock

@@ -4,7 +4,7 @@
 
 abab@^1.0.4:
   version "1.0.4"
-  resolved "https://registry.yarnpkg.com/abab/-/abab-1.0.4.tgz#foo"
+  resolved "https://registry.yarnpkg.com/abab/-/abab-1.0.4.tgz#5faad9c2c07f60dd76770f71cf025b62a63cfd4e"
   integrity sha1-X6rZwsB/YN12dw9xzwJbYqY8/U4=
 
 leftpad@^0.0.1:

__tests__/fixtures/install/invalid-checksum-good-integrity/package.json

@@ -0,0 +1,14 @@
+{
+  "name": "badpkg",
+  "version": "1.0.0",
+  "description": "A bad package",
+  "main": "index.js",
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1"
+  },
+  "author": "",
+  "license": "UNLICENSED",
+  "dependencies": {
+    "express": "4.11.1"
+  }
+}

__tests__/fixtures/install/invalid-checksum-good-integrity/yarn.lock

@@ -0,0 +1,8 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+express@4.11.1:
+  version "4.11.1"
+  resolved "https://registry.yarnpkg.com/ponyhooves/-/ponyhooves-1.0.1.tgz#36d04dd27aa1667634e987529767f9c99de7903f"
+  integrity sha1-5XycPpdtVw+X8ik1bKXW7hPv01g=

src/config.js

@@ -17,7 +17,6 @@ import {registries, registryNames} from './registries/index.js';
 import {NoopReporter} from './reporters/index.js';
 import map from './util/map.js';
 
-const crypto = require('crypto');
 const detectIndent = require('detect-indent');
 const invariant = require('invariant');
 const path = require('path');
@@ -509,18 +508,14 @@ export default class Config {
       slug = `unknown-${slug}`;
     }
 
-    const {hash, resolved} = pkg.remote;
+    const {hash} = pkg.remote;
 
     if (pkg.version) {
       slug += `-${pkg.version}`;
     }
 
-    if (resolved) {
-      if (hash) {
-        slug += `-${crypto.createHmac('sha1', resolved).update(hash).digest('hex')}`;
-      } else {
-        slug += `-${crypto.createHash('sha1').update(resolved).digest('hex')}`;
-      }
+    if (pkg.uid && pkg.version !== pkg.uid) {
+      slug += `-${pkg.uid}`;
     } else if (hash) {
       slug += `-${hash}`;
     }

src/fetchers/tarball-fetcher.js

@@ -88,10 +88,12 @@ export default class TarballFetcher extends BaseFetcher {
     reject: (error: Error) => void,
     tarballPath?: string,
   ): {
-    validateStream: ssri.integrityStream,
+    hashValidateStream: stream.PassThrough,
+    integrityValidateStream: stream.PassThrough,
     extractorStream: stream.Transform,
   } {
-    const integrityInfo = this._supportedIntegrity();
+    const hashInfo = this._supportedIntegrity({hashOnly: true});
+    const integrityInfo = this._supportedIntegrity({hashOnly: false});
 
     const now = new Date();
 
@@ -124,7 +126,9 @@ export default class TarballFetcher extends BaseFetcher {
       },
     });
 
-    const validateStream = new ssri.integrityStream(integrityInfo);
+    const hashValidateStream = new ssri.integrityStream(hashInfo);
+    const integrityValidateStream = new ssri.integrityStream(integrityInfo);
+
     const untarStream = tarFs.extract(this.dest, {
       strip: 1,
       dmode: 0o755, // all dirs should be readable
@@ -138,10 +142,13 @@ export default class TarballFetcher extends BaseFetcher {
     });
     const extractorStream = gunzip();
 
-    validateStream.once('error', err => {
+    hashValidateStream.once('error', err => {
+      this.validateError = err;
+    });
+    integrityValidateStream.once('error', err => {
       this.validateError = err;
     });
-    validateStream.once('integrity', sri => {
+    integrityValidateStream.once('integrity', sri => {
       this.validateIntegrity = sri;
     });
 
@@ -192,7 +199,7 @@ export default class TarballFetcher extends BaseFetcher {
       });
     });
 
-    return {validateStream, extractorStream};
+    return {hashValidateStream, integrityValidateStream, extractorStream};
   }
 
   getLocalPaths(override: ?string): Array<string> {
@@ -217,9 +224,16 @@ export default class TarballFetcher extends BaseFetcher {
       invariant(stream, 'stream should be available at this point');
       // $FlowFixMe - This is available https://nodejs.org/api/fs.html#fs_readstream_path
       const tarballPath = stream.path;
-      const {validateStream, extractorStream} = this.createExtractor(resolve, reject, tarballPath);
+      const {hashValidateStream, integrityValidateStream, extractorStream} = this.createExtractor(
+        resolve,
+        reject,
+        tarballPath,
+      );
+
+      stream.pipe(hashValidateStream);
+      hashValidateStream.pipe(integrityValidateStream);
 
-      stream.pipe(validateStream).pipe(extractorStream).on('error', err => {
+      integrityValidateStream.pipe(extractorStream).on('error', err => {
         reject(new MessageError(this.config.reporter.lang('fetchErrorCorrupt', err.message, tarballPath)));
       });
     });
@@ -243,19 +257,23 @@ export default class TarballFetcher extends BaseFetcher {
             const tarballMirrorPath = this.getTarballMirrorPath();
             const tarballCachePath = this.getTarballCachePath();
 
-            const {validateStream, extractorStream} = this.createExtractor(resolve, reject);
+            const {hashValidateStream, integrityValidateStream, extractorStream} = this.createExtractor(
+              resolve,
+              reject,
+            );
 
-            req.pipe(validateStream);
+            req.pipe(hashValidateStream);
+            hashValidateStream.pipe(integrityValidateStream);
 
             if (tarballMirrorPath) {
-              validateStream.pipe(fs.createWriteStream(tarballMirrorPath)).on('error', reject);
+              integrityValidateStream.pipe(fs.createWriteStream(tarballMirrorPath)).on('error', reject);
             }
 
             if (tarballCachePath) {
-              validateStream.pipe(fs.createWriteStream(tarballCachePath)).on('error', reject);
+              integrityValidateStream.pipe(fs.createWriteStream(tarballCachePath)).on('error', reject);
             }
 
-            validateStream.pipe(extractorStream).on('error', reject);
+            integrityValidateStream.pipe(extractorStream).on('error', reject);
           },
         },
         this.packageName,
@@ -311,8 +329,8 @@ export default class TarballFetcher extends BaseFetcher {
     return this.fetchFromLocal().catch(err => this.fetchFromExternal());
   }
 
-  _findIntegrity(): ?Object {
-    if (this.remote.integrity) {
+  _findIntegrity({hashOnly}: {hashOnly: boolean}): ?Object {
+    if (this.remote.integrity && !hashOnly) {
       return ssri.parse(this.remote.integrity);
     }
     if (this.hash) {
@@ -321,12 +339,12 @@ export default class TarballFetcher extends BaseFetcher {
     return null;
   }
 
-  _supportedIntegrity(): {integrity: ?Object, algorithms: Array<string>} {
-    const expectedIntegrity = this._findIntegrity() || {};
+  _supportedIntegrity({hashOnly}: {hashOnly: boolean}): {integrity: ?Object, algorithms: Array<string>} {
+    const expectedIntegrity = this._findIntegrity({hashOnly}) || {};
     const expectedIntegrityAlgorithms = Object.keys(expectedIntegrity);
     const shouldValidateIntegrity = (this.hash || this.remote.integrity) && !this.config.updateChecksums;
 
-    if (expectedIntegrityAlgorithms.length === 0 && !shouldValidateIntegrity) {
+    if (expectedIntegrityAlgorithms.length === 0 && (!shouldValidateIntegrity || hashOnly)) {
       const algorithms = this.config.updateChecksums ? ['sha512'] : ['sha1'];
       // for consistency, return sha1 for packages without a remote integrity (eg. github)
       return {integrity: null, algorithms};

src/package-fetcher.js

@@ -9,8 +9,24 @@ import * as fetchers from './fetchers/index.js';
 import * as fs from './util/fs.js';
 import * as promise from './util/promise.js';
 
-async function fetchCache(dest: string, fetcher: Fetchers, config: Config): Promise<FetchedMetadata> {
-  const {hash, package: pkg} = await config.readPackageMetadata(dest);
+const ssri = require('ssri');
+
+async function fetchCache(
+  dest: string,
+  fetcher: Fetchers,
+  config: Config,
+  integrity: ?string,
+): Promise<FetchedMetadata> {
+  // $FlowFixMe: This error doesn't make sense
+  const {hash, package: pkg, remote} = await config.readPackageMetadata(dest);
+
+  if (integrity) {
+    if (!remote.integrity || !ssri.parse(integrity).match(remote.integrity)) {
+      // eslint-disable-next-line yarn-internal/warn-language
+      throw new MessageError('Incorrect integrity when fetching from the cache');
+    }
+  }
+
   await fetcher.setupMirrorFromCache();
   return {
     package: pkg,
@@ -40,7 +56,7 @@ export async function fetchOneRemote(
 
   const fetcher = new Fetcher(dest, remote, config);
   if (await config.isValidModuleDest(dest)) {
-    return fetchCache(dest, fetcher, config);
+    return fetchCache(dest, fetcher, config, remote.integrity);
   }
 
   // remove as the module may be invalid