diff --git a/sdk/src/__tests__/change-file.test.ts b/sdk/src/__tests__/change-file.test.ts index dff8969c7..656244906 100644 --- a/sdk/src/__tests__/change-file.test.ts +++ b/sdk/src/__tests__/change-file.test.ts @@ -36,6 +36,37 @@ describe('changeFile', () => { ) }) + test('tolerates absolute paths inside the project for string replacements', async () => { + const fs = createMockFs({ + files: { + '/repo/src/file.ts': 'const value = 1\n', + }, + }) + + const result = await changeFile({ + parameters: { + type: 'patch', + path: '/repo/src/file.ts', + content: '@@ -1,1 +1,1 @@\n-const value = 1\n+const value = 2\n', + }, + cwd: '/repo', + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + file: 'src/file.ts', + message: 'String replace applied successfully.', + }, + }, + ]) + expect(await fs.readFile('/repo/src/file.ts', 'utf-8')).toBe( + 'const value = 2\n', + ) + }) + test('returns a simple success message for new file writes', async () => { const fs = createMockFs() @@ -63,6 +94,58 @@ describe('changeFile', () => { ) }) + test('tolerates absolute paths inside the project for file writes', async () => { + const fs = createMockFs() + + const result = await changeFile({ + parameters: { + type: 'file', + path: '/repo/src/file.ts', + content: 'const value = 1\n', + }, + cwd: '/repo', + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + file: 'src/file.ts', + message: 'Created file successfully.', + }, + }, + ]) + expect(await fs.readFile('/repo/src/file.ts', 'utf-8')).toBe( + 'const value = 1\n', + ) + }) + + test('accepts paths whose file names start with two dots inside the project', async () => { + const fs = createMockFs() + + const result = await changeFile({ + parameters: { + type: 'file', + path: '/repo/..config', + content: 'value = true\n', + }, + cwd: '/repo', + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + file: '..config', + message: 'Created file successfully.', + }, + }, + ]) + expect(await fs.readFile('/repo/..config', 'utf-8')).toBe('value = true\n') + }) + test('returns a simple success message for overwritten file writes', async () => { const fs = createMockFs({ files: { @@ -93,4 +176,20 @@ describe('changeFile', () => { 'const value = 2\n', ) }) + + test('rejects absolute paths outside the project', async () => { + const fs = createMockFs() + + await expect( + changeFile({ + parameters: { + type: 'file', + path: '/outside/file.ts', + content: 'const value = 1\n', + }, + cwd: '/repo', + fs, + }), + ).rejects.toThrow('file path is outside the project directory') + }) }) diff --git a/sdk/src/__tests__/path-utils.test.ts b/sdk/src/__tests__/path-utils.test.ts new file mode 100644 index 000000000..4910dbcaf --- /dev/null +++ b/sdk/src/__tests__/path-utils.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, test } from 'bun:test' + +import { + getProjectPathLookupKeys, + resolveFilePathWithinProject, +} from '../tools/path-utils' + +describe('resolveFilePathWithinProject', () => { + test('normalizes relative paths to full and project-relative paths', () => { + expect(resolveFilePathWithinProject('/repo', 'src/file.ts')).toEqual({ + fullPath: '/repo/src/file.ts', + relativePath: 'src/file.ts', + }) + }) + + test('normalizes absolute paths inside the project', () => { + expect(resolveFilePathWithinProject('/repo', '/repo/src/file.ts')).toEqual({ + fullPath: '/repo/src/file.ts', + relativePath: 'src/file.ts', + }) + }) + + test('allows file names that start with two dots inside the project', () => { + expect(resolveFilePathWithinProject('/repo', '/repo/..config')).toEqual({ + fullPath: '/repo/..config', + relativePath: '..config', + }) + }) + + test('rejects paths outside the project', () => { + expect(resolveFilePathWithinProject('/repo', '../outside.ts')).toBeNull() + expect(resolveFilePathWithinProject('/repo', '/outside.ts')).toBeNull() + expect( + resolveFilePathWithinProject('/repo', '/repo-sibling/file.ts'), + ).toBeNull() + }) +}) + +describe('getProjectPathLookupKeys', () => { + test('returns the normalized relative key before the original absolute key', () => { + expect(getProjectPathLookupKeys('/repo', '/repo/src/file.ts')).toEqual([ + 'src/file.ts', + '/repo/src/file.ts', + ]) + }) + + test('dedupes relative paths that are already normalized', () => { + expect(getProjectPathLookupKeys('/repo', 'src/file.ts')).toEqual([ + 'src/file.ts', + ]) + }) + + test('returns only the original key for paths outside the project', () => { + expect(getProjectPathLookupKeys('/repo', '/outside.ts')).toEqual([ + '/outside.ts', + ]) + }) +}) diff --git a/sdk/src/__tests__/read-files.test.ts b/sdk/src/__tests__/read-files.test.ts index 965662286..afcafb7ac 100644 --- a/sdk/src/__tests__/read-files.test.ts +++ b/sdk/src/__tests__/read-files.test.ts @@ -11,13 +11,11 @@ import { spyOn, } from 'bun:test' - import { getFiles } from '../tools/read-files' import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem' import type { PathLike } from 'node:fs' - // Helper to create a mock filesystem function createMockFs(config: { files?: Record @@ -75,9 +73,10 @@ describe('getFiles', () => { beforeEach(() => { // Default: no files are ignored - isFileIgnoredSpy = spyOn(projectFileTree, 'isFileIgnored').mockResolvedValue( - false, - ) + isFileIgnoredSpy = spyOn( + projectFileTree, + 'isFileIgnored', + ).mockResolvedValue(false) }) afterEach(() => { @@ -320,9 +319,7 @@ describe('getFiles', () => { test('should handle mix of ignored and non-ignored files', async () => { // First call returns false (not ignored), second returns true (ignored) - isFileIgnoredSpy - .mockResolvedValueOnce(false) - .mockResolvedValueOnce(true) + isFileIgnoredSpy.mockResolvedValueOnce(false).mockResolvedValueOnce(true) const mockFs = createMockFs({ files: { @@ -393,7 +390,10 @@ describe('getFiles', () => { const mockFs = createMockFs({ files: {}, errors: { - '/project/broken.ts': { code: 'EACCES', message: 'Permission denied' }, + '/project/broken.ts': { + code: 'EACCES', + message: 'Permission denied', + }, }, }) @@ -423,6 +423,24 @@ describe('getFiles', () => { expect(result['src/index.ts']).toBe('content') }) + + test('should reject absolute paths in sibling directories with matching prefixes', async () => { + const mockFs = createMockFs({ + files: { + '/project-other/src/index.ts': { content: 'outside' }, + }, + }) + + const result = await getFiles({ + filePaths: ['/project-other/src/index.ts'], + cwd: '/project', + fs: mockFs, + }) + + expect(result['/project-other/src/index.ts']).toBe( + FILE_READ_STATUS.OUTSIDE_PROJECT, + ) + }) }) describe('fileFilter option', () => { diff --git a/sdk/src/__tests__/run-file-filter.test.ts b/sdk/src/__tests__/run-file-filter.test.ts index 9f49aff80..5d1be280a 100644 --- a/sdk/src/__tests__/run-file-filter.test.ts +++ b/sdk/src/__tests__/run-file-filter.test.ts @@ -1,4 +1,3 @@ - import * as mainPromptModule from '@codebuff/agent-runtime/main-prompt' import { FILE_READ_STATUS } from '@codebuff/common/old-constants' import * as projectFileTree from '@codebuff/common/project-file-tree' @@ -91,9 +90,7 @@ describe('CodebuffClientOptions fileFilter', () => { let requestedFiles: Record = {} spyOn(mainPromptModule, 'callMainPrompt').mockImplementation( - async ( - params: Parameters[0], - ) => { + async (params: Parameters[0]) => { const { sendAction, promptId, requestFiles } = params const sessionState = getInitialSessionState(getStubProjectFileContext()) @@ -177,9 +174,7 @@ describe('CodebuffClientOptions fileFilter', () => { let requestedFiles: Record = {} spyOn(mainPromptModule, 'callMainPrompt').mockImplementation( - async ( - params: Parameters[0], - ) => { + async (params: Parameters[0]) => { const { sendAction, promptId, requestFiles } = params const sessionState = getInitialSessionState(getStubProjectFileContext()) @@ -259,9 +254,7 @@ describe('CodebuffClientOptions fileFilter', () => { let optionalFileResult: string | null = null spyOn(mainPromptModule, 'callMainPrompt').mockImplementation( - async ( - params: Parameters[0], - ) => { + async (params: Parameters[0]) => { const { sendAction, promptId, requestOptionalFile } = params const sessionState = getInitialSessionState(getStubProjectFileContext()) @@ -319,6 +312,75 @@ describe('CodebuffClientOptions fileFilter', () => { expect(optionalFileResult).toBeNull() }) + it('should tolerate absolute requestOptionalFile paths inside cwd', async () => { + spyOn(databaseModule, 'getUserInfoFromApiKey').mockResolvedValue({ + id: 'user-123', + email: 'test@example.com', + discord_id: null, + stripe_customer_id: null, + banned: false, + created_at: new Date('2024-01-01T00:00:00Z'), + }) + spyOn(databaseModule, 'fetchAgentFromDatabase').mockResolvedValue(null) + spyOn(databaseModule, 'startAgentRun').mockResolvedValue('run-1') + spyOn(databaseModule, 'finishAgentRun').mockResolvedValue(undefined) + spyOn(databaseModule, 'addAgentStep').mockResolvedValue('step-1') + spyOn(projectFileTree, 'isFileIgnored').mockResolvedValue(false) + + const mockFs = createMockFs({ + files: { + '/project/src/index.ts': { content: 'normal file content' }, + }, + }) + + const optionalFileResult: { current: string | null } = { current: null } + + spyOn(mainPromptModule, 'callMainPrompt').mockImplementation( + async (params: Parameters[0]) => { + const { sendAction, promptId, requestOptionalFile } = params + const sessionState = getInitialSessionState(getStubProjectFileContext()) + + optionalFileResult.current = await requestOptionalFile({ + filePath: '/project/src/index.ts', + }) + + await sendAction({ + action: { + type: 'prompt-response', + promptId, + sessionState, + output: { + type: 'lastMessage', + value: [], + }, + }, + }) + + return { + sessionState, + output: { + type: 'lastMessage' as const, + value: [], + }, + } + }, + ) + + const client = new CodebuffClient({ + apiKey: 'test-key', + cwd: '/project', + fsSource: mockFs, + }) + + const result = await client.run({ + agent: 'base2', + prompt: 'read optional file', + }) + + expect(result.output.type).toBe('lastMessage') + expect(optionalFileResult.current).toBe('normal file content') + }) + it('should allow all files when no fileFilter is provided', async () => { spyOn(databaseModule, 'getUserInfoFromApiKey').mockResolvedValue({ id: 'user-123', @@ -343,9 +405,7 @@ describe('CodebuffClientOptions fileFilter', () => { let requestedFiles: Record = {} spyOn(mainPromptModule, 'callMainPrompt').mockImplementation( - async ( - params: Parameters[0], - ) => { + async (params: Parameters[0]) => { const { sendAction, promptId, requestFiles } = params const sessionState = getInitialSessionState(getStubProjectFileContext()) @@ -417,9 +477,7 @@ describe('CodebuffClientOptions fileFilter', () => { }) spyOn(mainPromptModule, 'callMainPrompt').mockImplementation( - async ( - params: Parameters[0], - ) => { + async (params: Parameters[0]) => { const { sendAction, promptId, requestFiles } = params const sessionState = getInitialSessionState(getStubProjectFileContext()) diff --git a/sdk/src/run.ts b/sdk/src/run.ts index 8d0c7986f..89044ab82 100644 --- a/sdk/src/run.ts +++ b/sdk/src/run.ts @@ -27,6 +27,7 @@ import { applyPatchTool } from './tools/apply-patch' import { codeSearch } from './tools/code-search' import { glob } from './tools/glob' import { listDirectory } from './tools/list-directory' +import { getProjectPathLookupKeys } from './tools/path-utils' import { getFiles } from './tools/read-files' import { runTerminalCommand } from './tools/run-terminal-command' @@ -434,7 +435,11 @@ async function runOnce({ cwd, fs, }) - return toOptionalFile(files[filePath] ?? null) + const lookupKeys = cwd + ? getProjectPathLookupKeys(cwd, filePath) + : [filePath] + const fileKey = lookupKeys.find((key) => key in files) + return toOptionalFile(fileKey === undefined ? null : files[fileKey]!) }, sendAction: ({ action }) => { if (action.type === 'action-error') { diff --git a/sdk/src/tools/change-file.ts b/sdk/src/tools/change-file.ts index ff34cc547..dbcb55eff 100644 --- a/sdk/src/tools/change-file.ts +++ b/sdk/src/tools/change-file.ts @@ -4,8 +4,11 @@ import { fileExists } from '@codebuff/common/util/file' import { applyPatch } from 'diff' import z from 'zod/v4' +import { resolveFilePathWithinProject } from './path-utils' + import type { CodebuffToolOutput } from '@codebuff/common/tools/list' import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem' +import type { ResolvedProjectPath } from './path-utils' const FileChangeSchema = z.object({ type: z.enum(['patch', 'file']), @@ -13,20 +16,12 @@ const FileChangeSchema = z.object({ content: z.string(), }) -function containsUpwardTraversal(dirPath: string): boolean { - const normalized = path.normalize(dirPath) - return normalized.includes('..') -} +type FileChange = z.infer -/** - * Checks if a path contains path traversal sequences that would escape the root. - * Uses proper path normalization to prevent traversal attacks. - */ -function containsPathTraversal(filePath: string): boolean { - const normalized = path.normalize(filePath) - // Check for absolute paths or paths starting with .. that escape root - return path.isAbsolute(normalized) || normalized.startsWith('..') -} +type ApplyChangeResult = + | { status: 'created' | 'modified'; file: string } + | { status: 'patchFailed'; file: string; patch: string } + | { status: 'invalid'; file: string } export async function changeFile(params: { parameters: unknown @@ -35,117 +30,78 @@ export async function changeFile(params: { }): Promise> { const { parameters, cwd, fs } = params - if (containsUpwardTraversal(cwd)) { - throw new Error('cwd contains invalid path traversal') - } const fileChange = FileChangeSchema.parse(parameters) - if (containsPathTraversal(fileChange.path)) { - throw new Error('file path contains invalid path traversal') + const resolvedPath = resolveFilePathWithinProject(cwd, fileChange.path) + if (!resolvedPath) { + throw new Error('file path is outside the project directory') } - const { created, modified, invalid, patchFailed } = await applyChanges({ - projectRoot: cwd, - changes: [fileChange], - fs, - }) - - const results: CodebuffToolOutput<'str_replace'>[0]['value'][] = [] + const result = await applyChange({ change: fileChange, resolvedPath, fs }) - for (const file of created) { - results.push({ - file, - message: - fileChange.type === 'patch' - ? 'String replace applied successfully.' - : 'Created file successfully.', - }) - } + return [{ type: 'json', value: formatApplyChangeResult(result, fileChange) }] +} - for (const file of modified) { - results.push({ - file, +function formatApplyChangeResult( + result: ApplyChangeResult, + fileChange: FileChange, +): CodebuffToolOutput<'str_replace'>[0]['value'] { + if (result.status === 'created' || result.status === 'modified') { + return { + file: result.file, message: fileChange.type === 'patch' ? 'String replace applied successfully.' - : 'Overwrote file successfully.', - }) + : result.status === 'created' + ? 'Created file successfully.' + : 'Overwrote file successfully.', + } } - for (const file of patchFailed) { - results.push({ - file, + if (result.status === 'patchFailed') { + return { + file: result.file, errorMessage: `Failed to apply patch.`, - patch: fileChange.content, - }) - } - - for (const file of invalid) { - results.push({ - file, - errorMessage: - 'Failed to write to file: file path caused an error or file could not be written', - }) + patch: result.patch, + } } - if (results.length !== 1) { - throw new Error( - `Internal error: Unexpected result length while modifying files: ${ - results.length - }`, - ) + return { + file: result.file, + errorMessage: + 'Failed to write to file: file path caused an error or file could not be written', } - - return [{ type: 'json', value: results[0] }] } -async function applyChanges(params: { - projectRoot: string - changes: { - type: 'patch' | 'file' - path: string - content: string - }[] +async function applyChange(params: { + change: FileChange + resolvedPath: ResolvedProjectPath fs: CodebuffFileSystem -}) { - const { projectRoot, changes, fs } = params - - const created: string[] = [] - const modified: string[] = [] - const patchFailed: string[] = [] - const invalid: string[] = [] - - for (const change of changes) { - const { path: filePath, content, type } = change - try { - const fullPath = path.join(projectRoot, filePath) - const exists = await fileExists({ filePath: fullPath, fs }) - if (!exists) { - const dirPath = path.dirname(fullPath) - await fs.mkdir(dirPath, { recursive: true }) - } - - if (type === 'file') { - await fs.writeFile(fullPath, content) - } else { - const oldContent = await fs.readFile(fullPath, 'utf-8') - const newContent = applyPatch(oldContent, content) - if (newContent === false) { - patchFailed.push(filePath) - continue - } - await fs.writeFile(fullPath, newContent) - } +}): Promise { + const { change, resolvedPath, fs } = params + const { content, type } = change + const { fullPath, relativePath } = resolvedPath + + try { + const exists = await fileExists({ filePath: fullPath, fs }) + if (!exists) { + const dirPath = path.dirname(fullPath) + await fs.mkdir(dirPath, { recursive: true }) + } - if (exists) { - modified.push(filePath) - } else { - created.push(filePath) + if (type === 'file') { + await fs.writeFile(fullPath, content) + } else { + const oldContent = await fs.readFile(fullPath, 'utf-8') + const newContent = applyPatch(oldContent, content) + if (newContent === false) { + return { status: 'patchFailed', file: relativePath, patch: content } } - } catch (error) { - console.error(`Failed to apply patch to ${filePath}:`, error, content) - invalid.push(filePath) + await fs.writeFile(fullPath, newContent) } - } - return { created, modified, invalid, patchFailed } + return { status: exists ? 'modified' : 'created', file: relativePath } + } catch (error) { + console.error(`Failed to apply patch to ${relativePath}:`, error, content) + return { status: 'invalid', file: relativePath } + } } diff --git a/sdk/src/tools/path-utils.ts b/sdk/src/tools/path-utils.ts new file mode 100644 index 000000000..92fe8a132 --- /dev/null +++ b/sdk/src/tools/path-utils.ts @@ -0,0 +1,41 @@ +import path from 'path' + +export type ResolvedProjectPath = { + fullPath: string + relativePath: string +} + +function escapesProject(relativePath: string): boolean { + return ( + relativePath === '..' || + relativePath.startsWith(`..${path.sep}`) || + path.isAbsolute(relativePath) + ) +} + +export function resolveFilePathWithinProject( + projectRoot: string, + filePath: string, +): ResolvedProjectPath | null { + const resolvedRoot = path.resolve(projectRoot) + const fullPath = path.isAbsolute(filePath) + ? path.resolve(filePath) + : path.resolve(resolvedRoot, filePath) + const relativePath = path.relative(resolvedRoot, fullPath) + + if (relativePath === '' || escapesProject(relativePath)) { + return null + } + + return { fullPath, relativePath } +} + +export function getProjectPathLookupKeys( + projectRoot: string, + filePath: string, +): string[] { + const resolvedPath = resolveFilePathWithinProject(projectRoot, filePath) + const keys = resolvedPath ? [resolvedPath.relativePath, filePath] : [filePath] + + return [...new Set(keys)] +} diff --git a/sdk/src/tools/read-files.ts b/sdk/src/tools/read-files.ts index c3c85cc68..a6462f1a2 100644 --- a/sdk/src/tools/read-files.ts +++ b/sdk/src/tools/read-files.ts @@ -1,8 +1,8 @@ -import path, { isAbsolute } from 'path' - import { FILE_READ_STATUS } from '@codebuff/common/old-constants' import { isFileIgnored } from '@codebuff/common/project-file-tree' +import { resolveFilePathWithinProject } from './path-utils' + import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem' export type FileFilterResult = { @@ -38,15 +38,12 @@ export async function getFiles(params: { continue } - // Convert absolute paths within project to relative paths - const relativePath = filePath.startsWith(cwd) - ? path.relative(cwd, filePath) - : filePath - const fullPath = path.join(cwd, relativePath) - if (isAbsolute(relativePath) || !fullPath.startsWith(cwd)) { - result[relativePath] = FILE_READ_STATUS.OUTSIDE_PROJECT + const resolvedPath = resolveFilePathWithinProject(cwd, filePath) + if (!resolvedPath) { + result[filePath] = FILE_READ_STATUS.OUTSIDE_PROJECT continue } + const { relativePath, fullPath } = resolvedPath // Apply file filter if provided const filterResult = fileFilter?.(relativePath)