fix: use sync fs method to avoid race condition

snyk · Feb 1, 2021 · 2b211b9 · 2b211b9
1 parent 3f4a57e
commit 2b211b9
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 44 deletions.
diff --git a/src/cli/index.ts b/src/cli/index.ts
@@ -76,9 +76,9 @@ async function runCommand(args: Args) {
   // also save the json (in error.json) to file if option is set
   if (args.command === 'test') {
     const jsonResults = (commandResult as TestCommandResult).getJsonResult();
-    saveResultsToFile(args.options, 'json', jsonResults);
+    await saveResultsToFile(args.options, 'json', jsonResults);
     const sarifResults = (commandResult as TestCommandResult).getSarifResult();
-    saveResultsToFile(args.options, 'sarif', sarifResults);
+    await saveResultsToFile(args.options, 'sarif', sarifResults);
   }
 
   return res;
@@ -128,8 +128,8 @@ async function handleError(args, error) {
     }
   }
 
-  saveResultsToFile(args.options, 'json', error.jsonStringifiedResults);
-  saveResultsToFile(args.options, 'sarif', error.sarifStringifiedResults);
+  await saveResultsToFile(args.options, 'json', error.jsonStringifiedResults);
+  await saveResultsToFile(args.options, 'sarif', error.sarifStringifiedResults);
 
   const analyticsError = vulnsFound
     ? {
@@ -174,7 +174,7 @@ function getFullPath(filepathFragment: string): string {
   }
 }
 
