fix(files): refuse a dangling symlink that would create .git state from nothing

`realpath` returns ENOENT for a dangling symlink exactly as it does for a path
that is simply absent, and both were treated as "nothing to follow" — so the
link's own name was classified, and it carries no `.git` segment. `files.write`
then follows the link and CREATES the target.

Reproduced against real bytes: writing through such a link created
`.git/hooks/post-commit` with attacker content. Every earlier bypass on this
branch needed the hook to already exist; this one creates it from nothing, so it
is code execution at the next commit with no precondition on the victim's repo.

ENOENT now resolves the link by hand — readlink, resolve a relative target
against the link's directory, canonicalize whatever of the target's parent
exists — and classifies that. A genuinely absent non-link path still returns
itself, so ordinary creation is unaffected.

`files.createFile` does not carry this: `flag: 'wx'` fails EEXIST on the link.
That is the second time an exclusive-create flag has been the only thing between
a path and a hook, after COPYFILE_EXCL on the copy destination. Both are
deconfliction flags, not security controls — relaxing either reopens a
code-execution path.

Also makes the hard-link refusal fail closed, matching the symlink check beside
it: a link count we cannot read leaves us unable to rule out another name
reaching the same bytes, so it refuses rather than allowing. Both branches now
have tests that distinguish them — a partial stat is injectable only through a
mock, since a real lstat always populates these fields.

Remote lane cannot do the dangling case: there is no readlink on the provider
contract or the relay, so a dangling remote link still classifies as its own
name. Closing it needs a new relay method plus capability negotiation. The
remote guard still catches non-dangling ancestor and leaf aliases.
This commit is contained in:
Merge Sim
2026-08-31 23:47:23 -07:00
parent b882521d62
commit ffc8f447ec
4 changed files with 153 additions and 13 deletions
@@ -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<string, [number, number]>) {
@@ -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 () => {
@@ -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<typeof FsPromises>()
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')
})
})
@@ -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<void> {
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<void> {
}
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<void> {
* Partial by nature: `nlink` is not dependable on Windows, so this closes the POSIX case only.
*/
async function assertNotHardLinked(path: string): Promise<void> {
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<void> {
}
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<string> {
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<string> {
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)
}