fix(desktop): harden external editor and session cookie IPC (#1627)

* fix(desktop): stop the renderer choosing the external editor binary or reading the session cookie

open-external-editor now launches only the editor saved from a file dialog
and refuses to hand executable file types to the OS opener. The unused
get-session-cookie and wait-session-cookie channels, which returned the
httpOnly jwt, are removed.

* fix: validate setting keys before editor authorization

electron/external-editor.cjs

@@ -0,0 +1,70 @@
+const path = require("path");
+
+// Saved by the file manager after the user picks an editor in a file dialog.
+const EXTERNAL_EDITOR_SETTING = "fileManager.externalEditorPath";
+
+// Extensions the OS would run rather than open in an editor. With no editor
+// configured the temp copy goes to shell.openPath, and its name comes from
+// the remote server.
+const LAUNCHABLE_EXTENSIONS = new Set([
+  // Windows
+  "application",
+  "appref-ms",
+  "bat",
+  "chm",
+  "cmd",
+  "com",
+  "cpl",
+  "exe",
+  "gadget",
+  "hta",
+  "inf",
+  "jar",
+  "js",
+  "jse",
+  "library-ms",
+  "lnk",
+  "msc",
+  "msi",
+  "msp",
+  "mst",
+  "pif",
+  "ps1",
+  "psd1",
+  "psm1",
+  "py",
+  "pyw",
+  "reg",
+  "scf",
+  "scr",
+  "search-ms",
+  "settingcontent-ms",
+  "url",
+  "vbe",
+  "vbs",
+  "ws",
+  "wsf",
+  "wsh",
+  "xll",
+  // macOS
+  "app",
+  "command",
+  "pkg",
+  "terminal",
+  "tool",
+  "workflow",
+  // Linux
+  "appimage",
+  "desktop",
+  "run",
+  "sh",
+]);
+
+function isLaunchableFileName(fileName) {
+  // Windows drops trailing dots and spaces, so "a.exe. " is saved as a.exe.
+  const name = path.basename(String(fileName || "")).replace(/[. ]+$/, "");
+  const extension = path.extname(name).slice(1).toLowerCase();
+  return LAUNCHABLE_EXTENSIONS.has(extension);
+}
+
+module.exports = { EXTERNAL_EDITOR_SETTING, isLaunchableFileName };

electron/main.cjs

@@ -36,6 +36,10 @@ const { quitApp } = require("./app-quit.cjs");
 const { selectLinuxPasswordStore } = require("./linux-password-store.cjs");
 const { resolveLocalShell, repairSpawnHelpers } = require("./local-shell.cjs");
 const { registerLocalFileHandlers } = require("./local-files.cjs");
+const {
+  EXTERNAL_EDITOR_SETTING,
+  isLaunchableFileName,
+} = require("./external-editor.cjs");
 const { isExternalLoginUrl, isCallbackPort } = require("./external-login.cjs");
 
 const localTerminalSessions = new Map();
@@ -701,6 +705,9 @@ let tray = null;
 let isQuitting = false;
 const tempFiles = new Map();
 const externalEditorSessions = new Map();
+// Paths the user picked in an open dialog this session. Only these may be
+// saved as the external editor, so the renderer cannot name an executable.
+const dialogChosenPaths = new Set();
 
 const isDev = process.env.NODE_ENV === "development" || !app.isPackaged;
 const appRoot = isDev ? process.cwd() : path.join(__dirname, "..");
@@ -3163,18 +3170,16 @@ ipcMain.handle("get-c2s-tunnel-preset-default-name", () => {
   return `[${date}] ${computerName} (${platform} ${release})`;
 });
 
+function readSetting(key) {
+  const settingsPath = path.join(app.getPath("userData"), "settings.json");
+  if (!fs.existsSync(settingsPath)) return null;
+  const settings = JSON.parse(fs.readFileSync(settingsPath, "utf8"));
+  return settings[key] !== undefined ? settings[key] : null;
+}
+
 ipcMain.handle("get-setting", (event, key) => {
   try {
-    const userDataPath = app.getPath("userData");
+    return readSetting(key);
-    const settingsPath = path.join(userDataPath, "settings.json");
-
-    if (!fs.existsSync(settingsPath)) {
-      return null;
-    }
-
-    const settingsData = fs.readFileSync(settingsPath, "utf8");
-    const settings = JSON.parse(settingsData);
-    return settings[key] !== undefined ? settings[key] : null;
   } catch (error) {
     console.error("Error reading setting:", error);
     return null;
@@ -3182,6 +3187,16 @@ ipcMain.handle("get-setting", (event, key) => {
 });
 
 ipcMain.handle("set-setting", (event, key, value) => {
+  if (typeof key !== "string") {
+    return { success: false, error: "Invalid setting key" };
+  }
+  if (
+    key === EXTERNAL_EDITOR_SETTING &&
+    value &&
+    !dialogChosenPaths.has(value)
+  ) {
+    return { success: false, error: "Choose the editor in the file dialog" };
+  }
   try {
     const userDataPath = app.getPath("userData");
     const settingsPath = path.join(userDataPath, "settings.json");
@@ -3205,94 +3220,6 @@ ipcMain.handle("set-setting", (event, key, value) => {
   }
 });
 
-ipcMain.handle("get-session-cookie", async (_event, name, targetUrl) => {
-  try {
-    const ses = mainWindow?.webContents?.session;
-    if (!ses) return null;
-    const cookies = await ses.cookies.get({
-      name,
-      ...(targetUrl ? { url: targetUrl } : {}),
-    });
-    const cookie = cookies.find((candidate) =>
-      cookieMatchesUrl(candidate, targetUrl),
-    );
-    return (
-      cookie?.value ||
-      getRememberedElectronAuthCookie(name, targetUrl)?.value ||
-      null
-    );
-  } catch (error) {
-    console.error("Failed to get session cookie:", error);
-    return getRememberedElectronAuthCookie(name, targetUrl)?.value || null;
-  }
-});
-
-function cookieMatchesUrl(cookie, targetUrl) {
-  if (!targetUrl) return true;
-
-  try {
-    const targetHost = new URL(targetUrl).hostname;
-    const cookieDomain = (cookie.domain || "").replace(/^\./, "");
-
-    return (
-      cookieDomain === targetHost ||
-      targetHost.endsWith(`.${cookieDomain}`) ||
-      (!cookieDomain && targetHost === "localhost")
-    );
-  } catch {
-    return true;
-  }
-}
-
-ipcMain.handle(
-  "wait-session-cookie",
-  async (_event, name, targetUrl, previousValue, timeoutMs = 5000) => {
-    const ses = mainWindow?.webContents?.session;
-    if (!ses) return { success: false, error: "No Electron session" };
-
-    const existingCookies = await ses.cookies.get({
-      name,
-      ...(targetUrl ? { url: targetUrl } : {}),
-    });
-    const existingCookie = existingCookies.find((cookie) =>
-      cookieMatchesUrl(cookie, targetUrl),
-    );
-    if (existingCookie?.value && existingCookie.value !== previousValue) {
-      return { success: true, value: existingCookie.value };
-    }
-
-    const rememberedCookie = getRememberedElectronAuthCookie(name, targetUrl);
-    if (rememberedCookie?.value && rememberedCookie.value !== previousValue) {
-      return { success: true, value: rememberedCookie.value };
-    }
-
-    return new Promise((resolve) => {
-      const timeout = setTimeout(() => {
-        ses.cookies.off("changed", onCookieChanged);
-        resolve({ success: false, error: "Timed out waiting for cookie" });
-      }, timeoutMs);
-
-      function onCookieChanged(_event, cookie, _cause, removed) {
-        if (
-          removed ||
-          cookie.name !== name ||
-          !cookie.value ||
-          cookie.value === previousValue ||
-          !cookieMatchesUrl(cookie, targetUrl)
-        ) {
-          return;
-        }
-
-        clearTimeout(timeout);
-        ses.cookies.off("changed", onCookieChanged);
-        resolve({ success: true, value: cookie.value });
-      }
-
-      ses.cookies.on("changed", onCookieChanged);
-    });
-  },
-);
-
 ipcMain.handle("clear-session-cookies", async () => {
   try {
     clearPersistedElectronAuthCookies();
@@ -3420,7 +3347,9 @@ ipcMain.handle("show-save-dialog", async (_event, options) => {
 });
 
 ipcMain.handle("show-open-dialog", async (_event, options) => {
-  return dialog.showOpenDialog(mainWindow, options || {});
+  const result = await dialog.showOpenDialog(mainWindow, options || {});
+  for (const filePath of result.filePaths) dialogChosenPaths.add(filePath);
+  return result;
 });
 
 function toLocalFileType(stat) {
@@ -3743,6 +3672,15 @@ ipcMain.handle("cleanup-temp-file", (_event, tempId) => {
 
 ipcMain.handle("open-external-editor", async (_event, fileData) => {
   try {
+    // The editor comes from the saved setting, never from the caller.
+    const editorPath = readSetting(EXTERNAL_EDITOR_SETTING) || null;
+    if (!editorPath && isLaunchableFileName(fileData?.fileName)) {
+      return {
+        success: false,
+        error: "Choose an external editor to open this file type",
+      };
+    }
+
     const result = createManagedTempFile(
       fileData?.fileName,
       fileData?.content,
@@ -3764,10 +3702,6 @@ ipcMain.handle("open-external-editor", async (_event, fileData) => {
       lastMtimeMs: stat.mtimeMs,
     });
 
-    const editorPath =
-      typeof fileData?.editorPath === "string" && fileData.editorPath.trim()
-        ? fileData.editorPath.trim()
-        : null;
     const openError = await openPathWithEditor(result.path, editorPath);
     if (openError) {
       closeExternalEditorSession(editId);

electron/preload.js

@@ -87,16 +87,6 @@ contextBridge.exposeInMainWorld("electronAPI", {
   },
 
   clearSessionCookies: () => ipcRenderer.invoke("clear-session-cookies"),
-  getSessionCookie: (name, targetUrl) =>
-    ipcRenderer.invoke("get-session-cookie", name, targetUrl),
-  waitForSessionCookie: (name, targetUrl, previousValue, timeoutMs) =>
-    ipcRenderer.invoke(
-      "wait-session-cookie",
-      name,
-      targetUrl,
-      previousValue,
-      timeoutMs,
-    ),
 
   externalBrowserLogin: (authUrl, callbackPort) =>
     ipcRenderer.invoke("external-browser-login", authUrl, callbackPort),

scripts/electron-external-editor.test.ts

@@ -0,0 +1,40 @@
+import { createRequire } from "node:module";
+import { describe, expect, it } from "vitest";
+
+const require = createRequire(import.meta.url);
+const { isLaunchableFileName } = require("../electron/external-editor.cjs") as {
+  isLaunchableFileName: (fileName: unknown) => boolean;
+};
+
+describe("Electron external editor", () => {
+  it("refuses remote file names the OS would run instead of open", () => {
+    for (const name of [
+      "payload.exe",
+      "run.BAT",
+      "script.ps1",
+      "a.exe. ",
+      "a.cmd...",
+      "../../evil.vbs",
+      "start.command",
+      "launch.desktop",
+      "install.sh",
+    ]) {
+      expect(isLaunchableFileName(name)).toBe(true);
+    }
+  });
+
+  it("opens ordinary text files", () => {
+    for (const name of [
+      "nginx.conf",
+      "README.md",
+      "Dockerfile",
+      ".bashrc",
+      "config.json",
+      "index.ts",
+      "",
+      undefined,
+    ]) {
+      expect(isLaunchableFileName(name)).toBe(false);
+    }
+  });
+});

scripts/electron-security-boundary.test.ts

@@ -32,9 +32,62 @@ describe("Electron security boundary", () => {
     }
   });
 
+  it("never hands the session cookie to the renderer over IPC", () => {
+    for (const channel of ["get-session-cookie", "wait-session-cookie"]) {
+      expect(main).not.toContain(`"${channel}"`);
+      expect(preload).not.toContain(`"${channel}"`);
+    }
+  });
+
+  it("launches only the editor saved from a file dialog", () => {
+    expect(main).not.toContain("fileData?.editorPath");
+    expect(main).toContain("readSetting(EXTERNAL_EDITOR_SETTING)");
+    expect(main).toContain("dialogChosenPaths.has(value)");
+  });
+
   it("scopes proxy headers and basic auth to the linked origin", () => {
     expect(main).toContain('"sync-proxy-config"');
     expect(main).toContain("linkedServer.applyLinkHeaders(");
     expect(main).toContain("linkedServer.answerLogin(");
   });
 });
+
+it("rejects setting keys that coerce to the protected editor key", async () => {
+  const { runInNewContext } = await import("node:vm");
+  const { vi } = await import("vitest");
+  const writeFileSync = vi.fn();
+  let handler: (event: unknown, key: unknown, value: unknown) => unknown;
+  runInNewContext(
+    main.slice(
+      main.indexOf('ipcMain.handle("set-setting"'),
+      main.indexOf('ipcMain.handle("clear-session-cookies"'),
+    ),
+    {
+      ipcMain: {
+        handle: (_: string, callback: typeof handler) => {
+          handler = callback;
+        },
+      },
+      EXTERNAL_EDITOR_SETTING: "fileManager.externalEditorPath",
+      dialogChosenPaths: new Set(["/chosen/editor"]),
+      app: { getPath: () => "/settings" },
+      path: { join: (...parts: string[]) => parts.join("/") },
+      fs: { existsSync: () => false, mkdirSync: vi.fn(), writeFileSync },
+      console,
+    },
+  );
+  for (const key of [["fileManager.externalEditorPath"], null, {}, 42]) {
+    expect(handler!({}, key, "/unapproved/editor")).toEqual({
+      success: false,
+      error: "Invalid setting key",
+    });
+  }
+  expect(writeFileSync).not.toHaveBeenCalled();
+  expect(
+    handler!({}, "fileManager.externalEditorPath", "/unapproved/editor"),
+  ).toEqual({ success: false, error: "Choose the editor in the file dialog" });
+  expect(
+    handler!({}, "fileManager.externalEditorPath", "/chosen/editor"),
+  ).toEqual({ success: true });
+  expect(writeFileSync).toHaveBeenCalledOnce();
+});