-function saveJsonResultsToFile(
+async function saveJsonResultsToFile(
   stringifiedJson: string,
   jsonOutputFile: string,
 ) {
@@ -192,7 +192,7 @@ function saveJsonResultsToFile(
   const dirPath = pathLib.dirname(jsonOutputFile);
   const createDirSuccess = createDirectory(dirPath);
   if (createDirSuccess) {
-    writeContentsToFileSwallowingErrors(jsonOutputFile, stringifiedJson);
+    await writeContentsToFileSwallowingErrors(jsonOutputFile, stringifiedJson);
   }
 }
 
@@ -437,7 +437,7 @@ function validateUnsupportedSarifCombinations(args) {
   }
 }
 
-function saveResultsToFile(
+async function saveResultsToFile(
   options: ArgsOptions,
   outputType: string,
   jsonResults: string,
@@ -447,7 +447,7 @@ function saveResultsToFile(
   if (outputFile && jsonResults) {
     const outputFileStr = outputFile as string;
     const fullOutputFilePath = getFullPath(outputFileStr);
-    saveJsonResultsToFile(stripAnsi(jsonResults), fullOutputFilePath);
+    await saveJsonResultsToFile(stripAnsi(jsonResults), fullOutputFilePath);
   }
 }
 

diff --git a/src/lib/json-file-output.ts b/src/lib/json-file-output.ts
@@ -38,18 +38,31 @@ export function createDirectory(newDirectoryFullPath: string): boolean {
   }
 }
 
-export function writeContentsToFileSwallowingErrors(
+/**
+ * Write the given contents to a file.
+ * If any errors are thrown in the process they are caught, logged, and discarded.
+ * @param jsonOutputFile the path of the file you want to write.
+ * @param contents the contents you want to write.
+ */
+export async function writeContentsToFileSwallowingErrors(
   jsonOutputFile: string,
   contents: string,
-) {
-  try {
-    const ws = createWriteStream(jsonOutputFile, { flags: 'w' });
-    ws.on('error', (err) => {
+): Promise<void> {
+  return new Promise((resolve) => {
+    try {
+      const ws = createWriteStream(jsonOutputFile, { flags: 'w' });
+      ws.on('error', (err) => {
+        console.error(err);
+        resolve();
+      });
+      ws.write(contents);
+      ws.end('\n');
+      ws.on('finish', () => {
+        resolve();
+      });
+    } catch (err) {
       console.error(err);
-    });
-    ws.write(contents);
-    ws.end('\n');
-  } catch (err) {
-    console.error(err);
-  }
+      return Promise.resolve();
+    }
+  });
 }
diff --git a/test/acceptance/cli-args.test.ts b/test/acceptance/cli-args.test.ts
@@ -357,21 +357,27 @@ test('`test --json-file-output can save JSON output to file while sending human
     `${tmpFolder}/snyk-direct-json-test-output.json`,
   );
 
-  exec(`node ${main} test --json-file-output=${jsonPath}`, (err, stdout) => {
-    if (err) {
-      console.log('CLI stdout: ', stdout);
-      throw err;
-    }
-    if (!existsSync(jsonPath)) {
-      console.log('CLI stdout: ', stdout);
-    }
-    const outputFileContents = readFileSync(jsonPath, 'utf-8');
-    const jsonObj = JSON.parse(outputFileContents);
-    const okValue = jsonObj.ok as boolean;
+  const testFixture =
+    'test/acceptance/workspaces/npm-package-no-vulns/package.json';
 
-    t.match(stdout, 'Organization:', 'contains human readable output');
-    t.ok(okValue, 'JSON output ok');
-  });
+  exec(
+    `node ${main} test --file=${testFixture} --json-file-output=${jsonPath}`,
+    (err, stdout) => {
+      if (err) {
+        console.log('CLI stdout: ', stdout);
+        throw err;
+      }
+      if (!existsSync(jsonPath)) {
+        console.log('CLI stdout: ', stdout);
+      }
+      const outputFileContents = readFileSync(jsonPath, 'utf-8');
+      const jsonObj = JSON.parse(outputFileContents);
+      const okValue = jsonObj.ok as boolean;
+
+      t.match(stdout, 'Organization:', 'contains human readable output');
+      t.ok(okValue, 'JSON output ok');
+    },
+  );
 });
 
 test('`test --json-file-output produces same JSON output as normal JSON output to stdout`', (t) => {
@@ -380,9 +386,10 @@ test('`test --json-file-output produces same JSON output as normal JSON output t
   const jsonPath = path.normalize(
     `${tmpFolder}/snyk-direct-json-test-output.json`,
   );
-
+  const testFixture =
+    'test/acceptance/workspaces/npm-package-no-vulns/package.json';
   exec(
-    `node ${main} test --json --json-file-output=${jsonPath}`,
+    `node ${main} test --file=${testFixture} --json --json-file-output=${jsonPath}`,
     (err, stdout) => {
       if (err) {
         console.log('CLI stdout: ', stdout);
@@ -405,9 +412,10 @@ test('`test --json-file-output can handle a relative path`', (t) => {
   const outputPath = path.normalize(
     `${tmpFolder}/snyk-direct-json-test-output.json`,
   );
-
+  const testFixture =
+    'test/acceptance/workspaces/npm-package-no-vulns/package.json';
   exec(
-    `node ${main} test --json --json-file-output=${outputPath}`,
+    `node ${main} test --file=${testFixture} --json --json-file-output=${outputPath}`,
     (err, stdout) => {
       if (err) {
         console.log('CLI stdout: ', stdout);
@@ -433,9 +441,10 @@ test(
     const outputPath = path.normalize(
       `${tmpFolder}/snyk-direct-json-test-output.json`,
     );
-
+    const testFixture =
+      'test/acceptance/workspaces/npm-package-no-vulns/package.json';
     exec(
-      `node ${main} test --json --json-file-output=${outputPath}`,
+      `node ${main} test --file=${testFixture} --json --json-file-output=${outputPath}`,
       (err, stdout) => {
         if (err) {
           console.log('CLI stdout: ', stdout);

diff --git a/test/json-file-output.test.ts b/test/json-file-output.test.ts
@@ -104,21 +104,25 @@ test('createDirectory creates directory - recursive', (t) => {
   }
 });
 
-test('writeContentsToFileSwallowingErrors can write a file', (t) => {
+test('writeContentsToFileSwallowingErrors can write a file', async (t) => {
   t.plan(1);
 
   // initially create the directory
   fs.mkdirSync(testOutputFull);
 
-  writeContentsToFileSwallowingErrors(testOutputFileFull, 'fake-contents');
+  // this should throw an error within writeContentsToFileSwallowingErrors but that error should be caught, logged, and disregarded
+  await writeContentsToFileSwallowingErrors(
+    testOutputFileFull,
+    'fake-contents',
+  );
   const fileExists = fs.existsSync(testOutputFileFull);
-  t.ok(fileExists, 'file exists after writing it');
+  t.ok(fileExists, 'and file exists after writing it');
 });
 
 test(
   'writeContentsToFileSwallowingErrors captures any errors when attempting to write to a readonly directory',
   { skip: iswindows },
-  (t) => {
+  async (t) => {
     t.plan(2);
 
     // initially create the directory
@@ -129,7 +133,7 @@ test(
 
     const outputPath = pathLib.join(readonlyFull, 'test-output.json');
 
-    writeContentsToFileSwallowingErrors(outputPath, 'fake-contents');
+    await writeContentsToFileSwallowingErrors(outputPath, 'fake-contents');
     const fileExists = fs.existsSync(outputPath);
     t.equals(fileExists, false);
     t.pass('no exception is thrown'); // we expect to not get an error even though we can't write to this folder