Fix yarnpkg#3697 Improve error handling when tarball fetcher receives…

… bad hash (yarnpkg#4240) * test * alter commit message * update snapshot * review feedback * move to unlink files in all failed blocks * lint * Update tarball-fetcher.js
joaolucasl · Oct 27, 2017 · d1837ee · d1837ee
1 parent cc264bf
commit d1837ee
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 5 deletions.
diff --git a/__tests__/__snapshots__/fetchers.js.snap b/__tests__/__snapshots__/fetchers.js.snap
@@ -1,3 +1,3 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`TarballFetcher.fetch throws on invalid hash 1`] = `"https://github.com/sindresorhus/beeper/archive/master.tar.gz: Hashes don't match when extracting file \\"https://github.com/sindresorhus/beeper/archive/master.tar.gz\\". Expected \\"foo\\" but got \\"51f12d36860fc3d2ab747377991746e8ea3faabb\\""`;
+exports[`TarballFetcher.fetch throws on invalid hash 1`] = `"https://github.com/sindresorhus/beeper/archive/master.tar.gz: Fetch succeeded for undefined. However, extracting \\"https://github.com/sindresorhus/beeper/archive/master.tar.gz\\" resulted in hash \\"foo\\", which did not match the requested hash \\"51f12d36860fc3d2ab747377991746e8ea3faabb\\"."`;
diff --git a/__tests__/fetchers.js b/__tests__/fetchers.js
@@ -9,6 +9,7 @@ import GitFetcher from '../src/fetchers/git-fetcher.js';
 import Config from '../src/config.js';
 import mkdir from './_temp.js';
 import * as fs from '../src/util/fs.js';
+import {readdirSync} from 'fs';
 
 const path = require('path');
 
@@ -143,6 +144,11 @@ test('TarballFetcher.fetch', async () => {
 
 test('TarballFetcher.fetch throws on invalid hash', async () => {
   const dir = await mkdir('tarball-fetcher');
+  const offlineMirrorDir = await mkdir('offline-mirror');
+
+  const config = await Config.create({}, new Reporter());
+  config.registries.npm.config['yarn-offline-mirror'] = offlineMirrorDir;
+
   const url = 'https://github.com/sindresorhus/beeper/archive/master.tar.gz';
   const fetcher = new TarballFetcher(
     dir,
@@ -152,15 +158,17 @@ test('TarballFetcher.fetch throws on invalid hash', async () => {
       reference: url,
       registry: 'npm',
     },
-    await Config.create({}, new Reporter()),
+    config,
   );
   let error;
   try {
     await fetcher.fetch();
   } catch (e) {
     error = e;
   }
+
   expect(error && error.message).toMatchSnapshot();
+  expect(readdirSync(path.join(offlineMirrorDir))).toEqual([]);
 });
 
 test('TarballFetcher.fetch supports local ungzipped tarball', async () => {

diff --git a/src/fetchers/git-fetcher.js b/src/fetchers/git-fetcher.js
@@ -101,7 +101,17 @@ export default class GitFetcher extends BaseFetcher {
               hash: expectHash,
             });
           } else {
-            reject(new SecurityError(this.reporter.lang('fetchBadHashWithPath', tarballPath, expectHash, actualHash)));
+            reject(
+              new SecurityError(
+                this.config.reporter.lang(
+                  'fetchBadHashWithPath',
+                  this.packageName,
+                  this.remote.reference,
+                  expectHash,
+                  actualHash,
+                ),
+              ),
+            );
           }
         })
         .on('error', function(err) {

diff --git a/src/fetchers/tarball-fetcher.js b/src/fetchers/tarball-fetcher.js
@@ -81,14 +81,21 @@ export default class TarballFetcher extends BaseFetcher {
       .on('finish', () => {
         const expectHash = this.hash;
         const actualHash = validateStream.getHash();
+
         if (!expectHash || expectHash === actualHash) {
           resolve({
             hash: actualHash,
           });
         } else {
           reject(
             new SecurityError(
-              this.config.reporter.lang('fetchBadHashWithPath', this.remote.reference, expectHash, actualHash),
+              this.config.reporter.lang(
+                'fetchBadHashWithPath',
+                this.packageName,
+                this.remote.reference,
+                expectHash,
+                actualHash,
+              ),
             ),
           );
         }
@@ -178,6 +185,17 @@ export default class TarballFetcher extends BaseFetcher {
           this.reporter.warn(this.reporter.lang('retryOnInternalServerError'));
           await sleep(3000);
         } else {
+          const tarballMirrorPath = this.getTarballMirrorPath();
+          const tarballCachePath = this.getTarballCachePath();
+
+          if (tarballMirrorPath && (await fsUtil.exists(tarballMirrorPath))) {
+            await fsUtil.unlink(tarballMirrorPath);
+          }
+
+          if (tarballCachePath && (await fsUtil.exists(tarballCachePath))) {
+            await fsUtil.unlink(tarballCachePath);
+          }
+
           throw err;
         }
       }

diff --git a/src/reporters/lang/en.js b/src/reporters/lang/en.js
@@ -302,7 +302,8 @@ const messages = {
   requestError: 'Request $0 returned a $1',
   requestFailed: 'Request failed $0',
   tarballNotInNetworkOrCache: '$0: Tarball is not in network and can not be located in cache ($1)',
-  fetchBadHashWithPath: "Hashes don't match when extracting file $0. Expected $1 but got $2",
+  fetchBadHashWithPath:
+    'Fetch succeeded for $0. However, extracting $1 resulted in hash $2, which did not match the requested hash $3.',
   fetchErrorCorrupt:
     '$0. Mirror tarball appears to be corrupt. You can resolve this by running:\n\n  rm -rf $1\n  yarn install',
   errorDecompressingTarball: '$0. Error decompressing $1, it appears to be corrupt.',