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
This commit is contained in:
@@ -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',
|
||||
|
||||
@@ -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 Record<string, unknown>).createdById).toBeUndefined();
|
||||
expect((result.apps[0] as unknown as Record<string, unknown>).id).toBeUndefined();
|
||||
expect((result.apps[0] as unknown as Record<string, unknown>).createdById).toBeUndefined();
|
||||
});
|
||||
|
||||
it('maps boards with nested sections and widgets', async () => {
|
||||
|
||||
@@ -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 }
|
||||
);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user