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

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<typeof vi.fn>;
+const mockExecFile = execFile as unknown as ReturnType<typeof vi.fn>;
 const mockFindAllApps = findAllApps as ReturnType<typeof vi.fn>;
 
 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',

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<string, unknown>).id).toBeUndefined();
+			expect((result.apps[0] as unknown as Record<string, unknown>).id).toBeUndefined();
-			expect((result.apps[0] as Record<string, unknown>).createdById).toBeUndefined();
+			expect((result.apps[0] as unknown as Record<string, unknown>).createdById).toBeUndefined();
 		});
 
 		it('maps boards with nested sections and widgets', async () => {

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
+		// Use execFile (no shell) to avoid command injection
-		const { stdout } = await execAsync(
+		const { stdout } = await execFileAsync(
-			`curl -s --unix-socket "${socketPath}" http://localhost/containers/json?all=false`,
+			'curl',
+			['-s', '--unix-socket', socketPath, 'http://localhost/containers/json?all=false'],
 			{ timeout: 10000 }
 		);
 

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
+		// Use setTimeout to ensure all Svelte 5 effects have fired before re-enabling broadcast
-		queueMicrotask(() => {
+		setTimeout(() => {
 			this.#suppressBroadcast = false;
-		});
+		}, 100);
 	}
 
 	/**

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();
 	};
 }

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),
+	apps: z.array(importAppSchema).max(1000),
-	boards: z.array(importBoardSchema),
+	boards: z.array(importBoardSchema).max(100),
-	groups: z.array(importGroupSchema),
+	groups: z.array(importGroupSchema).max(100),
 	settings: importSettingsSchema
 });
 

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,
+		dockerSocketPath: parsed.data.dockerSocketPath || undefined,
-		traefikApiUrl: body.traefikApiUrl || undefined
+		traefikApiUrl: parsed.data.traefikApiUrl || undefined
 	};
 
 	if (!config.dockerSocketPath && !config.traefikApiUrl) {

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 {
+const approveSchema = z.object({
-	readonly name: string;
+	services: z.array(z.object({
-	readonly url: string;
+		name: z.string().min(1),
-	readonly source: 'docker' | 'traefik';
+		url: z.string().url(),
-	readonly icon?: string;
+		source: z.enum(['docker', 'traefik']),
-	readonly description?: string;
+		icon: z.string().optional(),
-}
+		description: z.string().optional()
-
+	})).min(1).max(100)
-interface ApproveBody {
+});
-	readonly services: readonly ApproveServiceInput[];
-}
 
 /**
  * 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) {
+	const parsed = approveSchema.safeParse(rawBody);
-		return json(error('At least one service must be provided for approval'), { status: 400 });
+	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,

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));