fix: Correctly quote NODE_OPTIONS when spaces occur in paths

This had a workaround but it turns out that it is in fact our bug.
Fixes #122

src/bin.ts

@@ -11,6 +11,7 @@ import YAML from "yaml";
 import config from "./config";
 import { info } from "./message";
 import { version } from "./metadata";
+import NodeOptions from "./util/NodeOptions";
 import forwardSignals from "./util/forwardSignals";
 import { readPkgUp } from "./util/readPkgUp";
 
@@ -22,11 +23,12 @@ const loaderPath = pathToFileURL(resolve(__dirname, "../dist/loader.js")).href;
 
 export function main() {
   const [cmd, ...args] = process.argv.slice(2);
+  const nodeOptions = new NodeOptions(process.env.NODE_OPTIONS);
 
   if (!cmd) return usage();
 
   info("Running with appmap-node version %s", version);
-  addNodeOptions("--require", registerPath);
+  nodeOptions.push("--require", registerPath);
   if (config.default) {
     info("Writing default config to %s", config.configPath);
     writeFileSync(config.configPath, YAML.stringify(config));
@@ -35,9 +37,11 @@ export function main() {
 
   // FIXME: Probably there should be a way to remove this altogether
   // by changing our custom loader implementation
-  if (isTsEsmLoaderNeeded(cmd, args)) addNodeOptions("--loader", "ts-node/esm");
+  if (isTsEsmLoaderNeeded(cmd, args)) nodeOptions.push("--loader", "ts-node/esm");
 
-  addNodeOptions("--loader", loaderPath, "--no-warnings");
+  nodeOptions.push("--loader", loaderPath, "--no-warnings");
+
+  process.env.NODE_OPTIONS = nodeOptions.toString();
 
   let child: ChildProcess | undefined;
   if (isScript(cmd)) {
@@ -100,12 +104,6 @@ function isTsEsmLoaderNeeded(cmd: string, args: string[]) {
   return [cmd, ...args].includes("ts-node") || tsConfigTsNodeEsmIsTrue();
 }
 
-function addNodeOptions(...options: string[]) {
-  const envar = (process.env.NODE_OPTIONS ?? "").split(" ");
-  envar.push(...options);
-  process.env.NODE_OPTIONS = envar.join(" ");
-}
-
 /* Heuristic to check if argument is a node script. Currently just checks if it's an existing file
 with a known extension; the assumption being if it's not, then it's probably a command. */
 function isScript(arg: string) {

src/util/NodeOptions.ts

@@ -0,0 +1,49 @@
+export default class NodeOptions extends Array {
+  constructor(value?: string) {
+    super();
+    if (!value) return;
+    if (typeof value !== "string") throw new Error("NODE_OPTIONS must be a string");
+
+    this.push(...parseNodeOptionsEnvVar(value));
+  }
+
+  toString(): string {
+    return this.map(maybeQuote).join(" ");
+  }
+
+  static get [Symbol.species]() {
+    return Array;
+  }
+}
+
+// based on ParseNodeOptionsEnvVar()
+// https://github.com/nodejs/node/blob/39f1b899cd536de4d4c9bbf56f24927d8d06999a/src/node_options.cc#L1397
+function parseNodeOptionsEnvVar(nodeOptions: string): string[] {
+  const result: string[] = [];
+  let buffer = "";
+  let inString = false;
+  for (let i = 0; i < nodeOptions.length; i++) {
+    let c = nodeOptions[i];
+    if (c === "\\" && inString) {
+      if (i + 1 === nodeOptions.length) break;
+      c = nodeOptions[++i];
+    } else if (c === " " && !inString) {
+      result.push(buffer);
+      buffer = "";
+      continue;
+    } else if (c === '"') {
+      inString = !inString;
+      continue;
+    }
+
+    buffer += c;
+  }
+
+  result.push(buffer);
+  return result;
+}
+
+function maybeQuote(value: string): string {
+  if (value.includes(" ")) return `"${value.replaceAll("\\", "\\\\").replaceAll('"', '\\"')}"`;
+  return value;
+}

src/util/__tests__/NodeOptions.test.ts

@@ -0,0 +1,15 @@
+import NodeOptions from "../NodeOptions";
+
+describe(NodeOptions, () => {
+  it("splits the options correctly", () => {
+    const options = new NodeOptions('--option "spaced argument"');
+    expect(Array.from(options)).toEqual(["--option", "spaced argument"]);
+  });
+
+  it("stringifies spaced options properly", () => {
+    const opts = new NodeOptions("");
+    opts.push("--option", 'spaced \\argu"me\\nt');
+    expect(opts.toString()).toBe('--option "spaced \\\\argu\\"me\\\\nt"');
+    expect(Array.from(new NodeOptions(opts.toString()))).toEqual(Array.from(opts));
+  });
+});

test/smoketest.mjs

@@ -16,17 +16,17 @@ import tmp from "tmp";
  * it doesn't crash and produces an AppMap. */
 
 // Create a temporary directory to work in.
-const tmpDir = tmp.dirSync({ unsafeCleanup: true }).name;
+// To test NODE_OPTIONS handling, use a space in the directory name and use mjs.
+const tmpDir = tmp.dirSync({ unsafeCleanup: true, template: "appmap node test XXXXXX" }).name;
 
-// Package up the appmap-node package.
-runCommand("yarn", "pack", "-o", join(tmpDir, "appmap-node.tgz"));
+runCommand("yarn", "pack", "-o", JSON.stringify(join(tmpDir, "appmap-node.tgz")));
 
 chdir(tmpDir);
 runCommand("npm", "init", "-y");
-runCommand("npm", "install", join(tmpDir, "appmap-node.tgz"));
+runCommand("npm", "install", "appmap-node.tgz");
 
 writeFileSync(
-  "index.js",
+  "index.mjs",
   `
 function main() {
   console.log("Hello world!");
@@ -35,7 +35,7 @@ main();
 `,
 );
 
-runCommand("npm", "exec", "appmap-node", "index.js");
+runCommand("npm", "exec", "appmap-node", "index.mjs");
 
 // verify that appmap has been created
 const files = glob.globSync("tmp/**/*.appmap.json");
@@ -44,7 +44,7 @@ assert(files.length === 1);
 function runCommand(command, ...args) {
   const { status } = spawnSync(command, args, {
     stdio: "inherit",
-    shell: process.platform == "win32",
+    shell: true,
   });
   assert(status === 0);
 }