From 05c483f2133e06d0c984e110d12766911b0a3c54 Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 28 Jun 2026 08:13:43 -0400 Subject: [PATCH] fix: harden cross-node fleet label actions and guard container reads (#1503) * fix: harden cross-node fleet label actions and guard container reads Release-stabilization fixes for the Fleet Actions surface: - Stop-by-label binds execution to the nodes shown in the confirmed preview. The real stop sends the confirmed node ids and the backend restricts the fan-out to them, so a node that was unreachable during preview and reconnects before the stop can no longer enter execution and have unlisted stacks stopped. - Bulk label assign validates each remote node's result against the stacks it was asked to label: a body whose results are empty, partial, duplicated, or shaped wrong is a per-node failure instead of reading as a successful zero-stack assign. The card mirrors this, rejecting a missing or non-array results body and only reporting success when at least one stack was assigned. - Bulk label assign re-reads authoritative per-node stacks and labels on demand via a Refresh control, and the confirmation lists the affected node and stack names rather than bare counts. - The stack-specific and fleet container/stack read routes require the stack:read permission, matching the generic container and stack routes. Every shipped role already carries stack:read, so reachability is unchanged; the guard closes the routes that were auth-only. Adds unit coverage for the assign-result validator, route coverage for the stop allowlist and assign membership checks, and authorization coverage for the newly guarded reads. * test: assert the confirmed node allowlist in the fleet stop-card test The stop-card component test pinned the real-stop request body to { labelName, dryRun } and broke once the stop began carrying the confirmed-preview node ids. Update it to expect the nodeIds allowlist derived from the resolved preview, so the test asserts the binding rather than the pre-fix shape. --- .../fleet-action-card-endpoints.test.ts | 87 +++++++++++++ backend/src/__tests__/fleet-actions.test.ts | 48 +++++++ .../fleet-label-assign-validate.test.ts | 92 ++++++++++++++ .../fleet-stack-container-authz.test.ts | 118 ++++++++++++++++++ backend/src/helpers/fleetLabelAssign.ts | 42 +++++++ backend/src/routes/fleet.ts | 45 +++++-- backend/src/routes/stacks.ts | 1 + .../cards/BulkLabelAssignCard.tsx | 82 ++++++++++-- .../cards/LabelFleetStopCard.test.tsx | 4 +- .../FleetActions/cards/LabelFleetStopCard.tsx | 22 ++-- 10 files changed, 510 insertions(+), 31 deletions(-) create mode 100644 backend/src/__tests__/fleet-label-assign-validate.test.ts create mode 100644 backend/src/__tests__/fleet-stack-container-authz.test.ts diff --git a/backend/src/__tests__/fleet-action-card-endpoints.test.ts b/backend/src/__tests__/fleet-action-card-endpoints.test.ts index 45198341..c5021038 100644 --- a/backend/src/__tests__/fleet-action-card-endpoints.test.ts +++ b/backend/src/__tests__/fleet-action-card-endpoints.test.ts @@ -757,6 +757,93 @@ describe('POST /api/fleet/labels/fleet-stop remote leg', () => { }); }); +describe('POST /api/fleet/labels/fleet-stop nodeIds allowlist', () => { + // Guards the wrong-node drift fix: the real stop carries the node ids the + // operator confirmed in the preview, so a node that was unreachable then and + // reconnects before execution cannot silently enter the fan-out. + it('contacts a confirmed remote in the allowlist and excludes the unconfirmed local node', async () => { + const remoteId = db.addNode({ + name: 'confirmed-remote', type: 'remote', api_url: 'http://confirmed.example:1852', + api_token: 'tok', compose_dir: '/app/compose', is_default: false, + }); + try { + const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue({ + ok: true, status: 200, json: async () => ({ matched: true, results: [{ stackName: 'alpha', success: true }] }), + } as unknown as Response); + const res = await request(app) + .post('/api/fleet/labels/fleet-stop') + .set('Authorization', authHeader) + .send({ labelName: 'any-label', nodeIds: [remoteId] }); + expect(res.status).toBe(200); + // Only the confirmed remote is in the fan-out: its local-stop receiver is + // contacted and the unconfirmed local node is absent from the results. + expect(res.body.results).toHaveLength(1); + expect(res.body.results[0].nodeId).toBe(remoteId); + const urls = fetchSpy.mock.calls.map(c => String(c[0])); + expect(urls.some(u => u.endsWith('/api/fleet-actions/labels/local-stop'))).toBe(true); + } finally { + db.deleteNode(remoteId); + } + }); + + it('excludes an otherwise-reachable remote that is not in the allowlist', async () => { + const label = await createAssignedLabel('confirmed-only', ['alpha']); + const localId = label.node_id; + const remoteId = db.addNode({ + name: 'excluded-remote', type: 'remote', api_url: 'http://excluded.example:1852', + api_token: 'tok', compose_dir: '/app/compose', is_default: false, + }); + try { + const fetchSpy = vi.spyOn(globalThis, 'fetch'); + const res = await request(app) + .post('/api/fleet/labels/fleet-stop') + .set('Authorization', authHeader) + .send({ labelName: label.name, nodeIds: [localId] }); + expect(res.status).toBe(200); + // The excluded remote has a proxy target and would otherwise be contacted; + // the allowlist keeps it out of execution entirely. + expect(res.body.results).toHaveLength(1); + expect(res.body.results[0].nodeId).toBe(localId); + expect(res.body.results.some((r: { nodeId: number }) => r.nodeId === remoteId)).toBe(false); + expect(fetchSpy).not.toHaveBeenCalled(); + } finally { + db.deleteNode(remoteId); + } + }); + + it('treats an empty allowlist as zero target nodes, stopping nothing', async () => { + const label = await createAssignedLabel('empty-allow', ['alpha']); + const fetchSpy = vi.spyOn(globalThis, 'fetch'); + const res = await request(app) + .post('/api/fleet/labels/fleet-stop') + .set('Authorization', authHeader) + .send({ labelName: label.name, nodeIds: [] }); + expect(res.status).toBe(200); + // An empty allowlist filters to no nodes: a fail-safe no-op rather than a + // full-fleet stop. The local node is not acted on and no remote is contacted. + expect(res.body.results).toEqual([]); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('rejects a non-array nodeIds with 400', async () => { + const res = await request(app) + .post('/api/fleet/labels/fleet-stop') + .set('Authorization', authHeader) + .send({ labelName: 'whatever', nodeIds: 'oops' }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/nodeIds/); + }); + + it('rejects a non-integer nodeIds entry with 400', async () => { + const res = await request(app) + .post('/api/fleet/labels/fleet-stop') + .set('Authorization', authHeader) + .send({ labelName: 'whatever', nodeIds: [1, 2.5] }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/nodeIds/); + }); +}); + describe('POST /api/fleet/labels/fleet-stop with dryRun: true', () => { it('marks each stack dryRun: true and does not invoke containerActionForStack', async () => { const label = await createAssignedLabel('dry-stop', ['alpha', 'beta']); diff --git a/backend/src/__tests__/fleet-actions.test.ts b/backend/src/__tests__/fleet-actions.test.ts index 27d01ed6..25cf58c6 100644 --- a/backend/src/__tests__/fleet-actions.test.ts +++ b/backend/src/__tests__/fleet-actions.test.ts @@ -640,6 +640,54 @@ describe('bulk-assign orchestrator: remote fan-out', () => { expect(row.error).toMatch(/malformed/); expect(row.stackResults).toEqual([{ stackName: 'r1', success: false, error: 'Remote returned a malformed response' }]); }); + + it('fails a node whose 200 body returns an empty results array for a non-empty request', async () => { + const remoteId = addRemote('assign-remote-empty'); + vi.spyOn(NodeRegistry.getInstance(), 'getProxyTarget').mockReturnValue({ apiUrl: 'https://remote.example.com:1852', apiToken: 'remote-tok' }); + // A well-shaped { created, results } body whose results are empty used to + // pass the bare Array.isArray check and read as a successful zero-stack + // assign. Membership validation must fail the node instead. + vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response( + JSON.stringify({ created: true, results: [] }), + { status: 200, headers: { 'content-type': 'application/json' } }, + )); + + const res = await request(app) + .post('/api/fleet/labels/bulk-assign') + .set('Authorization', authHeader) + .send({ label: { name: 'media', color: 'teal' }, targets: [{ nodeId: remoteId, stackNames: ['r1'] }] }); + + expect(res.status).toBe(200); + const row = res.body.results.find((r: { nodeId: number }) => r.nodeId === remoteId); + expect(row.reachable).toBe(false); + expect(row.error).toMatch(/malformed/); + expect(row.stackResults).toEqual([{ stackName: 'r1', success: false, error: 'Remote returned a malformed response' }]); + }); + + it('fails a node whose results omit one of the requested stacks', async () => { + const remoteId = addRemote('assign-remote-partial'); + vi.spyOn(NodeRegistry.getInstance(), 'getProxyTarget').mockReturnValue({ apiUrl: 'https://remote.example.com:1852', apiToken: 'remote-tok' }); + // Two stacks requested, only one row returned: a partial body the control + // must not accept as a clean assign of the covered stack alone. + vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response( + JSON.stringify({ created: true, results: [{ stackName: 'r1', success: true }] }), + { status: 200, headers: { 'content-type': 'application/json' } }, + )); + + const res = await request(app) + .post('/api/fleet/labels/bulk-assign') + .set('Authorization', authHeader) + .send({ label: { name: 'media', color: 'teal' }, targets: [{ nodeId: remoteId, stackNames: ['r1', 'r2'] }] }); + + expect(res.status).toBe(200); + const row = res.body.results.find((r: { nodeId: number }) => r.nodeId === remoteId); + expect(row.reachable).toBe(false); + expect(row.error).toMatch(/malformed/); + expect(row.stackResults).toEqual([ + { stackName: 'r1', success: false, error: 'Remote returned a malformed response' }, + { stackName: 'r2', success: false, error: 'Remote returned a malformed response' }, + ]); + }); }); describe('fleet-stop degrades the local leg per-node instead of failing the whole fan-out', () => { diff --git a/backend/src/__tests__/fleet-label-assign-validate.test.ts b/backend/src/__tests__/fleet-label-assign-validate.test.ts new file mode 100644 index 00000000..7bafaa5d --- /dev/null +++ b/backend/src/__tests__/fleet-label-assign-validate.test.ts @@ -0,0 +1,92 @@ +/** + * Unit tests for validateRemoteAssignResults: the membership + shape guard the + * bulk-assign orchestrator applies to a remote node's local-assign 200 body. + * The receiver returns exactly one result row per unique requested stack, so a + * body that drops, duplicates, or adds rows is a contract failure the control + * must not read as a successful (possibly zero-stack) assign. + */ +import { describe, it, expect } from 'vitest'; +import { validateRemoteAssignResults } from '../helpers/fleetLabelAssign'; + +describe('validateRemoteAssignResults', () => { + it('accepts a body whose results cover exactly the requested stacks', () => { + const out = validateRemoteAssignResults(['a', 'b'], { + created: true, + results: [ + { stackName: 'a', success: true }, + { stackName: 'b', success: false, error: 'Stack not found' }, + ], + }); + expect(out).toEqual({ + ok: true, + created: true, + results: [ + { stackName: 'a', success: true }, + { stackName: 'b', success: false, error: 'Stack not found' }, + ], + }); + }); + + it('dedupes the requested set so a duplicated request still validates one row each', () => { + const out = validateRemoteAssignResults(['a', 'a'], { + created: false, + results: [{ stackName: 'a', success: true }], + }); + expect(out).toEqual({ ok: true, created: false, results: [{ stackName: 'a', success: true }] }); + }); + + it('rejects an empty results array for a non-empty request (the false-success case)', () => { + expect(validateRemoteAssignResults(['a'], { created: true, results: [] })).toEqual({ ok: false }); + }); + + it('rejects a body that omits a requested stack', () => { + expect( + validateRemoteAssignResults(['a', 'b'], { created: true, results: [{ stackName: 'a', success: true }] }), + ).toEqual({ ok: false }); + }); + + it('rejects a result for a stack that was never requested', () => { + expect( + validateRemoteAssignResults(['a'], { + created: true, + results: [{ stackName: 'a', success: true }, { stackName: 'rogue', success: true }], + }), + ).toEqual({ ok: false }); + }); + + it('rejects a duplicated result row for the same stack', () => { + expect( + validateRemoteAssignResults(['a'], { + created: true, + results: [{ stackName: 'a', success: true }, { stackName: 'a', success: false }], + }), + ).toEqual({ ok: false }); + }); + + it('rejects a malformed result row (missing success)', () => { + expect( + validateRemoteAssignResults(['a'], { created: true, results: [{ stackName: 'a' }] }), + ).toEqual({ ok: false }); + }); + + it('rejects a result row with a non-string error', () => { + expect( + validateRemoteAssignResults(['a'], { created: true, results: [{ stackName: 'a', success: false, error: 5 }] }), + ).toEqual({ ok: false }); + }); + + it('rejects a non-boolean created', () => { + expect( + validateRemoteAssignResults(['a'], { created: 'yes', results: [{ stackName: 'a', success: true }] }), + ).toEqual({ ok: false }); + }); + + it('rejects a non-array results', () => { + expect(validateRemoteAssignResults(['a'], { created: true, results: 'nope' })).toEqual({ ok: false }); + }); + + it('rejects a null or non-object body', () => { + expect(validateRemoteAssignResults(['a'], null)).toEqual({ ok: false }); + expect(validateRemoteAssignResults(['a'], 'string')).toEqual({ ok: false }); + }); +}); diff --git a/backend/src/__tests__/fleet-stack-container-authz.test.ts b/backend/src/__tests__/fleet-stack-container-authz.test.ts new file mode 100644 index 00000000..f6c76e83 --- /dev/null +++ b/backend/src/__tests__/fleet-stack-container-authz.test.ts @@ -0,0 +1,118 @@ +/** + * Authorization tests for the stack-specific and fleet container/stack read + * routes that were previously auth-only (no permission check): + * - GET /api/stacks/:stackName/containers + * - GET /api/fleet/node/:nodeId/stacks + * - GET /api/fleet/node/:nodeId/stacks/:stackName/containers + * + * They are now gated by `requirePermission('stack:read')`, the same read model + * the generic container/port routes and the rest of the stacks router use. + * Every shipped role carries `stack:read`, so the denial path is exercised by + * temporarily removing it from a role at runtime. + */ +import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach, vi } from 'vitest'; +import request from 'supertest'; +import jwt from 'jsonwebtoken'; +import bcrypt from 'bcrypt'; +import { setupTestDb, cleanupTestDb, TEST_JWT_SECRET } from './helpers/setupTestDb'; + +let tmpDir: string; +let app: import('express').Express; +let DatabaseService: typeof import('../services/DatabaseService').DatabaseService; +let DockerController: typeof import('../services/DockerController').default; +let FileSystemService: typeof import('../services/FileSystemService').FileSystemService; +let ROLE_PERMISSIONS: typeof import('../middleware/permissions').ROLE_PERMISSIONS; + +const VIEWER = 'fleet-read-viewer'; +let READ_PATHS: string[]; + +function viewerToken(): string { + const user = DatabaseService.getInstance().getUserByUsername(VIEWER)!; + return jwt.sign({ username: VIEWER, role: 'viewer', tv: user.token_version }, TEST_JWT_SECRET, { expiresIn: '1m' }); +} + +/** Stub the Docker/FS singletons so the admitted path resolves without a daemon. */ +function stubDockerAndFs(): { docker: ReturnType; fs: ReturnType } { + const docker = vi.spyOn(DockerController, 'getInstance').mockReturnValue({ + getContainersByStack: vi.fn().mockResolvedValue([]), + } as unknown as ReturnType); + const fs = vi.spyOn(FileSystemService, 'getInstance').mockReturnValue({ + getStacks: vi.fn().mockResolvedValue([]), + } as unknown as ReturnType); + return { docker, fs }; +} + +beforeAll(async () => { + tmpDir = await setupTestDb(); + ({ DatabaseService } = await import('../services/DatabaseService')); + ({ default: DockerController } = await import('../services/DockerController')); + ({ FileSystemService } = await import('../services/FileSystemService')); + ({ ROLE_PERMISSIONS } = await import('../middleware/permissions')); + ({ app } = await import('../index')); + + const hash = await bcrypt.hash('password123', 1); + DatabaseService.getInstance().addUser({ username: VIEWER, password_hash: hash, role: 'viewer' }); + const localId = DatabaseService.getInstance().getNodes().find(n => n.is_default)!.id; + READ_PATHS = [ + '/api/stacks/alpha/containers', + `/api/fleet/node/${localId}/stacks`, + `/api/fleet/node/${localId}/stacks/alpha/containers`, + ]; +}); + +afterAll(() => { + cleanupTestDb(tmpDir); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('stack/fleet container reads deny a role without stack:read', () => { + let originalViewerPerms: typeof ROLE_PERMISSIONS.viewer; + let docker: ReturnType; + let fs: ReturnType; + + beforeEach(() => { + originalViewerPerms = ROLE_PERMISSIONS.viewer; + ROLE_PERMISSIONS.viewer = originalViewerPerms.filter((p) => p !== 'stack:read'); + ({ docker, fs } = stubDockerAndFs()); + }); + + afterEach(() => { + ROLE_PERMISSIONS.viewer = originalViewerPerms; + }); + + it('rejects each read before any Docker/FS work', async () => { + for (const path of READ_PATHS) { + const res = await request(app).get(path).set('Authorization', `Bearer ${viewerToken()}`); + expect(res.status, `denied ${path}`).toBe(403); + expect(res.body.code).toBe('PERMISSION_DENIED'); + } + // The guard short-circuits before the handler instantiates the controller. + expect(docker).not.toHaveBeenCalled(); + expect(fs).not.toHaveBeenCalled(); + }); +}); + +describe('stack/fleet container reads admit a role with stack:read', () => { + beforeEach(() => { + stubDockerAndFs(); + }); + + it('admits each read', async () => { + for (const path of READ_PATHS) { + const res = await request(app).get(path).set('Authorization', `Bearer ${viewerToken()}`); + expect(res.status, `admitted ${path}`).toBe(200); + } + }); +}); + +describe('stack/fleet container reads reject unauthenticated requests', () => { + it('returns 401 without a token', async () => { + for (const path of READ_PATHS) { + const res = await request(app).get(path); + expect(res.status, `unauth ${path}`).toBe(401); + } + }); +}); diff --git a/backend/src/helpers/fleetLabelAssign.ts b/backend/src/helpers/fleetLabelAssign.ts index c7c63286..7fd5023e 100644 --- a/backend/src/helpers/fleetLabelAssign.ts +++ b/backend/src/helpers/fleetLabelAssign.ts @@ -51,6 +51,48 @@ export function failAllAssign(stackNames: string[], error: string): LabelAssignR return Array.from(new Set(stackNames)).map(stackName => ({ stackName, success: false, error })); } +function isLabelAssignResult(value: unknown): value is LabelAssignResult { + if (typeof value !== 'object' || value === null) return false; + const r = value as Record; + return typeof r.stackName === 'string' + && typeof r.success === 'boolean' + && (r.error === undefined || typeof r.error === 'string'); +} + +/** + * Validate a remote node's `local-assign` 200 body before the control trusts it. + * + * Beyond the `{ created: boolean, results: LabelAssignResult[] }` shape, this + * checks result *membership*: the receiver returns exactly one row per unique + * requested stack, so a body that drops rows (an empty `results` for a non-empty + * request), duplicates a stack, or returns a stack that was never requested is a + * remote contract failure, not a clean assign. Without this, an empty `results` + * passes the bare `Array.isArray` check and the control reports the node as a + * successful zero-stack assign, which the UI then renders as success. + * + * `requestedStacks` is the per-node target list the control sent; it is deduped + * here so the caller does not have to. + */ +export function validateRemoteAssignResults( + requestedStacks: string[], + body: unknown, +): { ok: true; created: boolean; results: LabelAssignResult[] } | { ok: false } { + if (!body || typeof body !== 'object') return { ok: false }; + const b = body as Record; + if (typeof b.created !== 'boolean' || !Array.isArray(b.results)) return { ok: false }; + const requested = new Set(requestedStacks); + const seen = new Set(); + const results: LabelAssignResult[] = []; + for (const row of b.results) { + if (!isLabelAssignResult(row)) return { ok: false }; + if (!requested.has(row.stackName) || seen.has(row.stackName)) return { ok: false }; + seen.add(row.stackName); + results.push(row); + } + if (seen.size !== requested.size) return { ok: false }; + return { ok: true, created: b.created, results }; +} + /** * Validate a label template (the name/color a cross-node assign propagates). * Mirrors the create-label rules in `routes/labels.ts` and is the single diff --git a/backend/src/routes/fleet.ts b/backend/src/routes/fleet.ts index 4fecaffb..eaf2ee26 100644 --- a/backend/src/routes/fleet.ts +++ b/backend/src/routes/fleet.ts @@ -17,6 +17,7 @@ import SelfUpdateService from '../services/SelfUpdateService'; import { getSenchoVersion, isValidVersion } from '../services/CapabilityRegistry'; import { authMiddleware } from '../middleware/auth'; import { requirePaid, requireAdmin, requireNodeProxy } from '../middleware/tierGates'; +import { requirePermission } from '../middleware/permissions'; import { scheduleLocalUpdate } from './license'; import { runPolicyGate, assertPolicyGateAllows, buildPolicyGateOptions } from '../helpers/policyGate'; import { captureLocalNodeFiles, captureRemoteNodeFiles, buildSnapshotDocumentation, pickDossierFields, dossierHasContent, type SnapshotNodeData, type SnapshotDocumentation } from '../utils/snapshot-capture'; @@ -40,7 +41,7 @@ import { invalidateNodeCaches, invalidateRemoteMetaCache } from '../helpers/cach import { activeBulkActions } from './labels'; import { runLocalLabelStop, isLabelLocalStopResponse, type StackStopResult } from '../helpers/fleetLabelStop'; import { collectFleetLabelSummaries } from '../helpers/fleetLabelSummary'; -import { runLocalLabelAssign, validateLabelTemplate, failAllAssign, type LabelLocalAssignResponse, type AssignNodeResult } from '../helpers/fleetLabelAssign'; +import { runLocalLabelAssign, validateLabelTemplate, validateRemoteAssignResults, failAllAssign, type AssignNodeResult } from '../helpers/fleetLabelAssign'; import { MAX_ASSIGNMENTS } from '../helpers/constants'; import { buildLocalConfigurationStatus, type ConfigurationStatus } from './dashboard'; import { buildLocalGraph, mergeFleetGraph, isLocalDependencyGraph, type FleetNodeGraphResult } from '../services/DependencyGraphService'; @@ -791,7 +792,12 @@ fleetRouter.get('/networking-summary', authMiddleware, async (_req: Request, res } }); +// Read guard uses the unscoped stack:read (no resource): a fleet node view is a +// cross-node aggregate with no single control-DB stack to scope a per-stack +// assignment against, so it requires the global stack:read every shipped role +// holds. The per-stack scoped form is correct only on the local stacks router. fleetRouter.get('/node/:nodeId/stacks', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requirePermission(req, res, 'stack:read')) return; try { const nodeId = parseIntParam(req, res, 'nodeId', 'node ID'); if (nodeId === null) return; @@ -830,7 +836,10 @@ fleetRouter.get('/node/:nodeId/stacks', authMiddleware, async (req: Request, res } }); +// Unscoped stack:read for the same reason as /node/:nodeId/stacks above: a +// fleet-routed read has no local control-DB stack resource to scope against. fleetRouter.get('/node/:nodeId/stacks/:stackName/containers', authMiddleware, async (req: Request, res: Response): Promise => { + if (!requirePermission(req, res, 'stack:read')) return; try { const nodeId = parseIntParam(req, res, 'nodeId', 'node ID'); if (nodeId === null) return; @@ -1370,23 +1379,37 @@ type FleetStopNodeResult = { // Tier: requireAdmin (admin-only fleet plumbing; available on every license). fleetRouter.post('/labels/fleet-stop', authMiddleware, async (req: Request, res: Response): Promise => { if (!requireAdmin(req, res)) return; - const body = req.body as { labelName?: unknown; dryRun?: unknown } | undefined; + const body = req.body as { labelName?: unknown; dryRun?: unknown; nodeIds?: unknown } | undefined; if (!body || typeof body !== 'object') { res.status(400).json({ error: 'Request body is required' }); return; } - const { labelName, dryRun } = body; + const { labelName, dryRun, nodeIds } = body; if (typeof labelName !== 'string' || labelName.trim().length === 0) { res.status(400).json({ error: 'labelName is required' }); return; } + // Optional allowlist binding execution to the preview the operator confirmed. + // The confirm flow sends exactly the nodes shown in the resolved blast radius, + // so a node that was unreachable during preview and reconnects before the stop + // cannot silently enter execution and have unlisted stacks stopped. Absent + // (e.g. a dry run) the fan-out scans the whole fleet as before. + let allowedNodeIds: Set | null = null; + if (nodeIds !== undefined) { + if (!Array.isArray(nodeIds) || !nodeIds.every(n => typeof n === 'number' && Number.isInteger(n))) { + res.status(400).json({ error: 'nodeIds must be an array of integers' }); + return; + } + allowedNodeIds = new Set(nodeIds as number[]); + } const trimmed = labelName.trim(); const isDryRun = dryRun === true; try { const db = DatabaseService.getInstance(); const nodes = db.getNodes(); - if (isDebugEnabled()) console.debug('[Fleet:debug] fleet-stop:', { labelName: trimmed, dryRun: isDryRun, nodes: nodes.length }); - const results = await Promise.all(nodes.map(async (node): Promise => { + const targetNodes = allowedNodeIds ? nodes.filter(n => allowedNodeIds.has(n.id)) : nodes; + if (isDebugEnabled()) console.debug('[Fleet:debug] fleet-stop:', { labelName: trimmed, dryRun: isDryRun, nodes: targetNodes.length, scoped: allowedNodeIds !== null }); + const results = await Promise.all(targetNodes.map(async (node): Promise => { if (node.type === 'local') { // Match + stop runs in-process against the control's own Docker. The // helper shares the per-node `bulk:` lock with the per-label action @@ -1562,11 +1585,13 @@ fleetRouter.post('/labels/bulk-assign', authMiddleware, async (req: Request, res const message = err.error || `Remote returned ${response.status}`; return { nodeId: node.id, nodeName: node.name, reachable: false, created: false, error: message, stackResults: failAllAssign(target.stackNames, message) }; } - // A 200 whose body is not the expected { created, results } shape is a - // degraded node, not a clean no-op: report it as a per-node failure so a - // malformed remote cannot read as a successful zero-stack assign. - const remote = (await response.json().catch(() => null)) as Partial | null; - if (!remote || typeof remote.created !== 'boolean' || !Array.isArray(remote.results)) { + // A 200 whose body is not the expected { created, results } shape, or + // whose results do not cover exactly the stacks this node was asked to + // label, is a degraded node, not a clean no-op: report it as a per-node + // failure so a malformed or partial remote cannot read as a successful + // zero-stack assign. + const remote = validateRemoteAssignResults(target.stackNames, await response.json().catch(() => null)); + if (!remote.ok) { const message = 'Remote returned a malformed response'; return { nodeId: node.id, nodeName: node.name, reachable: false, created: false, error: message, stackResults: failAllAssign(target.stackNames, message) }; } diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts index c6455e35..b853070f 100644 --- a/backend/src/routes/stacks.ts +++ b/backend/src/routes/stacks.ts @@ -1102,6 +1102,7 @@ stacksRouter.get('/:stackName/containers', async (req: Request, res: Response) = res.status(400).json({ error: 'Invalid stack name' }); return; } + if (!requirePermission(req, res, 'stack:read', 'stack', stackName)) return; try { const dockerController = DockerController.getInstance(req.nodeId); const containers = await dockerController.getContainersByStack(stackName); diff --git a/frontend/src/components/fleet/FleetActions/cards/BulkLabelAssignCard.tsx b/frontend/src/components/fleet/FleetActions/cards/BulkLabelAssignCard.tsx index 562c03f2..408249c8 100644 --- a/frontend/src/components/fleet/FleetActions/cards/BulkLabelAssignCard.tsx +++ b/frontend/src/components/fleet/FleetActions/cards/BulkLabelAssignCard.tsx @@ -42,6 +42,11 @@ interface Props { nodes: FleetNode[]; } +// One node's slice of the pending assignment: the stacks selected on it and +// whether the label will be created there. Shared by the preview builder and the +// confirm-modal list so the two cannot drift. +type PreviewNode = { nodeId: number; nodeName: string; willCreate: boolean; stacks: string[] }; + const KICKER = 'font-mono text-[10px] uppercase tracking-[0.18em]'; export function BulkLabelAssignCard({ nodes }: Props) { @@ -55,14 +60,21 @@ export function BulkLabelAssignCard({ nodes }: Props) { const [running, setRunning] = useState(false); const [results, setResults] = useState([]); - // Re-fetch only when the set of node ids changes, not on every parent render - // (the nodes array is a fresh reference each render, e.g. on each fleet poll). // The latest nodes are read through a ref so the load effect can rebuild - // per-node state without taking the array as a reactive dependency. + // per-node state without taking the array as a reactive dependency (it is a + // fresh reference each parent render, e.g. on each fleet poll). const nodesRef = useRef(nodes); useEffect(() => { nodesRef.current = nodes; }); const nodeIds = nodes.map(n => n.id).join(','); + // Bumped by the manual Refresh so the snapshot is never trusted indefinitely: + // a remote that recovers, or a label added/removed after the card opened, is + // re-read on demand rather than confirmed against stale state. + const [refreshKey, setRefreshKey] = useState(0); + + // Reload when the node set changes or the operator refreshes (not on every + // parent render). Resets the selection because a refreshed fleet can no longer + // guarantee the previously picked stacks still exist. useEffect(() => { let cancelled = false; async function load() { @@ -94,7 +106,7 @@ export function BulkLabelAssignCard({ nodes }: Props) { } load(); return () => { cancelled = true; }; - }, [nodeIds]); + }, [nodeIds, refreshKey]); // Distinct label templates across the fleet (name + deterministic color). const templates = useMemo(() => { @@ -186,7 +198,15 @@ export function BulkLabelAssignCard({ nodes }: Props) { toast.error(body.error || 'Bulk label assign failed'); return; } - const apiResults = (body.results as AssignNodeResult[]) ?? []; + // A 200 with a missing or non-array `results` is a server/contract bug, + // not an empty assign: don't let it fall through to the success path with + // zero counts. Log it and tell the operator it was unexpected. + if (!Array.isArray(body.results)) { + console.error('[BulkLabelAssign] bulk-assign returned a malformed body', body); + toast.error('Bulk label assign returned an unexpected response. Check the server logs and retry.'); + return; + } + const apiResults = body.results as AssignNodeResult[]; const rows: ResultRow[] = apiResults.map((node) => { const unreachable = node.reachable === false; const ok = node.stackResults.filter(s => s.success).length; @@ -210,7 +230,13 @@ export function BulkLabelAssignCard({ nodes }: Props) { const ok = allStacks.filter(s => s.success).length; const failed = allStacks.length - ok; const unreachableCount = apiResults.filter(n => n.reachable === false).length; - if (failed === 0 && unreachableCount === 0) toast.success(`Assigned "${selectedTemplate.name}" to ${ok} stack${ok === 1 ? '' : 's'} across ${apiResults.length} node${apiResults.length === 1 ? '' : 's'}.`); + if (ok > 0 && failed === 0 && unreachableCount === 0) toast.success(`Assigned "${selectedTemplate.name}" to ${ok} stack${ok === 1 ? '' : 's'} across ${apiResults.length} node${apiResults.length === 1 ? '' : 's'}.`); + else if (ok === 0 && failed === 0 && unreachableCount === 0) { + // Every node reported zero stack results for a non-empty request: a + // contract break the success path above must not absorb. + console.error('[BulkLabelAssign] bulk-assign returned no stack results', apiResults); + toast.error('Bulk label assign returned no results. Check the server logs and retry.'); + } else toast.warning(`${ok} assigned, ${failed} failed${unreachableCount > 0 ? `, ${unreachableCount} node${unreachableCount === 1 ? '' : 's'} unreachable` : ''}. See results below.`); } catch (err) { toast.dismiss(toastId); @@ -236,7 +262,7 @@ export function BulkLabelAssignCard({ nodes }: Props) { const willCreate = !entry.labels.some(l => l.name === selectedTemplate.name); return { nodeId: entry.node.id, nodeName: entry.node.name, willCreate, stacks }; }) - .filter((n): n is { nodeId: number; nodeName: string; willCreate: boolean; stacks: string[] } => n !== null); + .filter((n): n is PreviewNode => n !== null); }, [nodeData, selected, selectedTemplate]); return ( @@ -264,9 +290,20 @@ export function BulkLabelAssignCard({ nodes }: Props) { title="Label · source" meta={loading ? 'loading…' : `${templates.length} label${templates.length === 1 ? '' : 's'}`} > -

- Pick a stack label from anywhere in the fleet. It is added to the selected stacks on each node, and created there with this color if the node does not have it yet. -

+
+

+ Pick a stack label from anywhere in the fleet. It is added to the selected stacks on each node, and created there with this color if the node does not have it yet. +

+ +
{templates.length === 0 && ( @@ -398,7 +435,30 @@ export function BulkLabelAssignCard({ nodes }: Props) { confirmLabel="Apply" confirming={running} onConfirm={run} - /> + > + {previewNodes.length > 0 && } + ); } + +// The concrete node/stack list carried into the confirm modal so the operator +// approves against the names being changed, not a bare stack/node count. Mirrors +// the stop card's resolved-targets list; `creates label` flags nodes where the +// label does not exist yet and will be created. +function AffectedTargetsList({ nodes }: { nodes: PreviewNode[] }) { + return ( +
+
    + {nodes.map(n => ( +
  • + + {n.nodeName}{n.willCreate ? ' · creates label' : ''} + + {n.stacks.join(', ')} +
    • + ))} +
+
+ ); +} diff --git a/frontend/src/components/fleet/FleetActions/cards/LabelFleetStopCard.test.tsx b/frontend/src/components/fleet/FleetActions/cards/LabelFleetStopCard.test.tsx index d9644e33..4588c57b 100644 --- a/frontend/src/components/fleet/FleetActions/cards/LabelFleetStopCard.test.tsx +++ b/frontend/src/components/fleet/FleetActions/cards/LabelFleetStopCard.test.tsx @@ -230,7 +230,9 @@ it('gates Stop fleet on a resolved preview, carries the node/stack list into the await waitFor(() => { const stopCall = mockedFetch.mock.calls.find(c => c[0] === '/fleet/labels/fleet-stop'); - expect(JSON.parse(stopCall![1].body)).toEqual({ labelName: 'prod', dryRun: false }); + // The real stop binds execution to the nodes resolved in the preview: it + // carries their ids so a node that was unreachable then cannot be added later. + expect(JSON.parse(stopCall![1].body)).toEqual({ labelName: 'prod', dryRun: false, nodeIds: [1] }); }); // Per-node results render in the card (the "· 1 stack" label is unique to the // results list, distinguishing it from the preview well). diff --git a/frontend/src/components/fleet/FleetActions/cards/LabelFleetStopCard.tsx b/frontend/src/components/fleet/FleetActions/cards/LabelFleetStopCard.tsx index 0b00a92c..372810f2 100644 --- a/frontend/src/components/fleet/FleetActions/cards/LabelFleetStopCard.tsx +++ b/frontend/src/components/fleet/FleetActions/cards/LabelFleetStopCard.tsx @@ -36,7 +36,7 @@ type PreviewState = // One node and the stacks the resolved blast radius would stop on it. The // destructive Stop stays disabled until this is known, so the operator always // confirms against a concrete node/stack list rather than a label name alone. -interface ResolvedTarget { nodeName: string; stackNames: string[] } +interface ResolvedTarget { nodeId: number; nodeName: string; stackNames: string[] } const KICKER = 'font-mono text-[10px] uppercase tracking-[0.18em]'; const PREVIEW_ROW_LIMIT = 6; @@ -190,7 +190,7 @@ export function LabelFleetStopCard() { const blastTone = preview.kind === 'loading' || preview.kind === 'unavailable' ? 'muted' as const : undefined; - async function run(opts: { dryRun: boolean }) { + async function run(opts: { dryRun: boolean; nodeIds?: number[] }) { const trimmed = labelName.trim(); if (!trimmed) return; const verb = opts.dryRun ? 'Dry-running' : 'Stopping'; @@ -198,9 +198,12 @@ export function LabelFleetStopCard() { setRunning(true); setResults([]); try { + // The real stop carries the node ids the operator confirmed in the preview + // so execution cannot expand to a node that was unreachable then and has + // since reconnected. The dry run sends no allowlist and scans the fleet. const res = await apiFetch('/fleet/labels/fleet-stop', { method: 'POST', - body: JSON.stringify({ labelName: trimmed, dryRun: opts.dryRun }), + body: JSON.stringify({ labelName: trimmed, dryRun: opts.dryRun, ...(opts.nodeIds ? { nodeIds: opts.nodeIds } : {}) }), }); const body = await res.json().catch(() => ({})); toast.dismiss(toastId); @@ -251,7 +254,7 @@ export function LabelFleetStopCard() { // against it even when the live preview endpoint is down. const targets = apiResults .filter(n => n.reachable && n.matched && Array.isArray(n.stackResults) && n.stackResults.length > 0) - .map(n => ({ nodeName: n.nodeName, stackNames: n.stackResults.map(s => s.stackName) })); + .map(n => ({ nodeId: n.nodeId, nodeName: n.nodeName, stackNames: n.stackResults.map(s => s.stackName) })); setDryRunResolved({ label: trimmed, targets }); } const reachable = apiResults.filter(n => n.reachable); @@ -288,7 +291,7 @@ export function LabelFleetStopCard() { if (preview.kind === 'ready') { return preview.data.perNode .filter(n => n.reachable && n.stackCount > 0) - .map(n => ({ nodeName: n.nodeName, stackNames: n.stackNames })); + .map(n => ({ nodeId: n.nodeId, nodeName: n.nodeName, stackNames: n.stackNames })); } if (dryRunResolved && dryRunResolved.label === trimmed && trimmed.length > 0) { return dryRunResolved.targets; @@ -370,7 +373,7 @@ export function LabelFleetStopCard() { description="Sencho will stop the stacks listed below. Node labels are not used by this action. Services will be unavailable until restarted." confirmLabel="Stop fleet" confirming={running} - onConfirm={() => run({ dryRun: false })} + onConfirm={() => run({ dryRun: false, nodeIds: (resolvedTargets ?? []).map(t => t.nodeId) })} > {resolvedTargets && resolvedTargets.length > 0 && } @@ -428,14 +431,15 @@ function renderPreviewSection(preview: PreviewState, trimmed: string) { // The resolved node/stack list carried into the destructive confirm modal, so // the operator confirms against the concrete blast radius rather than a label -// name. Final membership is re-resolved per node at execution time (state can -// drift between preview and confirm), which the header states plainly. +// name. The node set is bound to this list (the stop sends these node ids and +// the backend acts on no others); only the per-node stacks are re-matched by +// label at execution, where state can still drift between preview and confirm. function ResolvedTargetsList({ targets }: { targets: ResolvedTarget[] }) { const stackCount = targets.reduce((n, t) => n + t.stackNames.length, 0); return (
- Will stop {stackCount} stack{stackCount === 1 ? '' : 's'} across {targets.length} node{targets.length === 1 ? '' : 's'}. The final set is re-resolved on each node at execution time. + Will stop {stackCount} stack{stackCount === 1 ? '' : 's'} across {targets.length} node{targets.length === 1 ? '' : 's'}. The stacks on each node are re-matched by label at execution; the node set is fixed to those listed.