From ba42cbdf8770c5356294188ef28e1dd65eef3002 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sat, 28 Feb 2026 15:42:32 +0000 Subject: [PATCH] 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 - Fix FileWatcher to initialize fallback RooIgnoreController in initialize() so .rooignore rules load even when manager controller is not passed - Add tests for realpath-outside-cwd fallback, fail-closed error handling, CodebaseSearchTool rooignore filtering, and scanner controller passthrough Fixes #11797 --- src/core/ignore/RooIgnoreController.ts | 20 ++- .../RooIgnoreController.security.spec.ts | 18 +- .../__tests__/RooIgnoreController.spec.ts | 45 ++++- src/core/tools/CodebaseSearchTool.ts | 17 +- .../__tests__/codebaseSearchTool.spec.ts | 166 ++++++++++++++++++ .../processors/__tests__/scanner.spec.ts | 50 ++++++ .../code-index/processors/file-watcher.ts | 17 +- src/services/code-index/processors/scanner.ts | 15 +- src/services/code-index/service-factory.ts | 13 +- 9 files changed, 335 insertions(+), 26 deletions(-) create mode 100644 src/core/tools/__tests__/codebaseSearchTool.spec.ts diff --git a/src/core/ignore/RooIgnoreController.ts b/src/core/ignore/RooIgnoreController.ts index 45054cce96d..c1a2c0138c8 100644 --- a/src/core/ignore/RooIgnoreController.ts +++ b/src/core/ignore/RooIgnoreController.ts @@ -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 } } diff --git a/src/core/ignore/__tests__/RooIgnoreController.security.spec.ts b/src/core/ignore/__tests__/RooIgnoreController.security.spec.ts index bb4fec1f941..bafad00b088 100644 --- a/src/core/ignore/__tests__/RooIgnoreController.security.spec.ts +++ b/src/core/ignore/__tests__/RooIgnoreController.security.spec.ts @@ -182,21 +182,21 @@ describe("RooIgnoreController Security Tests", () => { const absolutePathToAllowed = path.join(TEST_CWD, "src/app.js") expect(controller.validateAccess(absolutePathToAllowed)).toBe(true) - // Absolute path outside cwd should be allowed - expect(controller.validateAccess("/etc/hosts")).toBe(true) - expect(controller.validateAccess("/var/log/system.log")).toBe(true) + // Absolute path outside cwd should be denied (fail closed) + expect(controller.validateAccess("/etc/hosts")).toBe(false) + expect(controller.validateAccess("/var/log/system.log")).toBe(false) }) /** - * Tests that paths outside cwd are allowed + * Tests that paths outside cwd are denied (fail closed for security) */ - it("should allow paths outside the current working directory", () => { - // Paths outside cwd should be allowed - expect(controller.validateAccess("../outside-project/file.txt")).toBe(true) - expect(controller.validateAccess("../../other-project/secrets/keys.json")).toBe(true) + it("should deny access to paths outside the current working directory", () => { + // Paths outside cwd should be denied + expect(controller.validateAccess("../outside-project/file.txt")).toBe(false) + expect(controller.validateAccess("../../other-project/secrets/keys.json")).toBe(false) // Edge case: path that would be ignored if inside cwd - expect(controller.validateAccess("/other/path/secrets/keys.json")).toBe(true) + expect(controller.validateAccess("/other/path/secrets/keys.json")).toBe(false) }) }) diff --git a/src/core/ignore/__tests__/RooIgnoreController.spec.ts b/src/core/ignore/__tests__/RooIgnoreController.spec.ts index 41d79476c68..f19d6591f9c 100644 --- a/src/core/ignore/__tests__/RooIgnoreController.spec.ts +++ b/src/core/ignore/__tests__/RooIgnoreController.spec.ts @@ -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", () => { diff --git a/src/core/tools/CodebaseSearchTool.ts b/src/core/tools/CodebaseSearchTool.ts index f0d906fabd8..ae0d2c78427 100644 --- a/src/core/tools/CodebaseSearchTool.ts +++ b/src/core/tools/CodebaseSearchTool.ts @@ -72,7 +72,20 @@ 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) => { + // Guard clause: skip entries without a valid payload/filePath. + // These are structural no-ops (not an ignore decision). + 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 +104,7 @@ export class CodebaseSearchTool extends BaseTool<"codebase_search"> { }> } - searchResults.forEach((result) => { + filteredResults.forEach((result) => { if (!result.payload) return if (!("filePath" in result.payload)) return diff --git a/src/core/tools/__tests__/codebaseSearchTool.spec.ts b/src/core/tools/__tests__/codebaseSearchTool.spec.ts new file mode 100644 index 00000000000..a931e5d40e8 --- /dev/null +++ b/src/core/tools/__tests__/codebaseSearchTool.spec.ts @@ -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") + }) +}) diff --git a/src/services/code-index/processors/__tests__/scanner.spec.ts b/src/services/code-index/processors/__tests__/scanner.spec.ts index a6e68bc96b6..091a2629891 100644 --- a/src/services/code-index/processors/__tests__/scanner.spec.ts +++ b/src/services/code-index/processors/__tests__/scanner.spec.ts @@ -457,5 +457,55 @@ describe("DirectoryScanner", () => { // Deleted file cleanup should not have run expect(mockVectorStore.deletePointsByFilePath).not.toHaveBeenCalled() }) + + it("should use the provided RooIgnoreController and not create a new one", async () => { + const { RooIgnoreController } = await import("../../../../core/ignore/RooIgnoreController") + const { listFiles } = await import("../../../glob/list-files") + vi.mocked(listFiles).mockResolvedValue([["test/file1.js"], false]) + + // Create a mock controller with a spy on filterPaths + const providedController = new RooIgnoreController("/test") + const filterPathsSpy = vi.spyOn(providedController, "filterPaths").mockReturnValue(["test/file1.js"]) + const initializeSpy = vi.spyOn(providedController, "initialize") + + // Create scanner WITH the provided controller + const scannerWithController = new DirectoryScanner( + mockEmbedder, + mockVectorStore, + mockCodeParser, + mockCacheManager, + mockIgnoreInstance, + undefined, + providedController, + ) + + await scannerWithController.scanDirectory("/test") + + // The provided controller's filterPaths should have been called + expect(filterPathsSpy).toHaveBeenCalled() + // The provided controller should NOT have been re-initialized + // (it was already initialized by the manager) + expect(initializeSpy).not.toHaveBeenCalled() + }) + + it("should create and initialize a new RooIgnoreController when none is provided", async () => { + const { RooIgnoreController } = await import("../../../../core/ignore/RooIgnoreController") + const { listFiles } = await import("../../../glob/list-files") + vi.mocked(listFiles).mockResolvedValue([["test/file1.js"], false]) + + // Spy on the prototype's initialize and filterPaths methods + const initSpy = vi.spyOn(RooIgnoreController.prototype, "initialize") + const filterSpy = vi.spyOn(RooIgnoreController.prototype, "filterPaths") + + // Scanner without a provided controller (default from beforeEach) + await scanner.scanDirectory("/test") + + // A new RooIgnoreController should have been created and initialized internally + expect(initSpy).toHaveBeenCalled() + expect(filterSpy).toHaveBeenCalled() + + initSpy.mockRestore() + filterSpy.mockRestore() + }) }) }) diff --git a/src/services/code-index/processors/file-watcher.ts b/src/services/code-index/processors/file-watcher.ts index a6a3122c36c..adafdd734ee 100644 --- a/src/services/code-index/processors/file-watcher.ts +++ b/src/services/code-index/processors/file-watcher.ts @@ -72,6 +72,8 @@ export class FileWatcher implements IFileWatcher { * @param vectorStore Optional vector store * @param cacheManager Cache manager */ + private ignoreControllerIsOwned = false + constructor( private workspacePath: string, private context: vscode.ExtensionContext, @@ -82,7 +84,14 @@ export class FileWatcher implements IFileWatcher { ignoreController?: RooIgnoreController, batchSegmentThreshold?: number, ) { - this.ignoreController = ignoreController || new RooIgnoreController(workspacePath) + if (ignoreController) { + this.ignoreController = ignoreController + } else { + // Fallback: create a local controller. It must be initialized in initialize() + // before it can enforce .rooignore rules. Prefer passing the manager's controller. + this.ignoreController = new RooIgnoreController(workspacePath) + this.ignoreControllerIsOwned = true + } if (ignoreInstance) { this.ignoreInstance = ignoreInstance } @@ -106,6 +115,12 @@ export class FileWatcher implements IFileWatcher { * Initializes the file watcher */ async initialize(): Promise { + // If we created a fallback RooIgnoreController in the constructor, + // initialize it now so it loads .rooignore patterns before use. + if (this.ignoreControllerIsOwned) { + await this.ignoreController.initialize() + } + // Create file watcher const filePattern = new vscode.RelativePattern( this.workspacePath, diff --git a/src/services/code-index/processors/scanner.ts b/src/services/code-index/processors/scanner.ts index 5d9ff5e362d..ab8df71914a 100644 --- a/src/services/code-index/processors/scanner.ts +++ b/src/services/code-index/processors/scanner.ts @@ -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) diff --git a/src/services/code-index/service-factory.ts b/src/services/code-index/service-factory.ts index d23eff4810b..e37812b579a 100644 --- a/src/services/code-index/service-factory.ts +++ b/src/services/code-index/service-factory.ts @@ -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,