fix(security): remove unsanitized icacls duplicate; require proven identity before kill escalation

Two review findings on PR #18, both verified real:

1. cloudflared-named-setup.js carried its own pre-existing
   applyPrivatePermissions copy that did NOT sanitize USERNAME/USERDOMAIN
   before building the icacls principal — the gap the PR's token.js fix
   was meant to close, and reachable via writeTunnelConfig on every named
   tunnel create/reconfigure. Deleted the local copy; the file now
   imports the shared sanitized helper from ../token.js.

2. stopDaemon's 'rejected' branch killed the lockfile pid on ANY non-2xx
   shutdown response. A non-2xx only proves something answered on the
   port — a stale lock whose port was reused by an unrelated HTTP
   service also rejects, while the recorded pid may belong to an
   innocent recycled process. Kill escalation now requires proven
   identity: an accepted (bearer-authenticated) shutdown, or
   /daemon/health echoing the lockfile's uuid. Unproven answers fall
   through to reclaim-without-kill like the unreachable path. Tests
   updated: proven-identity 401 still escalates; unproven rejection
   must not kill.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: ARHAEEM

packages/extension/src/mcp/daemon-manager.ts

@@ -13,6 +13,8 @@ export interface DaemonStatus {
   bearerToken: string | null;
   tunnelUrl: string | null;
   uptime: number | null;
+  /** Lockfile uuid — used to verify daemon identity before kill escalation. */
+  uuid: string | null;
 }
 
 export interface DaemonConnectionInfo {
@@ -27,7 +29,7 @@ export interface DaemonConnectionInfo {
 
 const EMPTY_STATUS: DaemonStatus = {
   running: false, healthy: false, pid: null, port: null,
-  port_lsp: null, bearerToken: null, tunnelUrl: null, uptime: null,
+  port_lsp: null, bearerToken: null, tunnelUrl: null, uptime: null, uuid: null,
 };
 
 export interface StopResult {
@@ -91,6 +93,7 @@ export class DaemonManager implements vscode.Disposable {
         bearerToken,
         tunnelUrl: typeof record.tunnelUrl === 'string' ? record.tunnelUrl : null,
         uptime: typeof record.startedAt === 'string' ? Date.now() - Date.parse(record.startedAt) : null,
+        uuid: typeof record.uuid === 'string' && record.uuid.length > 0 ? record.uuid : null,
       };
       this._status = status;
       return status;
@@ -193,10 +196,19 @@ export class DaemonManager implements vscode.Disposable {
     const pid = status.pid;
     const pidAlive = typeof pid === 'number' && pid > 0 && this._isPidAlive(pid);
 
-    // 3) The port answered, so the pid in the lockfile really is the daemon —
-    //    safe to kill. When the port is unreachable we do NOT kill: after a
-    //    crash the OS may have recycled the pid onto an innocent process.
-    if (outcome !== 'unreachable' && pidAlive && typeof pid === 'number') {
+    // 3) Escalate to kill ONLY with proven daemon identity. An accepted
+    //    (bearer-authenticated) shutdown proves it. A rejected response does
+    //    NOT — it only proves SOMETHING answered on the port; a stale lock
+    //    whose port was reused by an unrelated HTTP service also rejects,
+    //    while the recorded pid may belong to an innocent recycled process.
+    //    For rejected, require /daemon/health to echo the lockfile's uuid.
+    const provenOurDaemon = outcome === 'accepted' || (
+      outcome === 'rejected'
+      && status.port != null && status.bearerToken != null
+      && await this._verifyDaemonIdentity(status.port, status.bearerToken, status.uuid)
+    );
+
+    if (provenOurDaemon && pidAlive && typeof pid === 'number') {
       this._killPid(pid);
       const deadline = Date.now() + 3_000;
       let escalated = false;
@@ -214,19 +226,43 @@ export class DaemonManager implements vscode.Disposable {
       return { stopped: true, forced: true };
     }
 
-    // 4) Unreachable: the lockfile is stale (dead pid) or points at a process
-    //    that is not serving the daemon port. Reclaim the lock either way so
-    //    the dashboard stops showing a phantom daemon.
+    // 4) Unreachable or unproven identity: the lockfile is stale (dead pid),
+    //    or whatever answers on the port could not be verified as our daemon.
+    //    Reclaim the lock so the dashboard stops showing a phantom daemon,
+    //    but leave the recorded pid untouched (PID-reuse safety).
     this._reclaimLockfile();
     return {
       stopped: true,
       forced: false,
       reason: pidAlive
-        ? `Removed stale daemon.lock; process ${pid} was not answering on the daemon port and was left untouched.`
+        ? `Removed stale daemon.lock; process ${pid} could not be verified as the daemon and was left untouched.`
         : undefined,
     };
   }
 
+  /**
+   * Proof of identity for kill escalation: /daemon/health, authenticated with
+   * the lockfile's bearer token, must echo the lockfile's uuid.
+   */
+  private async _verifyDaemonIdentity(port: number, bearerToken: string, uuid: string | null): Promise<boolean> {
+    if (!uuid) return false;
+    const controller = new AbortController();
+    const timeout = setTimeout(() => controller.abort(), 2_000);
+    try {
+      const res = await fetch(`http://127.0.0.1:${port}/daemon/health`, {
+        headers: { Authorization: `Bearer ${bearerToken}` },
+        signal: controller.signal,
+      });
+      if (!res.ok) return false;
+      const body = await res.json().catch(() => null) as { uuid?: unknown } | null;
+      return body?.uuid === uuid;
+    } catch {
+      return false;
+    } finally {
+      clearTimeout(timeout);
+    }
+  }
+
   async restartDaemon(): Promise<DaemonConnectionInfo> {
     await this.stopDaemon();
     await this._delay(500);

packages/extension/src/test/daemon-manager.test.ts

@@ -135,8 +135,13 @@ describe('DaemonManager.stopDaemon', () => {
     expect(lockExists()).toBe(false);
   });
 
-  it('rejected shutdown (401): escalates to killing the recorded pid and reclaims the lock', async () => {
-    const port = await listen((_req, res) => {
+  it('rejected shutdown with PROVEN identity (health echoes lock uuid): escalates to kill', async () => {
+    const port = await listen((req, res) => {
+      if (req.method === 'GET' && req.url === '/daemon/health') {
+        res.writeHead(200, { 'Content-Type': 'application/json' });
+        res.end(JSON.stringify({ ok: true, uuid: 'uuid-1' }));
+        return;
+      }
       res.writeHead(401, { 'Content-Type': 'application/json' });
       res.end('{"error":"Unauthorized"}');
     });
@@ -153,6 +158,25 @@ describe('DaemonManager.stopDaemon', () => {
     expect(lockExists()).toBe(false);
   });
 
+  it('rejected shutdown WITHOUT proven identity: does NOT kill (PID reuse), reclaims the lock', async () => {
+    // Simulates a stale lock whose port was reused by an unrelated HTTP
+    // service: it answers (non-2xx) but cannot echo the lockfile uuid.
+    const port = await listen((_req, res) => {
+      res.writeHead(404, { 'Content-Type': 'application/json' });
+      res.end('{"error":"Not found"}');
+    });
+    writeLock(port, process.pid);
+
+    (dm as any)._killPid = vi.fn();
+    (dm as any)._isPidAlive = vi.fn(() => true);
+
+    const result = await dm.stopDaemon();
+    expect((dm as any)._killPid).not.toHaveBeenCalled();
+    expect(result.stopped).toBe(true);
+    expect(result.reason).toBeTruthy();
+    expect(lockExists()).toBe(false);
+  });
+
   it('unreachable daemon with dead pid: reclaims the stale lock without killing anything', async () => {
     writeLock(1, 999_999); // port 1 — nothing listening
     (dm as any)._killPid = vi.fn();

packages/mcp-server/src/daemon/tunnel-providers/cloudflared-named-setup.js

@@ -11,7 +11,6 @@
  */
 
 import {
-  chmodSync,
   existsSync,
   mkdirSync,
   readFileSync,
@@ -21,10 +20,10 @@ import { safeAtomicWriteFileSync } from '../../safe-write.js';
 import { spawn as nodeSpawn } from 'node:child_process';
 import { dirname, join } from 'node:path';
 import { homedir } from 'node:os';
-import { spawnSync } from 'node:child_process';
 
 import { getTunnelBinaryPath } from '../install-tunnel.js';
 import { getHomeDir } from '../../paths.js';
+import { applyPrivatePermissions } from '../token.js';
 
 const CONFIG_FILENAME = 'cloudflared-named.yml';
 const DEFAULT_LOGIN_TIMEOUT_MS = 10 * 60 * 1000;
@@ -676,20 +675,7 @@ function unquoteYaml(value) {
   return value;
 }
 
-/**
- * @param {string} path
- */
-function applyPrivatePermissions(path) {
-  if (process.platform === 'win32') {
-    const username = process.env.USERNAME;
-    const domain = process.env.USERDOMAIN;
-    const target = domain && username ? `${domain}\\${username}` : username ?? '';
-    if (!target) return;
-    spawnSync('icacls', [path, '/inheritance:r', '/grant:r', `${target}:(R,W)`], {
-      encoding: 'utf8',
-      windowsHide: true,
-    });
-    return;
-  }
-  chmodSync(path, 0o600);
-}
+// applyPrivatePermissions is imported from ../token.js — the shared helper
+// sanitizes USERNAME/USERDOMAIN before building the icacls principal. Do not
+// re-introduce a local copy here (a previous unsanitized duplicate was a
+// security regression).