mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: validate file paths to prevent URL concatenation errors
- Add isUrl(), sanitizeFilePath(), and validateFilePath() utility functions - Validate paths in applyDiffTool.ts before processing - Validate paths in multiApplyDiffTool.ts before processing - Add comprehensive tests for path validation logic - Handle Windows paths with backslashes correctly - Prevent AI from accidentally concatenating URLs with file paths Fixes #9161
This commit is contained in:
parent
6e6341346e
commit
2ea619c513
4 changed files with 328 additions and 3 deletions
|
|
@ -5,7 +5,7 @@ import { TelemetryService } from "@roo-code/telemetry"
|
|||
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
|
||||
|
||||
import { ClineSayTool } from "../../shared/ExtensionMessage"
|
||||
import { getReadablePath } from "../../utils/path"
|
||||
import { getReadablePath, validateFilePath, sanitizeFilePath } from "../../utils/path"
|
||||
import { Task } from "../task/Task"
|
||||
import { ToolUse, RemoveClosingTag, AskApproval, HandleError, PushToolResult } from "../../shared/tools"
|
||||
import { formatResponse } from "../prompts/responses"
|
||||
|
|
@ -69,6 +69,26 @@ export async function applyDiffToolLegacy(
|
|||
return
|
||||
}
|
||||
|
||||
// Validate that the path doesn't contain URLs
|
||||
const pathValidation = validateFilePath(relPath)
|
||||
if (!pathValidation.isValid) {
|
||||
cline.consecutiveMistakeCount++
|
||||
cline.recordToolError("apply_diff")
|
||||
const errorMessage = pathValidation.error || `Invalid file path: ${relPath}`
|
||||
|
||||
// Try to suggest a sanitized version if possible
|
||||
const sanitized = sanitizeFilePath(relPath)
|
||||
let suggestion = ""
|
||||
if (sanitized && sanitized !== relPath) {
|
||||
suggestion = `\n\n<suggestion>Did you mean to use this path instead? ${sanitized}</suggestion>`
|
||||
}
|
||||
|
||||
const formattedError = `${errorMessage}${suggestion}\n\n<error_details>\nThe file path appears to contain a URL or invalid characters. Please provide a valid relative or absolute file path.\n</error_details>`
|
||||
await cline.say("error", formattedError)
|
||||
pushToolResult(formattedError)
|
||||
return
|
||||
}
|
||||
|
||||
const accessAllowed = cline.rooIgnoreController?.validateAccess(relPath)
|
||||
|
||||
if (!accessAllowed) {
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@ import { TelemetryService } from "@roo-code/telemetry"
|
|||
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
|
||||
|
||||
import { ClineSayTool } from "../../shared/ExtensionMessage"
|
||||
import { getReadablePath } from "../../utils/path"
|
||||
import { getReadablePath, validateFilePath, sanitizeFilePath } from "../../utils/path"
|
||||
import { Task } from "../task/Task"
|
||||
import { ToolUse, RemoveClosingTag, AskApproval, HandleError, PushToolResult } from "../../shared/tools"
|
||||
import { formatResponse } from "../prompts/responses"
|
||||
|
|
@ -120,6 +120,19 @@ export async function applyDiffTool(
|
|||
|
||||
const filePath = file.path
|
||||
|
||||
// Validate that the path doesn't contain URLs
|
||||
const pathValidation = validateFilePath(filePath)
|
||||
if (!pathValidation.isValid) {
|
||||
// Try to suggest a sanitized version if possible
|
||||
const sanitized = sanitizeFilePath(filePath)
|
||||
let errorMsg = pathValidation.error || `Invalid file path: ${filePath}`
|
||||
if (sanitized && sanitized !== filePath) {
|
||||
errorMsg += ` Did you mean to use: ${sanitized}?`
|
||||
}
|
||||
filteredOperationErrors.push(errorMsg)
|
||||
continue // Skip this file
|
||||
}
|
||||
|
||||
// Initialize the operation in the map if it doesn't exist
|
||||
if (!operationsMap[filePath]) {
|
||||
operationsMap[filePath] = {
|
||||
|
|
@ -179,6 +192,27 @@ Original error: ${errorMessage}`
|
|||
} else if (legacyPath && typeof legacyDiffContent === "string") {
|
||||
// Handle legacy parameters (old way)
|
||||
usingLegacyParams = true
|
||||
|
||||
// Validate the legacy path
|
||||
const pathValidation = validateFilePath(legacyPath)
|
||||
if (!pathValidation.isValid) {
|
||||
cline.consecutiveMistakeCount++
|
||||
cline.recordToolError("apply_diff")
|
||||
|
||||
// Try to suggest a sanitized version if possible
|
||||
const sanitized = sanitizeFilePath(legacyPath)
|
||||
let suggestion = ""
|
||||
if (sanitized && sanitized !== legacyPath) {
|
||||
suggestion = `\n\n<suggestion>Did you mean to use this path instead? ${sanitized}</suggestion>`
|
||||
}
|
||||
|
||||
const formattedError = `${pathValidation.error || `Invalid file path: ${legacyPath}`}${suggestion}\n\n<error_details>\nThe file path appears to contain a URL or invalid characters. Please provide a valid relative or absolute file path.\n</error_details>`
|
||||
await cline.say("error", formattedError)
|
||||
pushToolResult(formattedError)
|
||||
cline.processQueuedMessages()
|
||||
return
|
||||
}
|
||||
|
||||
operationsMap[legacyPath] = {
|
||||
path: legacyPath,
|
||||
diff: [
|
||||
|
|
@ -242,6 +276,16 @@ Original error: ${errorMessage}`
|
|||
for (const operation of operations) {
|
||||
const { path: relPath, diff: diffItems } = operation
|
||||
|
||||
// Additional validation before processing (in case path was modified after initial validation)
|
||||
const pathValidation = validateFilePath(relPath)
|
||||
if (!pathValidation.isValid) {
|
||||
updateOperationResult(relPath, {
|
||||
status: "blocked",
|
||||
error: pathValidation.error || `Invalid file path: ${relPath}`,
|
||||
})
|
||||
continue
|
||||
}
|
||||
|
||||
// Verify file access is allowed
|
||||
const accessAllowed = cline.rooIgnoreController?.validateAccess(relPath)
|
||||
if (!accessAllowed) {
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@
|
|||
import os from "os"
|
||||
import * as path from "path"
|
||||
|
||||
import { arePathsEqual, getReadablePath, getWorkspacePath } from "../path"
|
||||
import { arePathsEqual, getReadablePath, getWorkspacePath, isUrl, sanitizeFilePath, validateFilePath } from "../path"
|
||||
|
||||
// Mock modules
|
||||
|
||||
|
|
@ -123,6 +123,152 @@ describe("Path Utilities", () => {
|
|||
expect(arePathsEqual("C:\\", "C:\\")).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("isUrl", () => {
|
||||
it("should detect HTTP URLs", () => {
|
||||
expect(isUrl("http://example.com")).toBe(true)
|
||||
expect(isUrl("https://example.com")).toBe(true)
|
||||
expect(isUrl("https://www.google.com/search?q=package.json")).toBe(true)
|
||||
})
|
||||
|
||||
it("should detect other protocol URLs", () => {
|
||||
expect(isUrl("ftp://example.com")).toBe(true)
|
||||
expect(isUrl("file://localhost/path")).toBe(true)
|
||||
expect(isUrl("ssh://git@github.com")).toBe(true)
|
||||
expect(isUrl("ws://localhost:8080")).toBe(true)
|
||||
expect(isUrl("wss://secure.example.com")).toBe(true)
|
||||
})
|
||||
|
||||
it("should not detect regular file paths as URLs", () => {
|
||||
expect(isUrl("package.json")).toBe(false)
|
||||
expect(isUrl("/usr/local/bin")).toBe(false)
|
||||
expect(isUrl("C:\\Windows\\System32")).toBe(false)
|
||||
expect(isUrl("./src/index.ts")).toBe(false)
|
||||
expect(isUrl("../parent/file.txt")).toBe(false)
|
||||
})
|
||||
|
||||
it("should handle edge cases", () => {
|
||||
expect(isUrl("")).toBe(false)
|
||||
expect(isUrl(null as any)).toBe(false)
|
||||
expect(isUrl(undefined as any)).toBe(false)
|
||||
expect(isUrl(123 as any)).toBe(false)
|
||||
})
|
||||
|
||||
it("should be case-insensitive for protocols", () => {
|
||||
expect(isUrl("HTTP://example.com")).toBe(true)
|
||||
expect(isUrl("HTTPS://example.com")).toBe(true)
|
||||
expect(isUrl("FTP://example.com")).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("sanitizeFilePath", () => {
|
||||
it("should return null for complete URLs", () => {
|
||||
expect(sanitizeFilePath("https://www.google.com/search?q=package.json")).toBe(null)
|
||||
expect(sanitizeFilePath("http://example.com/file.txt")).toBe(null)
|
||||
expect(sanitizeFilePath("ftp://server.com/path")).toBe(null)
|
||||
})
|
||||
|
||||
it("should extract file path from concatenated URL and path", () => {
|
||||
expect(sanitizeFilePath("package.json\nhttps://google.com")).toBe("package.json")
|
||||
expect(sanitizeFilePath("src/file.ts https://example.com")).toBe("src/file.ts")
|
||||
// Note: The comma case returns with the comma because it's checking if there's a URL after it
|
||||
expect(sanitizeFilePath("./config.json,https://api.example.com")).toBe("./config.json")
|
||||
expect(sanitizeFilePath("test.txt;https://example.com")).toBe("test.txt")
|
||||
})
|
||||
|
||||
it("should handle paths with URLs concatenated without separators", () => {
|
||||
expect(sanitizeFilePath("package.jsonhttps://google.com")).toBe("package.json")
|
||||
expect(sanitizeFilePath("src/file.tshttps://example.com/api")).toBe("src/file.ts")
|
||||
expect(sanitizeFilePath("config.yamlftp://server.com")).toBe("config.yaml")
|
||||
})
|
||||
|
||||
it("should return the original path if no URL is present", () => {
|
||||
expect(sanitizeFilePath("package.json")).toBe("package.json")
|
||||
expect(sanitizeFilePath("/usr/local/bin/app")).toBe("/usr/local/bin/app")
|
||||
expect(sanitizeFilePath("C:\\Windows\\System32\\cmd.exe")).toBe("C:\\Windows\\System32\\cmd.exe")
|
||||
})
|
||||
|
||||
it("should trim whitespace", () => {
|
||||
expect(sanitizeFilePath(" package.json ")).toBe("package.json")
|
||||
expect(sanitizeFilePath("\t./src/index.ts\n")).toBe("./src/index.ts")
|
||||
})
|
||||
|
||||
it("should handle edge cases", () => {
|
||||
expect(sanitizeFilePath("")).toBe(null)
|
||||
expect(sanitizeFilePath(null as any)).toBe(null)
|
||||
expect(sanitizeFilePath(undefined as any)).toBe(null)
|
||||
expect(sanitizeFilePath(123 as any)).toBe(null)
|
||||
})
|
||||
|
||||
it("should handle Windows paths with URLs", () => {
|
||||
// The trailing backslash is removed as it's considered a separator before the URL
|
||||
expect(
|
||||
sanitizeFilePath(
|
||||
"d:\\01_Proyectos\\SUPER-ADMIN\\package.json\\https:\\www.google.com\\search?q=package.json",
|
||||
),
|
||||
).toBe("d:\\01_Proyectos\\SUPER-ADMIN\\package.json")
|
||||
expect(sanitizeFilePath("C:\\Users\\test\\file.txt https://example.com")).toBe(
|
||||
"C:\\Users\\test\\file.txt",
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
describe("validateFilePath", () => {
|
||||
it("should validate normal file paths", () => {
|
||||
expect(validateFilePath("package.json")).toEqual({ isValid: true })
|
||||
expect(validateFilePath("./src/index.ts")).toEqual({ isValid: true })
|
||||
expect(validateFilePath("/usr/local/bin/app")).toEqual({ isValid: true })
|
||||
expect(validateFilePath("C:\\Windows\\System32\\cmd.exe")).toEqual({ isValid: true })
|
||||
})
|
||||
|
||||
it("should reject complete URLs", () => {
|
||||
const result = validateFilePath("https://www.google.com/search?q=package.json")
|
||||
expect(result.isValid).toBe(false)
|
||||
expect(result.error).toContain("appears to be a URL")
|
||||
})
|
||||
|
||||
it("should reject paths with URL components and suggest sanitized version", () => {
|
||||
const result = validateFilePath("package.json https://google.com")
|
||||
expect(result.isValid).toBe(false)
|
||||
expect(result.error).toContain("contains URL components")
|
||||
expect(result.error).toContain("package.json")
|
||||
})
|
||||
|
||||
it("should reject paths with concatenated URLs", () => {
|
||||
const result = validateFilePath("package.jsonhttps://google.com")
|
||||
expect(result.isValid).toBe(false)
|
||||
expect(result.error).toContain("contains URL components")
|
||||
expect(result.error).toContain("package.json")
|
||||
})
|
||||
|
||||
it("should handle empty or invalid inputs", () => {
|
||||
expect(validateFilePath("").isValid).toBe(false)
|
||||
expect(validateFilePath("").error).toContain("empty or invalid")
|
||||
|
||||
expect(validateFilePath(null as any).isValid).toBe(false)
|
||||
expect(validateFilePath(undefined as any).isValid).toBe(false)
|
||||
expect(validateFilePath(123 as any).isValid).toBe(false)
|
||||
})
|
||||
|
||||
it("should handle the exact issue case from bug report", () => {
|
||||
const bugPath =
|
||||
"d:\\01_Proyectos\\...\\SUPER-ADMIN\\package.json\\https:\\www.google.com\\search?q=package.json"
|
||||
const result = validateFilePath(bugPath)
|
||||
expect(result.isValid).toBe(false)
|
||||
expect(result.error).toContain("contains URL components")
|
||||
})
|
||||
|
||||
it("should accept paths with spaces", () => {
|
||||
expect(validateFilePath("my folder/my file.txt")).toEqual({ isValid: true })
|
||||
expect(validateFilePath("C:\\Program Files\\app.exe")).toEqual({ isValid: true })
|
||||
})
|
||||
|
||||
it("should accept paths with special characters", () => {
|
||||
expect(validateFilePath("file-name_123.test.ts")).toEqual({ isValid: true })
|
||||
expect(validateFilePath("@types/node/index.d.ts")).toEqual({ isValid: true })
|
||||
expect(validateFilePath("file[1].txt")).toEqual({ isValid: true })
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe("getReadablePath", () => {
|
||||
|
|
|
|||
|
|
@ -130,3 +130,118 @@ export const getWorkspacePathForContext = (contextPath?: string): string => {
|
|||
// Fall back to current behavior
|
||||
return getWorkspacePath()
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks if a string appears to be a URL rather than a file path
|
||||
* @param path The string to check
|
||||
* @returns true if the string looks like a URL
|
||||
*/
|
||||
export function isUrl(path: string): boolean {
|
||||
if (!path || typeof path !== "string") {
|
||||
return false
|
||||
}
|
||||
|
||||
// Check for common URL protocols
|
||||
const urlProtocols = /^(https?|ftp|file|data|mailto|tel|ssh|git|ws|wss):\/\//i
|
||||
return urlProtocols.test(path)
|
||||
}
|
||||
|
||||
/**
|
||||
* Sanitizes a file path by removing any URL components if present
|
||||
* This helps prevent the AI from accidentally including URLs in file paths
|
||||
* @param filePath The file path to sanitize
|
||||
* @returns The sanitized file path, or null if the entire path is a URL
|
||||
*/
|
||||
export function sanitizeFilePath(filePath: string): string | null {
|
||||
if (!filePath || typeof filePath !== "string") {
|
||||
return null
|
||||
}
|
||||
|
||||
// If the entire path is a URL, return null
|
||||
if (isUrl(filePath)) {
|
||||
return null
|
||||
}
|
||||
|
||||
// First check for paths that have URLs concatenated without separators
|
||||
// This handles cases like "package.jsonhttps://google.com" or Windows paths with backslashes
|
||||
// Also handle the specific bug case: "d:\01_Proyectos\...\package.json\https:\www.google.com\..."
|
||||
// Match URLs with either :// or :\ (for Windows paths like https:\www.google.com)
|
||||
const urlMatch = filePath.match(/(.*?)(https?|ftp|file|data|mailto|tel|ssh|git|ws|wss):[\\\/]/i)
|
||||
if (urlMatch && urlMatch[1]) {
|
||||
// Return the part before the URL, trimming any trailing separators
|
||||
let cleanPath = urlMatch[1]
|
||||
// Remove trailing backslash, forward slash, comma, or semicolon if it's there
|
||||
cleanPath = cleanPath.replace(/[\\\/,;]$/, "")
|
||||
return cleanPath.trim() || null
|
||||
}
|
||||
|
||||
// Check if path contains a URL separated by whitespace or special characters
|
||||
// But be careful not to split on spaces that are part of valid file paths
|
||||
// Only split if we find actual URL protocols after the separator
|
||||
const separators = ["\n", "\r", "\t", "|", ",", ";"]
|
||||
for (const separator of separators) {
|
||||
const separatorIndex = filePath.indexOf(separator)
|
||||
if (separatorIndex !== -1) {
|
||||
// Check if what comes after the separator is a URL
|
||||
const afterSeparator = filePath.substring(separatorIndex + 1).trim()
|
||||
if (isUrl(afterSeparator)) {
|
||||
// Return everything before the separator
|
||||
const result = filePath.substring(0, separatorIndex).trim()
|
||||
return result || null
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Special handling for space separator - only split if there's a URL after the space
|
||||
if (filePath.includes(" ")) {
|
||||
// Check if any word after a space starts with a URL protocol
|
||||
const words = filePath.split(" ")
|
||||
for (let i = 1; i < words.length; i++) {
|
||||
if (isUrl(words[i])) {
|
||||
// Found a URL after a space, return everything before this point
|
||||
return words.slice(0, i).join(" ").trim()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// If no URL components found, return the original path
|
||||
return filePath.trim()
|
||||
}
|
||||
|
||||
/**
|
||||
* Validates that a file path is safe to use and doesn't contain URLs
|
||||
* @param filePath The file path to validate
|
||||
* @returns An object with isValid boolean and an optional error message
|
||||
*/
|
||||
export function validateFilePath(filePath: string): { isValid: boolean; error?: string } {
|
||||
if (!filePath || typeof filePath !== "string") {
|
||||
return { isValid: false, error: "File path is empty or invalid" }
|
||||
}
|
||||
|
||||
// Check if the entire path is a URL
|
||||
if (isUrl(filePath)) {
|
||||
return {
|
||||
isValid: false,
|
||||
error: `Invalid file path: "${filePath}" appears to be a URL. Please provide a valid file path instead.`,
|
||||
}
|
||||
}
|
||||
|
||||
// Try to sanitize the path
|
||||
const sanitized = sanitizeFilePath(filePath)
|
||||
if (!sanitized) {
|
||||
return {
|
||||
isValid: false,
|
||||
error: `Invalid file path: "${filePath}" could not be sanitized to a valid path.`,
|
||||
}
|
||||
}
|
||||
|
||||
// If sanitization changed the path, it contained URL components
|
||||
if (sanitized !== filePath.trim()) {
|
||||
return {
|
||||
isValid: false,
|
||||
error: `Invalid file path: "${filePath}" contains URL components. Did you mean "${sanitized}"?`,
|
||||
}
|
||||
}
|
||||
|
||||
return { isValid: true }
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue