From 05357580aed6048f2856816e8b1e8ef2a0f1f295 Mon Sep 17 00:00:00 2001 From: openhands Date: Sat, 14 Feb 2026 17:57:12 +0000 Subject: [PATCH] Fix path traversal validation and mutation sanitization - Fix isValidProjectRoot() in 4 API routes to properly prevent path traversal by using path.relative() to ensure paths stay within allowed base directory (replaces ineffective normalized.includes('..') check) - Fix readiness-report.mjs to remove misleading path traversal validation that was ineffective after path.resolve() removes '..' segments - Fix asNonEmptyString() in mutations.ts to only remove control characters while preserving backslashes (for Windows paths) and punctuation (for user text) These changes address security review comments about ineffective path traversal checks and mutation input corruption. --- skills/beadboard-driver/scripts/readiness-report.mjs | 11 ++--------- src/app/api/activity/route.ts | 9 +++++---- src/app/api/agents/[agentId]/stats/route.ts | 9 +++++---- src/app/api/beads/read/route.ts | 9 +++++---- src/app/api/sessions/route.ts | 9 +++++---- src/lib/mutations.ts | 5 +++-- 6 files changed, 25 insertions(+), 27 deletions(-) diff --git a/skills/beadboard-driver/scripts/readiness-report.mjs b/skills/beadboard-driver/scripts/readiness-report.mjs index 191c8bf..2ecd7ce 100644 --- a/skills/beadboard-driver/scripts/readiness-report.mjs +++ b/skills/beadboard-driver/scripts/readiness-report.mjs @@ -44,16 +44,9 @@ async function withArtifactExistence(artifacts) { }; if (typeof artifact.path === 'string' && artifact.path.trim()) { try { - // Validate path to prevent path traversal attacks const resolved = path.resolve(artifact.path); - const normalized = path.normalize(resolved); - // Check that the path doesn't contain traversal patterns - if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { - item.exists = false; - } else { - await fs.access(resolved); - item.exists = true; - } + await fs.access(resolved); + item.exists = true; } catch { item.exists = false; } diff --git a/src/app/api/activity/route.ts b/src/app/api/activity/route.ts index 0a80c88..0e5a0f9 100644 --- a/src/app/api/activity/route.ts +++ b/src/app/api/activity/route.ts @@ -8,10 +8,11 @@ function isValidProjectRoot(root: string): boolean { if (!path.isAbsolute(resolved)) { return false; } - // Prevent path traversal by ensuring resolved path doesn't escape the project - const normalized = path.normalize(resolved); - // Check that the path doesn't contain traversal patterns - if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { + // Prevent path traversal by ensuring resolved path stays within the project root + const allowedBase = process.cwd(); + const relative = path.relative(allowedBase, resolved); + // If "resolved" is outside "allowedBase", "relative" will start with ".." + if (relative.startsWith('..') || path.isAbsolute(relative)) { return false; } return true; diff --git a/src/app/api/agents/[agentId]/stats/route.ts b/src/app/api/agents/[agentId]/stats/route.ts index e090883..7db8999 100644 --- a/src/app/api/agents/[agentId]/stats/route.ts +++ b/src/app/api/agents/[agentId]/stats/route.ts @@ -10,10 +10,11 @@ function isValidProjectRoot(root: string): boolean { if (!path.isAbsolute(resolved)) { return false; } - // Prevent path traversal by ensuring resolved path doesn't escape the project - const normalized = path.normalize(resolved); - // Check that the path doesn't contain traversal patterns - if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { + // Prevent path traversal by ensuring resolved path stays within the project root + const allowedBase = process.cwd(); + const relative = path.relative(allowedBase, resolved); + // If "resolved" is outside "allowedBase", "relative" will start with ".." + if (relative.startsWith('..') || path.isAbsolute(relative)) { return false; } return true; diff --git a/src/app/api/beads/read/route.ts b/src/app/api/beads/read/route.ts index 6ab7c31..8158396 100644 --- a/src/app/api/beads/read/route.ts +++ b/src/app/api/beads/read/route.ts @@ -8,10 +8,11 @@ function isValidProjectRoot(root: string): boolean { if (!path.isAbsolute(resolved)) { return false; } - // Prevent path traversal by ensuring resolved path doesn't escape the project - const normalized = path.normalize(resolved); - // Check that the path doesn't contain traversal patterns - if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { + // Prevent path traversal by ensuring resolved path stays within the project root + const allowedBase = process.cwd(); + const relative = path.relative(allowedBase, resolved); + // If "resolved" is outside "allowedBase", "relative" will start with ".." + if (relative.startsWith('..') || path.isAbsolute(relative)) { return false; } return true; diff --git a/src/app/api/sessions/route.ts b/src/app/api/sessions/route.ts index 60514ac..280eff5 100644 --- a/src/app/api/sessions/route.ts +++ b/src/app/api/sessions/route.ts @@ -10,10 +10,11 @@ function isValidProjectRoot(root: string): boolean { if (!path.isAbsolute(resolved)) { return false; } - // Prevent path traversal by ensuring resolved path doesn't escape the project - const normalized = path.normalize(resolved); - // Check that the path doesn't contain traversal patterns - if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { + // Prevent path traversal by ensuring resolved path stays within the project root + const allowedBase = process.cwd(); + const relative = path.relative(allowedBase, resolved); + // If "resolved" is outside "allowedBase", "relative" will start with ".." + if (relative.startsWith('..') || path.isAbsolute(relative)) { return false; } return true; diff --git a/src/lib/mutations.ts b/src/lib/mutations.ts index feedc6a..e94eae1 100644 --- a/src/lib/mutations.ts +++ b/src/lib/mutations.ts @@ -76,8 +76,9 @@ function asNonEmptyString(value: unknown, field: string): string { throw new MutationValidationError(`"${field}" is required.`); } const trimmed = value.trim(); - // Sanitize to prevent command injection - remove control characters and shell metacharacters - const sanitized = trimmed.replace(/[\x00-\x1f\x7f]/g, '').replace(/[;&|`$(){}[\]\\*?<>!#"'%\n\r]/g, ''); + // Remove control characters that could cause issues in command execution + // Preserve backslashes for Windows paths and punctuation for user text + const sanitized = trimmed.replace(/[\x00-\x1f\x7f]/g, ''); if (!sanitized) { throw new MutationValidationError(`"${field}" contains only invalid characters.`); }