mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix: enforce .rooignore rules for codebase indexing, file reads, and file listing
- Harden validateAccess() to fall back to original path when realpath resolves outside cwd (fixes submodule/symlink bypass) - Change error handling in validateAccess() to fail closed (deny access) instead of fail open - Add .rooignore post-filtering in CodebaseSearchTool to exclude ignored files from search results even if they were previously indexed - Pass RooIgnoreController from manager through service-factory to scanner so the scanner reuses the workspace-root controller instead of creating its own from the scan directory - Add tests for realpath-outside-cwd fallback, fail-closed error handling, and CodebaseSearchTool rooignore filtering Fixes #11797
This commit is contained in:
parent
193053110b
commit
91f662a7b1
6 changed files with 258 additions and 16 deletions
|
|
@ -104,14 +104,26 @@ export class RooIgnoreController {
|
|||
realPath = absolutePath
|
||||
}
|
||||
|
||||
// Convert real path to relative for .rooignore checking
|
||||
const relativePath = path.relative(this.cwd, realPath).toPosix()
|
||||
// If realpath resolved outside cwd (e.g. symlinks, submodules),
|
||||
// fall back to the original absolute path for relative computation.
|
||||
// This prevents path.relative from producing "../" paths that the
|
||||
// ignore library cannot match against .rooignore patterns.
|
||||
const relativeFromReal = path.relative(this.cwd, realPath)
|
||||
const effectivePath = relativeFromReal.startsWith("..") ? absolutePath : realPath
|
||||
|
||||
// Convert to relative for .rooignore checking
|
||||
const relativePath = path.relative(this.cwd, effectivePath).toPosix()
|
||||
|
||||
// If the path is still outside cwd after fallback, deny access (fail closed)
|
||||
if (relativePath.startsWith("..")) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Check if the real path is ignored
|
||||
return !this.ignoreInstance.ignores(relativePath)
|
||||
} catch (error) {
|
||||
// Allow access to files outside cwd or on errors (backward compatibility)
|
||||
return true
|
||||
// Fail closed: deny access on unexpected errors for security
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -199,14 +199,14 @@ describe("RooIgnoreController", () => {
|
|||
})
|
||||
|
||||
/**
|
||||
* Tests handling of paths outside cwd
|
||||
* Tests handling of paths outside cwd - should deny access (fail closed)
|
||||
*/
|
||||
it("should allow access to paths outside cwd", () => {
|
||||
it("should deny access to paths outside cwd", () => {
|
||||
// Path traversal outside cwd
|
||||
expect(controller.validateAccess("../outside-project/file.txt")).toBe(true)
|
||||
expect(controller.validateAccess("../outside-project/file.txt")).toBe(false)
|
||||
|
||||
// Completely different path
|
||||
expect(controller.validateAccess("/etc/hosts")).toBe(true)
|
||||
expect(controller.validateAccess("/etc/hosts")).toBe(false)
|
||||
})
|
||||
|
||||
/**
|
||||
|
|
@ -244,6 +244,43 @@ describe("RooIgnoreController", () => {
|
|||
// Symlink to ignored file should also be blocked
|
||||
expect(controller.validateAccess("config.json")).toBe(false)
|
||||
})
|
||||
|
||||
/**
|
||||
* Tests that when realpathSync resolves outside cwd (e.g. submodules),
|
||||
* the original path is used for ignore checking instead.
|
||||
*/
|
||||
it("should fall back to original path when realpath resolves outside cwd", () => {
|
||||
const mockRealpathSync = vi.mocked(fsSync.realpathSync)
|
||||
mockRealpathSync.mockImplementation((filePath) => {
|
||||
// Simulate a submodule file resolving to a path outside the workspace
|
||||
if (filePath.toString().includes("node_modules/submod/file.js")) {
|
||||
return "/some/other/location/file.js"
|
||||
}
|
||||
return filePath.toString()
|
||||
})
|
||||
|
||||
// Even though realpath resolves outside cwd, the original relative path
|
||||
// should be used for .rooignore checking, and node_modules is ignored
|
||||
expect(controller.validateAccess("node_modules/submod/file.js")).toBe(false)
|
||||
})
|
||||
|
||||
/**
|
||||
* Tests that unexpected errors in validateAccess fail closed (deny access)
|
||||
*/
|
||||
it("should deny access on unexpected errors (fail closed)", () => {
|
||||
// Force the ignoreInstance.ignores method to throw an error
|
||||
// by corrupting the internal ignore instance
|
||||
const originalIgnores = controller["ignoreInstance"].ignores
|
||||
controller["ignoreInstance"].ignores = () => {
|
||||
throw new Error("Unexpected ignore error")
|
||||
}
|
||||
|
||||
// Should deny access on error (fail closed)
|
||||
expect(controller.validateAccess("src/app.ts")).toBe(false)
|
||||
|
||||
// Restore
|
||||
controller["ignoreInstance"].ignores = originalIgnores
|
||||
})
|
||||
})
|
||||
|
||||
describe("validateCommand", () => {
|
||||
|
|
|
|||
|
|
@ -72,7 +72,18 @@ export class CodebaseSearchTool extends BaseTool<"codebase_search"> {
|
|||
|
||||
const searchResults: VectorStoreSearchResult[] = await manager.searchIndex(query, directoryPrefix)
|
||||
|
||||
if (!searchResults || searchResults.length === 0) {
|
||||
// Post-filter results through .rooignore to exclude files that were
|
||||
// indexed before being added to .rooignore, or that bypassed filtering
|
||||
// during indexing (e.g. due to symlink/submodule path resolution).
|
||||
const filteredResults = task.rooIgnoreController
|
||||
? searchResults.filter((result) => {
|
||||
if (!result.payload || !("filePath" in result.payload)) return false
|
||||
const relativePath = vscode.workspace.asRelativePath(result.payload.filePath, false)
|
||||
return task.rooIgnoreController!.validateAccess(relativePath)
|
||||
})
|
||||
: searchResults
|
||||
|
||||
if (!filteredResults || filteredResults.length === 0) {
|
||||
pushToolResult(`No relevant code snippets found for the query: "${query}"`)
|
||||
return
|
||||
}
|
||||
|
|
@ -91,7 +102,7 @@ export class CodebaseSearchTool extends BaseTool<"codebase_search"> {
|
|||
}>
|
||||
}
|
||||
|
||||
searchResults.forEach((result) => {
|
||||
filteredResults.forEach((result) => {
|
||||
if (!result.payload) return
|
||||
if (!("filePath" in result.payload)) return
|
||||
|
||||
|
|
|
|||
166
src/core/tools/__tests__/codebaseSearchTool.spec.ts
Normal file
166
src/core/tools/__tests__/codebaseSearchTool.spec.ts
Normal file
|
|
@ -0,0 +1,166 @@
|
|||
// npx vitest core/tools/__tests__/codebaseSearchTool.spec.ts
|
||||
|
||||
import { codebaseSearchTool } from "../CodebaseSearchTool"
|
||||
import { CodeIndexManager } from "../../../services/code-index/manager"
|
||||
import { formatResponse } from "../../prompts/responses"
|
||||
|
||||
vi.mock("vscode", () => ({
|
||||
workspace: {
|
||||
asRelativePath: vi.fn((filePath: string) => {
|
||||
// Simple mock: return the path as-is for testing
|
||||
return filePath
|
||||
}),
|
||||
},
|
||||
}))
|
||||
|
||||
vi.mock("../../../services/code-index/manager", () => ({
|
||||
CodeIndexManager: {
|
||||
getInstance: vi.fn(),
|
||||
},
|
||||
}))
|
||||
|
||||
vi.mock("../../prompts/responses", () => ({
|
||||
formatResponse: {
|
||||
toolDenied: vi.fn(() => "Tool denied"),
|
||||
toolError: vi.fn((msg: string) => `Error: ${msg}`),
|
||||
},
|
||||
}))
|
||||
|
||||
vi.mock("../../../utils/path", () => ({
|
||||
getWorkspacePath: vi.fn(() => "/test/workspace"),
|
||||
}))
|
||||
|
||||
describe("CodebaseSearchTool", () => {
|
||||
let mockTask: any
|
||||
let mockCallbacks: any
|
||||
let mockManager: any
|
||||
let pushToolResultValue: string | undefined
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
pushToolResultValue = undefined
|
||||
|
||||
mockManager = {
|
||||
isFeatureEnabled: true,
|
||||
isFeatureConfigured: true,
|
||||
searchIndex: vi.fn(),
|
||||
}
|
||||
;(CodeIndexManager.getInstance as any).mockReturnValue(mockManager)
|
||||
|
||||
mockTask = {
|
||||
cwd: "/test/workspace",
|
||||
consecutiveMistakeCount: 0,
|
||||
didToolFailInCurrentTurn: false,
|
||||
sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing param error"),
|
||||
say: vi.fn().mockResolvedValue(undefined),
|
||||
providerRef: {
|
||||
deref: vi.fn().mockReturnValue({
|
||||
context: { extensionPath: "/test" },
|
||||
}),
|
||||
},
|
||||
rooIgnoreController: {
|
||||
validateAccess: vi.fn().mockReturnValue(true),
|
||||
},
|
||||
}
|
||||
|
||||
mockCallbacks = {
|
||||
askApproval: vi.fn().mockResolvedValue(true),
|
||||
handleError: vi.fn(),
|
||||
pushToolResult: vi.fn((result: string) => {
|
||||
pushToolResultValue = result
|
||||
}),
|
||||
}
|
||||
})
|
||||
|
||||
it("should filter out rooignored files from search results", async () => {
|
||||
// Set up search results with some files that should be ignored
|
||||
mockManager.searchIndex.mockResolvedValue([
|
||||
{
|
||||
score: 0.95,
|
||||
payload: {
|
||||
filePath: "src/app.ts",
|
||||
startLine: 1,
|
||||
endLine: 10,
|
||||
codeChunk: "const app = express()",
|
||||
},
|
||||
},
|
||||
{
|
||||
score: 0.9,
|
||||
payload: {
|
||||
filePath: "vendor/some-lib/crypto.c",
|
||||
startLine: 1,
|
||||
endLine: 20,
|
||||
codeChunk: "void crypto_init() {}",
|
||||
},
|
||||
},
|
||||
{
|
||||
score: 0.85,
|
||||
payload: {
|
||||
filePath: "src/utils.ts",
|
||||
startLine: 5,
|
||||
endLine: 15,
|
||||
codeChunk: "export function helper() {}",
|
||||
},
|
||||
},
|
||||
])
|
||||
|
||||
// Mock rooIgnoreController to block vendor/ files
|
||||
mockTask.rooIgnoreController.validateAccess.mockImplementation((filePath: string) => {
|
||||
return !filePath.includes("vendor/")
|
||||
})
|
||||
|
||||
await codebaseSearchTool.execute({ query: "crypto" }, mockTask, mockCallbacks)
|
||||
|
||||
// Should have called pushToolResult with results that don't include vendor/ files
|
||||
expect(mockCallbacks.pushToolResult).toHaveBeenCalled()
|
||||
const result = pushToolResultValue!
|
||||
expect(result).toContain("src/app.ts")
|
||||
expect(result).toContain("src/utils.ts")
|
||||
expect(result).not.toContain("vendor/some-lib/crypto.c")
|
||||
})
|
||||
|
||||
it("should return no results message when all results are filtered by rooignore", async () => {
|
||||
mockManager.searchIndex.mockResolvedValue([
|
||||
{
|
||||
score: 0.9,
|
||||
payload: {
|
||||
filePath: "vendor/some-lib/crypto.c",
|
||||
startLine: 1,
|
||||
endLine: 20,
|
||||
codeChunk: "void crypto_init() {}",
|
||||
},
|
||||
},
|
||||
])
|
||||
|
||||
// Mock rooIgnoreController to block all results
|
||||
mockTask.rooIgnoreController.validateAccess.mockReturnValue(false)
|
||||
|
||||
await codebaseSearchTool.execute({ query: "crypto" }, mockTask, mockCallbacks)
|
||||
|
||||
expect(mockCallbacks.pushToolResult).toHaveBeenCalledWith(
|
||||
'No relevant code snippets found for the query: "crypto"',
|
||||
)
|
||||
})
|
||||
|
||||
it("should pass all results through when no rooIgnoreController is set", async () => {
|
||||
mockTask.rooIgnoreController = undefined
|
||||
|
||||
mockManager.searchIndex.mockResolvedValue([
|
||||
{
|
||||
score: 0.95,
|
||||
payload: {
|
||||
filePath: "vendor/some-lib/crypto.c",
|
||||
startLine: 1,
|
||||
endLine: 20,
|
||||
codeChunk: "void crypto_init() {}",
|
||||
},
|
||||
},
|
||||
])
|
||||
|
||||
await codebaseSearchTool.execute({ query: "crypto" }, mockTask, mockCallbacks)
|
||||
|
||||
expect(mockCallbacks.pushToolResult).toHaveBeenCalled()
|
||||
const result = pushToolResultValue!
|
||||
expect(result).toContain("vendor/some-lib/crypto.c")
|
||||
})
|
||||
})
|
||||
|
|
@ -41,6 +41,7 @@ export class DirectoryScanner implements IDirectoryScanner {
|
|||
private readonly cacheManager: CacheManager,
|
||||
private readonly ignoreInstance: Ignore,
|
||||
batchSegmentThreshold?: number,
|
||||
private readonly rooIgnoreController?: RooIgnoreController,
|
||||
) {
|
||||
// Get the configurable batch size from VSCode settings, fallback to default
|
||||
// If not provided in constructor, try to get from VSCode settings
|
||||
|
|
@ -83,10 +84,16 @@ export class DirectoryScanner implements IDirectoryScanner {
|
|||
// Filter out directories (marked with trailing '/')
|
||||
const filePaths = allPaths.filter((p) => !p.endsWith("/"))
|
||||
|
||||
// Initialize RooIgnoreController if not provided
|
||||
const ignoreController = new RooIgnoreController(directoryPath)
|
||||
|
||||
await ignoreController.initialize()
|
||||
// Use the provided RooIgnoreController if available, otherwise create a new one.
|
||||
// Reusing the manager's controller ensures .rooignore is loaded from the
|
||||
// workspace root, not from the scan directory (which may differ for submodules).
|
||||
let ignoreController: RooIgnoreController
|
||||
if (this.rooIgnoreController) {
|
||||
ignoreController = this.rooIgnoreController
|
||||
} else {
|
||||
ignoreController = new RooIgnoreController(directoryPath)
|
||||
await ignoreController.initialize()
|
||||
}
|
||||
|
||||
// Filter paths using .rooignore
|
||||
const allowedPaths = ignoreController.filterPaths(filePaths)
|
||||
|
|
|
|||
|
|
@ -181,6 +181,7 @@ export class CodeIndexServiceFactory {
|
|||
vectorStore: IVectorStore,
|
||||
parser: ICodeParser,
|
||||
ignoreInstance: Ignore,
|
||||
rooIgnoreController?: RooIgnoreController,
|
||||
): DirectoryScanner {
|
||||
// Get the configurable batch size from VSCode settings
|
||||
let batchSize: number
|
||||
|
|
@ -192,7 +193,15 @@ export class CodeIndexServiceFactory {
|
|||
// In test environment, vscode.workspace might not be available
|
||||
batchSize = BATCH_SEGMENT_THRESHOLD
|
||||
}
|
||||
return new DirectoryScanner(embedder, vectorStore, parser, this.cacheManager, ignoreInstance, batchSize)
|
||||
return new DirectoryScanner(
|
||||
embedder,
|
||||
vectorStore,
|
||||
parser,
|
||||
this.cacheManager,
|
||||
ignoreInstance,
|
||||
batchSize,
|
||||
rooIgnoreController,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -251,7 +260,7 @@ export class CodeIndexServiceFactory {
|
|||
const embedder = this.createEmbedder()
|
||||
const vectorStore = this.createVectorStore()
|
||||
const parser = codeParser
|
||||
const scanner = this.createDirectoryScanner(embedder, vectorStore, parser, ignoreInstance)
|
||||
const scanner = this.createDirectoryScanner(embedder, vectorStore, parser, ignoreInstance, rooIgnoreController)
|
||||
const fileWatcher = this.createFileWatcher(
|
||||
context,
|
||||
embedder,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue