Enhance error handling for file path validation in tools

- Improved error handling in `readFileTool`, `listDirTool`, `globTool`, and other tools to catch invalid path errors and return user-friendly messages.
- Refactored path handling to ensure consistent validation across multiple tools, enhancing robustness and user feedback.
- Updated `safePath` function to normalize input paths and prevent potential security issues by checking workspace boundaries.
- Added utility functions for quoting paths in shell commands to handle spaces and special characters correctly.
This commit is contained in:
Pawan Osman
2026-07-15 16:43:54 +03:00
parent 7f000c254c
commit f01e337cb6
4 changed files with 141 additions and 29 deletions
+37 -7
View File
@@ -27,7 +27,12 @@ const IMAGE_MIME: Record<string, string> = {
// ---- Read ----
export const readFileTool = defineTool("Read", false, async (input) => {
if (typeof input.path !== "string" || !input.path) return { output: "error: path is required and must be a string" };
const p = safePath(input.path);
let p: string;
try {
p = safePath(input.path);
} catch (e) {
return { output: `error: invalid path: ${e instanceof Error ? e.message : String(e)}` };
}
const ext = path.extname(p).toLowerCase();
// Image files: return a base64 image block so it reaches the model.
@@ -83,7 +88,12 @@ export const readFileTool = defineTool("Read", false, async (input) => {
// ---- ListDir ----
export const listDirTool = defineTool("ListDir", false, async (input) => {
try {
const p = safePath(input.path ?? ".");
let p: string;
try {
p = safePath(input.path ?? ".");
} catch (e) {
return { output: `error: invalid path: ${e instanceof Error ? e.message : String(e)}` };
}
const entries = await fs.readdir(p, { withFileTypes: true });
const out =
entries
@@ -100,9 +110,14 @@ export const listDirTool = defineTool("ListDir", false, async (input) => {
// ---- Glob ----
export const globTool = defineTool("Glob", false, async (input, abortSignal) => {
try {
const root = input.target_directory ? safePath(input.target_directory) : getWorkspaceRoot();
let root: string;
try {
root = input.target_directory ? safePath(input.target_directory) : getWorkspaceRoot();
} catch (e) {
return { output: `error: invalid target_directory: ${e instanceof Error ? e.message : String(e)}` };
}
// Only walk ignored dirs when the pattern explicitly targets them
// (e.g. "**/node_modules/**") otherwise node_modules hangs the tool.
// (e.g. "**/node_modules/**") - otherwise node_modules hangs the tool.
let pattern: string = String(input.glob_pattern ?? "");
if (pattern && !pattern.startsWith("**/")) pattern = "**/" + pattern;
const wantsIgnored = /node_modules|\.git|[/\\]dist[/\\]|[/\\]out[/\\]|[/\\]build[/\\]/.test(pattern);
@@ -171,7 +186,12 @@ function blockedInMultitask(ctx?: ToolContext): boolean {
const editExecute: Tool["execute"] = async (input, _signal, _callId, ctx) => {
if (blockedInMultitask(ctx)) return MULTITASK_BLOCK;
if (typeof input.path !== "string" || !input.path) return { output: "error: path is required and must be a string" };
const p = safePath(input.path);
let p: string;
try {
p = safePath(input.path);
} catch (e) {
return { output: `error: invalid path: ${e instanceof Error ? e.message : String(e)}` };
}
let existedBefore = false;
try {
await fs.access(p);
@@ -250,7 +270,12 @@ export const writeTool = defineTool("Write", true, editExecute);
export const deleteFileTool = defineTool("Delete", true, async (input, _signal, _callId, ctx) => {
if (blockedInMultitask(ctx)) return MULTITASK_BLOCK;
if (typeof input.path !== "string" || !input.path) return { output: "error: path is required and must be a string" };
const p = safePath(input.path);
let p: string;
try {
p = safePath(input.path);
} catch (e) {
return { output: `error: invalid path: ${e instanceof Error ? e.message : String(e)}` };
}
let before = "";
try {
before = await fs.readFile(p, "utf8");
@@ -326,7 +351,12 @@ export const editNotebookTool = defineTool("EditNotebook", true, async (input, _
const oldString = String(input?.old_string ?? "");
const newString = String(input?.new_string ?? "");
const abs = safePath(target);
let abs: string;
try {
abs = safePath(target);
} catch (e) {
return { output: `error: invalid path: ${e instanceof Error ? e.message : String(e)}` };
}
// Read (or scaffold) the notebook JSON.
let nb: any;
+23 -3
View File
@@ -43,7 +43,14 @@ export const grepTool = defineTool("Grep", false, async (input, abortSignal) =>
if (abortSignal?.aborted) return { output: "(grep aborted)" };
const root = getWorkspaceRoot();
const mode: string = input.output_mode || "content";
const target = input.path ? safePath(input.path) : ".";
let target = ".";
if (input.path) {
try {
target = safePath(input.path); // spawn arg — spaces OK without shell quoting
} catch (e) {
return { output: `error: invalid path: ${e instanceof Error ? e.message : String(e)}` };
}
}
const cap = Math.max(1, Math.min(Number(input.head_limit) || 200, 2000));
const skip = Math.max(0, Number(input.offset) || 0);
const pattern = String(input.pattern ?? "");
@@ -119,7 +126,14 @@ export const grepTool = defineTool("Grep", false, async (input, abortSignal) =>
}
// Node fallback (no ripgrep available). Honor path/glob/type/-A/-B/-C/multiline.
const scopeRoot = input.path ? safePath(input.path) : root;
let scopeRoot = root;
if (input.path) {
try {
scopeRoot = safePath(input.path);
} catch (e) {
return { output: `error: invalid path: ${e instanceof Error ? e.message : String(e)}` };
}
}
const all: string[] = [];
await walk(scopeRoot, all, 0, false, abortSignal, 15_000);
if (abortSignal?.aborted) return { output: "(grep aborted)" };
@@ -235,7 +249,13 @@ export const semanticSearchTool = defineTool("SemanticSearch", false, async (inp
// Scope by target_directories (prefix match on workspace-relative paths).
const dirs: string[] = Array.isArray(input.target_directories) ? input.target_directories : [];
const prefixes = dirs
.map((d) => path.relative(root, safePath(String(d))).split(path.sep).join("/"))
.map((d) => {
try {
return path.relative(root, safePath(String(d))).split(path.sep).join("/");
} catch {
return "";
}
})
.filter((p) => p && !p.startsWith(".."));
const filter = prefixes.length
? (rel: string) => prefixes.some((p) => rel === p || rel.startsWith(p + "/"))
+61 -17
View File
@@ -47,33 +47,64 @@ function buildNotify(input: any, ctx: any): ShellNotify | undefined {
};
}
/** Quote a filesystem path for the session shell (spaces, quotes, unicode). */
function quotePath(p: string): string {
if (isWin) {
// PowerShell single-quoted literal; escape ' by doubling. Drop trailing
// backslash that would escape the closing quote if we ever used doubles.
return `'${p.replace(/'/g, "''")}'`;
}
// bash: single-quote with '\'' for embedded quotes
return `'${p.replace(/'/g, `'\\''`)}'`;
}
/**
* Frame a command so the session always prints a unique sentinel with exit code.
* PowerShell: use `if ($?)` — `$LASTEXITCODE` is often $null for native/cmdlets
* and would never emit a sentinel (classic hang).
* Frame a command so the session ALWAYS prints a sentinel with exit code.
*
* Critical: do NOT paste the user command raw into the script. Paths with
* spaces, unclosed quotes, or bad syntax leave PowerShell waiting for more
* input forever (no sentinel → tool looks "stuck"). Instead base64-encode the
* command and Invoke-Expression / bash -c it inside try/catch/finally so
* parse errors still emit the sentinel and free the session.
*/
function wrapCommand(command: string, cd: string): string {
const b64 = Buffer.from(command, "utf8").toString("base64");
if (isWin) {
const loc = cd
? `Push-Location -LiteralPath '${cd.replace(/'/g, "''")}'; try { `
: "";
const endLoc = cd ? ` } finally { Pop-Location } ` : " ";
// Always write the sentinel, even if the command throws.
// Use [Console]::Out so the line is not stuck in PowerShell's output pipeline.
const cdBlock = cd
? `Push-Location -LiteralPath ${quotePath(cd)}; $__oc_pop = $true; `
: `$__oc_pop = $false; `;
// Decode → Invoke-Expression inside try; sentinel always in finally.
// $? / $LASTEXITCODE after IEX covers native cmds and cmdlets.
return (
`${loc}${command}${endLoc}\n` +
`if ($?) { [Console]::Out.WriteLine("${SENTINEL}:0") } else { ` +
`$__oc = if ($null -ne $LASTEXITCODE -and "$LASTEXITCODE" -ne "") { $LASTEXITCODE } else { 1 }; ` +
`[Console]::Out.WriteLine("${SENTINEL}:$__oc") }\n`
`${cdBlock}` +
`$__oc_ok = $false; $__oc_code = 1; ` +
`try { ` +
`$__oc_cmd = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('${b64}')); ` +
`Invoke-Expression -Command $__oc_cmd; ` +
`if ($?) { $__oc_ok = $true; $__oc_code = 0 } ` +
`elseif ($null -ne $LASTEXITCODE -and "$LASTEXITCODE" -ne '') { $__oc_code = [int]$LASTEXITCODE } ` +
`else { $__oc_code = 1 } ` +
`} catch { ` +
`[Console]::Error.WriteLine($_.Exception.Message); $__oc_ok = $false; $__oc_code = 1 ` +
`} finally { ` +
`if ($__oc_pop) { Pop-Location -ErrorAction SilentlyContinue }; ` +
`if ($__oc_ok) { [Console]::Out.WriteLine("${SENTINEL}:0") } ` +
`else { [Console]::Out.WriteLine("${SENTINEL}:$__oc_code") } ` +
`}\n`
);
}
const push = cd ? `pushd '${cd.replace(/'/g, `'\\''`)}' >/dev/null 2>&1 || true\n` : "";
// bash: decode to a temp eval so spaces/quotes never break the outer script.
// Always print sentinel even if eval fails (set +e).
const push = cd ? `pushd ${quotePath(cd)} >/dev/null 2>&1 || true\n` : "";
const pop = cd ? `popd >/dev/null 2>&1 || true\n` : "";
return (
`${push}${command}\n` +
`set +e\n` +
`${push}` +
`__oc_cmd=$(printf '%s' '${b64}' | base64 -d 2>/dev/null || printf '%s' '${b64}' | base64 -D 2>/dev/null)\n` +
`eval "$__oc_cmd"\n` +
`__oc_rc=$?\n` +
`${pop}` +
`echo "${SENTINEL}:$__oc_rc"\n`
`printf '%s\\n' "${SENTINEL}:$__oc_rc"\n`
);
}
@@ -171,7 +202,20 @@ export const runTerminalTool = defineTool("Shell", true, async (input, abortSign
};
abortSignal?.addEventListener("abort", onAbort);
const cd = input.working_directory ? safePath(input.working_directory) : "";
let cd = "";
if (input.working_directory) {
try {
cd = safePath(String(input.working_directory));
} catch (e) {
abortSignal?.removeEventListener("abort", onAbort);
sh.done = true;
sh.exitCode = 1;
releaseQueue();
return {
output: `error: invalid working_directory: ${e instanceof Error ? e.message : String(e)}`,
};
}
}
const wrapped = wrapCommand(command, cd);
try {
+20 -2
View File
@@ -35,12 +35,30 @@ export function getRecentFiles(): string[] {
return out;
}
/**
* Resolve a workspace path safely. Handles spaces, unicode, and mixed
* separators. Does not shell-quote — callers that inject into a shell must
* quote the result (see shell.ts quotePath).
*/
export function safePath(rel: string): string {
const root = getWorkspaceRoot();
const abs = path.isAbsolute(rel) ? rel : path.join(root, rel);
// Normalize user input: trim, unify slashes, drop surrounding quotes the
// model sometimes wraps around paths that contain spaces.
let s = String(rel ?? "").trim();
if (
(s.startsWith('"') && s.endsWith('"')) ||
(s.startsWith("'") && s.endsWith("'"))
) {
s = s.slice(1, -1);
}
s = s.replace(/\//g, path.sep);
const abs = path.isAbsolute(s) ? s : path.join(root, s);
const norm = path.resolve(abs);
const ws = path.resolve(root);
if (norm !== ws && !norm.startsWith(ws + path.sep)) {
// Case-insensitive root check on Windows (C:\ vs c:\).
const normKey = process.platform === "win32" ? norm.toLowerCase() : norm;
const wsKey = process.platform === "win32" ? ws.toLowerCase() : ws;
if (normKey !== wsKey && !normKey.startsWith(wsKey + path.sep)) {
throw new Error(`path outside workspace: ${rel}`);
}
return norm;