diff --git a/src/main/ipc/content-handlers.ts b/src/main/ipc/content-handlers.ts index 5f923a0..1727f8d 100644 --- a/src/main/ipc/content-handlers.ts +++ b/src/main/ipc/content-handlers.ts @@ -9,6 +9,8 @@ import type { MessageAttachment } from '../../shared/types' import { ConversationModel } from '../database/models/conversation' import { MessageModel } from '../database/models/message' import { validateFilePath } from '../security/input-validator' +import { getFileAccessManager } from '../file-access/file-access-manager' +import { isPathWithinAllowedDirs } from '../../shared/file-access-types' import { wrapHandler, requireFeature } from './ipc-utils' export async function registerContentHandlers(): Promise { @@ -197,6 +199,13 @@ export async function registerContentHandlers(): Promise { if (!pathValidation.valid) { return { success: false, error: pathValidation.error } } + // Constrain reads to the granted-directory model — validateFilePath only + // blocks '..'/length, so without this any absolute path was readable + // (security audit 2026-08-21). + const allowedDirs = getFileAccessManager().getAllowedDirectories().map((d) => d.path) + if (!isPathWithinAllowedDirs(filePath, allowedDirs)) { + return { success: false, error: 'Access denied: file is not within a granted directory' } + } const content = await fs.readFile(filePath, 'utf-8') return { success: true, content } } catch (error) { diff --git a/src/main/utils/mcp-sanitizer.ts b/src/main/utils/mcp-sanitizer.ts index 19bdeba..4ca8beb 100644 --- a/src/main/utils/mcp-sanitizer.ts +++ b/src/main/utils/mcp-sanitizer.ts @@ -63,19 +63,23 @@ export function validateCommand(command: string): SanitizeResult { // Extract base command name (handle paths like /usr/bin/python3) const baseName = trimmed.split('/').pop()?.split('\\').pop() || '' + const isPath = trimmed.includes('/') || trimmed.includes('\\') - // Allow whitelisted commands - if (ALLOWED_COMMANDS.has(baseName)) { + // The basename allowlist applies ONLY to bare command names. A PATH must be + // validated as a path — a renamed binary in an unsafe directory + // (e.g. /tmp/evil/python3, ./evil/python3) must not pass just because its + // basename is allowlisted (security audit 2026-08-21). + if (!isPath && ALLOWED_COMMANDS.has(baseName)) { return { valid: true } } - // Allow absolute paths only from safe directories - if (trimmed.startsWith('/') || trimmed.startsWith('~') || /^[A-Z]:\\/.test(trimmed)) { - const resolved = trimmed.startsWith('~') - ? trimmed // tilde paths resolve at spawn time; validate structure only - : trimmed - const inSafeDir = SAFE_COMMAND_DIRS.some(dir => resolved.startsWith(dir)) - if (!inSafeDir && !trimmed.startsWith('~')) { + // Allow absolute paths only from safe directories. + if (isPath && (trimmed.startsWith('/') || trimmed.startsWith('~') || /^[A-Z]:\\/.test(trimmed))) { + if (trimmed.startsWith('~')) { + return { valid: true } // tilde resolves at spawn time; validate structure only + } + const inSafeDir = SAFE_COMMAND_DIRS.some(dir => trimmed.startsWith(dir)) + if (!inSafeDir) { return { valid: false, error: `Absolute path "${trimmed}" is outside safe directories. Allowed: ${SAFE_COMMAND_DIRS.join(', ')}` diff --git a/src/shared/file-access-types.ts b/src/shared/file-access-types.ts index cd88dd1..5fe697e 100644 --- a/src/shared/file-access-types.ts +++ b/src/shared/file-access-types.ts @@ -99,3 +99,11 @@ export function isPathSafe(requestedPath: string, allowedDir: string): boolean { // the grant for /a/photos (security audit 2026-08-21). return resolved === allowed || resolved.startsWith(allowed + path.sep) } + +/** + * Security: is the path inside ANY of the granted directories? + * Used to constrain raw file reads to the user's granted-directory model. + */ +export function isPathWithinAllowedDirs(requestedPath: string, allowedDirPaths: string[]): boolean { + return allowedDirPaths.some((dir) => isPathSafe(requestedPath, dir)) +} diff --git a/tests/unit/file-access-types.test.ts b/tests/unit/file-access-types.test.ts index 6f254fa..8319d9b 100644 --- a/tests/unit/file-access-types.test.ts +++ b/tests/unit/file-access-types.test.ts @@ -21,3 +21,19 @@ describe('isPathSafe', () => { expect(isPathSafe('/a/photos/../etc/passwd', '/a/photos')).toBe(false) }) }) + +import { isPathWithinAllowedDirs } from '../../src/shared/file-access-types' +describe('isPathWithinAllowedDirs', () => { + it('accepts a path inside one of the granted dirs', () => { + expect(isPathWithinAllowedDirs('/a/photos/x', ['/a/docs', '/a/photos'])).toBe(true) + }) + it('rejects a path outside every granted dir', () => { + expect(isPathWithinAllowedDirs('/etc/passwd', ['/a/docs', '/a/photos'])).toBe(false) + }) + it('rejects a sibling-prefix escape of a granted dir', () => { + expect(isPathWithinAllowedDirs('/a/photos-evil/x', ['/a/photos'])).toBe(false) + }) + it('rejects everything when no directory is granted', () => { + expect(isPathWithinAllowedDirs('/a/photos/x', [])).toBe(false) + }) +}) diff --git a/tests/unit/mcp-sanitizer.test.ts b/tests/unit/mcp-sanitizer.test.ts index c359262..9067f27 100644 --- a/tests/unit/mcp-sanitizer.test.ts +++ b/tests/unit/mcp-sanitizer.test.ts @@ -160,3 +160,15 @@ describe('MCP Sanitizer', () => { }) }) }) + +// Security regression (audit 2026-08-21): the basename allowlist was checked +// BEFORE the safe-directory rule, so a renamed binary passed on basename alone. +describe('validateCommand — path validated before basename allowlist', () => { + it('rejects an allowlisted basename in an UNSAFE absolute dir', () => { + expect(validateCommand('/tmp/evil/python3').valid).toBe(false) + }) + it('rejects an allowlisted basename via a RELATIVE path', () => { + expect(validateCommand('./evil/python3').valid).toBe(false) + expect(validateCommand('evil/python3').valid).toBe(false) + }) +})