Fix/safe hook execution

src/util/fs/executeScript.test.ts

@@ -1,40 +1,106 @@
-// @ts-nocheck
-// This file is escaping ts checks for now, because the child_process.exec
-// mock fn is selecting a specific exec overload that is incorrect.
-// @TODO: dig into this and figure out how to get typescript to use
-// the correct overload.
-import childproc from 'child_process';
+import { execFile } from 'child_process';
 import executeScript from './executeScript.js';
 
-const execMock = jest.spyOn(childproc, 'exec');
+jest.mock('child_process', () => ({
+  execFile: jest.fn(),
+}));
+
+const execFileMock = execFile as unknown as jest.Mock;
+
+type ExecFileCallback = (
+  error: Error | null,
+  stdout: string,
+  stderr: string,
+) => void;
+
+function mockExecFileResult(
+  error: Error | null,
+  stdout = '',
+  stderr = '',
+): void {
+  execFileMock.mockImplementationOnce(
+    (
+      _command: string,
+      _args: string[],
+      _options: { cwd: string; encoding: string },
+      callback: ExecFileCallback,
+    ) => {
+      callback(error, stdout, stderr);
+    },
+  );
+}
 
 describe('executeScript', () => {
-  it('can execute a script, and resolve the stdout', async () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('executes a hook script with node and resolves stdout', async () => {
     expect.assertions(2);
-    execMock.mockImplementationOnce((_, callback: () => void) =>
-      callback(null, 'done'),
+    const scriptPath = '/project/.cli/init.js';
+    mockExecFileResult(null, 'done');
+
+    await expect(executeScript(scriptPath)).resolves.toBe('done');
+    expect(execFileMock).toHaveBeenCalledWith(
+      process.execPath,
+      [scriptPath],
+      {
+        cwd: '/project/.cli',
+        encoding: 'utf8',
+      },
+      expect.any(Function),
     );
-    await expect(executeScript('path.js')).resolves.toBe('done');
-    expect(execMock).toHaveBeenCalledWith('path.js', expect.any(Function));
   });
 
-  it('can execute a script, and resolve the stderr', async () => {
+  it('passes a hook path with spaces as an argument instead of a shell string', async () => {
     expect.assertions(1);
-    execMock.mockImplementationOnce((_, callback: () => void) =>
-      callback(null, null, 'well, that went poorly'),
+    const scriptPath = '/project with spaces/.cli/init.js';
+    mockExecFileResult(null, 'done');
+
+    await executeScript(scriptPath);
+
+    expect(execFileMock).toHaveBeenCalledWith(
+      process.execPath,
+      [scriptPath],
+      {
+        cwd: '/project with spaces/.cli',
+        encoding: 'utf8',
+      },
+      expect.any(Function),
     );
-    await expect(executeScript('path.js')).resolves.toBe(
+  });
+
+  it('resolves stderr when stdout is empty', async () => {
+    expect.assertions(1);
+    mockExecFileResult(null, '', 'well, that went poorly');
+
+    await expect(executeScript('/project/.cli/init.js')).resolves.toBe(
       'well, that went poorly',
     );
   });
 
-  it('can execute a script, and reject with an error', async () => {
+  it('resolves an empty string when stdout and stderr are empty', async () => {
+    expect.assertions(1);
+    mockExecFileResult(null);
+
+    await expect(executeScript('/project/.cli/init.js')).resolves.toBe('');
+  });
+
+  it('rejects failed hooks with stderr context', async () => {
     expect.assertions(1);
-    execMock.mockImplementationOnce((_, callback: () => void) =>
-      callback(new Error('well, that went SUPER poorly')),
+    mockExecFileResult(new Error('Command failed'), '', 'hook failed\n');
+
+    await expect(executeScript('/project/.cli/init.js')).rejects.toThrow(
+      'Unable to execute hook script "/project/.cli/init.js": hook failed',
     );
-    await expect(executeScript('path.js')).rejects.toEqual(
-      Error('well, that went SUPER poorly'),
+  });
+
+  it('rejects execution failures with the process error message', async () => {
+    expect.assertions(1);
+    mockExecFileResult(new Error('spawn failed'));
+
+    await expect(executeScript('/project/.cli/init.js')).rejects.toThrow(
+      'Unable to execute hook script "/project/.cli/init.js": spawn failed',
     );
   });
 });

src/util/fs/executeScript.ts

@@ -1,4 +1,6 @@
-import { exec } from 'child_process';
+import { execFile } from 'child_process';
+import { dirname } from 'path';
+
 /**
  * Takes a path to a script, and executes it.
  *
@@ -8,12 +10,28 @@ export default async function executeScript(
   scriptPath: string,
 ): Promise<string> {
   return new Promise((resolve, reject) => {
-    exec(scriptPath, (error, stdout, stderr) => {
-      if (error) {
-        return reject(error);
-      }
+    execFile(
+      process.execPath,
+      [scriptPath],
+      {
+        // Run from the hook directory so hook-relative file operations do not
+        // depend on the shell location that invoked the CLI.
+        cwd: dirname(scriptPath),
+        encoding: 'utf8',
+      },
+      (error, stdout, stderr) => {
+        if (error) {
+          const output = stderr.trim() || error.message;
+
+          return reject(
+            new Error(
+              `Unable to execute hook script "${scriptPath}": ${output}`,
+            ),
+          );
+        }
 
-      resolve(stdout ? stdout : stderr);
-    });
+        resolve(stdout || stderr || '');
+      },
+    );
   });
 }