diff --git a/src/utils/__tests__/sessionStorage.test.ts b/src/utils/__tests__/sessionStorage.test.ts new file mode 100644 index 0000000000..b0cf13a5f4 --- /dev/null +++ b/src/utils/__tests__/sessionStorage.test.ts @@ -0,0 +1,151 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' +import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' + +const MAX_CACHED_ENTRIES = 200 // mirrors MAX_CACHED_SESSION_FILES in sessionStorage.ts + +const { + getSessionMessages, + getSessionMessagesCache, + clearSessionMessagesCache, +} = await import('../sessionStorage.js') + +function asUuid(s: string): any { + return s as unknown as any +} + +let tempDir: string +let originalConfigDir: string | undefined + +beforeEach(() => { + tempDir = join( + tmpdir(), + `claude-session-test-${Date.now()}-${Math.random().toString(36).slice(2)}`, + ) + mkdirSync(tempDir, { recursive: true }) + // `getProjectsDir()` returns `${CLAUDE_CONFIG_DIR}/projects`, and + // loadSessionFile reads from `${getProjectsDir()}/${sessionId}.jsonl`. + // Pre-create the projects subdir so writeFileSync doesn't fail. + mkdirSync(join(tempDir, 'projects'), { recursive: true }) + // Pin session-file lookups to a temp dir via CLAUDE_CONFIG_DIR. + // Restoring in afterEach keeps tests hermetic. + originalConfigDir = process.env.CLAUDE_CONFIG_DIR + process.env.CLAUDE_CONFIG_DIR = tempDir +}) + +afterEach(() => { + clearSessionMessagesCache() + if (originalConfigDir === undefined) { + delete process.env.CLAUDE_CONFIG_DIR + } else { + process.env.CLAUDE_CONFIG_DIR = originalConfigDir + } + if (tempDir && existsSync(tempDir)) { + rmSync(tempDir, { recursive: true, force: true }) + } +}) + +function sessionFilePath(sessionId: string): string { + // Mirror sessionStorage.ts's path computation: + // getSessionProjectDir() ?? getProjectDir(getOriginalCwd()) + // With CLAUDE_CONFIG_DIR=tempDir and getSessionProjectDir() returning + // null in tests, files live at `${tempDir}/projects/${sessionId}.jsonl`. + return join(tempDir, 'projects', `${sessionId}.jsonl`) +} + +describe('getSessionMessagesCache', () => { + test('returns the same Map instance across calls', () => { + // Cache identity must be stable — `getLastSessionLog` uses + // `getSessionMessagesCache()` directly to prime entries, so a + // different instance each call would break that priming. + expect(getSessionMessagesCache()).toBe(getSessionMessagesCache()) + }) + + test('clearSessionMessagesCache empties a populated cache', async () => { + const cache = getSessionMessagesCache() + writeFileSync(sessionFilePath('id-1'), '') + writeFileSync(sessionFilePath('id-2'), '') + await getSessionMessages(asUuid('id-1')) + await getSessionMessages(asUuid('id-2')) + expect(cache.size).toBeGreaterThan(0) + + clearSessionMessagesCache() + expect(cache.size).toBe(0) + }) + + test('clearSessionMessagesCache is a no-op on empty cache', () => { + const cache = getSessionMessagesCache() + expect(cache.size).toBe(0) + clearSessionMessagesCache() + expect(cache.size).toBe(0) + }) + + test('getSessionMessages dedups concurrent calls for the same sessionId', async () => { + const cache = getSessionMessagesCache() + const id = asUuid('same-id') + writeFileSync(sessionFilePath('same-id'), '') + const [a, b, c] = await Promise.all([ + getSessionMessages(id), + getSessionMessages(id), + getSessionMessages(id), + ]) + expect(a).toBe(b) + expect(b).toBe(c) + expect(cache.size).toBe(1) + }) +}) + +describe('getSessionMessages bounded cache (memory leak fix)', () => { + test('cache size stays at MAX_CACHED_ENTRIES after many distinct sessionIds', async () => { + // Bounded cache — calling getSessionMessages with N distinct + // sessionIds must NOT grow the cache beyond MAX_CACHED_ENTRIES. + // Pre-fix: lodash memoize grew unbounded. Post-fix: Map-based + // cache evicts oldest entry when at capacity. + const cache = getSessionMessagesCache() + const total = MAX_CACHED_ENTRIES * 3 // 600 distinct sessionIds + for (let i = 0; i < total; i++) { + writeFileSync(sessionFilePath(`id-${i}`), '') + await getSessionMessages(asUuid(`id-${i}`)) + } + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + }) + + test('FIFO eviction: oldest entry is removed first', async () => { + // Fill cache to MAX with sequential ids. The first inserted + // (`oldest`) should be evicted on the (MAX+1)th insertion. + const cache = getSessionMessagesCache() + const oldestId = asUuid('id-0') + writeFileSync(sessionFilePath('id-0'), '') + await getSessionMessages(oldestId) + for (let i = 1; i < MAX_CACHED_ENTRIES; i++) { + writeFileSync(sessionFilePath(`id-${i}`), '') + await getSessionMessages(asUuid(`id-${i}`)) + } + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + expect(cache.has(oldestId)).toBe(true) + + writeFileSync(sessionFilePath('id-overflow'), '') + await getSessionMessages(asUuid('id-overflow')) + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + expect(cache.has(oldestId)).toBe(false) + }) + + test('cleared cache can be refilled without leaking entries', async () => { + const cache = getSessionMessagesCache() + for (let i = 0; i < MAX_CACHED_ENTRIES; i++) { + writeFileSync(sessionFilePath(`id-${i}`), '') + await getSessionMessages(asUuid(`id-${i}`)) + } + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + + clearSessionMessagesCache() + expect(cache.size).toBe(0) + + for (let i = 0; i < MAX_CACHED_ENTRIES + 5; i++) { + writeFileSync(sessionFilePath(`refill-${i}`), '') + await getSessionMessages(asUuid(`refill-${i}`)) + } + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + }) +}) diff --git a/src/utils/permissions/__tests__/pathValidation.test.ts b/src/utils/permissions/__tests__/pathValidation.test.ts new file mode 100644 index 0000000000..7c9716cae0 --- /dev/null +++ b/src/utils/permissions/__tests__/pathValidation.test.ts @@ -0,0 +1,203 @@ +import { describe, expect, mock, test } from 'bun:test' +import { logMock } from '../../../../tests/mocks/log' +import { debugMock } from '../../../../tests/mocks/debug' + +// Cut the bootstrap/state dependency chain (mock.module requirement). +mock.module('src/utils/log.ts', logMock) +mock.module('src/utils/debug.ts', debugMock) +mock.module('bun:bundle', () => ({ + feature: (_name: string) => false, +})) + +// MACRO is a build-time define injected by `bun --define` (see +// scripts/dev.ts → -d flags). Without it, `declare const MACRO` references +// in source code resolve to `undefined` at runtime and crash any function +// that touches `MACRO.VERSION` (e.g. `getBundledSkillsRoot` via +// `checkReadableInternalPath`). +// Setting it on globalThis lets the bare `MACRO` identifier resolve at +// runtime in tests. +;(globalThis as unknown as { MACRO: { VERSION: string } }).MACRO = { + VERSION: 'test', +} + +const { validatePath } = await import('../pathValidation.js') +const { getEmptyToolPermissionContext } = await import('../../../Tool.js') + +function makeContext(): ReturnType { + return getEmptyToolPermissionContext() +} + +const isWindows = process.platform === 'win32' +const describeIfWindows = isWindows ? describe : describe.skip + +// ─── MinGW path normalization (Windows) ────────────────────────────────── +// +// These tests pin the fix for a sandbox-escape class: on Windows, the Git +// Bash shell interprets paths like `/c/Users/foo/bar.txt` as `C:\Users\foo\bar.txt` +// (the C: drive). However, the Node `path` module treats such paths as +// drive-relative absolute paths on the current drive, so: +// - path.isAbsolute('/c/Users/foo/bar.txt') === true (on Windows) +// - path.resolve('D:\\project', '/c/Users/foo/bar.txt') +// === 'D:\\c\\Users\\foo\\bar.txt' (on Windows) +// +// That means without normalization, validatePath would compare +// `D:\c\Users\foo\bar.txt` against the allowed-directories list, while +// Git Bash actually writes to `C:\Users\foo\bar.txt` — a completely +// different filesystem location. This is a TOCTOU/sandbox-escape bug. +// +// The fix runs `posixPathToWindowsPath` on Windows before resolution, +// converting `/c/...` and `/cygdrive/c/...` to their `C:\...` form so the +// validator's path space matches the shell's. +/** + * Tests that `validatePath` normalizes MinGW-style absolute paths + * (`/c/Users/foo`, `/cygdrive/c/Users/foo`) to Windows paths + * (`C:\\Users\\foo`) on Windows. Without this, the validator runs in + * Windows host path space while the Git Bash shell runs in MinGW path + * space, leading to a sandbox-escape class — see the comment block + * at the top of this file for the full security rationale. + */ +describeIfWindows('validatePath MinGW path normalization', () => { + test('converts /c/Users/foo/file.txt to C:\\Users\\foo\\file.txt', () => { + const result = validatePath( + '/c/Users/foo/file.txt', + 'D:\\project', + makeContext(), + 'read', + ) + // resolvedPath is the canonical form the validator (and ultimately the + // shell) operates on. It must be the Windows-style path, not the + // drive-relative form `D:\c\Users\foo\file.txt`. + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'C:\\Users\\foo\\file.txt', + ) + }) + + test('converts /cygdrive/c/Users/foo/file.txt to C:\\Users\\foo\\file.txt', () => { + const result = validatePath( + '/cygdrive/c/Users/foo/file.txt', + 'D:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'C:\\Users\\foo\\file.txt', + ) + }) + + test('uppercases the drive letter', () => { + const result = validatePath( + '/d/work/file.txt', + 'C:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe('D:\\work\\file.txt') + }) + + test('preserves Windows paths unchanged', () => { + // An already-Windows path should not be touched. + const result = validatePath( + 'C:\\Users\\foo\\file.txt', + 'D:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'C:\\Users\\foo\\file.txt', + ) + }) + + test('preserves relative paths (just flips slashes)', () => { + // Relative paths are not MinGW absolute paths; the conversion + // should be a no-op aside from slash direction. The path is then + // resolved against cwd by `validatePath`, which is expected behavior. + const result = validatePath( + 'src/file.txt', + 'D:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'D:\\project\\src\\file.txt', + ) + }) + + test('handles bare drive mount (no trailing path)', () => { + const result = validatePath('/c', 'D:\\project', makeContext(), 'read') + expect(result.resolvedPath.replace(/\//g, '\\')).toBe('C:\\') + }) + + test('handles drive root with trailing slash', () => { + const result = validatePath('/c/', 'D:\\project', makeContext(), 'read') + expect(result.resolvedPath.replace(/\//g, '\\')).toBe('C:\\') + }) + + test('handles deeply nested MinGW paths', () => { + const result = validatePath( + '/c/Users/me/Documents/project/src/index.ts', + 'D:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'C:\\Users\\me\\Documents\\project\\src\\index.ts', + ) + }) +}) + +// ─── Sandbox escape regression (Windows) ───────────────────────────────── +// +// This is the bug the MinGW-normalization fix exists to prevent: without +// it, the validator compares `:\c\Users\foo\file.txt` against +// the allowed dirs, while bash writes to `C:\Users\foo\file.txt`. With the +// fix, both sides of the comparison use the same `C:\Users\foo\file.txt` +// location. +// +// We pin this by setting up a context where: +// - cwd is `D:\project` (and D:\project is allowed) +// - `C:\Users\foo` is NOT in any allowed directory +// Then we check that `/c/Users/foo/sensitive.txt` is denied with a +// resolvedPath pointing at C:\Users\foo — proving the validator now sees +// the same path the shell will write to. +/** + * Regression tests for the sandbox-escape class the MinGW-normalization + * fix prevents. Without the fix, a MinGW-style path like + * `/c/Users/foo/sensitive.txt` is resolved (by `path.resolve`) against + * the current drive (`D:\c\Users\foo\sensitive.txt`) and compared to + * the allowed-directories list — while Git Bash actually writes to + * `C:\Users\foo\sensitive.txt`. With the fix, both sides of the + * comparison use the same Windows path so a path the shell will write + * to but isn't in any allowed dir is denied. + */ +describeIfWindows('validatePath sandbox escape regression', () => { + test('MinGW path that escapes allowed dirs is denied at correct location', () => { + // Without the fix, this would resolve to `D:\c\Users\foo\sensitive.txt` + // and (if D:\ is broadly allowed) pass validation, while bash actually + // writes to `C:\Users\foo\sensitive.txt`. With the fix, the validator + // sees the correct path and denies it because C:\Users\foo is not in + // any allowed directory. + const result = validatePath( + '/c/Users/foo/sensitive.txt', + 'D:\\project', + makeContext(), + 'create', + ) + expect(result.allowed).toBe(false) + // The resolvedPath should be at C:\Users\foo — not D:\c\Users\foo. + const normalized = result.resolvedPath.replace(/\//g, '\\') + expect(normalized.startsWith('C:\\Users\\foo')).toBe(true) + expect(normalized.startsWith('D:\\c\\')).toBe(false) + }) + + test('cygdrive path that escapes allowed dirs is denied at correct location', () => { + const result = validatePath( + '/cygdrive/c/Users/foo/sensitive.txt', + 'D:\\project', + makeContext(), + 'create', + ) + expect(result.allowed).toBe(false) + const normalized = result.resolvedPath.replace(/\//g, '\\') + expect(normalized.startsWith('C:\\Users\\foo')).toBe(true) + }) +}) diff --git a/src/utils/permissions/pathValidation.ts b/src/utils/permissions/pathValidation.ts index c8fdf7f8fb..76cf83b651 100644 --- a/src/utils/permissions/pathValidation.ts +++ b/src/utils/permissions/pathValidation.ts @@ -3,6 +3,7 @@ import { homedir } from 'os' import { dirname, isAbsolute, resolve } from 'path' import type { ToolPermissionContext } from '../../Tool.js' import { getPlatform } from '../../utils/platform.js' +import { posixPathToWindowsPath } from '../windowsPaths.js' import { getFsImplementation, getPathsForPermissionCheck, @@ -35,6 +36,12 @@ export type ResolvedPathCheckResult = PathCheckResult & { resolvedPath: string } +/** + * Format a list of working directories for inclusion in permission-prompt + * messages. When the list is short (≤ {@link MAX_DIRS_TO_LIST}) all + * directories are listed; when longer, a truncated form is shown with + * the remaining count. + */ export function formatDirectoryList(directories: string[]): string { const dirCount = directories.length @@ -472,9 +479,27 @@ export function validatePath( } // Resolve path - const absolutePath = isAbsolute(cleanPath) - ? cleanPath - : resolve(cwd, cleanPath) + // SECURITY: On Windows, normalize MinGW-style absolute paths + // (`/c/Users/foo`, `/cygdrive/c/Users/foo`) to Windows paths + // (`C:\Users\foo`) BEFORE the isAbsolute/resolve step below. + // + // Without this, `path.isAbsolute('/c/Users/foo')` returns true on + // Windows (interpreted as a drive-relative absolute path) and + // `path.resolve(cwd, '/c/Users/foo')` produces `:\c\Users\foo` + // — a completely different filesystem location from what Git Bash + // actually writes to (`C:\Users\foo`). The validator would compare + // the wrong path against the allowed-directories list, enabling a + // sandbox escape where `/c/Users/foo/sensitive.txt` passes validation + // when `C:\Users\foo\sensitive.txt` is not in any allowed dir. + // + // `posixPathToWindowsPath` is a no-op for already-Windows paths and + // for relative paths (it just flips slashes), so it's safe to apply + // unconditionally on Windows. + const pathForResolve = + getPlatform() === 'windows' ? posixPathToWindowsPath(cleanPath) : cleanPath + const absolutePath = isAbsolute(pathForResolve) + ? pathForResolve + : resolve(cwd, pathForResolve) const { resolvedPath, isCanonical } = safeResolvePath( getFsImplementation(), absolutePath, diff --git a/src/utils/sessionStorage.ts b/src/utils/sessionStorage.ts index 92e87e5c05..d6da26184a 100644 --- a/src/utils/sessionStorage.ts +++ b/src/utils/sessionStorage.ts @@ -3941,24 +3941,55 @@ async function loadSessionFile(sessionId: UUID): Promise<{ return loadTranscriptFile(sessionFile) } +/** + * Bounded cache for {@link getSessionMessages}. Bounded at + * {@link MAX_CACHED_SESSION_FILES} to prevent unbounded Map growth in + * long-running daemon / swarm sessions that spawn many agents — mirrors + * the existingSessionFiles pattern above. Entries are evicted in FIFO + * order via `Map` insertion order. + */ +const sessionMessagesCache = new Map>>() + /** * Gets message UUIDs for a specific session without loading all sessions. - * Memoized to avoid re-reading the same session file multiple times. + * Cached (bounded) to avoid re-reading the same session file multiple + * times. Concurrent calls for the same `sessionId` share one in-flight + * load promise. + * + * Exported so tests can verify cache eviction behavior directly; not + * intended as a public API — prefer {@link doesMessageExistInSession}. */ -const getSessionMessages = memoize( - async (sessionId: UUID): Promise> => { +export async function getSessionMessages(sessionId: UUID): Promise> { + const existing = sessionMessagesCache.get(sessionId) + if (existing !== undefined) { + return existing + } + // Evict oldest entry when at capacity so the Map stays bounded. + if (sessionMessagesCache.size >= MAX_CACHED_SESSION_FILES) { + const oldestKey = sessionMessagesCache.keys().next().value + if (oldestKey !== undefined) { + sessionMessagesCache.delete(oldestKey) + } + } + const promise = (async () => { const { messages } = await loadSessionFile(sessionId) return new Set(messages.keys()) - }, - (sessionId: UUID) => sessionId, -) + })() + sessionMessagesCache.set(sessionId, promise) + return promise +} + +/** Underlying cache for direct manipulation (priming, clearing, tests). */ +export function getSessionMessagesCache(): Map>> { + return sessionMessagesCache +} /** - * Clear the memoized session messages cache. + * Clear the cached session messages. * Call after compaction when old message UUIDs are no longer valid. */ export function clearSessionMessagesCache(): void { - getSessionMessages.cache.clear?.() + sessionMessagesCache.clear() } /** @@ -3996,11 +4027,9 @@ export async function getLastSessionLog( // Guard: only prime if cache is empty. Mid-session callers (e.g. IssueFeedback) // may call getLastSessionLog on the current session — overwriting a live cache // with a stale disk snapshot would lose unflushed UUIDs and break dedup. - if (!getSessionMessages.cache.has(sessionId)) { - getSessionMessages.cache.set( - sessionId, - Promise.resolve(new Set(messages.keys())), - ) + const messagesCache = getSessionMessagesCache() + if (!messagesCache.has(sessionId)) { + messagesCache.set(sessionId, Promise.resolve(new Set(messages.keys()))) } // Find the most recent non-sidechain message