From 2a29fed11703e98f39dd239b205ae8ac69888706 Mon Sep 17 00:00:00 2001 From: Anso Date: Mon, 25 May 2026 23:48:12 -0400 Subject: [PATCH] feat(labels): harden Stack Labels (gate parity, abort, dry-run, cap) (#1232) * feat(labels): harden stack-labels surface (gate parity, abort, dry-run policy, cap) - Cap labelIds array at MAX_LABELS_PER_NODE on PUT /api/stacks/:name/labels. - Hide sidebar Labels submenu and Settings mutation buttons for roles without stack:edit, matching the backend requirePermission gate. - Break the per-label bulk-action loop on req.aborted so cancelled requests release the per-node lock once the in-flight op completes. - Reset saving state on success in LabelInlineCreateForm so the form stays interactive when reused outside the kebab/context menus. - Invoke enforcePolicyPreDeploy inside the dry-run deploy branch and report blocked stacks honestly; previously dry-run skipped the gate and would predict success for stacks the real deploy would block. * fix(labels): split sidebar gate so inline create matches unscoped backend guard Codex review of #1232 surfaced a parity miss: POST /api/labels is guarded by unscoped requirePermission('stack:edit'), but the sidebar inline "New label" entry was gated by canEditLabels (per-stack scoped). An Admiral user with only scoped grants on a stack could toggle existing labels but the inline create request would 403. Splits the sidebar gate: - canEditLabels (scoped) keeps gating the Labels submenu trigger and toggle items, matching PUT /api/stacks/:name/labels. - canCreateLabels (unscoped) now gates the inline "New label" entry, matching POST /api/labels. Also restores the swallow-catch in LabelInlineCreateForm. The earlier catch-to-finally change in #1232 let the rethrow from createAndAssignLabel surface as an unhandled event-handler rejection in the browser console. Parents already toast on failure; swallowing in the form is the intended behavior with the finally reset still in place. --- .../src/__tests__/labels-bulk-actions.test.ts | 63 ++++++++++++++++++- .../__tests__/labels-community-tier.test.ts | 42 +++++++++++++ backend/src/routes/labels.ts | 46 +++++++++++--- .../hooks/useSidebarContextMenu.ts | 5 ++ .../src/components/settings/LabelsSection.tsx | 56 ++++++++++------- .../sidebar/LabelInlineCreateForm.tsx | 6 ++ .../components/sidebar/StackContextMenu.tsx | 2 +- .../src/components/sidebar/StackKebabMenu.tsx | 2 +- .../src/components/sidebar/sidebar-types.ts | 2 + .../__tests__/useStackMenuItems.test.tsx | 12 ++++ frontend/src/hooks/useStackMenuItems.tsx | 30 ++++----- 11 files changed, 219 insertions(+), 47 deletions(-) diff --git a/backend/src/__tests__/labels-bulk-actions.test.ts b/backend/src/__tests__/labels-bulk-actions.test.ts index 9de7cafb..5383376d 100644 --- a/backend/src/__tests__/labels-bulk-actions.test.ts +++ b/backend/src/__tests__/labels-bulk-actions.test.ts @@ -65,6 +65,10 @@ afterAll(() => cleanupTestDb(tmpDir)); beforeEach(() => { vi.restoreAllMocks(); + // restoreAllMocks only resets spies; bare vi.fn() mocks keep their call + // history across tests. Clear them all so each test sees a fresh slate + // before its `.not.toHaveBeenCalled()` assertions run. + vi.clearAllMocks(); vi.spyOn(LicenseService.getInstance(), 'getTier').mockReturnValue('paid'); mockFsStacks = ['alpha', 'beta']; deployStack.mockResolvedValue(undefined); @@ -72,7 +76,6 @@ beforeEach(() => { stopContainer.mockResolvedValue(undefined); restartContainer.mockResolvedValue(undefined); enforcePolicyPreDeploy.mockResolvedValue({ ok: true }); - invalidateNodeCaches.mockClear(); activeBulkActions.clear(); db.getDb().prepare('DELETE FROM stack_label_assignments').run(); db.getDb().prepare('DELETE FROM stack_labels').run(); @@ -146,4 +149,62 @@ describe('Stack Labels bulk actions', () => { expect(res.body.error).toContain('already running'); expect(restartContainer).not.toHaveBeenCalled(); }); + + it('dry-run deploy runs the policy gate and reports blocked stacks honestly', async () => { + const label = await createAssignedLabel(['alpha']); + enforcePolicyPreDeploy.mockResolvedValue({ + ok: false, + policy: { name: 'block-criticals', max_severity: 'high' }, + violations: [{ image: 'nginx:latest', severity: 'critical' }], + }); + + const res = await request(app) + .post(`/api/labels/${label.id}/action`) + .set('Authorization', authHeader) + .send({ action: 'deploy', dryRun: true }); + + expect(res.status).toBe(200); + expect(res.body.results).toEqual([ + expect.objectContaining({ stackName: 'alpha', success: false, dryRun: true }), + ]); + expect(res.body.results[0].error).toContain('Policy "block-criticals" blocked deploy'); + expect(deployStack).not.toHaveBeenCalled(); + expect(invalidateNodeCaches).not.toHaveBeenCalled(); + }); + + it('dry-run deploy reports success when the policy gate passes, without touching Docker', async () => { + const label = await createAssignedLabel(['alpha']); + enforcePolicyPreDeploy.mockResolvedValue({ ok: true }); + + const res = await request(app) + .post(`/api/labels/${label.id}/action`) + .set('Authorization', authHeader) + .send({ action: 'deploy', dryRun: true }); + + expect(res.status).toBe(200); + expect(res.body.results).toEqual([ + { stackName: 'alpha', success: true, dryRun: true }, + ]); + expect(enforcePolicyPreDeploy).toHaveBeenCalledWith('alpha', label.node_id, expect.any(Object)); + expect(deployStack).not.toHaveBeenCalled(); + expect(invalidateNodeCaches).not.toHaveBeenCalled(); + }); + + it('dry-run stop reports per-stack success without dispatching real stops', async () => { + const label = await createAssignedLabel(['alpha', 'beta']); + + const res = await request(app) + .post(`/api/labels/${label.id}/action`) + .set('Authorization', authHeader) + .send({ action: 'stop', dryRun: true }); + + expect(res.status).toBe(200); + expect(res.body.results).toEqual([ + { stackName: 'alpha', success: true, dryRun: true }, + { stackName: 'beta', success: true, dryRun: true }, + ]); + expect(getContainersByStack).not.toHaveBeenCalled(); + expect(stopContainer).not.toHaveBeenCalled(); + expect(invalidateNodeCaches).not.toHaveBeenCalled(); + }); }); diff --git a/backend/src/__tests__/labels-community-tier.test.ts b/backend/src/__tests__/labels-community-tier.test.ts index 7f5926ad..4e4879e1 100644 --- a/backend/src/__tests__/labels-community-tier.test.ts +++ b/backend/src/__tests__/labels-community-tier.test.ts @@ -34,6 +34,14 @@ beforeAll(async () => { afterAll(() => cleanupTestDb(tmpDir)); +// Tests in this file accumulate labels in the shared DB; clear them between +// runs so later tests do not bump into MAX_LABELS_PER_NODE. +afterEach(() => { + const db = DatabaseService.getInstance().getDb(); + db.prepare('DELETE FROM stack_label_assignments').run(); + db.prepare('DELETE FROM stack_labels').run(); +}); + function mockTier(tier: 'paid' | 'community') { vi.spyOn(LicenseService.getInstance(), 'getTier').mockReturnValue(tier); } @@ -126,6 +134,40 @@ describe('Stack Labels on Community tier', () => { expect(res.status).toBe(400); expect(res.body.error).toBe('Invalid stack name'); }); + + it('rejects labelIds arrays over the per-node cap', async () => { + mockTier('community'); + const oversized = Array.from({ length: 51 }, (_, i) => i + 1); + const res = await request(app) + .put('/api/stacks/cap-stack/labels') + .set('Authorization', authHeader) + .send({ labelIds: oversized }); + expect(res.status).toBe(400); + expect(res.body.error).toContain('may not exceed'); + }); + + it('accepts labelIds at exactly the per-node cap', async () => { + mockTier('community'); + // Seed 50 labels directly so the FK check on assignment passes without + // running 50 HTTP round trips per test. The route resolves nodeId via + // nodeContextMiddleware (default node when no x-node-id header), so the + // seeded rows must use the same nodeId the request will look up. + const { NodeRegistry } = await import('../services/NodeRegistry'); + const nodeId = NodeRegistry.getInstance().getDefaultNodeId(); + const db = DatabaseService.getInstance().getDb(); + const insert = db.prepare('INSERT INTO stack_labels (node_id, name, color) VALUES (?, ?, ?)'); + const ids: number[] = []; + for (let i = 0; i < 50; i++) { + const r = insert.run(nodeId, `cap-edge-${i}`, 'teal'); + ids.push(r.lastInsertRowid as number); + } + const res = await request(app) + .put('/api/stacks/cap-edge-stack/labels') + .set('Authorization', authHeader) + .send({ labelIds: ids }); + expect(res.status).toBe(200); + expect(res.body.success).toBe(true); + }); }); describe('Stack Labels RBAC', () => { diff --git a/backend/src/routes/labels.ts b/backend/src/routes/labels.ts index b1bd48c7..fb2459f6 100644 --- a/backend/src/routes/labels.ts +++ b/backend/src/routes/labels.ts @@ -206,14 +206,21 @@ labelsRouter.post('/:id/action', authMiddleware, async (req: Request, res: Respo const results: { stackName: string; success: boolean; error?: string; dryRun?: boolean }[] = []; for (const stackName of validStacks) { - if (isDryRun) { - // Rehearse the action under the same lock + label resolution + fs - // intersection. Skip the destructive leaf call. - results.push({ stackName, success: true, dryRun: true }); - continue; + // Client disconnected mid-bulk: stop dispatching new per-stack ops. + // The currently in-flight call still runs to completion; the outer + // finally releases the lock when it returns. Stays on `req.aborted` + // rather than `req.destroyed` because supertest's in-process server + // mode flips `destroyed` between handler and response write, which + // would cause every bulk-action test to look like a client abort. + if (req.aborted) { + if (isDebugEnabled()) console.debug('[Labels:debug] Bulk action aborted by client at stack:', stackName); + break; } try { if (action === 'deploy') { + // Policy gate runs for both real and dry-run deploys: a dry-run + // that omits the policy check would falsely report success for + // stacks the real deploy would block. const gate = await enforcePolicyPreDeploy( stackName, req.nodeId, @@ -221,11 +228,21 @@ labelsRouter.post('/:id/action', authMiddleware, async (req: Request, res: Respo ); if (!gate.ok) { const blockedMsg = `Policy "${gate.policy?.name}" blocked deploy: ${gate.violations.length} image(s) exceed ${gate.policy?.max_severity}`; - results.push({ stackName, success: false, error: blockedMsg }); + results.push({ stackName, success: false, error: blockedMsg, ...(isDryRun ? { dryRun: true } : {}) }); + continue; + } + if (isDryRun) { + results.push({ stackName, success: true, dryRun: true }); continue; } await ComposeService.getInstance(req.nodeId).deployStack(stackName, undefined, false); } else { + // stop / restart have no pre-action policy gate; dry-run just + // confirms the stack would be reached. + if (isDryRun) { + results.push({ stackName, success: true, dryRun: true }); + continue; + } const dockerController = DockerController.getInstance(req.nodeId); const containers = await dockerController.getContainersByStack(stackName); if (action === 'stop') { @@ -242,12 +259,18 @@ labelsRouter.post('/:id/action', authMiddleware, async (req: Request, res: Respo const succeeded = results.filter(r => r.success).length; const failed = results.length - succeeded; - console.log(`[Labels] Bulk ${sanitizeForLog(action)}${isDryRun ? ' (dry run)' : ''} on label ${id}: ${validStacks.length} stacks (${succeeded} succeeded, ${failed} failed)`); - if (isDebugEnabled()) console.debug('[Labels:debug] Bulk action complete:', { id, action, total: results.length, succeeded, failed, dryRun: isDryRun }); + // Two-axis truncation reporting: `results.length` is what we actually + // processed; `validStacks.length` is what we set out to process. They + // differ when the client aborted mid-loop. + console.log(`[Labels] Bulk ${sanitizeForLog(action)}${isDryRun ? ' (dry run)' : ''} on label ${id}: ${results.length}/${validStacks.length} stacks (${succeeded} succeeded, ${failed} failed)`); + if (isDebugEnabled()) console.debug('[Labels:debug] Bulk action complete:', { id, action, processed: results.length, total: validStacks.length, succeeded, failed, dryRun: isDryRun }); if (succeeded > 0 && !isDryRun) { invalidateNodeCaches(req.nodeId); } + // Writing the response is harmless even if the client already + // disconnected; Node swallows the EPIPE / write-after-end. The + // lock release in the outer finally still happens. res.json({ results }); } finally { activeBulkActions.delete(lockKey); @@ -281,6 +304,13 @@ stackLabelsRouter.put('/:stackName/labels', authMiddleware, async (req: Request, res.status(400).json({ error: 'labelIds must be an array of numbers' }); return; } + // A node can hold at most MAX_LABELS_PER_NODE labels, so any stack + // assignment over that count is either an authenticated client mistake + // or a deliberate transaction-bloat attempt. Reject before the DB sees it. + if (labelIds.length > MAX_LABELS_PER_NODE) { + res.status(400).json({ error: `labelIds may not exceed ${MAX_LABELS_PER_NODE} entries` }); + return; + } if (isDebugEnabled()) console.debug('[Labels:debug] Set stack labels:', { stackName, nodeId, labelIds }); DatabaseService.getInstance().setStackLabels(stackName, nodeId, labelIds); diff --git a/frontend/src/components/EditorLayout/hooks/useSidebarContextMenu.ts b/frontend/src/components/EditorLayout/hooks/useSidebarContextMenu.ts index 783905ff..f8b702ee 100644 --- a/frontend/src/components/EditorLayout/hooks/useSidebarContextMenu.ts +++ b/frontend/src/components/EditorLayout/hooks/useSidebarContextMenu.ts @@ -43,6 +43,11 @@ export function useSidebarContextMenu({ isPaid, isAdmiral, canDelete: can('stack:delete', 'stack', sName), + canEditLabels: can('stack:edit', 'stack', sName), + // POST /api/labels (the inline "New label" entry) is guarded by the + // unscoped requirePermission('stack:edit'); a user with only per-stack + // scoped edit can toggle existing labels but cannot create new ones. + canCreateLabels: can('stack:edit'), isPinned: stackListState.isPinned(file), labels: stackListState.labels, assignedLabelIds: (stackListState.stackLabelMap[file] ?? []).map(l => l.id), diff --git a/frontend/src/components/settings/LabelsSection.tsx b/frontend/src/components/settings/LabelsSection.tsx index e6148c5b..9b1115a7 100644 --- a/frontend/src/components/settings/LabelsSection.tsx +++ b/frontend/src/components/settings/LabelsSection.tsx @@ -6,6 +6,7 @@ import { Modal, ModalHeader, ModalBody, ModalFooter, ConfirmModal } from '@/comp import { apiFetch } from '@/lib/api'; import { toast } from '@/components/ui/toast-store'; import { SENCHO_LABELS_CHANGED } from '@/lib/events'; +import { useAuth } from '@/context/AuthContext'; import { CapabilityGate } from '../CapabilityGate'; import { LabelDot } from '../LabelPill'; import { LABEL_COLORS, MAX_LABELS_PER_NODE, type Label, type LabelColor } from '../label-types'; @@ -18,6 +19,11 @@ interface LabelsSectionProps { } export function LabelsSection({ onLabelsChanged }: LabelsSectionProps = {}) { + const { can } = useAuth(); + // Mirrors the backend `requirePermission('stack:edit')` guard on + // POST/PUT/DELETE /api/labels, keeping a viewer/deployer/auditor from + // clicking through to a guaranteed 403. + const canMutate = can('stack:edit'); const [labels, setLabels] = useState([]); const [loading, setLoading] = useState(true); const [assignmentCounts, setAssignmentCounts] = useState>({}); @@ -128,12 +134,14 @@ export function LabelsSection({ onLabelsChanged }: LabelsSectionProps = {}) { return (
-
- = MAX_LABELS_PER_NODE}> - - {labels.length >= MAX_LABELS_PER_NODE ? 'Limit reached' : 'New label'} - -
+ {canMutate && ( +
+ = MAX_LABELS_PER_NODE}> + + {labels.length >= MAX_LABELS_PER_NODE ? 'Limit reached' : 'New label'} + +
+ )}
{loading ? ( @@ -153,22 +161,26 @@ export function LabelsSection({ onLabelsChanged }: LabelsSectionProps = {}) { {assignmentCounts[label.id] || 0} stack{(assignmentCounts[label.id] || 0) !== 1 ? 's' : ''} - - + {canMutate && ( + <> + + + + )}
))}
diff --git a/frontend/src/components/sidebar/LabelInlineCreateForm.tsx b/frontend/src/components/sidebar/LabelInlineCreateForm.tsx index 367b8b4c..a94e2b0e 100644 --- a/frontend/src/components/sidebar/LabelInlineCreateForm.tsx +++ b/frontend/src/components/sidebar/LabelInlineCreateForm.tsx @@ -20,6 +20,12 @@ export function LabelInlineCreateForm({ onSubmit, onCancel }: LabelInlineCreateF try { await onSubmit(trimmed, color); } catch { + // Parents (createAndAssignLabel) toast and rethrow to signal failure; + // swallow here so the rejection does not surface as an unhandled + // event-handler rejection in the browser console. + } finally { + // Reset even on success so the form stays interactive if the parent + // keeps it mounted (e.g. when reused outside the kebab/context menu). setSaving(false); } }; diff --git a/frontend/src/components/sidebar/StackContextMenu.tsx b/frontend/src/components/sidebar/StackContextMenu.tsx index 7191b4f1..f762d38c 100644 --- a/frontend/src/components/sidebar/StackContextMenu.tsx +++ b/frontend/src/components/sidebar/StackContextMenu.tsx @@ -63,7 +63,7 @@ function LabelsSub({ item, ctx }: { item: MenuItem; ctx: StackMenuCtx }) { ); })} - {ctx.labels.length < MAX_LABELS_PER_NODE && ( + {ctx.canCreateLabels && ctx.labels.length < MAX_LABELS_PER_NODE && ( { e.preventDefault(); setCreating(true); }}> New label diff --git a/frontend/src/components/sidebar/StackKebabMenu.tsx b/frontend/src/components/sidebar/StackKebabMenu.tsx index b719922a..d7025df8 100644 --- a/frontend/src/components/sidebar/StackKebabMenu.tsx +++ b/frontend/src/components/sidebar/StackKebabMenu.tsx @@ -63,7 +63,7 @@ function LabelsSub({ item, ctx }: { item: MenuItem; ctx: StackMenuCtx }) { ); })} - {ctx.labels.length < MAX_LABELS_PER_NODE && ( + {ctx.canCreateLabels && ctx.labels.length < MAX_LABELS_PER_NODE && ( { e.preventDefault(); setCreating(true); }}> New label diff --git a/frontend/src/components/sidebar/sidebar-types.ts b/frontend/src/components/sidebar/sidebar-types.ts index 03b01e12..681ac766 100644 --- a/frontend/src/components/sidebar/sidebar-types.ts +++ b/frontend/src/components/sidebar/sidebar-types.ts @@ -28,6 +28,8 @@ export interface StackMenuCtx { isPaid: boolean; isAdmiral: boolean; canDelete: boolean; + canEditLabels: boolean; + canCreateLabels: boolean; isPinned: boolean; labels: Label[]; assignedLabelIds: number[]; diff --git a/frontend/src/hooks/__tests__/useStackMenuItems.test.tsx b/frontend/src/hooks/__tests__/useStackMenuItems.test.tsx index 4d0e0962..12807a8b 100644 --- a/frontend/src/hooks/__tests__/useStackMenuItems.test.tsx +++ b/frontend/src/hooks/__tests__/useStackMenuItems.test.tsx @@ -12,6 +12,8 @@ function makeCtx(overrides: Partial = {}): StackMenuCtx { isPaid: true, isAdmiral: false, canDelete: true, + canEditLabels: true, + canCreateLabels: true, isPinned: false, labels: [], assignedLabelIds: [], @@ -106,6 +108,16 @@ describe('useStackMenuItems', () => { expect(labelsItem?.subItems?.[0]).toMatchObject({ id: 'label:1', label: 'prod' }); }); + it('hides the Labels submenu when !canEditLabels (viewer role)', () => { + const { result } = renderHook(() => useStackMenuItems('web.yml', makeCtx({ + canEditLabels: false, + labels: [{ id: 1, node_id: 0, name: 'prod', color: 'teal' }], + }))); + const organize = result.current.find(g => g.id === 'organize')!; + expect(organize.items.find(i => i.id === 'labels')).toBeUndefined(); + expect(organize.items.find(i => i.id === 'pin')).toBeDefined(); + }); + it('auto-update toggle calls setAutoUpdateEnabled with toggled value', () => { const setAutoUpdateEnabled = vi.fn(); const { result } = renderHook(() => diff --git a/frontend/src/hooks/useStackMenuItems.tsx b/frontend/src/hooks/useStackMenuItems.tsx index abb4961b..32518cac 100644 --- a/frontend/src/hooks/useStackMenuItems.tsx +++ b/frontend/src/hooks/useStackMenuItems.tsx @@ -19,7 +19,7 @@ import type { MenuGroup, MenuItem, StackMenuCtx } from '@/components/sidebar/sid export function useStackMenuItems(_file: string, ctx: StackMenuCtx): MenuGroup[] { const { - stackStatus, hasPort, isBusy, isPaid, canDelete, isPinned, labels, + stackStatus, hasPort, isBusy, isPaid, canDelete, canEditLabels, isPinned, labels, openAlertSheet, openAutoHeal, checkUpdates, openStackApp, deploy, stop, restart, update, remove, pin, unpin, toggleLabel, menuVisibility, autoUpdateEnabled, setAutoUpdateEnabled, openScheduleTask, @@ -48,19 +48,21 @@ export function useStackMenuItems(_file: string, ctx: StackMenuCtx): MenuGroup[] groups.push({ id: 'inspect', items: inspect }); const organize: MenuItem[] = []; - organize.push({ - id: 'labels', - label: 'Labels', - icon: Tag, - shortcut: 'L ›', - onSelect: () => {}, - subItems: labels.map(l => ({ - id: `label:${l.id}`, - label: l.name, + if (canEditLabels) { + organize.push({ + id: 'labels', + label: 'Labels', icon: Tag, - onSelect: () => toggleLabel(l.id), - })), - }); + shortcut: 'L ›', + onSelect: () => {}, + subItems: labels.map(l => ({ + id: `label:${l.id}`, + label: l.name, + icon: Tag, + onSelect: () => toggleLabel(l.id), + })), + }); + } organize.push( isPinned ? { id: 'pin', label: 'Unpin', icon: PinOff, shortcut: 'P', onSelect: unpin } @@ -85,7 +87,7 @@ export function useStackMenuItems(_file: string, ctx: StackMenuCtx): MenuGroup[] return groups; }, [ - stackStatus, hasPort, isBusy, isPaid, canDelete, isPinned, labels, + stackStatus, hasPort, isBusy, isPaid, canDelete, canEditLabels, isPinned, labels, showDeploy, showStop, showRestart, showUpdate, autoUpdateEnabled, setAutoUpdateEnabled, openAlertSheet, openAutoHeal, checkUpdates, openStackApp,