-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Follow symlinks in rooignore checks #7405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import path from "path" | ||
| import { fileExistsAtPath } from "../../utils/fs" | ||
| import fs from "fs/promises" | ||
| import fsSync from "fs" | ||
| import ignore, { Ignore } from "ignore" | ||
| import * as vscode from "vscode" | ||
|
|
||
|
|
@@ -81,6 +82,7 @@ export class RooIgnoreController { | |
|
|
||
| /** | ||
| * Check if a file should be accessible to the LLM | ||
| * Automatically resolves symlinks | ||
| * @param filePath - Path to check (relative to cwd) | ||
| * @returns true if file is accessible, false if ignored | ||
| */ | ||
|
|
@@ -90,15 +92,25 @@ export class RooIgnoreController { | |
| return true | ||
| } | ||
| try { | ||
| // Normalize path to be relative to cwd and use forward slashes | ||
| const absolutePath = path.resolve(this.cwd, filePath) | ||
| const relativePath = path.relative(this.cwd, absolutePath).toPosix() | ||
|
|
||
| // Ignore expects paths to be path.relative()'d | ||
| // Follow symlinks to get the real path | ||
| let realPath: string | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider extracting this symlink resolution logic into a private helper method for better readability and potential reuse. This would make the main logic cleaner and the helper method could be reused if needed elsewhere. |
||
| try { | ||
| realPath = fsSync.realpathSync(absolutePath) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I notice we are using fsSync.realpathSync() which is a synchronous operation. Since validateAccess might be called frequently during file operations, have you considered the performance impact? Would it be worth exploring an async version or perhaps caching resolved paths to minimize the blocking I/O operations? |
||
| } catch { | ||
| // If realpath fails (file doesn't exist, broken symlink, etc.), | ||
| // use the original path | ||
| realPath = absolutePath | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When realpathSync fails, we silently fall back to the original path. While this maintains backward compatibility, it could mask issues like broken symlinks or permission problems. Would it be helpful to at least log these failures for debugging purposes? |
||
| } | ||
|
|
||
| // Convert real path to relative for .rooignore checking | ||
| const relativePath = path.relative(this.cwd, realPath).toPosix() | ||
|
|
||
| // Check if the real path is ignored | ||
| return !this.ignoreInstance.ignores(relativePath) | ||
| } catch (error) { | ||
| // console.error(`Error validating access for ${filePath}:`, error) | ||
| // Ignore is designed to work with relative file paths, so will throw error for paths outside cwd. We are allowing access to all files outside cwd. | ||
| // Allow access to files outside cwd or on errors (backward compatibility) | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,12 @@ import { RooIgnoreController, LOCK_TEXT_SYMBOL } from "../RooIgnoreController" | |
| import * as vscode from "vscode" | ||
| import * as path from "path" | ||
| import * as fs from "fs/promises" | ||
| import * as fsSync from "fs" | ||
| import { fileExistsAtPath } from "../../../utils/fs" | ||
|
|
||
| // Mock dependencies | ||
| vi.mock("fs/promises") | ||
| vi.mock("fs") | ||
| vi.mock("../../../utils/fs") | ||
|
|
||
| // Mock vscode | ||
|
|
@@ -66,6 +68,10 @@ describe("RooIgnoreController", () => { | |
| mockFileExists = fileExistsAtPath as Mock<typeof fileExistsAtPath> | ||
| mockReadFile = fs.readFile as Mock<typeof fs.readFile> | ||
|
|
||
| // Setup fsSync mocks with default behavior (return path as-is, like regular files) | ||
| const mockRealpathSync = vi.mocked(fsSync.realpathSync) | ||
| mockRealpathSync.mockImplementation((filePath) => filePath.toString()) | ||
|
|
||
| // Create controller | ||
| controller = new RooIgnoreController(TEST_CWD) | ||
| }) | ||
|
|
@@ -217,6 +223,27 @@ describe("RooIgnoreController", () => { | |
| expect(emptyController.validateAccess("secrets/api-keys.json")).toBe(true) | ||
| expect(emptyController.validateAccess(".git/HEAD")).toBe(true) | ||
| }) | ||
|
|
||
| /** | ||
| * Tests symlink resolution - the simplest test for TOCTOU fix | ||
| */ | ||
| it("should block symlinks pointing to ignored files", () => { | ||
| // Mock fsSync.realpathSync to simulate symlink resolution | ||
| const mockRealpathSync = vi.mocked(fsSync.realpathSync) | ||
| mockRealpathSync.mockImplementation((filePath) => { | ||
| // Simulate "config.json" being a symlink to "node_modules/package.json" | ||
| if (filePath.toString().endsWith("config.json")) { | ||
| return path.join(TEST_CWD, "node_modules/package.json") | ||
| } | ||
| return filePath.toString() | ||
| }) | ||
|
|
||
| // Direct access to ignored file should be blocked | ||
| expect(controller.validateAccess("node_modules/package.json")).toBe(false) | ||
|
|
||
| // Symlink to ignored file should also be blocked (TOCTOU fix) | ||
| expect(controller.validateAccess("config.json")).toBe(false) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great test for the basic symlink scenario! Would it be valuable to add tests for edge cases like: broken symlinks (where realpathSync would throw), symlink chains (symlink pointing to another symlink), and circular symlinks? |
||
| }) | ||
| }) | ||
|
|
||
| describe("validateCommand", () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment mentions Automatically resolves symlinks but does not explain why this is important. Would it help future maintainers to add a brief note about the security implications (TOCTOU prevention)?