Harden main-process traversal and add stress benchmarks#57
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideHardens the main-process directory traversal logic against symlink escapes and recursion loops, and introduces a Jest-based stress benchmark suite to measure IPC latency and event-loop lag for the fs:getDirectoryTree handler, along with associated test/mocking updates and documentation alignment. Sequence diagram for hardened fs_getDirectoryTree traversalsequenceDiagram
actor Renderer
participant MainProcessHandler_fs_getDirectoryTree as MainProcessHandler_fs_getDirectoryTree
participant FileSystem as FileSystem
Renderer->>MainProcessHandler_fs_getDirectoryTree: IPC invoke fs_getDirectoryTree(authorizedDirPath)
activate MainProcessHandler_fs_getDirectoryTree
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: init visitedDirectoryRealPaths
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: walkDirectory(dir)
loop For each directory
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: resolveRealPath(dir)
MainProcessHandler_fs_getDirectoryTree->>FileSystem: realpathSync or realpathSync_native
FileSystem-->>MainProcessHandler_fs_getDirectoryTree: realDirectoryPath
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: check visitedDirectoryRealPaths
alt Directory already visited
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: skip to avoid recursion loop
else New directory
MainProcessHandler_fs_getDirectoryTree->>FileSystem: readdirSync(dir)
FileSystem-->>MainProcessHandler_fs_getDirectoryTree: items
loop For each item
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: readPathStats(itemPath)
MainProcessHandler_fs_getDirectoryTree->>FileSystem: lstatSync(itemPath)
FileSystem-->>MainProcessHandler_fs_getDirectoryTree: lstatResult
alt Item is symbolic link
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: resolveRealPath(itemPath)
MainProcessHandler_fs_getDirectoryTree->>FileSystem: realpathSync
FileSystem-->>MainProcessHandler_fs_getDirectoryTree: resolvedSymlinkPath
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: isPathWithinRoot(authorizedDirPath, resolvedSymlinkPath)
alt Symlink escapes root
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: log skip symlink outside root
else Symlink within root
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: still skip to avoid aliasing and loops
end
else Item is not symbolic link
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: isPathWithinRoot(authorizedDirPath, itemPath)
alt Item outside root
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: log skip path outside root
else Item within root
alt stats.isDirectory
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: walkDirectory(subdirectory)
else stats.isFile
MainProcessHandler_fs_getDirectoryTree->>MainProcessHandler_fs_getDirectoryTree: apply filters and add to result
end
end
end
end
end
end
MainProcessHandler_fs_getDirectoryTree-->>Renderer: DirectoryTreeItem[]
deactivate MainProcessHandler_fs_getDirectoryTree
Class diagram for hardened traversal helpers and path boundary checksclassDiagram
class MainProcessHandler_fs_getDirectoryTree {
+authorizedRootPath string
+walkDirectory(dir string) DirectoryTreeItem[]
+shouldExclude(itemPath string, authorizedDirPath string, excludePatterns string[], config any) boolean
}
class DirectoryTreeItem {
+path string
+name string
+isDirectory boolean
+extension string
}
class PathSecurityUtils {
+resolveRealPath(inputPath string) string
+readPathStats(itemPath string) PathStatsResult
+isPathWithinRoot(rootPath string, candidatePath string) boolean
}
class PathStatsResult {
+stats Stats
+isSymbolicLink boolean
}
class Stats {
+isDirectory() boolean
+isSymbolicLink() boolean
}
class VisitedDirectoryTracker {
+visitedDirectoryRealPaths Set~string~
+hasVisited(realDirectoryPath string) boolean
+markVisited(realDirectoryPath string) void
}
MainProcessHandler_fs_getDirectoryTree --> DirectoryTreeItem : builds
MainProcessHandler_fs_getDirectoryTree --> PathSecurityUtils : uses
PathSecurityUtils --> PathStatsResult : returns
PathStatsResult --> Stats : wraps
MainProcessHandler_fs_getDirectoryTree --> VisitedDirectoryTracker : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoHarden directory traversal and add IPC stress benchmarks
WalkthroughsDescription• Harden directory traversal with real path resolution and symlink detection • Skip symlinks and prevent recursion loops via visited directory tracking • Add stress benchmarks for IPC latency and event-loop lag sampling • Refactor path validation into reusable helper functions • Update test catalog and add npm script for stress test suite Diagramflowchart LR
A["Directory Traversal"] --> B["resolveRealPath"]
A --> C["readPathStats"]
B --> D["Symlink Detection"]
C --> D
D --> E["Skip Symlinks"]
D --> F["Prevent Loops"]
E --> G["Safe Tree Walk"]
F --> G
H["Stress Tests"] --> I["IPC Latency"]
H --> J["Event-Loop Lag"]
I --> K["Benchmark Artifacts"]
J --> K
File Changes1. src/main/index.ts
|
Summary of ChangesHello @Mehdi-Bl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and performance measurement capabilities of the application. It addresses potential vulnerabilities and inefficiencies in file system traversal by implementing safer path resolution and preventing infinite loops caused by symbolic links. Concurrently, it establishes a critical benchmarking framework to gather performance metrics for inter-process communication, enabling data-driven optimization decisions for future development. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds symlink detection, real-path resolution, and recursion-guarding to main-process directory traversal; introduces IPC stress-test scaffolding and a new npm script to run stress tests; updates unit/integration test catalog and adds tests covering symlink edge cases and traversal loops. Changes
Sequence DiagramsequenceDiagram
participant App as Caller
participant Traverse as DirectoryTraversal
participant FS as Filesystem
participant Resolve as PathResolver
participant Guard as RecursionGuard
App->>Traverse: request directory tree for path
Traverse->>Guard: is path's realpath visited?
alt not visited
Traverse->>FS: readPathStats(path)
FS-->>Traverse: stats + isSymbolicLink
alt isSymbolicLink
Traverse->>Resolve: resolveRealPath(path)
Resolve->>FS: realpath lookup
FS-->>Resolve: real path (or fallback)
Resolve-->>Traverse: resolved real path
Traverse->>Guard: is resolved path within root?
alt within root
Traverse->>Guard: mark resolved realpath visited
Traverse->>FS: read children / recurse
else outside root
Traverse->>App: log warning, skip path
end
else regular file/dir
Traverse->>Guard: mark realpath visited
Traverse->>FS: read children / recurse
end
else visited
Traverse->>App: log warning, skip to avoid loop
end
FS-->>Traverse: children entries
Traverse-->>App: return assembled tree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
readPathStatshelper assumesfs.lstatSyncwill always succeed when present; consider wrapping thelstatSynccall in a try/catch and falling back tofs.statSyncon errors (e.g., ENOENT or unusual mocks) to keep behavior resilient and consistent withresolveRealPath’s defensive style. - The test setups for
fs.readdirSync/statSync/lstatSync/realpathSyncinhandlers.test.tsand the new stress suite duplicate a fair amount of mocking boilerplate; extracting a shared helper or factory for the mocked filesystem behavior would reduce drift and make future traversal changes easier to test consistently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `readPathStats` helper assumes `fs.lstatSync` will always succeed when present; consider wrapping the `lstatSync` call in a try/catch and falling back to `fs.statSync` on errors (e.g., ENOENT or unusual mocks) to keep behavior resilient and consistent with `resolveRealPath`’s defensive style.
- The test setups for `fs.readdirSync`/`statSync`/`lstatSync`/`realpathSync` in `handlers.test.ts` and the new stress suite duplicate a fair amount of mocking boilerplate; extracting a shared helper or factory for the mocked filesystem behavior would reduce drift and make future traversal changes easier to test consistently.
## Individual Comments
### Comment 1
<location> `tests/integration/main-process/handlers.test.ts:414-423` </location>
<code_context>
+ test('should avoid directory traversal loops caused by symlink cycles', async () => {
</code_context>
<issue_to_address>
**issue (testing):** The traversal-loop detection test may be flaky due to a hard-coded 200ms timeout in Promise.race.
A 200ms `Promise.race` timeout makes this test timing-sensitive and likely flaky on slower or loaded CI runners, even when the implementation is correct. Either increase the timeout substantially (e.g. 2–5s) or, preferably, replace the time-based check with a structural one—such as mocking `fs.readdirSync` / `resolveRealPath` and enforcing a max call count, failing if that limit is exceeded. That way the test validates loop prevention without depending on wall-clock timing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+414
to
+423
| test('should avoid directory traversal loops caused by symlink cycles', async () => { | ||
| fs.readdirSync.mockImplementation((dir) => { | ||
| if (dir === '/mock/repo') { | ||
| return ['src', 'loop-link']; | ||
| } | ||
| if (dir === '/mock/repo/src') { | ||
| return ['index.js']; | ||
| } | ||
| return []; | ||
| }); |
There was a problem hiding this comment.
issue (testing): The traversal-loop detection test may be flaky due to a hard-coded 200ms timeout in Promise.race.
A 200ms Promise.race timeout makes this test timing-sensitive and likely flaky on slower or loaded CI runners, even when the implementation is correct. Either increase the timeout substantially (e.g. 2–5s) or, preferably, replace the time-based check with a structural one—such as mocking fs.readdirSync / resolveRealPath and enforcing a max call count, failing if that limit is exceeded. That way the test validates loop prevention without depending on wall-clock timing.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly hardens the file system traversal logic in fs:getDirectoryTree by adding protection against symlink-based attacks and introduces resolveRealPath and readPathStats helpers. However, a medium-severity path traversal vulnerability was identified in the fallback logic of the readPathStats function. If lstatSync is unavailable, the code falls back to the insecure statSync function, which follows symlinks and could bypass the introduced protections. The PR also includes new integration tests and a stress benchmark suite, which are positive additions for security and performance observability.
Comment on lines
+250
to
+252
| stats: fs.statSync(itemPath), | ||
| isSymbolicLink: false, | ||
| }; |
There was a problem hiding this comment.
The readPathStats function has a medium-severity path traversal vulnerability due to an insecure fallback to fs.statSync if fs.lstatSync is not available. fs.statSync follows symbolic links, which would bypass the symbolic link protection mechanism this PR introduces, potentially allowing an attacker to access files outside the authorized root path. It is safer to explicitly fail if lstatSync is unavailable rather than falling back to an insecure method.
throw new Error(
'fs.lstatSync is not available or returned an invalid result. Cannot safely determine file stats.'
);
Code Review by Qodo
1. Stress tests run by default
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+88
to
+95
| const writeBenchmarkArtifact = (fileNamePrefix: string, payload: Record<string, unknown>) => { | ||
| const actualFs = jest.requireActual('fs'); | ||
| const actualPath = jest.requireActual('path'); | ||
| const outputDir = actualPath.join(process.cwd(), 'dist', 'benchmarks'); | ||
| actualFs.mkdirSync(outputDir, { recursive: true }); | ||
| const outputPath = actualPath.join(outputDir, `${fileNamePrefix}-${Date.now()}.json`); | ||
| actualFs.writeFileSync(outputPath, JSON.stringify(payload, null, 2), 'utf8'); | ||
| }; |
There was a problem hiding this comment.
1. Stress tests run by default 🐞 Bug ⛯ Reliability
The newly-added stress/benchmark test is included by the default Jest testMatch, so it will run in normal npm test (including CI) and will write benchmark artifacts into dist/benchmarks during those runs.
Agent Prompt
## Issue description
A new stress/benchmark test under `tests/stress/` will run as part of the default `npm test` due to the broad Jest `testMatch`. It also writes benchmark artifacts to `dist/benchmarks`, adding side effects and extra time to CI and normal developer test runs.
## Issue Context
- CI runs `npm test`, which uses `jest.config.js`.
- `jest.config.js` currently matches all files under `tests/**`.
- The stress test writes JSON artifacts to `dist/benchmarks`.
## Fix Focus Areas
- jest.config.js[15-18]
- package.json[24-27]
- tests/stress/main-process/ipc-latency.stress.test.ts[88-95]
## Suggested direction
- Narrow default `testMatch` to unit/integration patterns (e.g. `tests/unit/**/*.test.*`, `tests/integration/**/*.test.*`).
- Keep `npm run test:stress` using `--testMatch` to run only `tests/stress/**`.
- Optional: add an env gate in `writeBenchmarkArtifact` (e.g. only write when `process.env.WRITE_BENCHMARKS === '1'`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
This was referenced Feb 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
This implements the first measurable slice of the facts-first backend plan: concrete safety hardening plus benchmark scaffolding so performance decisions are evidence-based.
Validation
Markdown docs lint passed: 13 markdown files checked, 8 links/images scanned, no decorative icons found.
All passed locally.
Summary by Sourcery
Harden main-process directory traversal around symlinks and add stress benchmarks for main IPC handlers.
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests
Chores