From dfd97f069a673e70c4918eaa6704cc1a3d6fed7d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 25 Apr 2026 15:23:24 -0700 Subject: [PATCH] fix(pi): stop deleting user Pi data through overlay junctions (#1083) (#1092) --- .../pi/titlebar-extension-service.test.ts | 171 ++++++++++++++++++ src/main/pi/titlebar-extension-service.ts | 132 ++++++++++++-- 2 files changed, 291 insertions(+), 12 deletions(-) create mode 100644 src/main/pi/titlebar-extension-service.test.ts diff --git a/src/main/pi/titlebar-extension-service.test.ts b/src/main/pi/titlebar-extension-service.test.ts new file mode 100644 index 000000000..e8458c77f --- /dev/null +++ b/src/main/pi/titlebar-extension-service.test.ts @@ -0,0 +1,171 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + rmSync, + symlinkSync, + writeFileSync +} from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' + +// The service calls app.getPath('userData') for its overlay root. Point that +// at a real tmp dir so we can exercise the filesystem behavior end-to-end. +const userDataDir = mkdtempSync(join(tmpdir(), 'orca-pi-test-userdata-')) + +vi.mock('electron', () => ({ + app: { + getPath: (name: string) => { + if (name === 'userData') { + return userDataDir + } + throw new Error(`unexpected app.getPath(${name})`) + } + } +})) + +import { PiTitlebarExtensionService, isSafeDescendCandidate } from './titlebar-extension-service' + +describe('PiTitlebarExtensionService', () => { + let piHome: string + + beforeEach(() => { + piHome = mkdtempSync(join(tmpdir(), 'orca-pi-test-pihome-')) + // Seed a realistic Pi agent dir with skills, extensions, auth, sessions. + mkdirSync(join(piHome, 'skills', 'my-skill', 'nested'), { recursive: true }) + writeFileSync(join(piHome, 'skills', 'my-skill', 'SKILL.md'), 'critical user skill') + writeFileSync(join(piHome, 'skills', 'my-skill', 'nested', 'data.txt'), 'nested data') + mkdirSync(join(piHome, 'extensions', 'user-ext'), { recursive: true }) + writeFileSync(join(piHome, 'extensions', 'user-ext', 'ext.ts'), 'user extension') + mkdirSync(join(piHome, 'sessions'), { recursive: true }) + writeFileSync(join(piHome, 'sessions', 'session-1.json'), '{}') + writeFileSync(join(piHome, 'auth.json'), 'secret token') + }) + + afterEach(() => { + rmSync(piHome, { recursive: true, force: true }) + rmSync(join(userDataDir, 'pi-agent-overlays'), { recursive: true, force: true }) + }) + + function expectPiHomeIntact(): void { + expect(readFileSync(join(piHome, 'auth.json'), 'utf-8')).toBe('secret token') + expect(readFileSync(join(piHome, 'skills', 'my-skill', 'SKILL.md'), 'utf-8')).toBe( + 'critical user skill' + ) + expect(readFileSync(join(piHome, 'skills', 'my-skill', 'nested', 'data.txt'), 'utf-8')).toBe( + 'nested data' + ) + expect(readFileSync(join(piHome, 'extensions', 'user-ext', 'ext.ts'), 'utf-8')).toBe( + 'user extension' + ) + expect(readFileSync(join(piHome, 'sessions', 'session-1.json'), 'utf-8')).toBe('{}') + } + + it('buildPtyEnv mirrors the user Pi dir into an overlay under userData', () => { + const svc = new PiTitlebarExtensionService() + const env = svc.buildPtyEnv('pty-1', piHome) + + expect(env.PI_CODING_AGENT_DIR).toBe(join(userDataDir, 'pi-agent-overlays', 'pty-1')) + // Orca's titlebar extension is added alongside user extensions, not replacing them. + const overlayExtensions = readdirSync(join(env.PI_CODING_AGENT_DIR!, 'extensions')).sort() + expect(overlayExtensions).toEqual(['orca-titlebar-spinner.ts', 'user-ext']) + // User's top-level resources are reachable via the overlay. + expect(existsSync(join(env.PI_CODING_AGENT_DIR!, 'skills', 'my-skill', 'SKILL.md'))).toBe(true) + expect(existsSync(join(env.PI_CODING_AGENT_DIR!, 'auth.json'))).toBe(true) + expectPiHomeIntact() + }) + + it('clearPty removes the overlay without touching the user Pi dir (issue #1083)', () => { + const svc = new PiTitlebarExtensionService() + svc.buildPtyEnv('pty-2', piHome) + svc.clearPty('pty-2') + + expect(existsSync(join(userDataDir, 'pi-agent-overlays', 'pty-2'))).toBe(false) + // Critical regression guard: destroying the overlay MUST NOT destroy the + // user's Pi home, even though every top-level entry in the overlay is a + // symlink/junction pointing back into it. + expectPiHomeIntact() + }) + + it('rebuilding an overlay for the same ptyId does not corrupt the user Pi dir', () => { + const svc = new PiTitlebarExtensionService() + svc.buildPtyEnv('pty-3', piHome) + svc.buildPtyEnv('pty-3', piHome) + svc.buildPtyEnv('pty-3', piHome) + expectPiHomeIntact() + }) + + // Why: symlinkSync on Windows requires developer mode or admin — skip on + // Windows rather than fail for environmental reasons. The isSafeDescendCandidate + // unit tests above cover the Windows ordering invariant separately. + it.skipIf(process.platform === 'win32')( + 'safely handles a pre-existing stale overlay with dangling symlinks', + () => { + // Why: simulate an overlay that was left behind by a prior Orca session, + // where the original Pi home it mirrored has since moved. The teardown + // should unlink the dangling symlinks in place without trying to follow them. + const overlayDir = join(userDataDir, 'pi-agent-overlays', 'pty-4') + mkdirSync(overlayDir, { recursive: true }) + symlinkSync('/nonexistent-pi-target/skills', join(overlayDir, 'skills'), 'dir') + symlinkSync('/nonexistent-pi-target/auth.json', join(overlayDir, 'auth.json'), 'file') + + const svc = new PiTitlebarExtensionService() + const env = svc.buildPtyEnv('pty-4', piHome) + + expect(env.PI_CODING_AGENT_DIR).toBe(overlayDir) + expect(existsSync(join(overlayDir, 'skills', 'my-skill', 'SKILL.md'))).toBe(true) + expectPiHomeIntact() + } + ) + + describe('isSafeDescendCandidate (Windows junction regression guard)', () => { + // Why: the #1083 regression cannot reproduce on POSIX CI because + // fs.rmSync({recursive:true}) handles symlinks correctly on macOS/Linux. + // The behavior that DID cause the data loss on Windows was directory + // junctions reporting BOTH isSymbolicLink() === true AND isDirectory() + // === true from lstat/Dirent. These unit tests pin the predicate's + // ordering so a future refactor cannot reverse it without the test suite + // failing, regardless of which OS the tests run on. + it('rejects a Windows directory junction (symlink + directory both true)', () => { + const junctionLike = { + isSymbolicLink: () => true, + isDirectory: () => true + } + expect(isSafeDescendCandidate(junctionLike)).toBe(false) + }) + + it('rejects a plain symlink', () => { + expect(isSafeDescendCandidate({ isSymbolicLink: () => true, isDirectory: () => false })).toBe( + false + ) + }) + + it('rejects a regular file', () => { + expect( + isSafeDescendCandidate({ isSymbolicLink: () => false, isDirectory: () => false }) + ).toBe(false) + }) + + it('accepts a true directory (non-symlink)', () => { + expect(isSafeDescendCandidate({ isSymbolicLink: () => false, isDirectory: () => true })).toBe( + true + ) + }) + }) + + it('refuses to remove anything outside the overlay root', () => { + // Why: hard guard against a misresolved overlay path (regression defense). + // The overlay root is userData/pi-agent-overlays; any path outside it + // must be a no-op, not a `rm -rf` on arbitrary filesystem locations. + const svc = new PiTitlebarExtensionService() as unknown as { + safeRemoveOverlay: (p: string) => void + } + svc.safeRemoveOverlay(piHome) + svc.safeRemoveOverlay('/') + svc.safeRemoveOverlay(join(userDataDir, 'pi-agent-overlays')) // root itself + expectPiHomeIntact() + }) +}) diff --git a/src/main/pi/titlebar-extension-service.ts b/src/main/pi/titlebar-extension-service.ts index 62f9ba88e..407adbc18 100644 --- a/src/main/pi/titlebar-extension-service.ts +++ b/src/main/pi/titlebar-extension-service.ts @@ -1,16 +1,17 @@ import { cpSync, existsSync, + lstatSync, linkSync, mkdirSync, readdirSync, - rmSync, - statSync, + rmdirSync, symlinkSync, + unlinkSync, writeFileSync } from 'fs' import { homedir } from 'os' -import { basename, join } from 'path' +import { basename, join, relative, resolve, sep } from 'path' import { app } from 'electron' const ORCA_PI_EXTENSION_FILE = 'orca-titlebar-spinner.ts' @@ -85,10 +86,17 @@ function getDefaultPiAgentDir(): string { } function mirrorEntry(sourcePath: string, targetPath: string): void { - const sourceStats = statSync(sourcePath) + // Why: lstatSync (not statSync) so that if the user's Pi dir contains its + // OWN symlinks (e.g. skills symlinked from ~/.agents/skills), we mirror the + // link itself rather than resolving it to a type and then creating a junction + // at an unrelated path. isSymbolicLink() MUST be checked before isDirectory() + // on Windows because directory junctions/reparse points report both true. + const sourceStats = lstatSync(sourcePath) + const isSymlink = sourceStats.isSymbolicLink() + const isDirectoryLike = !isSymlink && sourceStats.isDirectory() if (process.platform === 'win32') { - if (sourceStats.isDirectory()) { + if (isDirectoryLike) { symlinkSync(sourcePath, targetPath, 'junction') return } @@ -102,12 +110,112 @@ function mirrorEntry(sourcePath: string, targetPath: string): void { } } - symlinkSync(sourcePath, targetPath, sourceStats.isDirectory() ? 'dir' : 'file') + symlinkSync(sourcePath, targetPath, isDirectoryLike ? 'dir' : 'file') +} + +// Exported for tests. A "descend candidate" is an entry whose children we +// should recurse into when tearing down the overlay. Anything that is a +// symlink (including a Windows directory junction) must NOT be a candidate +// even if it also reports isDirectory() — following it would walk into the +// link target and delete user data, which is the bug in #1083. +export function isSafeDescendCandidate(stats: { + isSymbolicLink(): boolean + isDirectory(): boolean +}): boolean { + if (stats.isSymbolicLink()) { + return false + } + return stats.isDirectory() } export class PiTitlebarExtensionService { + private getOverlayRoot(): string { + return join(app.getPath('userData'), PI_OVERLAY_DIR_NAME) + } + private getOverlayDir(ptyId: string): string { - return join(app.getPath('userData'), PI_OVERLAY_DIR_NAME, ptyId) + return join(this.getOverlayRoot(), ptyId) + } + + // Why: the overlay tree contains symlinks/junctions that point back into the + // user's real Pi state (~/.pi/agent or $PI_CODING_AGENT_DIR). fs.rmSync with + // { recursive: true } has repeatedly regressed on Windows when walking + // NTFS junctions — it can follow them and delete the *target*, destroying + // the user's skills, extensions, sessions, and auth.json. See issue #1083. + // + // Never descend into a symlink/junction here: for any non-real-directory + // entry we unlink the link itself; only entries that are truly directories + // on disk (our own extensions/ dir and the overlay root) are recursed into. + // We also refuse to operate on any path outside the overlay root as a + // last-line guard against PI_OVERLAY_DIR_NAME ever being mis-resolved. + private safeRemoveOverlay(overlayDir: string): void { + const overlayRoot = this.getOverlayRoot() + const resolvedRoot = resolve(overlayRoot) + const resolvedTarget = resolve(overlayDir) + const rel = relative(resolvedRoot, resolvedTarget) + if (rel === '' || rel.startsWith('..') || rel.includes(`..${sep}`)) { + // Target is not strictly inside the overlay root — refuse to touch it. + // Log so a misconfigured caller does not silently leak overlays forever + // with no signal that this guard is firing. + console.warn( + `[pi-titlebar] refusing to remove overlay outside root: target=${resolvedTarget} root=${resolvedRoot}` + ) + return + } + this.safeRemoveTree(resolvedTarget) + } + + private safeRemoveTree(path: string): void { + let stat + try { + stat = lstatSync(path) + } catch { + return + } + + // Any symlink or Windows junction is unlinked in place, NEVER descended. + // statSync would follow the link and report the target's stats, which is + // exactly the bug we are guarding against, so the check uses lstat. + // + // On Windows, lstat on a directory junction can report BOTH + // isSymbolicLink() === true AND isDirectory() === true, so we MUST check + // isSymbolicLink first — otherwise a junction enters the recursive branch + // and readdirSync enumerates the link's target, the exact bug in #1083. + if (!isSafeDescendCandidate(stat)) { + try { + unlinkSync(path) + } catch { + // Best-effort: antivirus/indexers can hold handles briefly on Windows. + // A leftover link is harmless; the next spawn rebuilds the overlay. + } + return + } + + let entries + try { + entries = readdirSync(path, { withFileTypes: true }) + } catch { + return + } + + for (const entry of entries) { + const child = join(path, entry.name) + if (isSafeDescendCandidate(entry)) { + this.safeRemoveTree(child) + continue + } + try { + unlinkSync(child) + } catch { + // best-effort, see above + } + } + + try { + rmdirSync(path) + } catch { + // Directory may be non-empty if an unlink above failed; harmless. + } } private mirrorAgentDir(sourceAgentDir: string, overlayDir: string): void { @@ -143,13 +251,13 @@ export class PiTitlebarExtensionService { const overlayDir = this.getOverlayDir(ptyId) try { - rmSync(overlayDir, { recursive: true, force: true }) + this.safeRemoveOverlay(overlayDir) } catch { // Why: on Windows the overlay directory can be locked by another process // (e.g. antivirus, indexer, or a previous Orca session that didn't clean up). - // rmSync with force:true handles ENOENT but not EPERM/EBUSY. If we can't - // remove the stale overlay, fall back to the user's own Pi agent dir so the - // terminal still spawns — the titlebar spinner is not worth blocking the PTY. + // If we can't remove the stale overlay, fall back to the user's own Pi agent + // dir so the terminal still spawns — the titlebar spinner is not worth + // blocking the PTY. return existingAgentDir ? { PI_CODING_AGENT_DIR: existingAgentDir } : {} } @@ -180,7 +288,7 @@ export class PiTitlebarExtensionService { clearPty(ptyId: string): void { try { - rmSync(this.getOverlayDir(ptyId), { recursive: true, force: true }) + this.safeRemoveOverlay(this.getOverlayDir(ptyId)) } catch { // Why: on Windows the overlay dir can be locked (EPERM/EBUSY) by antivirus // or indexers. Overlay cleanup is best-effort — a stale directory in userData