v1.13.18-codecontext-file-path: resolve file_path against project root in codecontext wrappers
Four codecontext sidecar wrappers — get_file_analysis (required file_path), get_symbol_info, get_dependencies, and get_semantic_neighborhoods (optional) — forwarded file_path to the HTTP sidecar unchanged. The sidecar's internal file index is keyed on absolute paths, so any relative path from the model returned "File not found in graph". Three back-to-back failures observed in one chat on 2026-05-22 17:56 UTC, ~48 s of wasted tool budget. ## Resolver Add resolveProjectPath(projectRoot, rawPath) in codecontext_client.ts: trim check → absolute/relative branch (both go through resolve() so dot-segments normalise) → realpath with ENOENT fallthrough → escape check using the realpathed value. Error shape mirrors the existing target_dir escape error byte-for-byte; only the field name differs. Wired into callCodecontext at the args-spread site, guarded on file_path presence + non-empty. All four wrappers benefit from one call site; wrappers without file_path (overview, framework, watch, search) are unaffected. ## Schema trim .trim() added to all four file_path Zod schemas: get_file_analysis: z.string().trim().min(1) get_symbol_info: z.string().trim().optional() get_dependencies: z.string().trim().optional() get_semantic_neighborhoods: z.string().trim().optional() Absorbs trailing newlines / whitespace from model output before the resolver sees the value. ## Adversarial review fixes Adversarial pass surfaced two P2 findings: 1. Absolute path with `..` resolving outside the project root (e.g. `<projectRoot>/../etc/passwd`) that ENOENTs at realpath would slip through the literal prefix-check: the raw string starts with `<projectRoot>/`. Fix: resolve() the absolute branch's candidate too, so dot-segments normalise before the prefix check. 2. No symlink-escape test coverage. Realpath's stated purpose (catching in-project symlinks pointing outside the project) was never tested. Added: create a tmpdir outside projectRoot, symlink projectRoot/evil-link → outside file, assert rejection. ## Tests codecontext_client.test.ts: 19 tests (10 baseline + 9 new file_path resolution cases). Cases cover: relative→absolute, absolute-inside, relative-escape, absolute-outside, ENOENT-fallthrough, empty-string, wrapper-without-file_path, absolute-with-`..`-ENOENT, symlink-leaving-root. codecontext_tools.test.ts: one assertion updated to expect the resolved-absolute file_path on the wire (previously asserted the raw relative path passed through, which is exactly the bug being fixed). Full suite: 301 passed, 7 skipped. ## Affected / unaffected - get_codebase_overview, get_framework_analysis, watch_changes, search_symbols: no file_path arg → resolver guard skips them. No behavior change. - get_semantic_neighborhoods IS in SYNTHESIS_TOOLS — previously-failing relative-path calls will now successfully synthesize. Desirable, not a regression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { mkdir, mkdtemp, rm } from 'node:fs/promises';
|
||||
import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises';
|
||||
import { join } from 'node:path';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { callCodecontext } from '../codecontext_client.js';
|
||||
@@ -203,3 +203,197 @@ describe('callCodecontext — error paths', () => {
|
||||
).rejects.toThrow(/timed out after 30000ms/);
|
||||
});
|
||||
});
|
||||
|
||||
// ---- v1.13.18: file_path resolution tests -----------------------------------
|
||||
|
||||
describe('callCodecontext — file_path resolution', () => {
|
||||
// Case 1: relative path resolves to absolute under project root
|
||||
it('resolves a relative file_path to an absolute path inside project root', async () => {
|
||||
// Create a real file so realpath can canonicalise it
|
||||
const fileName = 'src_module.ts';
|
||||
await writeFile(join(projectDir, fileName), '// hello');
|
||||
const fetcher = vi.fn().mockResolvedValue(
|
||||
mockJSONResponse({ result: 'file analysis', error: null }),
|
||||
);
|
||||
await callCodecontext(
|
||||
{
|
||||
toolName: 'get_file_analysis',
|
||||
args: { file_path: fileName },
|
||||
projectPath: projectDir,
|
||||
},
|
||||
fetcher as unknown as typeof fetch,
|
||||
);
|
||||
expect(fetcher).toHaveBeenCalledTimes(1);
|
||||
const body = JSON.parse(fetcher.mock.calls[0]![1]!.body as string);
|
||||
// Should be the resolved absolute path
|
||||
expect(body.file_path).toBe(join(projectDir, fileName));
|
||||
});
|
||||
|
||||
// Case 2: absolute path inside project root → realpathed → forwarded
|
||||
it('passes through an absolute file_path inside project root', async () => {
|
||||
const fileName = 'absolute_target.ts';
|
||||
const absPath = join(projectDir, fileName);
|
||||
await writeFile(absPath, '// absolute');
|
||||
const fetcher = vi.fn().mockResolvedValue(
|
||||
mockJSONResponse({ result: 'analysis', error: null }),
|
||||
);
|
||||
await callCodecontext(
|
||||
{
|
||||
toolName: 'get_file_analysis',
|
||||
args: { file_path: absPath },
|
||||
projectPath: projectDir,
|
||||
},
|
||||
fetcher as unknown as typeof fetch,
|
||||
);
|
||||
const body = JSON.parse(fetcher.mock.calls[0]![1]!.body as string);
|
||||
expect(body.file_path).toBe(absPath);
|
||||
});
|
||||
|
||||
// Case 3: relative escape path → rejected with same error shape as target_dir escape
|
||||
it('rejects a relative file_path that escapes the project root', async () => {
|
||||
const fetcher = vi.fn();
|
||||
await expect(
|
||||
callCodecontext(
|
||||
{
|
||||
toolName: 'get_file_analysis',
|
||||
args: { file_path: '../../etc/passwd' },
|
||||
projectPath: projectDir,
|
||||
},
|
||||
fetcher as unknown as typeof fetch,
|
||||
),
|
||||
).rejects.toThrow(/escapes project root/);
|
||||
expect(fetcher).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Case 4: absolute path outside project root → rejected
|
||||
it('rejects an absolute file_path outside the project root', async () => {
|
||||
const fetcher = vi.fn();
|
||||
await expect(
|
||||
callCodecontext(
|
||||
{
|
||||
toolName: 'get_file_analysis',
|
||||
// /etc/passwd is outside any tmpdir project root
|
||||
args: { file_path: '/etc/passwd' },
|
||||
projectPath: projectDir,
|
||||
},
|
||||
fetcher as unknown as typeof fetch,
|
||||
),
|
||||
).rejects.toThrow(/escapes project root/);
|
||||
expect(fetcher).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Case 5: nonexistent file (ENOENT) → forwarded as un-realpath'd absolute
|
||||
it('forwards a nonexistent file_path as absolute without throwing', async () => {
|
||||
const missingPath = join(projectDir, 'does_not_exist.ts');
|
||||
const fetcher = vi.fn().mockResolvedValue(
|
||||
mockJSONResponse({ result: null, error: 'File not found in graph: ' + missingPath }),
|
||||
);
|
||||
// The resolver should NOT throw; the error comes back from the sidecar
|
||||
await expect(
|
||||
callCodecontext(
|
||||
{
|
||||
toolName: 'get_file_analysis',
|
||||
args: { file_path: 'does_not_exist.ts' },
|
||||
projectPath: projectDir,
|
||||
},
|
||||
fetcher as unknown as typeof fetch,
|
||||
),
|
||||
).rejects.toThrow(/File not found in graph/);
|
||||
// Wire was still called — resolver forwarded the path
|
||||
expect(fetcher).toHaveBeenCalledTimes(1);
|
||||
const body = JSON.parse(fetcher.mock.calls[0]![1]!.body as string);
|
||||
// Should receive the absolute (non-realpathed) path
|
||||
expect(body.file_path).toBe(missingPath);
|
||||
});
|
||||
|
||||
// Case 6: empty string → skipped by guard, reaches wire unmodified
|
||||
// Note: Zod .trim().min(1) in get_file_analysis rejects empty before the
|
||||
// shim is reached in production. At the shim layer, the guard
|
||||
// `file_path.trim() !== ''` skips the resolver for empty strings so that
|
||||
// optional-file_path wrappers treat '' as "not provided". This is a
|
||||
// deliberate design; callers that require file_path validate at the Zod layer.
|
||||
it('skips resolver for empty string file_path (treated as not provided)', async () => {
|
||||
const fetcher = vi.fn().mockResolvedValue(
|
||||
mockJSONResponse({ result: 'analysis', error: null }),
|
||||
);
|
||||
// Should succeed — empty string is treated as "no file_path"
|
||||
await callCodecontext(
|
||||
{
|
||||
toolName: 'get_file_analysis',
|
||||
args: { file_path: '' },
|
||||
projectPath: projectDir,
|
||||
},
|
||||
fetcher as unknown as typeof fetch,
|
||||
);
|
||||
expect(fetcher).toHaveBeenCalledTimes(1);
|
||||
const body = JSON.parse(fetcher.mock.calls[0]![1]!.body as string);
|
||||
// Empty string passes through unchanged (resolver not invoked)
|
||||
expect(body.file_path).toBe('');
|
||||
});
|
||||
|
||||
// Case 7: wrapper without file_path (e.g. get_codebase_overview) → resolver not invoked
|
||||
it('does not invoke file_path resolver when file_path is absent from args', async () => {
|
||||
const fetcher = vi.fn().mockResolvedValue(
|
||||
mockJSONResponse({ result: 'overview', error: null }),
|
||||
);
|
||||
await callCodecontext(
|
||||
{
|
||||
toolName: 'get_codebase_overview',
|
||||
args: { include_stats: true },
|
||||
projectPath: projectDir,
|
||||
},
|
||||
fetcher as unknown as typeof fetch,
|
||||
);
|
||||
expect(fetcher).toHaveBeenCalledTimes(1);
|
||||
const body = JSON.parse(fetcher.mock.calls[0]![1]!.body as string);
|
||||
// No file_path in the wire body
|
||||
expect('file_path' in body).toBe(false);
|
||||
});
|
||||
|
||||
// Case 8: absolute path with `..` that resolves outside project root, even
|
||||
// when the literal path is ENOENT. Without resolve() in the absolute branch
|
||||
// the prefix check false-positives because the raw `<projectDir>/../etc/x`
|
||||
// literal starts with `<projectDir>/`.
|
||||
it('rejects absolute file_path with `..` resolving outside project root (ENOENT branch)', async () => {
|
||||
const fetcher = vi.fn();
|
||||
const escapingAbsolute = `${projectDir}/../etc/non_existent_passwd`;
|
||||
await expect(
|
||||
callCodecontext(
|
||||
{
|
||||
toolName: 'get_file_analysis',
|
||||
args: { file_path: escapingAbsolute },
|
||||
projectPath: projectDir,
|
||||
},
|
||||
fetcher as unknown as typeof fetch,
|
||||
),
|
||||
).rejects.toThrow(/escapes project root/);
|
||||
expect(fetcher).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Case 9: in-project symlink targeting outside the project root. This is the
|
||||
// canonical realpath defense — realpath must canonicalise the symlink and
|
||||
// the escape check must reject. Without this test, a symlink-out hole could
|
||||
// regress silently.
|
||||
it('rejects file_path that resolves through a symlink leaving project root', async () => {
|
||||
const outsideDir = await mkdtemp(join(tmpdir(), 'codecontext-outside-'));
|
||||
try {
|
||||
const evilTarget = join(outsideDir, 'secrets.txt');
|
||||
await writeFile(evilTarget, 'top secret');
|
||||
await symlink(evilTarget, join(projectDir, 'evil-link'));
|
||||
const fetcher = vi.fn();
|
||||
await expect(
|
||||
callCodecontext(
|
||||
{
|
||||
toolName: 'get_file_analysis',
|
||||
args: { file_path: 'evil-link' },
|
||||
projectPath: projectDir,
|
||||
},
|
||||
fetcher as unknown as typeof fetch,
|
||||
),
|
||||
).rejects.toThrow(/escapes project root/);
|
||||
expect(fetcher).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
await rm(outsideDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -70,7 +70,7 @@ describe('codecontext wrappers — toolName + args forwarding', () => {
|
||||
const { url, body } = parsePOST(fetcher);
|
||||
expect(url).toMatch(/\/v1\/get_file_analysis$/);
|
||||
expect(body).toMatchObject({
|
||||
file_path: 'apps/server/src/index.ts',
|
||||
file_path: join(projectDir, 'apps/server/src/index.ts'),
|
||||
target_dir: projectDir,
|
||||
});
|
||||
});
|
||||
|
||||
@@ -17,7 +17,7 @@
|
||||
// which we re-surface with a hint to add the file to .codecontextignore.
|
||||
|
||||
import { access, copyFile, realpath } from 'node:fs/promises';
|
||||
import { join } from 'node:path';
|
||||
import { isAbsolute, join, resolve, sep } from 'node:path';
|
||||
import { truncateIfNeeded } from './truncate.js';
|
||||
|
||||
// v1.13.12 fix: codecontext crashes on empty source files (upstream issue #37)
|
||||
@@ -51,6 +51,45 @@ async function ensureIgnoreFile(projectRoot: string): Promise<void> {
|
||||
}
|
||||
}
|
||||
|
||||
// v1.13.18: resolve a `file_path` arg to an absolute path anchored within
|
||||
// the (already realpath'd) projectRoot. Contract:
|
||||
// - empty/whitespace-only → INVALID_FILE_PATH error
|
||||
// - relative path → resolve(projectRoot, rawPath) (normalises dot-segments)
|
||||
// - absolute path → resolve(rawPath) (also normalises — e.g. /root/../etc
|
||||
// becomes /etc so the prefix-check below rejects it even in the ENOENT
|
||||
// fallthrough where realpath couldn't canonicalise)
|
||||
// - try realpath; on ENOENT fall through with the (normalised) absolute
|
||||
// (the sidecar issues its own "File not found in graph" that the model
|
||||
// can self-correct on; re-implementing the check here would diverge)
|
||||
// - if the final path doesn't sit inside projectRoot → escape error
|
||||
// (same shape as target_dir escape, only the field name differs)
|
||||
async function resolveProjectPath(
|
||||
projectRoot: string,
|
||||
rawPath: string,
|
||||
): Promise<string> {
|
||||
if (rawPath.trim() === '') {
|
||||
throw new Error('INVALID_FILE_PATH: file_path must not be empty');
|
||||
}
|
||||
const candidate = isAbsolute(rawPath) ? resolve(rawPath) : resolve(projectRoot, rawPath);
|
||||
let resolved: string;
|
||||
try {
|
||||
resolved = await realpath(candidate);
|
||||
} catch (err: unknown) {
|
||||
if ((err as NodeJS.ErrnoException).code === 'ENOENT') {
|
||||
// File doesn't exist yet (or was deleted). Forward the absolute path;
|
||||
// codecontext will return "File not found in graph" which the model
|
||||
// can self-correct on.
|
||||
resolved = candidate;
|
||||
} else {
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
if (resolved !== projectRoot && !resolved.startsWith(projectRoot + sep)) {
|
||||
throw new Error(`file_path ${rawPath} escapes project root ${projectRoot}`);
|
||||
}
|
||||
return resolved;
|
||||
}
|
||||
|
||||
export interface CodecontextRequest {
|
||||
toolName: string;
|
||||
args: Record<string, unknown>;
|
||||
@@ -96,7 +135,14 @@ export async function callCodecontext(
|
||||
|
||||
// Step 2: re-build args with the resolved target_dir so codecontext sees
|
||||
// the real absolute path, not a symlink or relative form.
|
||||
const argsToSend = { ...req.args, target_dir: resolvedTarget };
|
||||
// v1.13.18: also resolve file_path when present — the sidecar index is keyed
|
||||
// on absolute paths, so a relative path from the model yields "File not found
|
||||
// in graph". Same escape check as target_dir; ENOENT falls through so the
|
||||
// sidecar produces the canonical "File not found in graph" the model can fix.
|
||||
const argsToSend: Record<string, unknown> = { ...req.args, target_dir: resolvedTarget };
|
||||
if (typeof req.args['file_path'] === 'string' && req.args['file_path'].trim() !== '') {
|
||||
argsToSend['file_path'] = await resolveProjectPath(resolvedProject, req.args['file_path']);
|
||||
}
|
||||
|
||||
// Step 3: POST with a hard timeout. AbortController + setTimeout pattern
|
||||
// matches web_fetch.ts; nothing fancier needed.
|
||||
|
||||
@@ -5,7 +5,7 @@ import type { ToolDef } from '../../tools.js';
|
||||
import { callCodecontext, type CodecontextResponse } from '../../codecontext_client.js';
|
||||
|
||||
export const GetDependenciesInput = z.object({
|
||||
file_path: z.string().optional(),
|
||||
file_path: z.string().trim().optional(),
|
||||
direction: z.enum(['incoming', 'outgoing', 'both']).optional(),
|
||||
});
|
||||
export type GetDependenciesInputT = z.infer<typeof GetDependenciesInput>;
|
||||
|
||||
@@ -5,7 +5,7 @@ import type { ToolDef } from '../../tools.js';
|
||||
import { callCodecontext, type CodecontextResponse } from '../../codecontext_client.js';
|
||||
|
||||
export const GetFileAnalysisInput = z.object({
|
||||
file_path: z.string().min(1),
|
||||
file_path: z.string().trim().min(1),
|
||||
});
|
||||
export type GetFileAnalysisInputT = z.infer<typeof GetFileAnalysisInput>;
|
||||
|
||||
|
||||
@@ -5,7 +5,7 @@ import type { ToolDef } from '../../tools.js';
|
||||
import { callCodecontext, type CodecontextResponse } from '../../codecontext_client.js';
|
||||
|
||||
export const GetSemanticNeighborhoodsInput = z.object({
|
||||
file_path: z.string().optional(),
|
||||
file_path: z.string().trim().optional(),
|
||||
include_basic: z.boolean().optional(),
|
||||
include_quality: z.boolean().optional(),
|
||||
max_results: z.number().int().positive().optional(),
|
||||
|
||||
@@ -6,7 +6,7 @@ import { callCodecontext, type CodecontextResponse } from '../../codecontext_cli
|
||||
|
||||
export const GetSymbolInfoInput = z.object({
|
||||
symbol_name: z.string().min(1),
|
||||
file_path: z.string().optional(),
|
||||
file_path: z.string().trim().optional(),
|
||||
framework_type: z.string().optional(),
|
||||
});
|
||||
export type GetSymbolInfoInputT = z.infer<typeof GetSymbolInfoInput>;
|
||||
|
||||
Reference in New Issue
Block a user