fix(fleet): harden remote node updates with admin enforcement, expiry fix, and diagnostics (#542)

- Fix completed-entry auto-expiry using resolvedAt instead of startedAt - Add admin role enforcement to update trigger and update-all endpoints - Add missing error field in rejected-promise fallback for update-status - Fix rejected promises in update-all losing node names - Harden frontend recheck button with try/catch/finally error handling - Align frontend isValidVersion with stricter regex validation - Add diagnostic logging gated behind developer_mode - Extract resolveTracker helper to centralize terminal state transitions - Add 15 new fleet test cases covering auth, tier gating, input validation, and admin roles - Document admin requirement and troubleshooting in fleet-view docs
2026-08-05 08:27:42 +00:00 · 2026-04-12 22:07:41 -04:00
parent a0fe84e84b
commit d23c6779af
6 changed files with 217 additions and 30 deletions
@@ -60,6 +60,21 @@ describe('Fleet endpoints require authentication', () => {
    const res = await request(app).post('/api/fleet/nodes/1/update');
    expect(res.status).toBe(401);
  });
+
+  it('POST /api/fleet/update-all returns 401 without auth', async () => {
+    const res = await request(app).post('/api/fleet/update-all');
+    expect(res.status).toBe(401);
+  });
+
+  it('DELETE /api/fleet/nodes/1/update-status returns 401 without auth', async () => {
+    const res = await request(app).delete('/api/fleet/nodes/1/update-status');
+    expect(res.status).toBe(401);
+  });
+
+  it('DELETE /api/fleet/update-status returns 401 without auth', async () => {
+    const res = await request(app).delete('/api/fleet/update-status');
+    expect(res.status).toBe(401);
+  });
 });

 // ─── Input Validation ───
@@ -159,6 +174,125 @@ describe('Fleet tier gating', () => {
    expect(res.status).toBe(403);
    expect(res.body.code).toBe('PAID_REQUIRED');
  });
