fix: add null-byte validation to resolveRegisteredWorktreePath (#251)

This commit is contained in:
Jinjing 2026-04-01 14:04:33 -07:00 committed by GitHub
parent 5b3f132519
commit 528e03e8a5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 72 additions and 5 deletions

View File

@ -83,24 +83,45 @@ async function normalizeExistingPath(targetPath: string): Promise<string> {
}
}
/**
* Resolve and verify that a worktree path belongs to a registered repo.
*
* Why this doesn't use resolveAuthorizedPath: linked worktrees can live
* anywhere on disk (e.g. ~/.codex/worktrees/), far outside the repo root
* and workspaceDir that resolveAuthorizedPath allows. The security boundary
* for git operations is *worktree registration* the path must match a
* worktree reported by `git worktree list` for a known repo not
* directory containment within allowed roots.
*/
export async function resolveRegisteredWorktreePath(
worktreePath: string,
store: Store
): Promise<string> {
const resolvedPath = await resolveAuthorizedPath(worktreePath, store)
// Reject obviously malformed paths early — mirrors the null-byte check in
// validateGitRelativeFilePath and prevents probing via realpath.
if (!worktreePath || worktreePath.includes('\0')) {
throw new Error('Access denied: invalid worktree path')
}
const resolvedTarget = resolve(worktreePath)
// Resolve through symlinks when the path exists on disk, so that we
// compare canonical paths on both sides (git worktree list also resolves
// symlinks).
const normalizedTarget = await normalizeExistingPath(resolvedTarget)
for (const repo of store.getRepos()) {
const normalizedRepoPath = await normalizeExistingPath(repo.path)
if (resolvedPath === normalizedRepoPath) {
return resolvedPath
if (normalizedTarget === normalizedRepoPath) {
return normalizedTarget
}
const worktrees = await listWorktrees(repo.path)
for (const worktree of worktrees) {
const normalizedWorktreePath = await normalizeExistingPath(worktree.path)
if (resolvedPath === normalizedWorktreePath) {
return resolvedPath
if (normalizedTarget === normalizedWorktreePath) {
return normalizedTarget
}
}
}

View File

@ -275,6 +275,52 @@ describe('registerFilesystemHandlers', () => {
expect(getBranchCompareMock).toHaveBeenCalledWith('/workspace/repo-feature', 'origin/main')
})
it('allows git operations on worktrees outside repo/workspace roots', async () => {
// Linked worktrees can live anywhere on disk (e.g. ~/.codex/worktrees/).
// As long as the path matches a worktree reported by `git worktree list`
// for a registered repo, it should be allowed — the security boundary is
// worktree registration, not directory containment.
listWorktreesMock.mockResolvedValue([
{
path: '/workspace/repo',
head: 'abc',
branch: 'refs/heads/main',
isBare: false,
isMainWorktree: true
},
{
path: '/external/worktrees/feature',
head: 'def',
branch: 'refs/heads/feature',
isBare: false,
isMainWorktree: false
}
])
getBranchCompareMock.mockResolvedValue({
summary: {
baseRef: 'origin/main',
baseOid: 'base-oid',
compareRef: 'feature',
headOid: 'head-oid',
mergeBase: 'merge-base-oid',
changedFiles: 0,
status: 'ready'
},
entries: []
})
registerFilesystemHandlers(store as never)
// /external/worktrees/feature is outside both /workspace/repo and /workspace
await handlers.get('git:branchCompare')!(null, {
worktreePath: '/external/worktrees/feature',
baseRef: 'origin/main'
})
expect(getBranchCompareMock).toHaveBeenCalledWith('/external/worktrees/feature', 'origin/main')
})
it('routes branch diff queries through the pinned branch diff helper', async () => {
getBranchDiffMock.mockResolvedValue({
kind: 'text',