fix: guard tailscale sudo fallback (#1551) (thanks @sweepies)

CHANGELOG.md

@@ -11,6 +11,7 @@ Docs: https://docs.clawd.bot
 
 ### Fixes
 - Voice wake: auto-save wake words on blur/submit across iOS/Android and align limits with macOS.
+- Tailscale: retry serve/funnel with sudo only for permission errors and keep original failure details. (#1551) Thanks @sweepies.
 - Discord: limit autoThread mention bypass to bot-owned threads; keep ack reactions mention-gated. (#1511) Thanks @pvoo.
 - Gateway: accept null optional fields in exec approval requests. (#1511) Thanks @pvoo.
 - TUI: forward unknown slash commands (for example, `/context`) to the Gateway.

src/infra/tailscale.test.ts

@@ -1,17 +1,21 @@
-import { describe, expect, it, vi } from "vitest";
+import { afterEach, describe, expect, it, vi } from "vitest";
 
-import {
+import * as tailscale from "./tailscale.js";
+
+const {
   ensureGoInstalled,
   ensureTailscaledInstalled,
   getTailnetHostname,
   enableTailscaleServe,
   disableTailscaleServe,
-  enableTailscaleFunnel,
+  ensureFunnel,
-  disableTailscaleFunnel,
+} = tailscale;
-  ensureFunnel
-} from "./tailscale.js";
 
 describe("tailscale helpers", () => {
+  afterEach(() => {
+    vi.restoreAllMocks();
+  });
+
   it("parses DNS name from tailscale status", async () => {
     const exec = vi.fn().mockResolvedValue({
       stdout: JSON.stringify({
@@ -61,7 +65,9 @@ describe("tailscale helpers", () => {
   it("enableTailscaleServe attempts normal first, then sudo", async () => {
     // 1. First attempt fails
     // 2. Second attempt (sudo) succeeds
-    const exec = vi.fn()
+    vi.spyOn(tailscale, "getTailscaleBinary").mockResolvedValue("tailscale");
+    const exec = vi
+      .fn()
       .mockRejectedValueOnce(new Error("permission denied"))
       .mockResolvedValueOnce({ stdout: "" });
 
@@ -71,18 +77,19 @@ describe("tailscale helpers", () => {
       1,
       "tailscale",
       expect.arrayContaining(["serve", "--bg", "--yes", "3000"]),
-      expect.any(Object)
+      expect.any(Object),
     );
 
     expect(exec).toHaveBeenNthCalledWith(
       2,
       "sudo",
       expect.arrayContaining(["-n", "tailscale", "serve", "--bg", "--yes", "3000"]),
-      expect.any(Object)
+      expect.any(Object),
     );
   });
 
   it("enableTailscaleServe does NOT use sudo if first attempt succeeds", async () => {
+    vi.spyOn(tailscale, "getTailscaleBinary").mockResolvedValue("tailscale");
     const exec = vi.fn().mockResolvedValue({ stdout: "" });
 
     await enableTailscaleServe(3000, exec as never);
@@ -91,13 +98,15 @@ describe("tailscale helpers", () => {
     expect(exec).toHaveBeenCalledWith(
       "tailscale",
       expect.arrayContaining(["serve", "--bg", "--yes", "3000"]),
-      expect.any(Object)
+      expect.any(Object),
     );
   });
 
   it("disableTailscaleServe uses fallback", async () => {
-    const exec = vi.fn()
+    vi.spyOn(tailscale, "getTailscaleBinary").mockResolvedValue("tailscale");
-      .mockRejectedValueOnce(new Error("failed"))
+    const exec = vi
+      .fn()
+      .mockRejectedValueOnce(new Error("permission denied"))
       .mockResolvedValueOnce({ stdout: "" });
 
     await disableTailscaleServe(exec as never);
@@ -107,7 +116,7 @@ describe("tailscale helpers", () => {
       2,
       "sudo",
       expect.arrayContaining(["-n", "tailscale", "serve", "reset"]),
-      expect.any(Object)
+      expect.any(Object),
     );
   });
 
@@ -116,9 +125,11 @@ describe("tailscale helpers", () => {
     // 1. status (success)
     // 2. enable (fails)
     // 3. enable sudo (success)
-    const exec = vi.fn()
+    vi.spyOn(tailscale, "getTailscaleBinary").mockResolvedValue("tailscale");
+    const exec = vi
+      .fn()
       .mockResolvedValueOnce({ stdout: JSON.stringify({ BackendState: "Running" }) }) // status
-      .mockRejectedValueOnce(new Error("failed")) // enable normal
+      .mockRejectedValueOnce(new Error("permission denied")) // enable normal
       .mockResolvedValueOnce({ stdout: "" }); // enable sudo
 
     const runtime = {
@@ -134,7 +145,7 @@ describe("tailscale helpers", () => {
     expect(exec).toHaveBeenNthCalledWith(
       1,
       "tailscale",
-      expect.arrayContaining(["funnel", "status", "--json"])
+      expect.arrayContaining(["funnel", "status", "--json"]),
     );
 
     // 2. enable normal
@@ -142,7 +153,7 @@ describe("tailscale helpers", () => {
       2,
       "tailscale",
       expect.arrayContaining(["funnel", "--yes", "--bg", "8080"]),
-      expect.any(Object)
+      expect.any(Object),
     );
 
     // 3. enable sudo
@@ -150,7 +161,31 @@ describe("tailscale helpers", () => {
       3,
       "sudo",
       expect.arrayContaining(["-n", "tailscale", "funnel", "--yes", "--bg", "8080"]),
-      expect.any(Object)
+      expect.any(Object),
     );
   });
+
+  it("enableTailscaleServe skips sudo on non-permission errors", async () => {
+    vi.spyOn(tailscale, "getTailscaleBinary").mockResolvedValue("tailscale");
+    const exec = vi.fn().mockRejectedValueOnce(new Error("boom"));
+
+    await expect(enableTailscaleServe(3000, exec as never)).rejects.toThrow("boom");
+
+    expect(exec).toHaveBeenCalledTimes(1);
+  });
+
+  it("enableTailscaleServe rethrows original error if sudo fails", async () => {
+    vi.spyOn(tailscale, "getTailscaleBinary").mockResolvedValue("tailscale");
+    const originalError = Object.assign(new Error("permission denied"), {
+      stderr: "permission denied",
+    });
+    const exec = vi
+      .fn()
+      .mockRejectedValueOnce(originalError)
+      .mockRejectedValueOnce(new Error("sudo: a password is required"));
+
+    await expect(enableTailscaleServe(3000, exec as never)).rejects.toBe(originalError);
+
+    expect(exec).toHaveBeenCalledTimes(2);
+  });
 });

src/infra/tailscale.ts

@@ -206,6 +206,39 @@ export async function ensureTailscaledInstalled(
   await exec("brew", ["install", "tailscale"]);
 }
 
+type ExecErrorDetails = {
+  stdout?: unknown;
+  stderr?: unknown;
+  message?: unknown;
+  code?: unknown;
+};
+
+function extractExecErrorText(err: unknown) {
+  const errOutput = err as ExecErrorDetails;
+  const stdout = typeof errOutput.stdout === "string" ? errOutput.stdout : "";
+  const stderr = typeof errOutput.stderr === "string" ? errOutput.stderr : "";
+  const message = typeof errOutput.message === "string" ? errOutput.message : "";
+  const code = typeof errOutput.code === "string" ? errOutput.code : "";
+  return { stdout, stderr, message, code };
+}
+
+function isPermissionDeniedError(err: unknown): boolean {
+  const { stdout, stderr, message, code } = extractExecErrorText(err);
+  if (code.toUpperCase() === "EACCES") return true;
+  const combined = `${stdout}\n${stderr}\n${message}`.toLowerCase();
+  return (
+    combined.includes("permission denied") ||
+    combined.includes("access denied") ||
+    combined.includes("operation not permitted") ||
+    combined.includes("not permitted") ||
+    combined.includes("requires root") ||
+    combined.includes("must be run as root") ||
+    combined.includes("must be run with sudo") ||
+    combined.includes("requires sudo") ||
+    combined.includes("need sudo")
+  );
+}
+
 // Helper to attempt a command, and retry with sudo if it fails.
 async function execWithSudoFallback(
   exec: typeof runExec,
@@ -216,12 +249,18 @@ async function execWithSudoFallback(
   try {
     return await exec(bin, args, opts);
   } catch (err) {
-    // If the error suggests permission denied or access denied, try with sudo.
+    if (!isPermissionDeniedError(err)) {
-    // Or honestly, for any error in these specific ops, trying sudo is a reasonable fallback
+      throw err;
-    // given the context of what we're doing (system-level network config).
+    }
-    // We'll log a verbose message that we're falling back.
     logVerbose(`Command failed, retrying with sudo: ${bin} ${args.join(" ")}`);
+    try {
       return await exec("sudo", ["-n", bin, ...args], opts);
+    } catch (sudoErr) {
+      const { stderr, message } = extractExecErrorText(sudoErr);
+      const detail = (stderr || message).trim();
+      if (detail) logVerbose(`Sudo retry failed: ${detail}`);
+      throw err;
+    }
   }
 }
 
@@ -313,52 +352,32 @@ export async function ensureFunnel(
 
 export async function enableTailscaleServe(port: number, exec: typeof runExec = runExec) {
   const tailscaleBin = await getTailscaleBinary();
-  await execWithSudoFallback(
+  await execWithSudoFallback(exec, tailscaleBin, ["serve", "--bg", "--yes", `${port}`], {
-    exec,
-    tailscaleBin,
-    ["serve", "--bg", "--yes", `${port}`],
-    {
     maxBuffer: 200_000,
     timeoutMs: 15_000,
-    },
+  });
-  );
 }
 
 export async function disableTailscaleServe(exec: typeof runExec = runExec) {
   const tailscaleBin = await getTailscaleBinary();
-  await execWithSudoFallback(
+  await execWithSudoFallback(exec, tailscaleBin, ["serve", "reset"], {
-    exec,
-    tailscaleBin,
-    ["serve", "reset"],
-    {
     maxBuffer: 200_000,
     timeoutMs: 15_000,
-    },
+  });
-  );
 }
 
 export async function enableTailscaleFunnel(port: number, exec: typeof runExec = runExec) {
   const tailscaleBin = await getTailscaleBinary();
-  await execWithSudoFallback(
+  await execWithSudoFallback(exec, tailscaleBin, ["funnel", "--bg", "--yes", `${port}`], {
-    exec,
-    tailscaleBin,
-    ["funnel", "--bg", "--yes", `${port}`],
-    {
     maxBuffer: 200_000,
     timeoutMs: 15_000,
-    },
+  });
-  );
 }
 
 export async function disableTailscaleFunnel(exec: typeof runExec = runExec) {
   const tailscaleBin = await getTailscaleBinary();
-  await execWithSudoFallback(
+  await execWithSudoFallback(exec, tailscaleBin, ["funnel", "reset"], {
-    exec,
-    tailscaleBin,
-    ["funnel", "reset"],
-    {
     maxBuffer: 200_000,
     timeoutMs: 15_000,
-    },
+  });
-  );
 }