From 395ed821b72436a08f6c8653d9dab8fda7e5c612 Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Wed, 25 Mar 2026 01:28:24 +0300 Subject: [PATCH] fix: address all final review findings for Phase 3 - CRITICAL: Fix command injection in discoveryService (execFile instead of exec, path validation regex) - CRITICAL: Add Zod validation on discover API endpoint - HIGH: Add Zod validation on discover/approve endpoint - HIGH: Add array length limits to import schema (1000/100/100) - HIGH: Fix theme broadcast echo loop (setTimeout vs queueMicrotask) - MEDIUM: Singleton BroadcastChannel instead of create-per-send - MEDIUM: Exclude sensitive APIs from service worker cache - MEDIUM: Fix TypeScript cast errors in exportService tests --- .../__tests__/discoveryService.test.ts | 18 ++++----- .../services/__tests__/exportService.test.ts | 4 +- src/lib/server/services/discoveryService.ts | 15 +++++--- src/lib/stores/theme.svelte.ts | 6 +-- src/lib/utils/broadcastSync.ts | 10 +++-- src/lib/utils/validators.ts | 6 +-- src/routes/api/admin/discover/+server.ts | 19 ++++++++-- .../api/admin/discover/approve/+server.ts | 37 +++++++++---------- src/service-worker.ts | 7 ++++ 9 files changed, 72 insertions(+), 50 deletions(-) diff --git a/src/lib/server/services/__tests__/discoveryService.test.ts b/src/lib/server/services/__tests__/discoveryService.test.ts index 7b7a4fb..bfa06d3 100644 --- a/src/lib/server/services/__tests__/discoveryService.test.ts +++ b/src/lib/server/services/__tests__/discoveryService.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; // Mock child_process for Docker discovery vi.mock('node:child_process', () => ({ - exec: vi.fn() + execFile: vi.fn() })); vi.mock('node:util', () => ({ @@ -14,11 +14,11 @@ vi.mock('../appService.js', () => ({ findAll: vi.fn() })); -import { exec } from 'node:child_process'; +import { execFile } from 'node:child_process'; import { findAll as findAllApps } from '../appService.js'; import { discoverDocker, discoverTraefik, discoverAll } from '../discoveryService.js'; -const mockExec = exec as unknown as ReturnType; +const mockExecFile = execFile as unknown as ReturnType; const mockFindAllApps = findAllApps as ReturnType; describe('discoveryService', () => { @@ -39,7 +39,7 @@ describe('discoveryService', () => { } ]; - mockExec.mockResolvedValue({ stdout: JSON.stringify(containers) }); + mockExecFile.mockResolvedValue({ stdout: JSON.stringify(containers) }); const result = await discoverDocker('/var/run/docker.sock'); @@ -65,7 +65,7 @@ describe('discoveryService', () => { } ]; - mockExec.mockResolvedValue({ stdout: JSON.stringify(containers) }); + mockExecFile.mockResolvedValue({ stdout: JSON.stringify(containers) }); const result = await discoverDocker('/var/run/docker.sock'); @@ -85,7 +85,7 @@ describe('discoveryService', () => { } ]; - mockExec.mockResolvedValue({ stdout: JSON.stringify(containers) }); + mockExecFile.mockResolvedValue({ stdout: JSON.stringify(containers) }); const result = await discoverDocker('/var/run/docker.sock'); @@ -93,7 +93,7 @@ describe('discoveryService', () => { }); it('returns error when Docker socket is inaccessible', async () => { - mockExec.mockRejectedValue(new Error('connect ENOENT /var/run/docker.sock')); + mockExecFile.mockRejectedValue(new Error('connect ENOENT /var/run/docker.sock')); const result = await discoverDocker('/var/run/docker.sock'); @@ -178,7 +178,7 @@ describe('discoveryService', () => { describe('discoverAll', () => { it('marks already-registered services', async () => { - mockExec.mockResolvedValue({ + mockExecFile.mockResolvedValue({ stdout: JSON.stringify([ { Id: 'c1', @@ -202,7 +202,7 @@ describe('discoveryService', () => { }); it('deduplicates by URL preferring Traefik', async () => { - mockExec.mockResolvedValue({ + mockExecFile.mockResolvedValue({ stdout: JSON.stringify([ { Id: 'c1', diff --git a/src/lib/server/services/__tests__/exportService.test.ts b/src/lib/server/services/__tests__/exportService.test.ts index 972ea1f..cbadcc4 100644 --- a/src/lib/server/services/__tests__/exportService.test.ts +++ b/src/lib/server/services/__tests__/exportService.test.ts @@ -102,8 +102,8 @@ describe('exportService', () => { healthcheckTimeout: 5000 }); // Internal fields should not be present - expect((result.apps[0] as Record).id).toBeUndefined(); - expect((result.apps[0] as Record).createdById).toBeUndefined(); + expect((result.apps[0] as unknown as Record).id).toBeUndefined(); + expect((result.apps[0] as unknown as Record).createdById).toBeUndefined(); }); it('maps boards with nested sections and widgets', async () => { diff --git a/src/lib/server/services/discoveryService.ts b/src/lib/server/services/discoveryService.ts index 0906e7a..100fdb9 100644 --- a/src/lib/server/services/discoveryService.ts +++ b/src/lib/server/services/discoveryService.ts @@ -1,8 +1,8 @@ -import { exec } from 'node:child_process'; +import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { findAll as findAllApps } from './appService.js'; -const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); // --- Types --- @@ -97,10 +97,15 @@ export async function discoverDocker(socketPath: string): Promise<{ readonly services: readonly DiscoveredService[]; readonly error?: string; }> { + if (!/^[\w/.:-]+$/.test(socketPath)) { + return { services: [], error: 'Invalid Docker socket path' }; + } + try { - // Use curl with Unix socket to query Docker API - const { stdout } = await execAsync( - `curl -s --unix-socket "${socketPath}" http://localhost/containers/json?all=false`, + // Use execFile (no shell) to avoid command injection + const { stdout } = await execFileAsync( + 'curl', + ['-s', '--unix-socket', socketPath, 'http://localhost/containers/json?all=false'], { timeout: 10000 } ); diff --git a/src/lib/stores/theme.svelte.ts b/src/lib/stores/theme.svelte.ts index ce0d4d4..bf910ee 100644 --- a/src/lib/stores/theme.svelte.ts +++ b/src/lib/stores/theme.svelte.ts @@ -151,10 +151,10 @@ class ThemeStore { this.primaryHue = values.primaryHue; this.primarySaturation = values.primarySaturation; this.backgroundType = values.backgroundType; - // Re-enable on next microtask so the effect reads suppressBroadcast=true - queueMicrotask(() => { + // Use setTimeout to ensure all Svelte 5 effects have fired before re-enabling broadcast + setTimeout(() => { this.#suppressBroadcast = false; - }); + }, 100); } /** diff --git a/src/lib/utils/broadcastSync.ts b/src/lib/utils/broadcastSync.ts index feebe42..5564426 100644 --- a/src/lib/utils/broadcastSync.ts +++ b/src/lib/utils/broadcastSync.ts @@ -21,10 +21,15 @@ export interface DataChangeMessage { export type SyncMessage = ThemeChangeMessage | DataChangeMessage; +/** Singleton channel instance, lazily created and reused for all sends and listens. */ +let singletonChannel: BroadcastChannel | null = null; + function getChannel(): BroadcastChannel | null { if (typeof window === 'undefined') return null; + if (singletonChannel) return singletonChannel; try { - return new BroadcastChannel(CHANNEL_NAME); + singletonChannel = new BroadcastChannel(CHANNEL_NAME); + return singletonChannel; } catch { return null; } @@ -38,7 +43,6 @@ export function broadcastThemeChange(theme: ThemeChangeMessage['payload']): void if (!channel) return; const message: ThemeChangeMessage = { type: 'theme-change', payload: theme }; channel.postMessage(message); - channel.close(); } /** @@ -49,7 +53,6 @@ export function broadcastDataChange(entity: 'board' | 'app' | 'widget'): void { if (!channel) return; const message: DataChangeMessage = { type: 'data-change', payload: { entity } }; channel.postMessage(message); - channel.close(); } /** @@ -70,6 +73,5 @@ export function onSyncMessage(callback: (msg: SyncMessage) => void): () => void return () => { channel.removeEventListener('message', handler); - channel.close(); }; } diff --git a/src/lib/utils/validators.ts b/src/lib/utils/validators.ts index 77f4ccd..f3e47c6 100644 --- a/src/lib/utils/validators.ts +++ b/src/lib/utils/validators.ts @@ -240,9 +240,9 @@ const importSettingsSchema = z.object({ export const importDataSchema = z.object({ version: z.string(), exportedAt: z.string(), - apps: z.array(importAppSchema), - boards: z.array(importBoardSchema), - groups: z.array(importGroupSchema), + apps: z.array(importAppSchema).max(1000), + boards: z.array(importBoardSchema).max(100), + groups: z.array(importGroupSchema).max(100), settings: importSettingsSchema }); diff --git a/src/routes/api/admin/discover/+server.ts b/src/routes/api/admin/discover/+server.ts index a2363a2..bb1f303 100644 --- a/src/routes/api/admin/discover/+server.ts +++ b/src/routes/api/admin/discover/+server.ts @@ -1,9 +1,15 @@ import { json } from '@sveltejs/kit'; +import { z } from 'zod'; import type { RequestHandler } from './$types'; import { requireAdmin } from '$lib/server/middleware/authorize.js'; import { discoverAll, type DiscoveryConfig } from '$lib/server/services/discoveryService.js'; import { success, error } from '$lib/server/utils/response.js'; +const discoverConfigSchema = z.object({ + dockerSocketPath: z.string().regex(/^[\w/.:-]+$/).optional(), + traefikApiUrl: z.string().url().optional() +}); + /** * POST /api/admin/discover — Scan Docker and Traefik for services. Admin only. * @@ -12,16 +18,21 @@ import { success, error } from '$lib/server/utils/response.js'; export const POST: RequestHandler = async (event) => { requireAdmin(event); - let body: DiscoveryConfig; + let rawBody: unknown; try { - body = await event.request.json(); + rawBody = await event.request.json(); } catch { return json(error('Invalid JSON body'), { status: 400 }); } + const parsed = discoverConfigSchema.safeParse(rawBody); + if (!parsed.success) { + return json(error(`Validation failed: ${parsed.error.issues.map((i) => i.message).join(', ')}`), { status: 400 }); + } + const config: DiscoveryConfig = { - dockerSocketPath: body.dockerSocketPath || undefined, - traefikApiUrl: body.traefikApiUrl || undefined + dockerSocketPath: parsed.data.dockerSocketPath || undefined, + traefikApiUrl: parsed.data.traefikApiUrl || undefined }; if (!config.dockerSocketPath && !config.traefikApiUrl) { diff --git a/src/routes/api/admin/discover/approve/+server.ts b/src/routes/api/admin/discover/approve/+server.ts index 596d934..4593899 100644 --- a/src/routes/api/admin/discover/approve/+server.ts +++ b/src/routes/api/admin/discover/approve/+server.ts @@ -1,20 +1,19 @@ import { json } from '@sveltejs/kit'; +import { z } from 'zod'; import type { RequestHandler } from './$types'; import { requireAdmin } from '$lib/server/middleware/authorize.js'; import { create } from '$lib/server/services/appService.js'; import { success, error } from '$lib/server/utils/response.js'; -interface ApproveServiceInput { - readonly name: string; - readonly url: string; - readonly source: 'docker' | 'traefik'; - readonly icon?: string; - readonly description?: string; -} - -interface ApproveBody { - readonly services: readonly ApproveServiceInput[]; -} +const approveSchema = z.object({ + services: z.array(z.object({ + name: z.string().min(1), + url: z.string().url(), + source: z.enum(['docker', 'traefik']), + icon: z.string().optional(), + description: z.string().optional() + })).min(1).max(100) +}); /** * POST /api/admin/discover/approve — Approve discovered services and create app entries. Admin only. @@ -24,26 +23,24 @@ interface ApproveBody { export const POST: RequestHandler = async (event) => { const user = requireAdmin(event); - let body: ApproveBody; + let rawBody: unknown; try { - body = await event.request.json(); + rawBody = await event.request.json(); } catch { return json(error('Invalid JSON body'), { status: 400 }); } - if (!body.services || !Array.isArray(body.services) || body.services.length === 0) { - return json(error('At least one service must be provided for approval'), { status: 400 }); + const parsed = approveSchema.safeParse(rawBody); + if (!parsed.success) { + return json(error(`Validation failed: ${parsed.error.issues.map((i) => i.message).join(', ')}`), { status: 400 }); } + const body = parsed.data; + const created: string[] = []; const errors: string[] = []; for (const service of body.services) { - if (!service.name || !service.url) { - errors.push(`Skipped invalid service entry (missing name or url)`); - continue; - } - try { const app = await create({ name: service.name, diff --git a/src/service-worker.ts b/src/service-worker.ts index 2be5451..8144eab 100644 --- a/src/service-worker.ts +++ b/src/service-worker.ts @@ -50,6 +50,13 @@ self.addEventListener('fetch', (event: FetchEvent) => { // Skip cross-origin requests if (url.origin !== self.location.origin) return; + // Sensitive API paths: never cache, always go to network + const sensitiveApiPrefixes = ['/api/users/', '/api/admin/', '/api/auth/']; + if (sensitiveApiPrefixes.some((prefix) => url.pathname.startsWith(prefix))) { + event.respondWith(fetch(request)); + return; + } + // API calls: network-first with cache fallback if (url.pathname.startsWith('/api/')) { event.respondWith(networkFirst(request));