fix: enforce .rooignore rules for codebase indexing, file reads, and file listing

roomote · roomote · commit ba42cbdf8770 · 2026-02-28T15:42:32.000Z
- 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
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/src/services/code-index/processors/scanner.ts b/src/services/code-index/processors/scanner.ts
diff --git a/src/services/code-index/service-factory.ts b/src/services/code-index/service-factory.ts
