fix(chat): bot-posted file links open the chat UI in a browser instead of downloading

electron/main.mjs

@@ -21,6 +21,7 @@ import { buildDiagnosticsReport, decodeLogTail, diagnosticsFileName } from "./di
 import { migrateWorkspaceCredentials, workspaceCredentialEnv } from "./workspace-credentials.mjs";
 import { activateExistingWindow } from "./single-instance.mjs";
 import { packageUrlFromCommandLine, packageUrlFromDeepLink } from "./package-link.mjs";
+import { defaultSaveName, withSavableFile } from "./save-file.mjs";
 import {
   ensureManagedComposioCredentials,
   managedComposioAccess,
@@ -1151,6 +1152,34 @@ ipcMain.handle("desktop:export-diagnostics", async (event) => {
   return result.filePath;
 });
 
+// Bots hand users files as markdown links to paths inside the OpenMausBot
+// home (workspaces, attachments). As plain anchors those resolved against the
+// page origin, so the click opened http://127.0.0.1:8799<path> in the default
+// browser and the server's SPA fallback answered with index.html — a second
+// copy of the chat UI instead of the file. Ask where to put it and copy it
+// there instead: a save dialog tells the user the file landed somewhere and
+// where, which a silent copy into ~/Downloads does not. The path is
+// renderer-controlled, so it must resolve inside ~/.openmausbot and be a
+// regular file — never a symlink escape or directory.
+ipcMain.handle("desktop:save-file", async (event, rawPath) => {
+  return withSavableFile(rawPath, { home: os.homedir() }, async ({ defaultName, copyTo }) => {
+    const parent = BrowserWindow.fromWebContents(event.sender);
+    const defaultPath = await defaultSaveName(app.getPath("downloads"), defaultName);
+    const choice = await dialog.showSaveDialog(parent ?? undefined, {
+      title: "Where do you want to save it?",
+      message: "Where do you want to save it?",
+      defaultPath,
+      buttonLabel: "Save",
+      properties: ["createDirectory", "showOverwriteConfirmation"],
+    });
+    // Cancelling is a decision, not a failure — the bubble stays quiet.
+    if (choice.canceled || !choice.filePath) return null;
+    await copyTo(choice.filePath);
+    shell.showItemInFolder(choice.filePath);
+    return choice.filePath;
+  });
+});
+
 ipcMain.handle("desktop:open-external", async (_event, rawUrl) => {
   if (typeof rawUrl !== "string") throw new Error("A web address is required");
   let url;

electron/preload.cjs

@@ -142,6 +142,16 @@ contextBridge.exposeInMainWorld("ogb", {
   /** Writes the redacted diagnostics report to a user-chosen file; resolves
    * the path, or null when the save dialog was cancelled. */
   exportDiagnostics: () => ipcRenderer.invoke("desktop:export-diagnostics"),
+  /** Ask where to save a bot-created file (inside ~/.openmausbot), copy it
+   * there and reveal it. Returns the chosen path, or null if the user
+   * cancelled the dialog. The chat bubble shows the
+   * rejection text verbatim, so strip the "Error invoking remote method"
+   * wrapper ipcRenderer adds around a main-process throw. */
+  saveFile: (filePath) =>
+    ipcRenderer.invoke("desktop:save-file", filePath).catch((error) => {
+      const message = String(error?.message ?? error);
+      throw new Error(message.replace(/^Error invoking remote method '[^']*':\s*(?:Error:\s*)?/, ""));
+    }),
   /** Store a provider credential with OS-backed encryption. */
   setCredential: (name, value) => ipcRenderer.invoke("credential:set", name, value),
 

electron/save-file.mjs

@@ -0,0 +1,132 @@
+import fs from "node:fs";
+import path from "node:path";
+import { pipeline } from "node:stream/promises";
+import { fileURLToPath } from "node:url";
+
+function normalizeSourcePath(rawPath) {
+  if (typeof rawPath !== "string" || !rawPath.trim()) {
+    throw new Error("A file path is required");
+  }
+
+  if (/^file:\/\//i.test(rawPath)) {
+    try {
+      return fileURLToPath(rawPath);
+    } catch {
+      throw new Error("That file path is invalid");
+    }
+  }
+
+  if (!path.isAbsolute(rawPath)) throw new Error("That file path is invalid");
+  return rawPath;
+}
+
+async function canonicalPath(target, fsp, message) {
+  try {
+    return await fsp.realpath(target);
+  } catch {
+    throw new Error(message);
+  }
+}
+
+function assertInside(root, target) {
+  if (target !== root && !target.startsWith(root + path.sep)) {
+    throw new Error("Only files created by your bots can be saved");
+  }
+}
+
+function assertRegularFile(stats) {
+  if (!stats.isFile()) throw new Error("That path is not a file");
+}
+
+function isSameFile(left, right) {
+  return left.dev === right.dev && left.ino === right.ino;
+}
+
+// Paths come from model-rendered markdown, so they are untrusted. Resolve the
+// root and target before checking containment, then retain the target identity
+// for the open step below.
+async function resolveSource(rawPath, { home, fsp, platform }) {
+  const target = normalizeSourcePath(rawPath);
+  const root = await canonicalPath(
+    path.join(home, ".openmausbot"),
+    fsp,
+    "Only files created by your bots can be saved",
+  );
+  const filePath = await canonicalPath(target, fsp, "That file no longer exists");
+  assertInside(root, filePath);
+
+  const stats = await fsp.stat(filePath, { bigint: true });
+  assertRegularFile(stats);
+  if (platform === "win32") {
+    const pathAfterStat = await canonicalPath(filePath, fsp, "That file no longer exists");
+    assertInside(root, pathAfterStat);
+  }
+  return { filePath, stats };
+}
+
+// Kept as a narrow validation seam for callers and tests that only need the
+// canonical path. The save flow uses withSavableFile so it cannot forget to
+// close the stable source handle.
+export async function resolveSavablePath(rawPath, { home, fsp = fs.promises, platform = process.platform } = {}) {
+  return (await resolveSource(rawPath, { home, fsp, platform })).filePath;
+}
+
+async function openSavableFile(rawPath, { home, fsp, platform }) {
+  const source = await resolveSource(rawPath, { home, fsp, platform });
+  const noFollow = platform === "win32" ? 0 : fs.constants.O_NOFOLLOW ?? 0;
+  const handle = await fsp.open(source.filePath, fs.constants.O_RDONLY | noFollow);
+  try {
+    const openedStats = await handle.stat({ bigint: true });
+    assertRegularFile(openedStats);
+    if (platform === "win32" && !isSameFile(source.stats, openedStats)) {
+      throw new Error("That file changed while it was being opened");
+    }
+    return { handle, filePath: source.filePath };
+  } catch (error) {
+    await handle.close();
+    throw error;
+  }
+}
+
+// The callback owns the save operation while this module owns the source
+// handle. This keeps validation, stable copying, and cleanup at one seam.
+export async function withSavableFile(
+  rawPath,
+  { home, fsp = fs.promises, platform = process.platform } = {},
+  operation,
+) {
+  const { handle, filePath } = await openSavableFile(rawPath, { home, fsp, platform });
+  try {
+    return await operation({
+      filePath,
+      defaultName: path.basename(filePath),
+      copyTo: async (destination) => {
+        await pipeline(
+          handle.createReadStream({ autoClose: false, start: 0 }),
+          fs.createWriteStream(destination),
+        );
+      },
+    });
+  } finally {
+    await handle.close();
+  }
+}
+
+// The name the save dialog opens on: "report.docx", or "report (2).docx" when
+// that already exists, so accepting the default never quietly replaces an
+// earlier download. Only a suggestion — the user can type over it, and the
+// dialog's own overwrite confirmation covers the final choice. Bounded so a
+// directory full of collisions cannot spin forever.
+export async function defaultSaveName(dir, sourcePath, { fsp = fs.promises } = {}) {
+  const ext = path.extname(sourcePath);
+  const stem = path.basename(sourcePath, ext);
+  for (let n = 1; n < 1000; n += 1) {
+    const candidate = path.join(dir, n === 1 ? `${stem}${ext}` : `${stem} (${n})${ext}`);
+    try {
+      await fsp.access(candidate);
+    } catch {
+      return candidate;
+    }
+  }
+  return path.join(dir, `${stem}${ext}`);
+}

electron/save-file.node-test.mjs

@@ -0,0 +1,172 @@
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import { after, before, describe, it } from "node:test";
+import { pathToFileURL } from "node:url";
+
+import { defaultSaveName, resolveSavablePath, withSavableFile } from "./save-file.mjs";
+
+// Creating a symlink on Windows needs elevation or developer mode, so the
+// symlink cases only run where the runner can actually make one.
+const canSymlink = (() => {
+  const probe = fs.mkdtempSync(path.join(os.tmpdir(), "omb-symlink-probe-"));
+  try {
+    fs.symlinkSync(probe, path.join(probe, "link"));
+    return true;
+  } catch {
+    return false;
+  } finally {
+    fs.rmSync(probe, { recursive: true, force: true });
+  }
+})();
+
+let home;
+let botHome;
+
+before(() => {
+  home = fs.mkdtempSync(path.join(os.tmpdir(), "omb-save-file-"));
+  botHome = path.join(home, ".openmausbot");
+  fs.mkdirSync(path.join(botHome, "workspaces", "bot"), { recursive: true });
+  fs.writeFileSync(path.join(botHome, "workspaces", "bot", "report.docx"), "docx");
+  fs.writeFileSync(path.join(home, "secret.txt"), "private");
+});
+
+after(() => {
+  fs.rmSync(home, { recursive: true, force: true });
+});
+
+describe("save-file path validation", () => {
+  it("accepts a file inside the bot home, as a path or a file:// URL", async () => {
+    const file = path.join(botHome, "workspaces", "bot", "report.docx");
+    // must be fs.promises.realpath, the same call the module makes: on Windows
+    // the callback API leaves 8.3 short names ("RUNNER~1") that the promises
+    // API expands ("runneradmin"), so mixing the two compares different strings
+    const expected = await fs.promises.realpath(file);
+    assert.equal(await resolveSavablePath(file, { home }), expected);
+    assert.equal(await resolveSavablePath(pathToFileURL(file).href, { home }), expected);
+  });
+
+  it("accepts a file under a symlinked bot home", { skip: !canSymlink }, async () => {
+    const realHome = fs.mkdtempSync(path.join(os.tmpdir(), "omb-real-home-"));
+    const linkedHome = fs.mkdtempSync(path.join(os.tmpdir(), "omb-linked-home-"));
+    const realBotHome = path.join(realHome, "bot-data");
+    fs.mkdirSync(realBotHome, { recursive: true });
+    fs.writeFileSync(path.join(realBotHome, "report.docx"), "docx");
+    fs.symlinkSync(realBotHome, path.join(linkedHome, ".openmausbot"));
+
+    const viaLink = path.join(linkedHome, ".openmausbot", "report.docx");
+    assert.equal(await resolveSavablePath(viaLink, { home: linkedHome }), await fs.promises.realpath(viaLink));
+
+    fs.rmSync(realHome, { recursive: true, force: true });
+    fs.rmSync(linkedHome, { recursive: true, force: true });
+  });
+
+  it("rejects paths outside the bot home, including via traversal", async () => {
+    const rejected = "Only files created by your bots can be saved";
+    await assert.rejects(resolveSavablePath(path.join(home, "secret.txt"), { home }), { message: rejected });
+    await assert.rejects(resolveSavablePath(path.join(botHome, "..", "secret.txt"), { home }), { message: rejected });
+  });
+
+  it("rejects a symlink inside the bot home pointing outside it", { skip: !canSymlink }, async () => {
+    const escape = path.join(botHome, "escape.txt");
+    fs.symlinkSync(path.join(home, "secret.txt"), escape);
+    await assert.rejects(resolveSavablePath(escape, { home }), {
+      message: "Only files created by your bots can be saved",
+    });
+    fs.rmSync(escape);
+  });
+
+  it("rejects empty, relative, and non-file targets", async () => {
+    await assert.rejects(resolveSavablePath("", { home }), { message: "A file path is required" });
+    await assert.rejects(resolveSavablePath("workspaces/bot/report.docx", { home }), { message: "That file path is invalid" });
+    await assert.rejects(resolveSavablePath(path.join(botHome, "nope.docx"), { home }), { message: "That file no longer exists" });
+    await assert.rejects(resolveSavablePath(path.join(botHome, "workspaces"), { home }), { message: "That path is not a file" });
+  });
+});
+
+describe("save-file dialog default name", () => {
+  it("suggests a name that does not overwrite an existing file", async () => {
+    const downloads = fs.mkdtempSync(path.join(os.tmpdir(), "omb-downloads-"));
+    const source = path.join(botHome, "workspaces", "bot", "report.docx");
+
+    assert.equal(await defaultSaveName(downloads, source), path.join(downloads, "report.docx"));
+    fs.writeFileSync(path.join(downloads, "report.docx"), "");
+    assert.equal(await defaultSaveName(downloads, source), path.join(downloads, "report (2).docx"));
+    fs.writeFileSync(path.join(downloads, "report (2).docx"), "");
+    assert.equal(await defaultSaveName(downloads, source), path.join(downloads, "report (3).docx"));
+
+    fs.rmSync(downloads, { recursive: true, force: true });
+  });
+
+  it("keeps the extension on the suggestion", async () => {
+    const downloads = fs.mkdtempSync(path.join(os.tmpdir(), "omb-downloads-ext-"));
+    const source = path.join(botHome, "workspaces", "bot", "report.docx");
+    fs.writeFileSync(path.join(downloads, "report.docx"), "");
+
+    assert.equal(path.extname(await defaultSaveName(downloads, source)), ".docx");
+
+    fs.rmSync(downloads, { recursive: true, force: true });
+  });
+});
+
+describe("save-file source handles", () => {
+  it("copies from the validated open handle", async () => {
+    const source = path.join(botHome, "workspaces", "bot", "report.docx");
+    const destination = path.join(home, "copied-report.docx");
+    await withSavableFile(source, { home }, ({ copyTo }) => copyTo(destination));
+    assert.equal(fs.readFileSync(destination, "utf8"), "docx");
+    fs.rmSync(destination);
+  });
+
+  it("does not follow a symlink swap after the source is opened", { skip: !canSymlink || process.platform === "win32" }, async () => {
+    const source = path.join(botHome, "workspaces", "bot", "report.docx");
+    const moved = `${source}.moved`;
+    const destination = path.join(home, "swapped-report.docx");
+    await withSavableFile(source, { home }, async ({ copyTo }) => {
+      fs.renameSync(source, moved);
+      fs.symlinkSync(path.join(home, "secret.txt"), source);
+      await copyTo(destination);
+      assert.equal(fs.readFileSync(destination, "utf8"), "docx");
+    }).finally(() => {
+      if (fs.existsSync(source)) fs.rmSync(source);
+      if (fs.existsSync(moved)) fs.renameSync(moved, source);
+      if (fs.existsSync(destination)) fs.rmSync(destination);
+    });
+  });
+
+  it("rejects a validation-to-open identity swap on Windows", async () => {
+    const source = path.join(botHome, "workspaces", "bot", "report.docx");
+    // These IDs are distinct BigInts but collapse to the same Number. The
+    // options assertions below make the precision guarantee executable.
+    const expected = { dev: 1n, ino: 9007199254740992n, isFile: () => true };
+    const opened = { dev: 1n, ino: 9007199254740993n, isFile: () => true };
+    let closed = false;
+    let statOptions;
+    let handleStatOptions;
+    const fsp = {
+      realpath: async (target) => target,
+      stat: async (_target, options) => {
+        statOptions = options;
+        return expected;
+      },
+      open: async () => ({
+        stat: async (options) => {
+          handleStatOptions = options;
+          return opened;
+        },
+        close: async () => {
+          closed = true;
+        },
+      }),
+    };
+
+    await assert.rejects(
+      withSavableFile(source, { home, fsp, platform: "win32" }, async () => {}),
+      { message: "That file changed while it was being opened" },
+    );
+    assert.equal(closed, true);
+    assert.deepEqual(statOptions, { bigint: true });
+    assert.deepEqual(handleStatOptions, { bigint: true });
+  });
+});

package.json

@@ -33,10 +33,11 @@
     "dev:desktop": "electron .",
     "build": "tsc -b && tsc -p tsconfig.server.json && vite build",
     "typecheck": "tsc -b && tsc -p tsconfig.server.json",
-    "test": "node scripts/test-floor.mjs && pnpm broker:test && pnpm test:updater && pnpm test:desktop-viewer && pnpm test:package-link && pnpm test:packaged-server",
+    "test": "node scripts/test-floor.mjs && pnpm broker:test && pnpm test:updater && pnpm test:desktop-viewer && pnpm test:package-link && pnpm test:save-file && pnpm test:packaged-server",
     "test:updater": "node --test electron/updater-coordinator.node-test.mjs",
     "test:desktop-viewer": "node --test electron/desktop-viewer.node-test.mjs",
     "test:package-link": "node --test electron/package-link.node-test.mjs",
+    "test:save-file": "node --test electron/save-file.node-test.mjs",
     "bench:observation": "node --experimental-strip-types scripts/bench-observation.ts",
     "test:watch": "vitest",
     "test:cua": "pnpm build:cua && node scripts/smoke-cua.mjs",