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).
This commit is contained in:
Roo Code 2026-02-28 15:32:16 +00:00
parent 91f662a7b1
commit 2c4bd4c9f0
4 changed files with 77 additions and 10 deletions

View file

@ -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)
})
})

View file

@ -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)

View file

@ -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()
})
})
})

View file

@ -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<void> {
// 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,