fix(session-storage): bound getSessionMessages cache to prevent unbounded growth

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)
+  })
+})

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<typeof getEmptyToolPermissionContext> {
+  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 `<currentDrive>:\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)
+  })
+})