+
+  it('POST /api/fleet/nodes/1/update returns 403 on free tier', async () => {
+    mockTier('community');
+    const res = await request(app)
+      .post('/api/fleet/nodes/1/update')
+      .set('Authorization', authHeader);
+    expect(res.status).toBe(403);
+    expect(res.body.code).toBe('PAID_REQUIRED');
+  });
+
+  it('POST /api/fleet/update-all returns 403 on free tier', async () => {
+    mockTier('community');
+    const res = await request(app)
+      .post('/api/fleet/update-all')
+      .set('Authorization', authHeader);
+    expect(res.status).toBe(403);
+    expect(res.body.code).toBe('PAID_REQUIRED');
+  });
+
+  it('DELETE /api/fleet/nodes/1/update-status returns 403 on free tier', async () => {
+    mockTier('community');
+    const res = await request(app)
+      .delete('/api/fleet/nodes/1/update-status')
+      .set('Authorization', authHeader);
+    expect(res.status).toBe(403);
+    expect(res.body.code).toBe('PAID_REQUIRED');
+  });
+
+  it('DELETE /api/fleet/update-status returns 403 on free tier', async () => {
+    mockTier('community');
+    const res = await request(app)
+      .delete('/api/fleet/update-status')
+      .set('Authorization', authHeader);
+    expect(res.status).toBe(403);
+    expect(res.body.code).toBe('PAID_REQUIRED');
+  });
+});
+
+// ─── Update Endpoint Input Validation ───
+
+describe('Fleet update input validation', () => {
+  afterEach(() => vi.restoreAllMocks());
+
+  it('POST /api/fleet/nodes/abc/update returns 400 for NaN nodeId', async () => {
+    mockTier('paid');
+    const res = await request(app)
+      .post('/api/fleet/nodes/abc/update')
+      .set('Authorization', authHeader);
+    expect(res.status).toBe(400);
+    expect(res.body.error).toMatch(/invalid node id/i);
+  });
+
+  it('POST /api/fleet/nodes/99999/update returns 404 for missing node', async () => {
+    mockTier('paid');
+    const res = await request(app)
+      .post('/api/fleet/nodes/99999/update')
+      .set('Authorization', authHeader);
+    expect(res.status).toBe(404);
+    expect(res.body.error).toMatch(/not found/i);
+  });
+
+  it('DELETE /api/fleet/nodes/abc/update-status returns 400 for NaN nodeId', async () => {
+    mockTier('paid');
+    const res = await request(app)
+      .delete('/api/fleet/nodes/abc/update-status')
+      .set('Authorization', authHeader);
+    expect(res.status).toBe(400);
+    expect(res.body.error).toMatch(/invalid node id/i);
+  });
+
+  it('DELETE /api/fleet/nodes/99999/update-status returns 404 for missing node', async () => {
+    mockTier('paid');
+    const res = await request(app)
+      .delete('/api/fleet/nodes/99999/update-status')
+      .set('Authorization', authHeader);
+    expect(res.status).toBe(404);
+    expect(res.body.error).toMatch(/not found/i);
+  });
+});
+
+// ─── Admin Role Enforcement ───
+
+describe('Fleet update admin enforcement', () => {
+  afterEach(() => vi.restoreAllMocks());
+
+  let viewerHeader: string;
+
+  beforeAll(async () => {
+    // Create a viewer user and generate a token for them
+    const { DatabaseService } = await import('../services/DatabaseService');
+    const db = DatabaseService.getInstance();
+    const bcrypt = await import('bcrypt');
+    const viewerHash = await bcrypt.hash('viewerpass', 1);
+    try {
+      db.addUser({ username: 'testviewer', password_hash: viewerHash, role: 'viewer' });
+    } catch {
+      // User may already exist from a prior run
+    }
+    const viewerToken = jwt.sign({ username: 'testviewer', role: 'viewer' }, TEST_JWT_SECRET, { expiresIn: '1m' });
+    viewerHeader = `Bearer ${viewerToken}`;
+  });
+
+  it('POST /api/fleet/nodes/1/update returns 403 for viewer', async () => {
+    mockTier('paid');
+    const res = await request(app)
+      .post('/api/fleet/nodes/1/update')
+      .set('Authorization', viewerHeader);
+    expect(res.status).toBe(403);
+    expect(res.body.code).toBe('ADMIN_REQUIRED');
+  });
+
+  it('POST /api/fleet/update-all returns 403 for viewer', async () => {
+    mockTier('paid');
+    const res = await request(app)
+      .post('/api/fleet/update-all')
+      .set('Authorization', viewerHeader);
+    expect(res.status).toBe(403);
+    expect(res.body.code).toBe('ADMIN_REQUIRED');
+  });
 });

 // ─── Snapshot CRUD ───
@@ -1277,6 +1277,8 @@ interface UpdateTracker {
  previousProcessStart: number | null;
  /** True when the node became unreachable at least once during the update window. */
  wasOffline: boolean;
+  /** Timestamp when the tracker transitioned to a terminal state (completed/failed/timeout). */
+  resolvedAt?: number;
 }
 const updateTracker = new Map<number, UpdateTracker>();
 const UPDATE_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes
