From 2c4bd4c9f0cb2d130caa310244da603e3f9c98a7 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sat, 28 Feb 2026 15:32:16 +0000 Subject: [PATCH] fix: address PR feedback on rooignore enforcement (#11797) 1. FileWatcher: initialize fallback RooIgnoreController in initialize() so .rooignore rules load even when manager controller is not passed. Added ignoreControllerIsOwned flag to track ownership. 2. validateAccess: update security spec tests to expect denial for paths outside cwd, matching the new fail-closed behavior. 3. Scanner: add passthrough test verifying that provided controller is used (and not re-initialized) vs fallback creation. 4. CodebaseSearchTool: add clarifying comment on guard clause that drops payload-less entries (structural no-op, not ignore decision). --- .../RooIgnoreController.security.spec.ts | 18 +++---- src/core/tools/CodebaseSearchTool.ts | 2 + .../processors/__tests__/scanner.spec.ts | 50 +++++++++++++++++++ .../code-index/processors/file-watcher.ts | 17 ++++++- 4 files changed, 77 insertions(+), 10 deletions(-) diff --git a/src/core/ignore/__tests__/RooIgnoreController.security.spec.ts b/src/core/ignore/__tests__/RooIgnoreController.security.spec.ts index bb4fec1f94..bafad00b08 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/tools/CodebaseSearchTool.ts b/src/core/tools/CodebaseSearchTool.ts index 11be7bc733..ae0d2c7842 100644 --- a/src/core/tools/CodebaseSearchTool.ts +++ b/src/core/tools/CodebaseSearchTool.ts @@ -77,6 +77,8 @@ export class CodebaseSearchTool extends BaseTool<"codebase_search"> { // 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) diff --git a/src/services/code-index/processors/__tests__/scanner.spec.ts b/src/services/code-index/processors/__tests__/scanner.spec.ts index a6e68bc96b..091a262989 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 a6a3122c36..adafdd734e 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,