open files in external apps via argv not shell

Author: charlesvien

packages/workspace-server/src/services/external-apps/external-apps.test.ts

@@ -0,0 +1,83 @@
+import { describe, expect, it } from "vitest";
+import { type OpenCommand, resolveOpenCommand } from "./external-apps";
+
+// A path a malicious repo could ship: shell metacharacters that would run under
+// /bin/sh if this were ever concatenated into a command string.
+const MALICIOUS_PATH = "/repo/$(touch /tmp/pwned)`whoami`; rm -rf x.txt";
+
+const app = (id: string, path: string) => ({ id, path });
+
+describe("resolveOpenCommand", () => {
+  it.each<{
+    name: string;
+    platform: NodeJS.Platform;
+    app: { id: string; path: string };
+    isFile: boolean;
+    expected: OpenCommand;
+  }>([
+    {
+      name: "darwin finder reveal",
+      platform: "darwin",
+      app: app("finder", "/System/Library/CoreServices/Finder.app"),
+      isFile: true,
+      expected: { file: "open", args: ["-R", MALICIOUS_PATH] },
+    },
+    {
+      name: "darwin gitkraken",
+      platform: "darwin",
+      app: app("gitkraken", "/Applications/GitKraken.app"),
+      isFile: false,
+      expected: {
+        file: "open",
+        args: [
+          "-na",
+          "/Applications/GitKraken.app",
+          "--args",
+          "-p",
+          MALICIOUS_PATH,
+        ],
+      },
+    },
+    {
+      name: "darwin default editor",
+      platform: "darwin",
+      app: app("vscode", "/Applications/Code.app"),
+      isFile: false,
+      expected: {
+        file: "open",
+        args: ["-a", "/Applications/Code.app", MALICIOUS_PATH],
+      },
+    },
+    {
+      name: "win32 explorer select",
+      platform: "win32",
+      app: app("explorer", "explorer.exe"),
+      isFile: true,
+      expected: { file: "explorer.exe", args: [`/select,${MALICIOUS_PATH}`] },
+    },
+    {
+      name: "win32 default editor",
+      platform: "win32",
+      app: app("vscode", "C:/Code/Code.exe"),
+      isFile: false,
+      expected: { file: "C:/Code/Code.exe", args: [MALICIOUS_PATH] },
+    },
+  ])(
+    "passes the crafted path as a single argv element: $name",
+    ({ platform, app: a, isFile, expected }) => {
+      const result = resolveOpenCommand(platform, a, MALICIOUS_PATH, isFile);
+
+      expect(result).toEqual(expected);
+      // The tainted path is never split or concatenated into another argument.
+      expect(result?.args).toContain(
+        expected.args.find((arg) => arg.includes(MALICIOUS_PATH)),
+      );
+    },
+  );
+
+  it("returns null for an unsupported platform", () => {
+    expect(
+      resolveOpenCommand("linux", app("vscode", "/usr/bin/code"), "/x", false),
+    ).toBeNull();
+  });
+});

packages/workspace-server/src/services/external-apps/external-apps.ts

@@ -1,4 +1,4 @@
-import { exec } from "node:child_process";
+import { exec, execFile } from "node:child_process";
 import fs from "node:fs/promises";
 import path from "node:path";
 import { promisify } from "node:util";
@@ -14,6 +14,44 @@ import type { DetectedApplication } from "./schemas";
 import type { AppDefinition } from "./types";
 
 const execAsync = promisify(exec);
+const execFileAsync = promisify(execFile);
+
+export interface OpenCommand {
+  file: string;
+  args: string[];
+}
+
+// argv form, never a shell string: targetPath and app.path come from an opened
+// repository, so a crafted filename would run under /bin/sh if concatenated
+// into a command (VERIA-355). Returns null for unsupported platforms.
+export function resolveOpenCommand(
+  platform: NodeJS.Platform,
+  app: { id: string; path: string },
+  targetPath: string,
+  isFile: boolean,
+): OpenCommand | null {
+  if (platform === "darwin") {
+    if (app.id === "finder" && isFile) {
+      return { file: "open", args: ["-R", targetPath] };
+    }
+    if (app.id === "gitkraken") {
+      // GitKraken ignores positional args; it needs `--args -p <path>`.
+      return {
+        file: "open",
+        args: ["-na", app.path, "--args", "-p", targetPath],
+      };
+    }
+    return { file: "open", args: ["-a", app.path, targetPath] };
+  }
+  if (platform === "win32") {
+    if (app.id === "explorer" && isFile) {
+      // Explorer needs `/select,<path>` as a single token.
+      return { file: "explorer.exe", args: [`/select,${targetPath}`] };
+    }
+    return { file: app.path, args: [targetPath] };
+  }
+  return null;
+}
 
 const LOCALAPPDATA = process.env.LOCALAPPDATA ?? "";
 const PROGRAMFILES = process.env.PROGRAMFILES ?? "C:\\Program Files";
@@ -620,27 +658,17 @@ export class ExternalAppsService {
         isFile = false;
       }
 
-      let command: string;
-
-      if (process.platform === "darwin") {
-        if (appToOpen.id === "finder" && isFile) {
-          command = `open -R "${targetPath}"`;
-        } else if (appToOpen.id === "gitkraken") {
-          // GitKraken ignores positional args; it needs `--args -p <path>`.
-          command = `open -na "${appToOpen.path}" --args -p "${targetPath}"`;
-        } else {
-          command = `open -a "${appToOpen.path}" "${targetPath}"`;
-        }
-      } else if (process.platform === "win32") {
-        command =
-          appToOpen.id === "explorer" && isFile
-            ? `explorer.exe /select,"${targetPath}"`
-            : `"${appToOpen.path}" "${targetPath}"`;
-      } else {
+      const command = resolveOpenCommand(
+        process.platform,
+        appToOpen,
+        targetPath,
+        isFile,
+      );
+      if (!command) {
         return { success: false, error: "Unsupported platform" };
       }
 
-      await execAsync(command);
+      await execFileAsync(command.file, command.args);
       return { success: true };
     } catch (error) {
       return {