@@ -1353,12 +1355,16 @@ async function getLatestVersion(forceRefresh = false): Promise<string | null> {
 async function getCompareTarget(gatewayVersion: string | null) {
  const latestVersion = await getLatestVersion();
  const latestValid = latestVersion !== null && isValidVersion(latestVersion);
-  return {
+  const result = {
    latestVersion,
    latestValid,
    compareVersion: latestValid ? latestVersion : gatewayVersion,
    compareValid: latestValid || isValidVersion(gatewayVersion),
  };
+  if (isDebugEnabled()) {
+    console.debug('[Fleet:debug] Compare target resolved:', { gatewayVersion, latestVersion, using: result.compareVersion, valid: result.compareValid });
+  }
+  return result;
 }

 function createTracker(
@@ -1367,7 +1373,16 @@ function createTracker(
  previousProcessStart: number | null,
  error?: string,
 ): UpdateTracker {
-  return { status, startedAt: Date.now(), previousVersion, previousProcessStart, wasOffline: false, error };
+  const now = Date.now();
+  return {
+    status, startedAt: now, previousVersion, previousProcessStart, wasOffline: false, error,
+    resolvedAt: status !== 'updating' ? now : undefined,
+  };
+}
+
+/** Transition a tracker to a terminal state, setting resolvedAt automatically. */
+function resolveTracker(tracker: UpdateTracker, status: 'completed' | 'failed' | 'timeout', error?: string): UpdateTracker {
+  return { ...tracker, status, resolvedAt: Date.now(), error };
 }

 interface FleetNodeOverview {
@@ -1528,6 +1543,7 @@ app.get('/api/fleet/update-status', authMiddleware, async (_req: Request, res: R
    const gatewayValid = isValidVersion(gatewayVersion);

    const { latestVersion, latestValid, compareVersion, compareValid } = await getCompareTarget(gatewayVersion);
+    const debug = isDebugEnabled();

    const results = await Promise.allSettled(
      nodes.map(async (node) => {
@@ -1551,31 +1567,41 @@ app.get('/api/fleet/update-status', authMiddleware, async (_req: Request, res: R
        if (tracker?.status === 'updating') {
          const elapsed = Date.now() - tracker.startedAt;

+          if (debug) {
+            console.debug('[Fleet:debug] Polling update status for node', node.id, node.name, '- elapsed:', Math.round(elapsed / 1000) + 's', 'version:', version, 'wasOffline:', tracker.wasOffline, 'remoteOnline:', remoteOnline);
+          }
+
          if (elapsed > UPDATE_TIMEOUT_MS) {
            // Final timeout (5 min)
-            updateTracker.set(node.id, { ...tracker, status: 'timeout', error: UPDATE_TIMEOUT_MSG });
+            if (debug) console.debug('[Fleet:debug] Node', node.id, 'timed out after', Math.round(elapsed / 1000) + 's');
+            updateTracker.set(node.id, resolveTracker(tracker, 'timeout', UPDATE_TIMEOUT_MSG));
          } else if (node.type === 'remote') {
            if (remoteUpdateError) {
              // Remote reported a pull failure via /api/meta
-              updateTracker.set(node.id, { ...tracker, status: 'failed', error: remoteUpdateError });
+              if (debug) console.debug('[Fleet:debug] Node', node.id, 'reported pull failure:', remoteUpdateError);
+              updateTracker.set(node.id, resolveTracker(tracker, 'failed', remoteUpdateError));
            } else if (!remoteOnline) {
              // Node is unreachable (restarting); record that it went offline
              if (!tracker.wasOffline) {
+                if (debug) console.debug('[Fleet:debug] Node', node.id, 'went offline (restarting)');
                updateTracker.set(node.id, { ...tracker, wasOffline: true });
              }
            } else if (version !== tracker.previousVersion) {
              // Signal 1: Version changed (or version now resolvable after being unknown)
-              updateTracker.set(node.id, { ...tracker, status: 'completed' });
+              if (debug) console.debug('[Fleet:debug] Node', node.id, 'completed via signal 1 (version changed):', tracker.previousVersion, '->', version);
+              updateTracker.set(node.id, resolveTracker(tracker, 'completed'));
            } else if (
              remoteStartedAt !== null &&
              tracker.previousProcessStart !== null &&
              remoteStartedAt !== tracker.previousProcessStart
            ) {
              // Signal 2: Process restarted (startedAt changed)
-              updateTracker.set(node.id, { ...tracker, status: 'completed' });
+              if (debug) console.debug('[Fleet:debug] Node', node.id, 'completed via signal 2 (process restarted):', tracker.previousProcessStart, '->', remoteStartedAt);
+              updateTracker.set(node.id, resolveTracker(tracker, 'completed'));
            } else if (tracker.wasOffline && remoteOnline) {
              // Signal 3: Node went offline and is back online (container was recreated)
-              updateTracker.set(node.id, { ...tracker, status: 'completed' });
+              if (debug) console.debug('[Fleet:debug] Node', node.id, 'completed via signal 3 (offline then online)');
+              updateTracker.set(node.id, resolveTracker(tracker, 'completed'));
            } else if (
              elapsed > 15_000 &&
              isValidVersion(version) &&
@@ -1585,14 +1611,12 @@ app.get('/api/fleet/update-status', authMiddleware, async (_req: Request, res: R
              // Signal 4: Remote is now at or above gateway version (after minimum processing time).
              // Catches fast restarts where the 5s polling interval misses the offline window
              // and startedAt hasn't been observed to change yet.
-              updateTracker.set(node.id, { ...tracker, status: 'completed' });
+              if (debug) console.debug('[Fleet:debug] Node', node.id, 'completed via signal 4 (version >= compare target):', version, '>=', compareVersion);
+              updateTracker.set(node.id, resolveTracker(tracker, 'completed'));
            } else if (elapsed > EARLY_FAIL_MS) {
              // Heuristic: node never went offline and nothing changed after 3 min
-              updateTracker.set(node.id, {
-                ...tracker,
-                status: 'failed',
-                error: 'Update may have failed. The node is still running and its version has not changed.',
-              });
+              if (debug) console.debug('[Fleet:debug] Node', node.id, 'early fail after', Math.round(elapsed / 1000) + 's - no signals detected');
+              updateTracker.set(node.id, resolveTracker(tracker, 'failed', 'Update may have failed. The node is still running and its version has not changed.'));
            }
          } else if (node.type === 'local') {
            // Local node has only two failure signals: an explicit pull/spawn error,
@@ -1602,21 +1626,19 @@ app.get('/api/fleet/update-status', authMiddleware, async (_req: Request, res: R
            const selfUpdate = SelfUpdateService.getInstance();
            const localError = selfUpdate.getLastError();
            if (localError) {
-              updateTracker.set(node.id, { ...tracker, status: 'failed', error: localError });
+              if (debug) console.debug('[Fleet:debug] Local node', node.id, 'update failed:', localError);
+              updateTracker.set(node.id, resolveTracker(tracker, 'failed', localError));
              selfUpdate.clearLastError();
            } else if (elapsed > EARLY_FAIL_MS) {
              // Helper container likely failed silently. Surface failure before the 5 min timeout.
-              updateTracker.set(node.id, {
-                ...tracker,
-                status: 'failed',
-                error: 'Local update did not complete. The container may not have restarted; check Docker logs on the host.',
-              });
+              if (debug) console.debug('[Fleet:debug] Local node', node.id, 'early fail after', Math.round(elapsed / 1000) + 's');
+              updateTracker.set(node.id, resolveTracker(tracker, 'failed', 'Local update did not complete. The container may not have restarted; check Docker logs on the host.'));
            }
          }
        }

-        // Auto-expire completed entries after 60 seconds so nodes return to "Up to date"
-        if (tracker?.status === 'completed' && Date.now() - tracker.startedAt > 60_000) {
+        // Auto-expire completed entries 60s after they resolved so the badge is visible
+        if (tracker?.status === 'completed' && tracker.resolvedAt && Date.now() - tracker.resolvedAt > 60_000) {
          updateTracker.delete(node.id);
        }

@@ -1652,6 +1674,7 @@ app.get('/api/fleet/update-status', authMiddleware, async (_req: Request, res: R
        latestVersion: latestValid ? latestVersion : gatewayVersion,
        updateAvailable: false,
        updateStatus: null,
+        error: null,
      };
    });

@@ -1669,6 +1692,7 @@ app.get('/api/fleet/update-status', authMiddleware, async (_req: Request, res: R
 // Trigger update on a specific node
 app.post('/api/fleet/nodes/:nodeId/update', authMiddleware, async (req: Request, res: Response): Promise<void> => {
  if (!requirePaid(req, res)) return;
+  if (!requireAdmin(req, res)) return;
  try {
    const nodeId = parseInt(req.params.nodeId as string, 10);
    if (isNaN(nodeId)) { res.status(400).json({ error: 'Invalid node ID' }); return; }
@@ -1682,7 +1706,7 @@ app.post('/api/fleet/nodes/:nodeId/update', authMiddleware, async (req: Request,
    const existing = updateTracker.get(nodeId);
    if (existing?.status === 'updating') {
      if (Date.now() - existing.startedAt > UPDATE_TIMEOUT_MS) {
-        updateTracker.set(nodeId, { ...existing, status: 'timeout', error: UPDATE_TIMEOUT_MSG });
+        updateTracker.set(nodeId, resolveTracker(existing, 'timeout', UPDATE_TIMEOUT_MSG));
      } else {
        res.status(409).json({ error: 'Update already in progress for this node.' });
        return;
@@ -1694,6 +1718,9 @@ app.post('/api/fleet/nodes/:nodeId/update', authMiddleware, async (req: Request,
    }

    console.log('[Fleet] Update triggered for node', node.name, node.type);
+    if (isDebugEnabled()) {
+      console.debug('[Fleet:debug] Update trigger details:', { nodeId, name: node.name, type: node.type, hasUrl: !!node.api_url, hasToken: !!node.api_token });
+    }

    if (node.type === 'local') {
      if (!SelfUpdateService.getInstance().isAvailable()) {
@@ -1713,6 +1740,9 @@ app.post('/api/fleet/nodes/:nodeId/update', authMiddleware, async (req: Request,

    // Check remote availability and capabilities
    const meta = await fetchRemoteMeta(node.api_url, node.api_token);
+    if (isDebugEnabled()) {
+      console.debug('[Fleet:debug] Remote meta for update:', { nodeId, online: meta.online, version: meta.version, capabilities: meta.capabilities, startedAt: meta.startedAt });
+    }
    if (!meta.online) {
      res.status(503).json({ error: 'Remote node is unreachable. Verify the node is running and the API URL is correct.' });
      return;
@@ -1756,13 +1786,16 @@ app.post('/api/fleet/nodes/:nodeId/update', authMiddleware, async (req: Request,
 // Trigger update on all outdated nodes
 app.post('/api/fleet/update-all', authMiddleware, async (req: Request, res: Response): Promise<void> => {
  if (!requirePaid(req, res)) return;
+  if (!requireAdmin(req, res)) return;
  try {
    const db = DatabaseService.getInstance();
    const nodes = db.getNodes();
    const gatewayVersion = getSenchoVersion();
    const { compareVersion, compareValid } = await getCompareTarget(gatewayVersion);

+    const debug = isDebugEnabled();
    console.log('[Fleet] Update-all triggered,', nodes.length, 'nodes registered');
+    if (debug) console.debug('[Fleet:debug] Update-all compare target:', { gatewayVersion, compareVersion, compareValid });

    // Filter to eligible candidates, then trigger all in parallel
    const candidates = nodes.filter(node => {
@@ -1802,11 +1835,13 @@ app.post('/api/fleet/update-all', authMiddleware, async (req: Request, res: Resp

    const updating: string[] = [];
    const skipped = nodes.filter(n => !candidates.includes(n)).map(n => n.name);
-    for (const r of results) {
-      const val = r.status === 'fulfilled' ? r.value : { name: 'unknown', triggered: false };
+    for (let i = 0; i < results.length; i++) {
+      const r = results[i];
+      const val = r.status === 'fulfilled' ? r.value : { name: candidates[i].name, triggered: false };
      (val.triggered ? updating : skipped).push(val.name);
    }

+    if (debug) console.debug('[Fleet:debug] Update-all results:', { updating, skippedCount: skipped.length, candidateCount: candidates.length });
    res.status(202).json({ updating, skipped });
  } catch (error) {
    console.error('[Fleet] Update all error:', error);
@@ -3,6 +3,7 @@ import { promisify } from 'util';
 import * as fs from 'fs';
 import DockerController from './DockerController';
 import { disableCapability } from './CapabilityRegistry';
+import { isDebugEnabled } from '../utils/debug';

 const execFileAsync = promisify(execFile);

@@ -148,12 +149,16 @@ class SelfUpdateService {

    // Async pull: a sync execFileSync blocks the event loop, which lets the frontend
    // overlay see a false "online" response between the pull finishing and the restart.
+    const debug = isDebugEnabled();
+    const pullStart = Date.now();
    console.log(`[SelfUpdate] Pulling latest image: ${imageName}...`);
+    if (debug) console.debug('[SelfUpdate:debug] Pull context:', { workingDir, configFiles, serviceName, dataDirHost, mountCount: hostBindMounts.length });
    try {
      await execFileAsync('docker', ['pull', imageName], {
        env,
        timeout: 300_000, // 5 min max for pull
      });
+      if (debug) console.debug('[SelfUpdate:debug] Pull completed in', Math.round((Date.now() - pullStart) / 1000) + 's');
    } catch (error) {
      const stderr = (error as { stderr?: Buffer | string })?.stderr?.toString().trim();
      this.lastUpdateError = stderr || (error as Error).message;
@@ -147,10 +147,14 @@ The modal shows:
 - **Recheck** button to refresh the latest version from GitHub and re-scan for available updates
 - **Update All** button to trigger updates on all remote nodes that have a pending update

-When you click **Update** on a remote node, Sencho sends the update command to the remote instance. The remote pulls the latest Docker image, then spawns a short-lived helper container that performs the compose recreate. The node restarts with the new version, and the status badge transitions from "Updating" to "Updated" once the gateway detects the version change.
+When you click **Update** on a remote node, Sencho sends the update command to the remote instance. The remote pulls the latest Docker image, then spawns a short-lived helper container that performs the compose recreate. The node restarts with the new version, and the status badge transitions from "Updating" to "Updated" once the gateway detects the version change. The "Updated" badge remains visible for 60 seconds before the node returns to "Up to date".

 If you update the local node, a confirmation dialog appears first, then a reconnection overlay shows while your primary instance restarts.

+<Note>
+  Triggering updates (both individual and bulk) requires admin privileges. Users with viewer or operator roles can see update status but cannot initiate updates.
+</Note>
+
 **How self-update works:** Each Sencho instance reads its own Docker Compose labels to determine the image name, compose file path, and service name. It pulls the latest image directly, then spawns a helper container that mounts the compose directory from the host and runs `docker compose up --force-recreate`. This approach works regardless of the container's own volume mounts.

 ---
@@ -205,3 +209,7 @@ This means the gateway could not connect to the remote node's `/api/meta` endpoi
 - The API URL configured for this node is correct and reachable from the gateway
 - The network allows traffic between the gateway and remote node on the configured port
 - The remote node's Sencho container is healthy (`docker ps` should show it as running)
+
+### Update button returns "Admin access required"
+
+Fleet update operations (individual updates, Update All) require an admin account. If you see a 403 error when clicking **Update**, ask your Sencho administrator to either grant you admin privileges or perform the update on your behalf.
@@ -1398,9 +1398,14 @@ export function FleetView({ onNavigateToNode }: FleetViewProps) {
                                        disabled={recheckingUpdates}
                                        onClick={async () => {
                                            setRecheckingUpdates(true);
-                                            await apiFetch('/fleet/update-status?recheck=true', { method: 'DELETE', localOnly: true });
-                                            await fetchUpdateStatus();
-                                            setRecheckingUpdates(false);
+                                            try {
+                                                await apiFetch('/fleet/update-status?recheck=true', { method: 'DELETE', localOnly: true });
+                                                await fetchUpdateStatus();
+                                            } catch (err) {
+                                                console.warn('[Fleet] Recheck failed:', err);
+                                            } finally {
+                                                setRecheckingUpdates(false);
+                                            }
                                        }}
                                    >
                                        <RefreshCw className={`w-3 h-3 mr-1.5 ${recheckingUpdates ? 'animate-spin' : ''}`} strokeWidth={1.5} />
@@ -1,6 +1,6 @@
-/** Returns true when the string is a displayable version (not a placeholder or missing). */
+/** Returns true when the string is a displayable semver version (not a placeholder or missing). */
 export function isValidVersion(v: string | null | undefined): v is string {
-  return !!v && v !== 'unknown' && v !== '0.0.0-dev';
+  return !!v && v !== 'unknown' && v !== '0.0.0-dev' && /^\d+\.\d+\.\d+(-[\w.]+)?$/.test(v);
 }

 /** Format a version string for display, returning null for invalid/missing values. */