Merge pull request #813 from snyk/fix/fetch-patch-use-request

fix: fetch patch ignoring command line arguments

package.json

@@ -103,6 +103,7 @@
     "@types/lodash": "^4.14.136",
     "@types/needle": "^2.0.4",
     "@types/node": "^6.14.4",
+    "@types/sinon": "^7.5.0",
     "@typescript-eslint/eslint-plugin": "^2.0.0",
     "@typescript-eslint/parser": "^2.0.0",
     "eslint": "^5.16.0",

src/lib/protect/fetch-patch.ts

@@ -1,57 +1,39 @@
-import * as needle from 'needle';
 import * as fs from 'fs';
 import * as analytics from '../analytics';
 import * as debugModule from 'debug';
-import * as proxyFromEnv from 'proxy-from-env';
-import * as ProxyAgent from 'proxy-agent';
+import request = require('../request');
 
 const debug = debugModule('snyk:fetch-patch');
-const getProxyForUrl = proxyFromEnv.getProxyForUrl;
 
 async function getPatchFile(
   patchUrl: string,
   patchFilename: string,
 ): Promise<string> {
-  let options;
-  const proxyUri = getProxyForUrl(patchUrl);
-  if (proxyUri) {
-    debug('Using proxy: ', proxyUri);
-    options = { agent: new ProxyAgent(proxyUri) };
-  }
-
-  let res: needle.NeedleResponse;
-  let patchData: string;
   try {
-    res = await needle('get', patchUrl, options || {});
-    if (!res || res.statusCode !== 200) {
-      throw res;
+    const response = await request({ url: patchUrl });
+    if (
+      !response ||
+      !response.res ||
+      !response.body ||
+      response.res.statusCode !== 200
+    ) {
+      throw response;
     }
-    patchData = res.body;
+    fs.writeFileSync(patchFilename, response.body);
     debug(
-      `Successfully downloaded patch from ${patchUrl}, patch size ${patchData.length} bytes`,
+      `Fetched patch from ${patchUrl} to ${patchFilename}, patch size ${response.body.length} bytes`,
     );
   } catch (error) {
-    debug(`Failed to download patch from ${patchUrl}`, error);
+    const errorMessage = `Failed to fetch patch from ${patchUrl} to ${patchFilename}`;
+    debug(errorMessage, error);
     analytics.add('patch-fetch-fail', {
-      message: error && (error.message || error.body),
-      code: error && error.statusCode,
-    });
-    throw error;
-  }
-
-  try {
-    fs.writeFileSync(patchFilename, patchData);
-    debug(`Successfully wrote patch to ${patchFilename}`);
-  } catch (error) {
-    debug(`Failed to write patch to ${patchFilename}`, error);
-    analytics.add('patch-fetch-fail', {
-      message: error && error.message,
+      message: (error && error.message) || errorMessage,
+      code: error && error.res && error.res.statusCode,
       patchFilename,
       patchUrl,
     });
-    throw error;
+    throw new Error(errorMessage);
   }
-
   return patchFilename;
 }
 

test/patch-fetch-fail.test.ts

@@ -0,0 +1,131 @@
+import { test } from 'tap';
+import * as proxyquire from 'proxyquire';
+
+let analytics;
+
+const proxyFetchPatch = proxyquire('../src/lib/protect/fetch-patch', {
+  '../request': () => {
+    return Promise.resolve({
+      res: {
+        statusCode: 200,
+      },
+      body: 'patch content',
+    });
+  },
+  fs: {
+    writeFileSync() {
+      return;
+    },
+  },
+  '../analytics': {
+    add(type, data) {
+      analytics = { type, data };
+    },
+  },
+});
+
+const proxyFetchPatchNotFound = proxyquire('../src/lib/protect/fetch-patch', {
+  '../request': () => {
+    return Promise.resolve({
+      res: {
+        statusCode: 404,
+      },
+      body: 'not found',
+    });
+  },
+  '../analytics': {
+    add(type, data) {
+      analytics = { type, data };
+    },
+  },
+});
+
+const proxyFetchPatchErrorWriting = proxyquire(
+  '../src/lib/protect/fetch-patch',
+  {
+    '../request': () => {
+      return Promise.resolve({
+        res: {
+          statusCode: 200,
+        },
+        body: 'patch content',
+      });
+    },
+    fs: {
+      writeFileSync() {
+        throw new Error('Error writing file');
+      },
+    },
+    '../analytics': {
+      add(type, data) {
+        analytics = { type, data };
+      },
+    },
+  },
+);
+
+test('fetch patch returns filename when successful', (t) => {
+  return proxyFetchPatch('https://test.patch.url', 'file.patch')
+    .then((name) => t.is(name, 'file.patch', 'filename returned'))
+    .catch((e) => t.fail('should not throw error', e));
+});
+
+test('fetch patch handles error responses', (t) => {
+  return proxyFetchPatchNotFound('https://test.patch.url', 'file.patch')
+    .then(() => t.fail('should have failed'))
+    .catch(() => {
+      t.is(
+        analytics.type,
+        'patch-fetch-fail',
+        'analytics type is patch-fetch-fail',
+      );
+      t.is(analytics.data.code, 404, 'analytics status code is 404');
+      t.is(
+        analytics.data.message,
+        'Failed to fetch patch from https://test.patch.url to file.patch',
+        'analytics message is expected',
+      );
+      t.is(
+        analytics.data.patchFilename,
+        'file.patch',
+        'analytics patch filename is expected',
+      );
+      t.is(
+        analytics.data.patchUrl,
+        'https://test.patch.url',
+        'analytics patch url is expected',
+      );
+    });
+});
+
+test('fetch patch handles errors thrown while writing file', (t) => {
+  return proxyFetchPatchErrorWriting('https://test.patch.url', 'file.patch')
+    .then(() => t.fail('should have failed'))
+    .catch(() => {
+      t.is(
+        analytics.type,
+        'patch-fetch-fail',
+        'analytics type is patch-fetch-fail',
+      );
+      t.is(
+        analytics.data.code,
+        undefined,
+        'analytics status code is undefined',
+      );
+      t.is(
+        analytics.data.message,
+        'Error writing file',
+        'analytics message indicates error writing file',
+      );
+      t.is(
+        analytics.data.patchFilename,
+        'file.patch',
+        'analytics patch filename is expected',
+      );
+      t.is(
+        analytics.data.patchUrl,
+        'https://test.patch.url',
+        'analytics patch url is expected',
+      );
+    });
+});