fix(nodes): gate node-management actions by role and release pilot tunnels on delete (#1280)

* fix(nodes): gate node-management actions by role and release pilot tunnels on delete

The node-management write actions in the Nodes panel (add, edit, delete,
generate node token, reset fleet-sync anchor) rendered for every signed-in role,
but the API enforces the manage-nodes permission on them, so lower-privilege
roles saw buttons that returned 403. The panel now renders each action against
the same permission its route enforces; the read-only node table stays visible
to every role.

Deleting a node now also tears down its live pilot-agent tunnel (and any mesh
bridge) immediately, releasing the loopback server, heartbeat timer, and open
streams instead of leaving them until the agent next disconnects, matching the
cleanup the re-enrollment path already performed.

Adds backend route tests for the permission boundaries and tunnel teardown, and
a Nodes panel render test covering the viewer, admin, and node-admin views.

* fix(nodes): close proxy mesh bridges via the dialer on node delete to skip a redial

Deleting a node closed any active mesh bridge through PilotTunnelManager, but a
proxy-mode bridge is owned by the mesh dialer, whose close listener then treated
the close as unexpected and scheduled a reactive redial against the node being
removed. The delete handler now closes a proxy bridge through the dialer's
intentional-close path first (which suppresses the redial), then closes a
pilot-agent tunnel as before. Adds a backend test that primes a live proxy
bridge and asserts deletion closes it without scheduling a redial.

backend/src/__tests__/node-management-hardening.test.ts

@@ -0,0 +1,231 @@
+/**
+ * Node-management route hardening.
+ *
+ * Gate parity: the write routes under /api/nodes enforce node:manage (admin or
+ * node-admin). viewer and deployer sessions must be refused, while the read
+ * route stays open to any authenticated session. This is the backend contract
+ * the frontend mirrors by showing the node table to everyone but gating the
+ * Add / Edit / Delete affordances on node:manage.
+ *
+ * Tunnel cleanup: deleting a node tears down any live pilot tunnel so the
+ * bridge (loopback server, ping timer, open streams) is released immediately
+ * instead of lingering until the agent happens to disconnect.
+ */
+import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest';
+import request from 'supertest';
+import jwt from 'jsonwebtoken';
+import { EventEmitter } from 'events';
+import { WebSocket } from 'ws';
+import { setupTestDb, cleanupTestDb, TEST_USERNAME, TEST_JWT_SECRET } from './helpers/setupTestDb';
+import { PilotTunnelManager } from '../services/PilotTunnelManager';
+import { MeshProxyTunnelDialer } from '../services/MeshProxyTunnelDialer';
+
+let tmpDir: string;
+let app: import('express').Express;
+let DatabaseService: typeof import('../services/DatabaseService').DatabaseService;
+
+type ManageRole = 'admin' | 'node-admin';
+type DeniedRole = 'viewer' | 'deployer';
+
+function authToken(username: string, role: string, tv: number): string {
+  return jwt.sign({ username, role, tv }, TEST_JWT_SECRET, { expiresIn: '1m' });
+}
+
+/**
+ * Token for a session in the given role. The auth middleware resolves the role
+ * from the DB, so the user must exist; password_hash is irrelevant because we
+ * sign the JWT directly rather than logging in. The seeded admin is reused so
+ * we never trip the seat or last-admin guards.
+ */
+function tokenForRole(role: ManageRole | DeniedRole): string {
+  const db = DatabaseService.getInstance();
+  const username = role === 'admin' ? TEST_USERNAME : `nm-${role}`;
+  let user = db.getUserByUsername(username);
+  if (!user) {
+    db.addUser({ username, password_hash: 'test-hash', role });
+    user = db.getUserByUsername(username)!;
+  }
+  return authToken(username, role, user.token_version);
+}
+
+function makeMockTunnelWs(): EventEmitter & {
+  readyState: number;
+  bufferedAmount: number;
+  send: (data: unknown) => void;
+  ping: () => void;
+  close: () => void;
+} {
+  const ws = new EventEmitter() as EventEmitter & {
+    readyState: number;
+    bufferedAmount: number;
+    send: (data: unknown) => void;
+    ping: () => void;
+    close: () => void;
+  };
+  ws.readyState = WebSocket.OPEN;
+  ws.bufferedAmount = 0;
+  ws.send = () => { /* no-op */ };
+  ws.ping = () => { /* no-op */ };
+  ws.close = () => { ws.readyState = WebSocket.CLOSED; ws.emit('close'); };
+  return ws;
+}
+
+function addPilotNode(name: string): number {
+  return DatabaseService.getInstance().addNode({
+    name,
+    type: 'remote',
+    mode: 'pilot_agent',
+    compose_dir: '/tmp/x',
+    is_default: false,
+    api_url: '',
+    api_token: '',
+  });
+}
+
+beforeAll(async () => {
+  tmpDir = await setupTestDb();
+  ({ DatabaseService } = await import('../services/DatabaseService'));
+  ({ app } = await import('../index'));
+});
+
+afterAll(() => cleanupTestDb(tmpDir));
+
+describe('node-management write routes require node:manage', () => {
+  it('lets a viewer read the node list (the table stays visible)', async () => {
+    const res = await request(app)
+      .get('/api/nodes')
+      .set('Authorization', `Bearer ${tokenForRole('viewer')}`);
+    expect(res.status).toBe(200);
+    expect(Array.isArray(res.body)).toBe(true);
+  });
+
+  for (const role of ['viewer', 'deployer'] as const) {
+    it(`refuses node creation for ${role} (403 PERMISSION_DENIED)`, async () => {
+      const res = await request(app)
+        .post('/api/nodes')
+        .set('Authorization', `Bearer ${tokenForRole(role)}`)
+        .send({ name: `nm-create-${role}`, type: 'remote', mode: 'pilot_agent' });
+      expect(res.status).toBe(403);
+      expect(res.body.code).toBe('PERMISSION_DENIED');
+    });
+
+    it(`refuses node deletion for ${role} and leaves the node intact (403)`, async () => {
+      const db = DatabaseService.getInstance();
+      const id = addPilotNode(`nm-del-${role}`);
+      const res = await request(app)
+        .delete(`/api/nodes/${id}`)
+        .set('Authorization', `Bearer ${tokenForRole(role)}`);
+      expect(res.status).toBe(403);
+      expect(res.body.code).toBe('PERMISSION_DENIED');
+      expect(db.getNode(id)).toBeTruthy();
+      db.deleteNode(id);
+    });
+  }
+
+  // Positive boundary: both manage-capable roles succeed. admin short-circuits
+  // the permission engine; node-admin must resolve node:manage from
+  // ROLE_PERMISSIONS, so this also guards against node-admin silently losing
+  // write access if that mapping ever changes.
+  for (const role of ['admin', 'node-admin'] as const) {
+    it(`allows node creation for ${role} (200)`, async () => {
+      const res = await request(app)
+        .post('/api/nodes')
+        .set('Authorization', `Bearer ${tokenForRole(role)}`)
+        .send({ name: `nm-create-${role}-${Date.now()}`, type: 'remote', mode: 'pilot_agent' });
+      expect(res.status).toBe(200);
+      expect(res.body.success).toBe(true);
+    });
+  }
+
+  it('allows a node-admin to delete a node (200)', async () => {
+    const db = DatabaseService.getInstance();
+    const id = addPilotNode(`nm-del-nodeadmin-${Date.now()}`);
+    const res = await request(app)
+      .delete(`/api/nodes/${id}`)
+      .set('Authorization', `Bearer ${tokenForRole('node-admin')}`);
+    expect(res.status).toBe(200);
+    expect(db.getNode(id)).toBeUndefined();
+  });
+});
+
+describe('deleting a node tears down its tunnel or mesh bridge', () => {
+  it('closes the active tunnel socket and removes the node row', async () => {
+    const db = DatabaseService.getInstance();
+    const mgr = PilotTunnelManager.getInstance();
+    const id = addPilotNode(`nm-tunnel-del-${Date.now()}`);
+
+    const ws = makeMockTunnelWs();
+    await mgr.registerTunnel(id, ws as unknown as WebSocket, 'test-1.0.0');
+    expect(mgr.hasActiveTunnel(id)).toBe(true);
+
+    const res = await request(app)
+      .delete(`/api/nodes/${id}`)
+      .set('Authorization', `Bearer ${tokenForRole('admin')}`);
+
+    expect(res.status).toBe(200);
+    // The manager forgot the tunnel AND the underlying socket was actually
+    // closed (not just dropped from the map), so the agent gets a clean close.
+    expect(mgr.hasActiveTunnel(id)).toBe(false);
+    expect(ws.readyState).toBe(WebSocket.CLOSED);
+    expect(db.getNode(id)).toBeUndefined();
+  });
+
+  it('deletes a node with no active tunnel without error (closeTunnel no-op path)', async () => {
+    const db = DatabaseService.getInstance();
+    const id = addPilotNode(`nm-no-tunnel-del-${Date.now()}`);
+    expect(PilotTunnelManager.getInstance().hasActiveTunnel(id)).toBe(false);
+
+    const res = await request(app)
+      .delete(`/api/nodes/${id}`)
+      .set('Authorization', `Bearer ${tokenForRole('admin')}`);
+
+    expect(res.status).toBe(200);
+    expect(db.getNode(id)).toBeUndefined();
+  });
+
+  it('closes a live proxy-mode mesh bridge on delete without scheduling a redial', async () => {
+    const db = DatabaseService.getInstance();
+    const dialer = MeshProxyTunnelDialer.resetForTest();
+    const redialSpy = vi.spyOn(
+      dialer as unknown as { scheduleReactiveRedial: (id: number) => void },
+      'scheduleReactiveRedial',
+    ).mockImplementation(() => {});
+
+    const id = db.addNode({
+      name: `nm-proxy-del-${Date.now()}`,
+      type: 'remote',
+      mode: 'proxy',
+      api_url: 'http://proxy-peer:1852',
+      api_token: 'tok',
+      compose_dir: '/tmp/x',
+      is_default: false,
+    });
+
+    // Prime a live proxy bridge the way dial() does: in the dialer's map with a
+    // close listener wired. A real bridge emits 'closed' when closed, so the
+    // fake does too. If the delete path closed it via the manager instead of the
+    // dialer's intentional path, that 'closed' event would drive tearDownBridge
+    // -> proxy-bridge-down -> a reactive redial against the deleted node.
+    const fakeBridge = new EventEmitter() as EventEmitter & {
+      close: ReturnType<typeof vi.fn>;
+      getActiveStreamCount: () => number;
+    };
+    fakeBridge.close = vi.fn(() => { fakeBridge.emit('closed', { code: 1000 }); });
+    fakeBridge.getActiveStreamCount = () => 0;
+    (dialer as unknown as { bridges: Map<number, unknown> }).bridges.set(id, fakeBridge);
+    (dialer as unknown as { attachBridgeCloseListener: (id: number, b: EventEmitter) => void })
+      .attachBridgeCloseListener(id, fakeBridge);
+
+    const res = await request(app)
+      .delete(`/api/nodes/${id}`)
+      .set('Authorization', `Bearer ${tokenForRole('admin')}`);
+
+    expect(res.status).toBe(200);
+    expect(fakeBridge.close).toHaveBeenCalled();
+    expect(dialer.hasBridge(id)).toBe(false);
+    expect(redialSpy).not.toHaveBeenCalled();
+    expect(db.getNode(id)).toBeUndefined();
+
+    redialSpy.mockRestore();
+  });
+});

backend/src/pilot/protocol.ts

@@ -308,6 +308,7 @@ export function wsDataToString(data: unknown): string | null {
 // --- Close codes ---
 
 export const PilotCloseCode = {
+    NormalClosure: 1000,
     Replaced: 4000,
     EnrollmentRegenerated: 4001,
     ProtocolError: 1002,

backend/src/routes/nodes.ts

@@ -313,6 +313,15 @@ nodesRouter.delete('/:id', async (req: Request, res: Response) => {
   if (!requirePermission(req, res, 'node:manage', 'node', nodeIdParam)) return;
   try {
     const id = parseInt(nodeIdParam);
+    // Release any live tunnel or mesh bridge before deleting the record so it is
+    // freed immediately rather than lingering until the peer disconnects. Close a
+    // proxy-mode mesh bridge through its dialer FIRST: closeBridge removes the
+    // bridge before closing it, so the dialer does not schedule a reactive redial
+    // against a node that is about to disappear. closeTunnel then closes a
+    // pilot-agent tunnel; both are no-ops when this node has no such bridge (a
+    // local node, or one with no active connection). Mirrors the re-enroll path.
+    MeshProxyTunnelDialer.getInstance().closeBridge(id, 'node deleted');
+    PilotTunnelManager.getInstance().closeTunnel(id, PilotCloseCode.NormalClosure, 'node deleted');
     DatabaseService.getInstance().deleteNode(id);
     NodeRegistry.getInstance().evictConnection(id);
     NodeRegistry.getInstance().notifyNodeRemoved(id);

frontend/src/components/NodeManager.tsx

@@ -35,8 +35,15 @@ export interface SenchoNavigateDetail {
 
 export function NodeManager() {
   const { isPaid } = useLicense();
-  const { isAdmin } = useAuth();
+  const { isAdmin, can } = useAuth();
   const canEditLabels = isPaid && isAdmin;
+  // Mirror the backend node:manage guard. This top-level flag checks the global
+  // role only (admin or global node-admin); the per-row Edit/Delete buttons below
+  // additionally honor scoped per-node grants via can('node:manage', 'node', id).
+  // Admins resolve immediately via isAdmin; node-admins once /permissions/me lands.
+  // Generate-token and reset-anchor below stay admin-only to match their stricter
+  // backend guards (requireAdmin, and requireAdmin + requirePaid).
+  const canManageNodes = isAdmin || can('node:manage');
   const { nodes, refreshNodeMeta } = useNodes();
   useMastheadStats([
     { label: 'NODES', value: `${nodes.length}` },
@@ -189,21 +196,29 @@ export function NodeManager() {
 
   return (
     <div className="space-y-6">
-      {/* Actions */}
-      <div className="flex justify-end">
-        <SettingsPrimaryButton
-          size="sm"
-          className="gap-1 shrink-0"
-          onClick={openCreate}
-        >
-          <Plus className="w-4 h-4" />
-          Add node
-        </SettingsPrimaryButton>
-      </div>
+      {/* Actions (node management is admin / node-admin only, mirroring the
+          node:manage backend guard). The read-only table below stays visible to
+          every role with node:read. */}
+      {canManageNodes && (
+        <>
+          <div className="flex justify-end">
+            <SettingsPrimaryButton
+              size="sm"
+              className="gap-1 shrink-0"
+              onClick={openCreate}
+            >
+              <Plus className="w-4 h-4" />
+              Add node
+            </SettingsPrimaryButton>
+          </div>
 
-      <Separator />
+          <Separator />
+        </>
+      )}
 
-      {/* Generate Node Token - for use on THIS instance as a remote target */}
+      {/* Generate a node token so THIS instance can serve as a remote target.
+          Admin-only, matching the requireAdmin guard on /auth/generate-node-token. */}
+      {isAdmin && (
       <div className="rounded-md border p-4 space-y-3">
         <div className="flex items-start justify-between gap-4">
           <div>
@@ -235,6 +250,7 @@ export function NodeManager() {
           </div>
         )}
       </div>
+      )}
 
       {/* Sync issues: surfaces FleetSync sticky errors (currently CONTROL_IDENTITY_MISMATCH). */}
       {anchorMismatches.length > 0 && (
@@ -257,15 +273,17 @@ export function NodeManager() {
                   {' '}Reset the anchor on the peer to resume sync, or remove the node from this fleet.
                 </div>
                 <div className="flex flex-wrap items-center gap-2 pt-1">
-                  <Button
-                    size="sm"
-                    variant="destructive"
-                    onClick={() => handleResetAnchor(nodeId)}
-                    disabled={resettingAnchor === nodeId}
-                  >
-                    {resettingAnchor === nodeId ? 'Resetting...' : 'Reset anchor on peer'}
-                  </Button>
-                  {node && !node.is_default && (
+                  {isAdmin && isPaid && (
+                    <Button
+                      size="sm"
+                      variant="destructive"
+                      onClick={() => handleResetAnchor(nodeId)}
+                      disabled={resettingAnchor === nodeId}
+                    >
+                      {resettingAnchor === nodeId ? 'Resetting...' : 'Reset anchor on peer'}
+                    </Button>
+                  )}
+                  {node && !node.is_default && (isAdmin || can('node:manage', 'node', String(nodeId))) && (
                     <Button
                       size="sm"
                       variant="outline"
@@ -299,7 +317,9 @@ export function NodeManager() {
             </TableRow>
           </TableHeader>
           <TableBody>
-            {nodes.map((node) => (
+            {nodes.map((node) => {
+              const canManageThis = isAdmin || can('node:manage', 'node', String(node.id));
+              return (
               <TableRow key={node.id}>
                 <TableCell>
                   {node.is_default && (
@@ -451,24 +471,26 @@ export function NodeManager() {
                       </Tooltip>
                     </TooltipProvider>
 
-                    <TooltipProvider>
-                      <Tooltip>
-                        <TooltipTrigger asChild>
-                          <Button
-                            variant="ghost"
-                            size="icon"
-                            className="h-8 w-8"
-                            onClick={() => openEdit(node)}
-                            aria-label="Edit node"
-                          >
-                            <Pencil className="w-4 h-4" />
-                          </Button>
-                        </TooltipTrigger>
-                        <TooltipContent>Edit Node</TooltipContent>
-                      </Tooltip>
-                    </TooltipProvider>
+                    {canManageThis && (
+                      <TooltipProvider>
+                        <Tooltip>
+                          <TooltipTrigger asChild>
+                            <Button
+                              variant="ghost"
+                              size="icon"
+                              className="h-8 w-8"
+                              onClick={() => openEdit(node)}
+                              aria-label="Edit node"
+                            >
+                              <Pencil className="w-4 h-4" />
+                            </Button>
+                          </TooltipTrigger>
+                          <TooltipContent>Edit Node</TooltipContent>
+                        </Tooltip>
+                      </TooltipProvider>
+                    )}
 
-                    {!node.is_default && (
+                    {!node.is_default && canManageThis && (
                       <TooltipProvider>
                         <Tooltip>
                           <TooltipTrigger asChild>
@@ -489,7 +511,8 @@ export function NodeManager() {
                   </div>
                 </TableCell>
               </TableRow>
-            ))}
+              );
+            })}
           </TableBody>
         </Table>
       </div>

frontend/src/components/__tests__/NodeManager.test.tsx

@@ -0,0 +1,96 @@
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import { render, screen } from '@testing-library/react';
+import type { Node } from '@/context/NodeContext';
+
+// NodeManager pulls in several contexts and a child action hook. Mock them so
+// the test renders the panel in isolation and can drive the role/permission
+// inputs that gate the write affordances.
+const useAuthMock = vi.fn();
+const useLicenseMock = vi.fn();
+
+const testNode: Node = {
+  id: 2,
+  name: 'Edge',
+  type: 'remote',
+  mode: 'pilot_agent',
+  compose_dir: '/app/compose',
+  is_default: false,
+  status: 'online',
+  created_at: 0,
+  api_url: '',
+  pilot_last_seen: Date.now(),
+};
+
+vi.mock('@/context/NodeContext', () => ({
+  useNodes: () => ({ nodes: [testNode], refreshNodeMeta: vi.fn() }),
+}));
+vi.mock('@/context/AuthContext', () => ({ useAuth: () => useAuthMock() }));
+vi.mock('@/context/LicenseContext', () => ({ useLicense: () => useLicenseMock() }));
+vi.mock('@/lib/api', () => ({
+  apiFetch: vi.fn(() => Promise.resolve({ ok: false, json: () => Promise.resolve({}) })),
+}));
+vi.mock('@/components/ui/toast-store', () => ({
+  toast: { success: vi.fn(), error: vi.fn(), warning: vi.fn() },
+}));
+vi.mock('@/hooks/useFleetSyncStatus', () => ({
+  useFleetSyncStatus: () => ({ statuses: [], refresh: vi.fn() }),
+}));
+vi.mock('../settings/MastheadStatsContext', () => ({ useMastheadStats: vi.fn() }));
+vi.mock('../nodes/useNodeActions', () => ({
+  useNodeActions: () => ({
+    openCreate: vi.fn(),
+    openEdit: vi.fn(),
+    openDelete: vi.fn(),
+    NodeActionModals: null,
+  }),
+}));
+vi.mock('../blueprints/NodeLabelPicker', () => ({ NodeLabelPicker: () => null }));
+
+import { NodeManager } from '../NodeManager';
+
+/** can() that grants only the named action regardless of resource scope. */
+function canFor(...granted: string[]) {
+  return (action: string) => granted.includes(action);
+}
+
+beforeEach(() => {
+  useLicenseMock.mockReturnValue({ isPaid: false });
+});
+afterEach(() => vi.clearAllMocks());
+
+describe('NodeManager write-affordance gating', () => {
+  it('hides every write affordance from a viewer but still shows the node table', () => {
+    useAuthMock.mockReturnValue({ isAdmin: false, can: canFor() });
+    render(<NodeManager />);
+
+    // Read-only surface stays visible.
+    expect(screen.getByText('Edge')).toBeInTheDocument();
+
+    expect(screen.queryByRole('button', { name: /Add node/i })).not.toBeInTheDocument();
+    expect(screen.queryByText('Generate Node Token')).not.toBeInTheDocument();
+    expect(screen.queryByRole('button', { name: 'Edit node' })).not.toBeInTheDocument();
+    expect(screen.queryByRole('button', { name: 'Delete node' })).not.toBeInTheDocument();
+  });
+
+  it('shows every affordance to an admin', () => {
+    useAuthMock.mockReturnValue({ isAdmin: true, can: canFor('node:manage') });
+    render(<NodeManager />);
+
+    expect(screen.getByRole('button', { name: /Add node/i })).toBeInTheDocument();
+    expect(screen.getByText('Generate Node Token')).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Edit node' })).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Delete node' })).toBeInTheDocument();
+  });
+
+  it('lets a node-admin manage nodes but hides the admin-only token card', () => {
+    // node-admin: holds node:manage but is not a global admin.
+    useAuthMock.mockReturnValue({ isAdmin: false, can: canFor('node:manage') });
+    render(<NodeManager />);
+
+    expect(screen.getByRole('button', { name: /Add node/i })).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Edit node' })).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Delete node' })).toBeInTheDocument();
+    // Generate Node Token mirrors requireAdmin, so a node-admin must not see it.
+    expect(screen.queryByText('Generate Node Token')).not.toBeInTheDocument();
+  });
+});