Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/main/ipc/floating-workspace-directory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
27 changes: 19 additions & 8 deletions src/main/ipc/floating-workspace-directory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@ async function canonicalizeAccessibleDirectory(dirPath: string): Promise<string
}
}

// Why no trim on read: entries are canonicalized real paths when stored, and a trailing
// space is a legal POSIX directory name (Google Drive exports them) — trimming here would
// break the trust match for those directories.
function getTrustedFloatingWorkspaceDirectories(settings: GlobalSettings): Set<string> {
return new Set(
(settings.floatingTerminalTrustedCwds ?? [])
.map((trustedPath) => trustedPath.trim())
.filter((trustedPath) => trustedPath.length > 0)
.map(resolveFloatingWorkspaceInput)
)
Expand Down Expand Up @@ -84,17 +86,23 @@ export async function resolveFloatingTerminalCwd(
store: Store,
args?: FloatingTerminalCwdRequest
): Promise<string> {
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.
Expand Down Expand Up @@ -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 ''
}
Expand Down
51 changes: 51 additions & 0 deletions src/main/ipc/repos-create.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
43 changes: 42 additions & 1 deletion src/main/ipc/repos/repo-creation-handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string> {
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
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

export function registerRepoCreationHandlers(mainWindow: BrowserWindow, store: Store): void {
ipcMain.handle('repos:isGitAvailable', () => probeLocalGitAvailability())
ipcMain.handle('repos:getDefaultCreateProjectParent', () => getDefaultCreateProjectParent(store))
Expand Down Expand Up @@ -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'

Expand Down
38 changes: 33 additions & 5 deletions src/main/runtime/runtime-repository-registration-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string> {
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) {}

Expand Down Expand Up @@ -93,29 +113,37 @@ 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' }
}
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 }
}

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
Expand Down
83 changes: 83 additions & 0 deletions src/main/runtime/runtime-repository-registration-create.test.ts
Original file line number Diff line number Diff line change
@@ -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/)
}
})
})
Loading