From 523ba5854caddebfc082865716f00e92be7f2c3b Mon Sep 17 00:00:00 2001 From: Anso Date: Tue, 19 May 2026 00:13:57 -0400 Subject: [PATCH] fix(stacks): return 404 for nonexistent stacks on deploy/down/update (F-7) (#1108) POST /api/stacks/:name/{deploy,down,update} previously returned HTTP 500 with body {"error":"spawn docker ENOENT"} when invoked against a stack whose compose directory was missing. The status code was wrong (the named resource did not exist, so 404 is the right answer) and the message misled operators into thinking the docker CLI was unavailable. Add a small requireStackExists(nodeId, stackName, res) helper in routes/stacks.ts that validates the stack name and confirms a compose file is present via FileSystemService.hasComposeFile before any of the three handlers spawn docker compose. The helper is called immediately after requirePermission and before runPolicyGate so unauthorized callers still get 403 first and the policy gate never runs against a phantom stack. In ComposeService.execute(), narrow the child.on('error') handler so the genuine docker-binary-missing case (ENOENT on the spawn itself) rejects with "Docker CLI unavailable on this node" instead of the raw "spawn docker ENOENT". This is defense in depth for the rare case the pre-check cannot cover, and it fixes the misleading-message half of the bug as well. Cover the new contract with stack-actions-missing-stack.test.ts (four cases: deploy/down/update return 404, invalid name returns 400). Mock ComposeService as a tripwire so a future code path that bypasses the guard would fail loudly. Fix stacks-failure-notifications.test.ts by adding hasComposeFile to its FileSystemService partial mock so the existing happy-path-error-handling cases continue to flow into ComposeService. --- .../stack-actions-missing-stack.test.ts | 114 ++++++++++++++++++ .../stacks-failure-notifications.test.ts | 1 + backend/src/routes/stacks.ts | 22 ++++ backend/src/services/ComposeService.ts | 10 +- 4 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 backend/src/__tests__/stack-actions-missing-stack.test.ts diff --git a/backend/src/__tests__/stack-actions-missing-stack.test.ts b/backend/src/__tests__/stack-actions-missing-stack.test.ts new file mode 100644 index 00000000..46b8a8f8 --- /dev/null +++ b/backend/src/__tests__/stack-actions-missing-stack.test.ts @@ -0,0 +1,114 @@ +/** + * Integration tests for the stack-action 404 contract (F-7). + * + * Verifies that POST /api/stacks/:stackName/{deploy,down,update} returns + * HTTP 404 with `{ error: 'Stack not found' }` when the named stack has no + * compose file under COMPOSE_DIR, instead of allowing the request to flow + * into ComposeService and surface the raw `spawn docker ENOENT` from the + * child_process spawn cwd failure. + * + * ComposeService is mocked to act as a tripwire: if any of these endpoints + * ever reach the service layer for a nonexistent stack, the mock assertion + * will fail loudly. + */ +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import request from 'supertest'; +import { setupTestDb, cleanupTestDb, loginAsTestAdmin } from './helpers/setupTestDb'; + +const { + mockDeployStack, + mockRunCommand, + mockUpdateStack, +} = vi.hoisted(() => ({ + mockDeployStack: vi.fn(), + mockRunCommand: vi.fn(), + mockUpdateStack: vi.fn(), +})); + +vi.mock('../services/ComposeService', async () => { + const actual = await vi.importActual( + '../services/ComposeService', + ); + return { + ...actual, + ComposeService: { + ...actual.ComposeService, + getInstance: () => ({ + deployStack: mockDeployStack, + runCommand: mockRunCommand, + updateStack: mockUpdateStack, + }), + }, + }; +}); + +let tmpDir: string; +let app: import('express').Express; +let authCookie: string; + +beforeAll(async () => { + tmpDir = await setupTestDb(); + ({ app } = await import('../index')); + authCookie = await loginAsTestAdmin(app); +}); + +afterAll(() => { + vi.restoreAllMocks(); + cleanupTestDb(tmpDir); +}); + +describe('POST /api/stacks/:stackName/deploy on a nonexistent stack', () => { + it('returns 404 with "Stack not found" and never enters ComposeService.deployStack', async () => { + mockDeployStack.mockClear(); + + const res = await request(app) + .post('/api/stacks/does-not-exist-f7/deploy') + .set('Cookie', authCookie); + + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Stack not found' }); + expect(mockDeployStack).not.toHaveBeenCalled(); + }); +}); + +describe('POST /api/stacks/:stackName/down on a nonexistent stack', () => { + it('returns 404 with "Stack not found" and never enters ComposeService.runCommand', async () => { + mockRunCommand.mockClear(); + + const res = await request(app) + .post('/api/stacks/does-not-exist-f7/down') + .set('Cookie', authCookie); + + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Stack not found' }); + expect(mockRunCommand).not.toHaveBeenCalled(); + }); +}); + +describe('POST /api/stacks/:stackName/update on a nonexistent stack', () => { + it('returns 404 with "Stack not found" and never enters ComposeService.updateStack', async () => { + mockUpdateStack.mockClear(); + + const res = await request(app) + .post('/api/stacks/does-not-exist-f7/update') + .set('Cookie', authCookie); + + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Stack not found' }); + expect(mockUpdateStack).not.toHaveBeenCalled(); + }); +}); + +describe('Invalid stack names are rejected with 400 before the existence check', () => { + it('POST /api/stacks/..bad../deploy returns 400 Invalid stack name', async () => { + mockDeployStack.mockClear(); + + const res = await request(app) + .post('/api/stacks/..bad../deploy') + .set('Cookie', authCookie); + + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Invalid stack name' }); + expect(mockDeployStack).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/src/__tests__/stacks-failure-notifications.test.ts b/backend/src/__tests__/stacks-failure-notifications.test.ts index f68545b9..74190f4f 100644 --- a/backend/src/__tests__/stacks-failure-notifications.test.ts +++ b/backend/src/__tests__/stacks-failure-notifications.test.ts @@ -99,6 +99,7 @@ vi.mock('../services/FileSystemService', () => ({ getStacks: vi.fn().mockResolvedValue([]), getBaseDir: () => '/tmp/compose', readComposeFile: vi.fn().mockResolvedValue(''), + hasComposeFile: vi.fn().mockResolvedValue(true), }), }, })); diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts index 0ff9bd95..81e87639 100644 --- a/backend/src/routes/stacks.ts +++ b/backend/src/routes/stacks.ts @@ -37,6 +37,25 @@ function notifyActionSuccess(category: NotificationCategory, message: string, st .catch(err => console.error('[Stacks] Failed to dispatch activity for %s:', sanitizeForLog(stackName), err)); } +async function requireStackExists(nodeId: number, stackName: string, res: Response): Promise { + if (!isValidStackName(stackName)) { + res.status(400).json({ error: 'Invalid stack name' }); + return false; + } + const fsSvc = FileSystemService.getInstance(nodeId); + const stackDir = path.join(fsSvc.getBaseDir(), stackName); + try { + if (!(await fsSvc.hasComposeFile(stackDir))) { + res.status(404).json({ error: 'Stack not found' }); + return false; + } + } catch { + res.status(404).json({ error: 'Stack not found' }); + return false; + } + return true; +} + export async function resolveAllEnvFilePaths(nodeId: number, stackName: string): Promise { const fsService = FileSystemService.getInstance(nodeId); const stackDir = path.join(fsService.getBaseDir(), stackName); @@ -620,6 +639,7 @@ stacksRouter.get('/:stackName/services', async (req: Request, res: Response) => stacksRouter.post('/:stackName/deploy', async (req: Request, res: Response) => { const stackName = req.params.stackName as string; if (!requirePermission(req, res, 'stack:deploy', 'stack', stackName)) return; + if (!(await requireStackExists(req.nodeId, stackName, res))) return; try { if (!(await runPolicyGate(req, res, stackName, req.nodeId))) return; const skipScan = req.body?.skip_scan === true; @@ -656,6 +676,7 @@ stacksRouter.post('/:stackName/deploy', async (req: Request, res: Response) => { stacksRouter.post('/:stackName/down', async (req: Request, res: Response) => { const stackName = req.params.stackName as string; if (!requirePermission(req, res, 'stack:deploy', 'stack', stackName)) return; + if (!(await requireStackExists(req.nodeId, stackName, res))) return; try { await ComposeService.getInstance(req.nodeId).runCommand(stackName, 'down', getTerminalWs()); invalidateNodeCaches(req.nodeId); @@ -802,6 +823,7 @@ stacksRouter.get('/:stackName/update-preview', async (req: Request, res: Respons stacksRouter.post('/:stackName/update', async (req: Request, res: Response) => { const stackName = req.params.stackName as string; if (!requirePermission(req, res, 'stack:deploy', 'stack', stackName)) return; + if (!(await requireStackExists(req.nodeId, stackName, res))) return; try { if (!(await runPolicyGate(req, res, stackName, req.nodeId))) return; const skipScan = req.body?.skip_scan === true; diff --git a/backend/src/services/ComposeService.ts b/backend/src/services/ComposeService.ts index 583a60a4..ba596bd2 100644 --- a/backend/src/services/ComposeService.ts +++ b/backend/src/services/ComposeService.ts @@ -199,16 +199,20 @@ export class ComposeService { }); }); - child.on('error', (error: Error) => { + child.on('error', (error: Error & { code?: string }) => { exited = true; finish(() => { - sendOutput(`Error: ${redactSensitiveText(error.message)}\n`); + let message = redactSensitiveText(error.message); + if (error.code === 'ENOENT' && /^spawn docker(?:$| )/.test(error.message)) { + message = 'Docker CLI unavailable on this node'; + } + sendOutput(`Error: ${message}\n`); if (pendingTerminationError) { if (throwOnError) reject(pendingTerminationError); else resolve(); return; } - if (throwOnError) reject(new Error(redactSensitiveText(error.message))); + if (throwOnError) reject(new Error(message)); else resolve(); }); });