diff --git a/src/main/ipc/floating-workspace-directory.test.ts b/src/main/ipc/floating-workspace-directory.test.ts index f10f16efdd6e..be7614cb857d 100644 --- a/src/main/ipc/floating-workspace-directory.test.ts +++ b/src/main/ipc/floating-workspace-directory.test.ts @@ -165,6 +165,32 @@ describe('floating workspace directory', () => { ).resolves.toBe(path.join(userDataDir, 'floating-workspace')) }) + // A trailing space is a legal POSIX directory name (Google Drive exports them), so a + // picker-verified path must survive trust, settings sanitization and cwd resolution. + // On Windows this passes before and after the fix: NTFS strips trailing spaces at + // creation, and Win32 path APIs normalize the spellings back together. + it('keeps a directory whose name ends in a space through trust and resolution', async () => { + const store = createStore() + const selectedDir = path.join(tempRoot, 'gdrive notes ') + await mkdir(selectedDir) + const canonicalSelectedDir = await realpath(selectedDir) + + await trustFloatingWorkspaceDirectory(store, selectedDir) + + expect(store.settings.floatingTerminalTrustedCwds).toEqual([canonicalSelectedDir]) + + await expect( + resolveFloatingTerminalCwd(store as never, { + path: selectedDir, + requireTrusted: true + }) + ).resolves.toBe(canonicalSelectedDir) + await expect( + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the sanitizer only reads the settings members createStore() stubs. + sanitizeFloatingWorkspaceDirectorySetting(store as never, selectedDir) + ).resolves.toBe(canonicalSelectedDir) + }) + it('still resolves accessible ad hoc terminal directories when trust is not required', async () => { const store = createStore() const arbitraryDir = path.join(tempRoot, 'terminal-only') diff --git a/src/main/ipc/floating-workspace-directory.ts b/src/main/ipc/floating-workspace-directory.ts index 021422481178..c9a471c7a09c 100644 --- a/src/main/ipc/floating-workspace-directory.ts +++ b/src/main/ipc/floating-workspace-directory.ts @@ -41,10 +41,12 @@ async function canonicalizeAccessibleDirectory(dirPath: string): Promise { return new Set( (settings.floatingTerminalTrustedCwds ?? []) - .map((trustedPath) => trustedPath.trim()) .filter((trustedPath) => trustedPath.length > 0) .map(resolveFloatingWorkspaceInput) ) @@ -84,17 +86,23 @@ export async function resolveFloatingTerminalCwd( store: Store, args?: FloatingTerminalCwdRequest ): Promise { - const configuredPath = typeof args?.path === 'string' ? args.path.trim() : '' - if (!configuredPath) { + const configuredPath = typeof args?.path === 'string' ? args.path : '' + if (!configuredPath.trim()) { return args?.requireTrusted === true ? ensureDefaultFloatingWorkspacePath() : resolveFloatingWorkspaceInput('~') } - const cwd = resolveFloatingWorkspaceInput(configuredPath) - const canonicalCwd = await canonicalizeAccessibleDirectory(cwd) + // Why raw first: a trailing space is a legal POSIX directory name, so a stored + // picker-verified path must resolve as typed; the trim only cleans typed typos. + let cwd = resolveFloatingWorkspaceInput(configuredPath) + let canonicalCwd = await canonicalizeAccessibleDirectory(cwd) if (!canonicalCwd) { - return ensureDefaultFloatingWorkspacePath() + cwd = resolveFloatingWorkspaceInput(configuredPath.trim()) + canonicalCwd = await canonicalizeAccessibleDirectory(cwd) + if (!canonicalCwd) { + return ensureDefaultFloatingWorkspacePath() + } } // Why: only picker-approved directories may become the cwd, so arbitrary settings text can't. @@ -134,8 +142,11 @@ export async function sanitizeFloatingWorkspaceDirectorySetting( if (trimmed === '~') { return '~' } - const resolvedDir = resolveFloatingWorkspaceInput(trimmed) - const canonicalDir = await canonicalizeAccessibleDirectory(resolvedDir) + // Why raw first: a trailing space is a legal POSIX directory name, so a picked + // directory must sanitize as typed; the trim only cleans typed typos. + const canonicalDir = + (await canonicalizeAccessibleDirectory(resolveFloatingWorkspaceInput(dirPath))) ?? + (await canonicalizeAccessibleDirectory(resolveFloatingWorkspaceInput(trimmed))) if (!canonicalDir || !isTrustedFloatingWorkspaceDirectory(canonicalDir, store.getSettings())) { return '' } diff --git a/src/main/ipc/repos-create.test.ts b/src/main/ipc/repos-create.test.ts index 6f465188b5dd..1bf3848a3289 100644 --- a/src/main/ipc/repos-create.test.ts +++ b/src/main/ipc/repos-create.test.ts @@ -279,6 +279,57 @@ describe('repos:create', () => { expect(mkdirMock).toHaveBeenNthCalledWith(2, tmpPath('brand-new'), { recursive: false }) }) + // A trailing space is a legal POSIX directory name (Google Drive exports them), so a + // picked parent that exists exactly as typed must win over the typed-typo trim. + it('creates inside a picked parent whose name ends in a space', async () => { + const spacedParent = '/tmp/gdrive repo ' + accessMock.mockImplementation(async (probed: string) => { + if (probed === spacedParent) { + return undefined + } + throw new Error('ENOENT') + }) + + const result = await callCreate({ parentPath: spacedParent, name: 'proj', kind: 'folder' }) + + expect(mkdirMock).toHaveBeenNthCalledWith(1, spacedParent, { recursive: true }) + expect(mkdirMock).toHaveBeenNthCalledWith(2, join(spacedParent, 'proj'), { recursive: false }) + expect(mockStore.addRepo).toHaveBeenCalledWith( + expect.objectContaining({ path: join(spacedParent, 'proj') }) + ) + expect(result).toHaveProperty('repo.path', join(spacedParent, 'proj')) + }) + + // The flip side: a typed typo whose trimmed spelling exists keeps the old cleanup. + it('falls back to the trimmed spelling when the raw parent does not exist', async () => { + accessMock.mockImplementation(async (probed: string) => { + if (probed === '/tmp/typo') { + return undefined + } + throw new Error('ENOENT') + }) + + const result = await callCreate({ parentPath: '/tmp/typo ', name: 'proj', kind: 'folder' }) + + expect(mkdirMock).toHaveBeenNthCalledWith(1, '/tmp/typo', { recursive: true }) + expect(result).toHaveProperty('repo.path', join('/tmp/typo', 'proj')) + }) + + // A probe failure that is not "absent" (EACCES, ELOOP...) must surface as an error, + // not silently fall back to the trimmed spelling and create in an unchosen directory. + it('returns an error when probing the raw parent fails with EACCES', async () => { + const lockedParent = '/tmp/locked repo ' + accessMock.mockRejectedValue( + Object.assign(new Error('EACCES: permission denied, access'), { code: 'EACCES' }) + ) + + const result = await callCreate({ parentPath: lockedParent, name: 'proj', kind: 'folder' }) + + expect(result).toMatchObject({ error: expect.stringContaining('EACCES') }) + expect(mkdirMock).not.toHaveBeenCalled() + expect(mockStore.addRepo).not.toHaveBeenCalled() + }) + it('creates a missing default parent before creating the project directory', async () => { const result = await callCreate({ parentPath: defaultProjectParent, diff --git a/src/main/ipc/repos/repo-creation-handlers.ts b/src/main/ipc/repos/repo-creation-handlers.ts index 9c98256697c3..be82f70d083f 100644 --- a/src/main/ipc/repos/repo-creation-handlers.ts +++ b/src/main/ipc/repos/repo-creation-handlers.ts @@ -57,6 +57,39 @@ function getDefaultCreateProjectParent(store: Store): string { return join(home, 'orca', 'projects') } +// Only ENOENT/ENOTDIR prove the probed path is absent (ENOTDIR: a parent component is a +// file); EACCES/ELOOP etc. must not trigger the trim fallback. Tests reject plain Errors +// without a code, so an ENOENT/ENOTDIR-looking message counts too. +function isAbsentPathError(err: unknown): boolean { + const code = + err && typeof err === 'object' && 'code' in err + ? (err as NodeJS.ErrnoException).code + : undefined + return ( + code === 'ENOENT' || + code === 'ENOTDIR' || + (code === undefined && err instanceof Error && /ENOENT|ENOTDIR/.test(err.message)) + ) +} + +// Why: a trailing space is a legal POSIX directory name (Google Drive exports them), so a +// picker-returned parent that exists exactly as typed wins; the trim stays only for typed typos. +async function resolveCreateParentPath(requestedPath: string): Promise { + const trimmed = requestedPath.trim() + if (!trimmed || trimmed === requestedPath) { + return trimmed + } + try { + await access(requestedPath) + return requestedPath + } catch (err) { + if (!isAbsentPathError(err)) { + throw err + } + return trimmed + } +} + export function registerRepoCreationHandlers(mainWindow: BrowserWindow, store: Store): void { ipcMain.handle('repos:isGitAvailable', () => probeLocalGitAvailability()) ipcMain.handle('repos:getDefaultCreateProjectParent', () => getDefaultCreateProjectParent(store)) @@ -89,7 +122,15 @@ export function registerRepoCreationHandlers(mainWindow: BrowserWindow, store: S args: { parentPath: string; name: string; kind: 'git' | 'folder' } ): Promise<{ repo: Repo } | { error: string }> => { const name = args.name?.trim() ?? '' - const parentPath = args.parentPath?.trim() ?? '' + let parentPath: string + try { + parentPath = await resolveCreateParentPath(args.parentPath ?? '') + } catch (err) { + // Why: an unexpected probe failure (EACCES...) must not fall back to the trimmed + // spelling and create the project in a directory the user did not pick. + const message = err instanceof Error ? err.message : String(err) + return { error: `Cannot access parent directory: ${message}` } + } // Why: IPC input is untrusted — coerce to the narrow union so a bogus kind can't skip git init yet persist in the store. const repoKind: 'git' | 'folder' = args.kind === 'folder' ? 'folder' : 'git' diff --git a/src/main/runtime/runtime-repository-registration-controller.ts b/src/main/runtime/runtime-repository-registration-controller.ts index 01099f34b63d..b072b4963323 100644 --- a/src/main/runtime/runtime-repository-registration-controller.ts +++ b/src/main/runtime/runtime-repository-registration-controller.ts @@ -24,6 +24,26 @@ type RuntimeRepositoryRegistrationDependencies = { notifyReposChanged: () => void } +// Why: a trailing space is a legal directory name (Google Drive exports them), so a +// picker-returned parent that exists exactly as requested wins; the trim stays only +// for typed typos. Only a genuinely absent raw spelling may fall back to the trim — +// other probe failures must surface instead of silently relocating the project. +async function resolveCreateParentPathRawFirst(requestedPath: string): Promise { + const trimmed = requestedPath.trim() + if (!trimmed || trimmed === requestedPath) { + return trimmed + } + try { + await stat(requestedPath) + return requestedPath + } catch (error) { + if (!isENOENT(error)) { + throw error + } + return trimmed + } +} + export class RuntimeRepositoryRegistrationController { constructor(private readonly deps: RuntimeRepositoryRegistrationDependencies) {} @@ -93,7 +113,15 @@ export class RuntimeRepositoryRegistrationController { ): Promise<{ repo: Repo } | { error: string }> { const store = this.requireStore() const trimmedName = name.trim() - const trimmedParentPath = parentPath.trim() + let parentDir: string + try { + parentDir = await resolveCreateParentPathRawFirst(parentPath) + } catch (error) { + // Why: an unexpected probe failure (EACCES...) must not fall back to the trimmed + // spelling and create the project in a directory the user did not pick. + const message = error instanceof Error ? error.message : String(error) + return { error: `Cannot access parent directory: ${message}` } + } const repoKind: 'git' | 'folder' = kind === 'folder' ? 'folder' : 'git' if (!trimmedName) { return { error: 'Name cannot be empty' } @@ -101,13 +129,13 @@ export class RuntimeRepositoryRegistrationController { if (/[\\/]/.test(trimmedName) || trimmedName === '.' || trimmedName === '..') { return { error: 'Name cannot contain slashes or be "." / ".."' } } - if (!trimmedParentPath) { + if (!parentDir) { return { error: 'Parent directory is required' } } - if (!isAbsolute(trimmedParentPath)) { + if (!isAbsolute(parentDir)) { return { error: 'Parent directory must be an absolute path' } } - const targetPath = join(trimmedParentPath, trimmedName) + const targetPath = join(parentDir, trimmedName) const existing = store.getRepos().find((repo) => runtimePathsEqual(repo.path, targetPath)) if (existing) { return { repo: existing } @@ -115,7 +143,7 @@ export class RuntimeRepositoryRegistrationController { let createdDir = false try { - await mkdir(trimmedParentPath, { recursive: true }) + await mkdir(parentDir, { recursive: true }) const existingStat = await stat(targetPath).catch((error: unknown) => { if (isENOENT(error)) { return null diff --git a/src/main/runtime/runtime-repository-registration-create.test.ts b/src/main/runtime/runtime-repository-registration-create.test.ts new file mode 100644 index 000000000000..fe27bd624082 --- /dev/null +++ b/src/main/runtime/runtime-repository-registration-create.test.ts @@ -0,0 +1,83 @@ +/** + * Pins the raw-first parent resolution of RuntimeRepositoryRegistrationController.create: + * a picker-returned parent with a trailing space (legal POSIX name) must be used as + * typed when it exists, with the trim kept only for typed typos (#25386). + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +const { prepareLocalWorktreeRootForRepoMock, detectRepoIconAndUpstreamMock } = vi.hoisted(() => ({ + prepareLocalWorktreeRootForRepoMock: vi.fn(async () => undefined), + detectRepoIconAndUpstreamMock: vi.fn(async () => ({})) +})) + +vi.mock('../repo-icon-autodetect', () => ({ + detectRepoIconAndUpstream: detectRepoIconAndUpstreamMock +})) + +vi.mock('../worktree-root-preparation', () => ({ + prepareLocalWorktreeRootForRepo: prepareLocalWorktreeRootForRepoMock +})) + +import { RuntimeRepositoryRegistrationController } from './runtime-repository-registration-controller' + +function makeController(): RuntimeRepositoryRegistrationController { + return new RuntimeRepositoryRegistrationController({ + getStore: () => + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: create() only reads the repo members stubbed here. + ({ + getRepos: () => [], + addRepo: () => undefined, + getRepo: () => undefined, + updateRepo: () => undefined + }) as never, + invalidateResolvedWorktrees: () => undefined, + invalidateWorktreeScan: () => undefined, + notifyReposChanged: () => undefined + }) +} + +describe('RuntimeRepositoryRegistrationController.create parent resolution', () => { + let base: string + + beforeEach(() => { + base = mkdtempSync(join(tmpdir(), 'orca-repo-create-')) + }) + + afterEach(() => { + rmSync(base, { recursive: true, force: true }) + }) + + it('creates inside a trailing-space parent that exists exactly as requested', async () => { + const parent = join(base, 'cloud dir ') + mkdirSync(parent) + const result = await makeController().create(parent, 'kid', 'folder') + expect('error' in result && result.error).toBeFalsy() + if (!('error' in result)) { + expect(result.repo.path).toBe(join(parent, 'kid')) + } + }) + + it('falls back to the trimmed spelling when the raw parent does not exist', async () => { + const requested = join(base, 'typo dir ') + const result = await makeController().create(requested, 'kid', 'folder') + expect('error' in result && result.error).toBeFalsy() + if (!('error' in result)) { + expect(result.repo.path).toBe(join(base, 'typo dir', 'kid')) + } + }) + + it('surfaces a non-absent probe failure instead of creating at the trimmed spelling', async () => { + const blocker = join(base, 'blocker') + writeFileSync(blocker, 'a file, not a directory') + const requested = join(blocker, 'cloud dir ') + const result = await makeController().create(requested, 'kid', 'folder') + expect('error' in result).toBe(true) + if ('error' in result) { + expect(result.error).toMatch(/Cannot access parent directory/) + } + }) +}) diff --git a/src/main/runtime/worktree-selector-trailing-space-path.test.ts b/src/main/runtime/worktree-selector-trailing-space-path.test.ts new file mode 100644 index 000000000000..2c8e68cff214 --- /dev/null +++ b/src/main/runtime/worktree-selector-trailing-space-path.test.ts @@ -0,0 +1,54 @@ +// A trailing space is a legal POSIX directory name (Google Drive exports them), so the +// worktree selector chain — scan parsing, id minting/parsing, selector matching — must +// preserve it end to end. Characterization: these pass today; a red here means a +// serialization layer started eating the space (#25386 cross-machine selector_not_found). +import { describe, expect, it } from 'vitest' +import { parseWorktreeList } from '../../shared/git-worktree-porcelain-parser' +import { + splitWorktreeId, + worktreeIdComparisonKey, + worktreeIdsEqual +} from '../../shared/worktree/id' +import { runtimePathsEqual } from './runtime-worktree-path-identity' + +const REPO_ID = '01234567-89ab-cdef-0123-456789abcdef' +const SPACED_REPO_PATH = '/Users/me/gdrive repo ' +const SPACED_WORKTREE_PATH = '/Users/me/orca/workspaces/gdrive repo /feature ' + +describe('trailing-space paths survive the selector chain', () => { + it('parses a porcelain scan without trimming the path', () => { + const output = + `worktree ${SPACED_REPO_PATH}\nHEAD abc\nbranch refs/heads/main\n\n` + + `worktree ${SPACED_WORKTREE_PATH}\nHEAD def\nbranch refs/heads/feature\n` + + const worktrees = parseWorktreeList(output) + + expect(worktrees.map((worktree) => worktree.path)).toEqual([ + SPACED_REPO_PATH, + SPACED_WORKTREE_PATH + ]) + }) + + it('mints and re-splits worktree ids with the space intact', () => { + const worktreeId = `${REPO_ID}::${SPACED_WORKTREE_PATH}` + + expect(splitWorktreeId(worktreeId)).toEqual({ + repoId: REPO_ID, + worktreePath: SPACED_WORKTREE_PATH + }) + expect(worktreeIdsEqual(worktreeId, `${REPO_ID}::${SPACED_WORKTREE_PATH}`)).toBe(true) + expect(worktreeIdComparisonKey(worktreeId)).toBe( + `${REPO_ID}::${SPACED_WORKTREE_PATH.normalize('NFC')}` + ) + }) + + it('matches path selectors byte-exact on the trailing space', () => { + // Interior duplicate slashes fold; the trailing space must survive the fold. + expect( + runtimePathsEqual(SPACED_WORKTREE_PATH, SPACED_WORKTREE_PATH.replace('/Users/', '/Users//')) + ).toBe(true) + expect(runtimePathsEqual(SPACED_WORKTREE_PATH, SPACED_WORKTREE_PATH.replace(/ $/, ''))).toBe( + false + ) + }) +}) diff --git a/src/renderer/src/components/sidebar/useCreateRepo.ts b/src/renderer/src/components/sidebar/useCreateRepo.ts index 2a870de153ee..a8a1227b2e98 100644 --- a/src/renderer/src/components/sidebar/useCreateRepo.ts +++ b/src/renderer/src/components/sidebar/useCreateRepo.ts @@ -76,12 +76,14 @@ export function useCreateRepo( const handleCreate = useCallback(async () => { const name = createName.trim() - const parentPath = createParent.trim() // Why: null means the chosen host is unresolved (e.g. its server is coming up); without a // host id the request below would run on this computer. - if (!name || !parentPath || options.hostId === null) { + if (!name || !createParent.trim() || options.hostId === null) { return } + // Why untrimmed: the picker returns a verified path and a trailing space can be the + // real directory name; main resolves raw-first and only trims typed typos. + const parentPath = createParent const requestHostToken = hostTokenRef.current const gen = ++createGenRef.current const isCurrent = (): boolean =>