diff --git a/src/main/runtime/orca-runtime-files-rename-authority.test.ts b/src/main/runtime/orca-runtime-files-rename-authority.test.ts index 2bd55db85b4..489a0981db2 100644 --- a/src/main/runtime/orca-runtime-files-rename-authority.test.ts +++ b/src/main/runtime/orca-runtime-files-rename-authority.test.ts @@ -44,7 +44,7 @@ vi.mock( ) function mockStats(dev: number, ino: number) { - return { dev, ino, isDirectory: () => false, isSymbolicLink: () => false } + return { dev, ino, isDirectory: () => false } } function mockLocalPathStats(entries: Record) { diff --git a/src/main/runtime/orca-runtime-files-repository-admin-path-aliases.test.ts b/src/main/runtime/orca-runtime-files-repository-admin-path-aliases.test.ts index 1c0195ef795..10027d7bd7c 100644 --- a/src/main/runtime/orca-runtime-files-repository-admin-path-aliases.test.ts +++ b/src/main/runtime/orca-runtime-files-repository-admin-path-aliases.test.ts @@ -223,6 +223,41 @@ describe.skipIf(process.platform === 'win32')( expect(existsSync(join(fixture.repoPath, 'stolen-config'))).toBe(false) }) + // Why: realpath ENOENTs on a DANGLING symlink exactly as it does on a path that is simply + // absent, but writeFile FOLLOWS the link and creates its target — so the link's own name is the + // wrong thing to classify. This one created .git/hooks/post-commit from nothing. + it('refuses a write through a dangling symlink into .git', async () => { + await symlink( + join(fixture.repoPath, '.git', 'hooks', 'post-commit'), + join(fixture.repoPath, 'dangling'), + 'file' + ) + + const response = await dispatchFileMethod('files.write', { + relativePath: 'dangling', + content: '#!/bin/sh\nEVIL\n' + }) + + expectRefused(response) + expect(existsSync(join(fixture.repoPath, '.git', 'hooks', 'post-commit'))).toBe(false) + }) + + it('still writes through a dangling symlink that stays in the working tree', async () => { + await symlink( + join(fixture.repoPath, 'not-yet.txt'), + join(fixture.repoPath, 'pending'), + 'file' + ) + + const response = await dispatchFileMethod('files.write', { + relativePath: 'pending', + content: 'ok\n' + }) + + expect(response.ok).toBe(true) + expect(await readFile(join(fixture.repoPath, 'not-yet.txt'), 'utf-8')).toBe('ok\n') + }) + // Why also the destination: COPYFILE_EXCL happens to block this today, and it is the only thing // that does. Classifying it too keeps the guard from depending on that flag staying put. it('refuses a leaf symlink into .git as the destination', async () => { diff --git a/src/main/runtime/repository-admin-path-authorization.test.ts b/src/main/runtime/repository-admin-path-authorization.test.ts new file mode 100644 index 00000000000..dcc012eae43 --- /dev/null +++ b/src/main/runtime/repository-admin-path-authorization.test.ts @@ -0,0 +1,86 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as FsPromises from 'node:fs/promises' + +const lstatMock = vi.fn() +const realpathMock = vi.fn() +const readlinkMock = vi.fn() + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, lstat: lstatMock, realpath: realpathMock, readlink: readlinkMock } +}) +vi.mock('../ipc/filesystem-auth', () => ({ + resolveAuthorizedPath: vi.fn(async (targetPath: string) => targetPath) +})) + +const { REPOSITORY_ADMIN_HARD_LINK_DENIED_MESSAGE, resolveAuthorizedMutablePath } = + await import('./repository-admin-path-authorization') + +const TARGET = '/repo/notes.txt' + +// Why this file exists: the guards below decide on fields of a `fs.Stats`, and 20+ suites in this +// repo hand production code a PARTIAL stat. These pin what happens when a field is absent, which no +// real-filesystem test can express — a real lstat always populates them. +describe('hard-link refusal on an uninterrogable stat', () => { + beforeEach(() => { + lstatMock.mockReset() + realpathMock.mockReset().mockImplementation(async (p: string) => p) + readlinkMock + .mockReset() + .mockRejectedValue(Object.assign(new Error('EINVAL'), { code: 'EINVAL' })) + }) + + it('refuses when nlink is missing, rather than silently allowing', async () => { + lstatMock.mockResolvedValue({ isSymbolicLink: () => false }) + + await expect( + resolveAuthorizedMutablePath(TARGET, {} as never, { followsLink: true }) + ).rejects.toThrow(REPOSITORY_ADMIN_HARD_LINK_DENIED_MESSAGE) + }) + + it('refuses a genuinely hard-linked file', async () => { + lstatMock.mockResolvedValue({ nlink: 2, isSymbolicLink: () => false }) + + await expect( + resolveAuthorizedMutablePath(TARGET, {} as never, { followsLink: true }) + ).rejects.toThrow(REPOSITORY_ADMIN_HARD_LINK_DENIED_MESSAGE) + }) + + it('allows an ordinary single-named file', async () => { + lstatMock.mockResolvedValue({ nlink: 1, isSymbolicLink: () => false }) + + await expect( + resolveAuthorizedMutablePath(TARGET, {} as never, { followsLink: true }) + ).resolves.toBe(TARGET) + }) +}) + +// Why: the symlink exemption is what keeps "delete the link itself" working, so a stat that cannot +// answer must NOT be treated as a symlink — it takes the classifying branch instead. +describe('symlink exemption on an uninterrogable stat', () => { + beforeEach(() => { + lstatMock.mockReset() + realpathMock.mockReset() + readlinkMock + .mockReset() + .mockRejectedValue(Object.assign(new Error('EINVAL'), { code: 'EINVAL' })) + }) + + it('classifies the canonical leaf when isSymbolicLink is absent', async () => { + lstatMock.mockResolvedValue({ nlink: 1 }) + realpathMock.mockResolvedValue('/repo/.git/config') + + await expect( + resolveAuthorizedMutablePath('/repo/alias', {} as never, { preserveSymlink: true }) + ).rejects.toThrow(/Git repository metadata/) + }) + + it('exempts a confirmed symlink so the link itself stays removable', async () => { + lstatMock.mockResolvedValue({ nlink: 1, isSymbolicLink: () => true }) + realpathMock.mockResolvedValue('/repo/.git/config') + + await expect( + resolveAuthorizedMutablePath('/repo/alias', {} as never, { preserveSymlink: true }) + ).resolves.toBe('/repo/alias') + }) +}) diff --git a/src/main/runtime/repository-admin-path-authorization.ts b/src/main/runtime/repository-admin-path-authorization.ts index 6efabcc5196..ec787830407 100644 --- a/src/main/runtime/repository-admin-path-authorization.ts +++ b/src/main/runtime/repository-admin-path-authorization.ts @@ -1,5 +1,5 @@ -import { lstat, realpath } from 'node:fs/promises' -import { posix, win32 } from 'node:path' +import { lstat, readlink, realpath } from 'node:fs/promises' +import { basename, dirname, isAbsolute, join, posix, resolve, win32 } from 'node:path' import type { Store } from '../persistence' import { isWindowsAbsolutePathLike } from '../../shared/cross-platform-path' import { resolveAuthorizedPath, type ResolveAuthorizedPathOptions } from '../ipc/filesystem-auth' @@ -134,9 +134,9 @@ export async function resolveAuthorizedMutablePath( * catches `GIT~1` on a short-name-enabled NTFS volume. */ async function assertMutableUnlessSymlink(path: string): Promise { - let isSymbolicLink: boolean + let stats: { isSymbolicLink?: () => boolean } try { - isSymbolicLink = (await lstat(path)).isSymbolicLink() + stats = await lstat(path) } catch (error) { // Nothing on disk yet cannot alias anything; other failures surface on the syscall itself. if (isENOENT(error)) { @@ -144,7 +144,8 @@ async function assertMutableUnlessSymlink(path: string): Promise { } throw error } - if (isSymbolicLink) { + // Only a confirmed symlink is exempt. Anything we cannot ask takes the stricter branch below. + if (typeof stats.isSymbolicLink === 'function' && stats.isSymbolicLink()) { return } assertMutablePath(await canonicalLeaf(path)) @@ -161,7 +162,7 @@ async function assertMutableUnlessSymlink(path: string): Promise { * Partial by nature: `nlink` is not dependable on Windows, so this closes the POSIX case only. */ async function assertNotHardLinked(path: string): Promise { - let linkCount: number + let linkCount: number | undefined try { linkCount = (await lstat(path)).nlink } catch (error) { @@ -172,7 +173,9 @@ async function assertNotHardLinked(path: string): Promise { } throw error } - if (linkCount > 1) { + // Fails closed like its neighbour above: a link count we cannot read leaves us unable to rule out + // another name reaching these bytes, and an unclassifiable input takes the refusing branch. + if (typeof linkCount !== 'number' || linkCount > 1) { throw new Error(REPOSITORY_ADMIN_HARD_LINK_DENIED_MESSAGE) } } @@ -187,11 +190,27 @@ async function canonicalLeaf(path: string): Promise { try { return await realpath(path) } catch (error) { - // A path that does not exist yet has no link to follow; the caller creates a real file there. - if (isENOENT(error)) { - return path + if (!isENOENT(error)) { + // Fail closed: the leaf exists but cannot be canonicalized, so what it points at is unknown. + throw new Error(REPOSITORY_ADMIN_PATH_DENIED_MESSAGE) } - // Fail closed: the leaf exists but cannot be canonicalized, so what it points at is unknown. - throw new Error(REPOSITORY_ADMIN_PATH_DENIED_MESSAGE) } + // ENOENT also covers a DANGLING symlink, and `writeFile` follows one to CREATE its target — so + // returning the link's own name here would classify the wrong path. Resolve it by hand. + return await danglingLinkTarget(path) +} + +async function danglingLinkTarget(path: string): Promise { + let linkTarget: string + try { + linkTarget = await readlink(path) + } catch { + // Not a symlink, or unreadable: nothing points anywhere, so the path stands for itself. + return path + } + const resolvedTarget = isAbsolute(linkTarget) ? linkTarget : resolve(dirname(path), linkTarget) + // The target's own parent may exist even though the target does not; canonicalize what is there. + return await realpath(dirname(resolvedTarget)) + .then((parent) => join(parent, basename(resolvedTarget))) + .catch(() => resolvedTarget) }