diff --git a/backend/src/__tests__/scheduled-tasks-rbac.test.ts b/backend/src/__tests__/scheduled-tasks-rbac.test.ts index 2cc2a91f..bd4e2133 100644 --- a/backend/src/__tests__/scheduled-tasks-rbac.test.ts +++ b/backend/src/__tests__/scheduled-tasks-rbac.test.ts @@ -7,6 +7,8 @@ import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest'; import request from 'supertest'; import bcrypt from 'bcrypt'; +import fs from 'fs'; +import path from 'path'; import { setupTestDb, cleanupTestDb, loginAsTestAdmin } from './helpers/setupTestDb'; let tmpDir: string; @@ -67,6 +69,13 @@ beforeAll(async () => { else auditorCookie = c; } + // Seed real stack directories so existence validators pass. + const composeDir = path.join(tmpDir, 'compose'); + for (const name of ['web', 'api']) { + fs.mkdirSync(path.join(composeDir, name), { recursive: true }); + fs.writeFileSync(path.join(composeDir, name, 'compose.yaml'), 'version: "3"\n'); + } + // Insert a local node for stack-target fixtures for (const [nodeName, nodeType] of [['local-test', 'local'], ['remote-test', 'remote']] as const) { const existing = db.getDb().prepare('SELECT id FROM nodes WHERE name = ?').get(nodeName) as { id: number } | undefined; diff --git a/backend/src/__tests__/scheduled-tasks-routes.test.ts b/backend/src/__tests__/scheduled-tasks-routes.test.ts index aff7d149..bc5d32d6 100644 --- a/backend/src/__tests__/scheduled-tasks-routes.test.ts +++ b/backend/src/__tests__/scheduled-tasks-routes.test.ts @@ -6,6 +6,8 @@ import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest'; import request from 'supertest'; import bcrypt from 'bcrypt'; +import fs from 'fs'; +import path from 'path'; import { setupTestDb, cleanupTestDb, loginAsTestAdmin } from './helpers/setupTestDb'; let tmpDir: string; @@ -30,6 +32,24 @@ beforeAll(async () => { const viewerRes = await request(app).post('/api/auth/login').send({ username: 'sched-viewer', password: 'viewerpass' }); const cookies = viewerRes.headers['set-cookie'] as string | string[]; viewerCookie = Array.isArray(cookies) ? cookies[0] : cookies; + + // Seed real stack directories so existence validators pass. + const composeDir = path.join(tmpDir, 'compose'); + for (const name of ['my-stack', 's']) { + fs.mkdirSync(path.join(composeDir, name), { recursive: true }); + fs.writeFileSync(path.join(composeDir, name, 'compose.yaml'), 'version: "3"\n'); + } + + // Mock DockerController.findContainerByName so container existence checks pass + // in tests (no real Docker daemon available). Only resolve for names used by + // the test fixtures; everything else returns null to exercise the 400 path. + const { default: DockerController } = await import('../services/DockerController'); + const containerFixture = { id: 'abc123test', name: 'test-container', state: 'running', image: 'test:latest', stackProject: null }; + vi.spyOn(DockerController.prototype, 'findContainerByName') + .mockImplementation(async (name: string) => { + if (name === 'watchtower' || name === 'sidecar') return { ...containerFixture, name }; + return null; + }); }); afterAll(() => cleanupTestDb(tmpDir)); @@ -178,6 +198,15 @@ describe('POST /api/scheduled-tasks', () => { expect(res.status).toBe(403); }); + it('returns 403 (not 400) for unauthorized caller on nonexistent target', async () => { + // Permissions check runs first; an unauthorized caller must not learn + // whether a stack exists through the error code difference. + const res = await request(app).post('/api/scheduled-tasks').set('Cookie', viewerCookie).send({ + ...basePayload, target_id: 'nonexistent-stack', + }); + expect(res.status).toBe(403); + }); + it('creates a task and returns the new record', async () => { const res = await request(app).post('/api/scheduled-tasks').set('Cookie', adminCookie).send(basePayload); expect(res.status).toBe(201); @@ -202,6 +231,52 @@ describe('POST /api/scheduled-tasks', () => { expect(res.body.error).toMatch(/5 fields/); }); + it('rejects a nonexistent node_id with 400', async () => { + const res = await request(app).post('/api/scheduled-tasks').set('Cookie', adminCookie).send({ + ...basePayload, node_id: 9999, + }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/require an existing node/); + }); + + it('returns 403 (not 400) for unauthorized caller probing nonexistent node via fleet update', async () => { + // A viewer must never learn whether a node ID exists through the error + // code difference (400 "node doesn't exist" vs 403 "permission denied"). + const res = await request(app).post('/api/scheduled-tasks').set('Cookie', viewerCookie).send({ + name: 'probe-node', + action: 'update', + target_type: 'fleet', + node_id: 999999, + cron_expression: '0 0 * * *', + enabled: true, + }); + expect(res.status).toBe(403); + }); + + it('returns same 403 for unauthorized caller regardless of node existence', async () => { + const resNonexistent = await request(app).post('/api/scheduled-tasks') + .set('Cookie', viewerCookie).send({ + name: 'probe-nonexistent', action: 'update', target_type: 'fleet', + node_id: 999999, cron_expression: '0 0 * * *', enabled: true, + }); + const resExisting = await request(app).post('/api/scheduled-tasks') + .set('Cookie', viewerCookie).send({ + name: 'probe-existing', action: 'update', target_type: 'fleet', + node_id: 1, cron_expression: '0 0 * * *', enabled: true, + }); + expect(resNonexistent.status).toBe(403); + expect(resExisting.status).toBe(403); + expect(resNonexistent.body.error).toBe(resExisting.body.error); + }); + + it('rejects a nonexistent stack target with 400', async () => { + const res = await request(app).post('/api/scheduled-tasks').set('Cookie', adminCookie).send({ + ...basePayload, target_id: 'nonexistent-stack', + }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/not found on the target node/); + }); + it('rejects a missing cron expression with a clear message', async () => { const res = await request(app).post('/api/scheduled-tasks').set('Cookie', adminCookie).send({ ...basePayload, cron_expression: undefined, @@ -616,6 +691,23 @@ describe('POST /api/scheduled-tasks - container lifecycle', () => { expect(res.body.action).toBe('restart'); }); + it('rejects a nonexistent container target with 400', async () => { + const res = await request(app) + .post('/api/scheduled-tasks') + .set('Cookie', adminCookie) + .send({ + name: 'missing-ctr', + target_type: 'container', + target_id: 'nonexistent-container', + node_id: 1, + action: 'restart', + cron_expression: '0 3 * * *', + enabled: true, + }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/not found on the target node/); + }); + it('rejects invalid container names', async () => { const res = await request(app) .post('/api/scheduled-tasks') diff --git a/backend/src/routes/scheduledTasks.ts b/backend/src/routes/scheduledTasks.ts index bdc7a9cc..5ed9e0cb 100644 --- a/backend/src/routes/scheduledTasks.ts +++ b/backend/src/routes/scheduledTasks.ts @@ -13,6 +13,8 @@ import { } from '../services/scheduledActionRegistry'; import { SchedulerService } from '../services/SchedulerService'; import { NotificationService } from '../services/NotificationService'; +import DockerController from '../services/DockerController'; +import { FileSystemService } from '../services/FileSystemService'; import { checkPermission, requirePermission } from '../middleware/permissions'; import { escapeCsvField } from '../utils/csv'; import { getErrorMessage } from '../utils/errors'; @@ -108,6 +110,29 @@ function validateContainerTarget(targetType: TargetType, targetId: unknown, node return null; } +/** + * Validate that a stack or container target actually exists on the target + * node. Skipped for remote nodes (would require a proxy call). + */ +async function validateTargetExists( + targetType: TargetType, + targetId: string | null, + nodeId: number | null, +): Promise { + const isStack = targetType === 'stack'; + if ((!isStack && targetType !== 'container') || !targetId || nodeId == null) return null; + const node = DatabaseService.getInstance().getNode(nodeId); + if (!node) return `${isStack ? 'Stack' : 'Container'} operations require an existing node.`; + if (node.type === 'remote') return null; // Skip existence check (would need proxy). + const exists = isStack + ? (await FileSystemService.getInstance(nodeId).getStacks()).includes(targetId) + : (await DockerController.getInstance(nodeId).findContainerByName(targetId)) != null; + if (!exists) { + return `${isStack ? 'Stack' : 'Container'} "${targetId}" not found on the target node.`; + } + return null; +} + /** * Shared guard for non-stack actions that require a node. Stack actions use * validateStackTarget because they also require target_id. Label-targeted @@ -121,6 +146,7 @@ function validateActionNode( selectorType?: unknown, ): string | null { if (targetType === 'stack' || targetType === 'container') return null; + const def = getScheduledActionDefinition(action); if (!def?.requiresNode) return null; @@ -137,11 +163,14 @@ function validateActionNode( const parsedNodeId = parsePositiveNodeId(nodeId); if (parsedNodeId === null) return `${labelSingular} action requires a valid node_id.`; - if (def.nodeScope !== 'local') return null; + // Validate node existence for every action, not only local-scoped ones. const node = DatabaseService.getInstance().getNode(parsedNodeId); - if (!node) return `${labelPlural} require an existing local node.`; - if (node.type === 'remote') return `${labelPlural} currently require a local node.`; + if (!node) return `${labelSingular} action requires an existing node.`; + + if (def.nodeScope === 'local' && node.type === 'remote') { + return `${labelPlural} currently require a local node.`; + } return null; } @@ -345,7 +374,7 @@ scheduledTasksRouter.get('/', (req: Request, res: Response): void => { } }); -scheduledTasksRouter.post('/', (req: Request, res: Response): void => { +scheduledTasksRouter.post('/', async (req: Request, res: Response): Promise => { try { const { name, target_type, target_id, node_id, action, cron_expression, enabled, @@ -366,8 +395,6 @@ scheduledTasksRouter.post('/', (req: Request, res: Response): void => { const targetErr = validateActionTarget(action, target_type); if (targetErr) { res.status(400).json({ error: targetErr }); return; } - const nodeErr = validateActionNode(action, target_type, node_id, selector_type); - if (nodeErr) { res.status(400).json({ error: nodeErr }); return; } const stackTargetErr = validateStackTarget(target_type, target_id, node_id); if (stackTargetErr) { res.status(400).json({ error: stackTargetErr }); return; } const containerTargetErr = validateContainerTarget(target_type, target_id, node_id); @@ -384,6 +411,7 @@ scheduledTasksRouter.post('/', (req: Request, res: Response): void => { const runAtErr = validateRunAt(run_at); if (runAtErr) { res.status(400).json({ error: runAtErr }); return; } + // Compute normalized IDs early so existence validators can use them. const labelSelector = usesStackLabelSelector(action, target_type, selector_type); const normalizedNodeId = labelSelector ? (node_id == null || node_id === '' ? null : parsePositiveNodeId(node_id)) @@ -394,7 +422,9 @@ scheduledTasksRouter.post('/', (req: Request, res: Response): void => { const normalizedTargetId = target_type === 'stack' || target_type === 'container' ? target_id : null; - // Permission check on the resolved action+target scope. + // Permission check on the resolved action+target scope. Runs before + // existence validators so unauthorized callers cannot probe whether a + // stack or container exists on a node they should not reach. if (!requireTaskPermission(req, res, { action, target_type, @@ -403,6 +433,16 @@ scheduledTasksRouter.post('/', (req: Request, res: Response): void => { selector_type: labelSelector ? STACK_LABEL_SELECTOR : null, })) return; + // Node existence validation for fleet and system actions. Runs after + // permission so unauthorized callers cannot probe node IDs via the error + // code difference (400 "node doesn't exist" vs 403 "permission denied"). + const nodeErr = validateActionNode(action, target_type, node_id, selector_type); + if (nodeErr) { res.status(400).json({ error: nodeErr }); return; } + + // Validate target existence for stack and container targets on local nodes. + const targetExistErr = await validateTargetExists(target_type, normalizedTargetId, normalizedNodeId); + if (targetExistErr) { res.status(400).json({ error: targetExistErr }); return; } + const scheduler = SchedulerService.getInstance(); const now = Date.now(); const pinnedRunAt = typeof run_at === 'number' ? run_at : null; @@ -460,7 +500,7 @@ scheduledTasksRouter.get('/:id', (req: Request, res: Response): void => { } }); -scheduledTasksRouter.put('/:id', (req: Request, res: Response): void => { +scheduledTasksRouter.put('/:id', async (req: Request, res: Response): Promise => { try { const id = parseIntParam(req, res, 'id', 'task ID'); if (id === null) return; @@ -506,9 +546,6 @@ scheduledTasksRouter.put('/:id', (req: Request, res: Response): void => { const targetErr = validateActionTarget(finalAction, finalTargetType); if (targetErr) { res.status(400).json({ error: targetErr }); return; } - const nodeErr = validateActionNode(finalAction, finalTargetType, finalNodeId, finalSelectorType); - if (nodeErr) { res.status(400).json({ error: nodeErr }); return; } - const stackTargetErr = validateStackTarget(finalTargetType, finalTargetId, finalNodeId); if (stackTargetErr) { res.status(400).json({ error: stackTargetErr }); return; } @@ -600,14 +637,26 @@ scheduledTasksRouter.put('/:id', (req: Request, res: Response): void => { } // Second phase: the caller must have permission for the merged scope. + const parsedFinalNodeId = finalNodeId != null ? parsePositiveNodeId(finalNodeId) : null; if (!requireTaskPermission(req, res, { action: finalAction, target_type: finalTargetType, target_id: finalTargetId, - node_id: finalNodeId != null ? parsePositiveNodeId(finalNodeId) : null, + node_id: parsedFinalNodeId, selector_type: finalSelectorType, })) return; + // Node existence validation for fleet and system actions. Runs after + // both permission phases so unauthorized callers cannot probe node IDs. + const nodeErr = validateActionNode(finalAction, finalTargetType, finalNodeId, finalSelectorType); + if (nodeErr) { res.status(400).json({ error: nodeErr }); return; } + + // Validate target existence for stack and container targets on local nodes. + // Runs after both permission phases so unauthorized callers cannot probe + // whether a stack or container exists on a target they cannot access. + const targetExistErr = await validateTargetExists(finalTargetType, finalTargetId, parsedFinalNodeId); + if (targetExistErr) { res.status(400).json({ error: targetExistErr }); return; } + db.updateScheduledTask(id, updates); console.log(`[ScheduledTasks] Updated task id=${id}`); const task = db.getScheduledTask(id);