Repository navigation
fix(core): contain legacy checkpoint paths to the checkpoint directory - #29521
ManoharPaturi wants to merge 3 commits into
Conversation
The legacy fallback path in _getCheckpointPath and deleteCheckpoint is
built with path.join(geminiDir, `checkpoint-${tag}.json`) using the raw
tag, and path.join normalizes the '..' segments away, so a tag such as
'x/../../secret' resolves outside the checkpoint directory: /chat delete
unlinks the file and checkpoint load reads it back.
Gate both legacy uses behind a resolve-based containment check so a
traversal tag simply misses (ENOENT semantics) while flat legacy tags
keep working. Regression tests cover delete/load/exists traversal plus
the flat legacy path.
Summary of ChangesHello, 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 addresses a security vulnerability where legacy checkpoint paths could be manipulated via directory traversal to access or delete files outside the intended checkpoint directory. By introducing a validation helper, the system now ensures that any legacy path resolution is constrained to the safe directory, effectively neutralizing traversal attempts while preserving existing functionality for legitimate legacy checkpoints. Highlights
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 the 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 counterproductive. 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. Footnotes
|
|
📊 PR Size: size/M
|
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security by preventing path traversal vulnerabilities when loading or deleting legacy raw-path checkpoints. It introduces a helper method _isInsideCheckpointDir to verify that resolved paths remain within the designated checkpoint directory, accompanied by comprehensive unit tests. The feedback suggests improving this validation by using a robust resolveToRealPath function and handling case-insensitive filesystems (like Windows and macOS) to avoid potential mismatches with legitimate checkpoints.
| private _isInsideCheckpointDir(candidatePath: string): boolean { | ||
| const checkpointDir = path.resolve(this.geminiDir!); | ||
| return path.resolve(candidatePath).startsWith(checkpointDir + path.sep); | ||
| } |
There was a problem hiding this comment.
On case-insensitive filesystems (such as Windows and macOS), path casing can vary. Using a simple case-sensitive startsWith check can cause legitimate legacy checkpoints to fail to load or delete due to casing mismatches. Additionally, ensure consistent path resolution and prevent path traversal by using resolveToRealPath instead of standard path.resolve for all related path validations.
private _isInsideCheckpointDir(candidatePath: string): boolean {
const checkpointDir = resolveToRealPath(this.geminiDir!);
const resolvedCandidate = resolveToRealPath(candidatePath);
const isCaseInsensitive = process.platform === 'win32' || process.platform === 'darwin';
const prefix = checkpointDir + path.sep;
return isCaseInsensitive
? resolvedCandidate.toLowerCase().startsWith(prefix.toLowerCase())
: resolvedCandidate.startsWith(prefix);
}References
- Ensure consistent path resolution by using a single, robust function (e.g.,
resolveToRealPath) for all related path validations, including internal validations in components likeWorkspaceContext. - Sanitize user-provided file paths used in file system operations to prevent path traversal vulnerabilities.
There was a problem hiding this comment.
Applied it with one change: I kept resolveToRealPath on the directory only and left the candidate path lexical. resolveToRealPath decodes %XX sequences, and the tag here is raw user input, so the guard has to compare the exact string the fs calls will open. Case-insensitive prefix check is in for win32/darwin now.
gemini-cli
Bot
added
priority/p1
Realpath the checkpoint directory so symlinked segments can't split the prefix, and compare case-insensitively on win32/darwin. The candidate path stays lexical on purpose: resolveToRealPath decodes %XX sequences, and the raw tag is user input that must be checked exactly as the fs calls will open it.
|
Small update in the 2nd commit, based on the code assist review:
logger tests 43/43, eslint + tsc clean on the changed file. |
resolveToRealPath on the directory while resolving the candidate lexically caused a prefix mismatch wherever a segment of the checkpoint directory is itself a symlink (e.g. /var -> /private/var on macOS), dropping legitimate legacy checkpoints. Use the repo's isSubpath (path.relative-based, case-handling for win32/darwin included) against BOTH the raw and the realpath'd directory. The candidate itself stays lexical on purpose: resolveToRealPath decodes %XX sequences and the raw tag is user input, so it must be checked as the fs calls will open it.
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
This branch has not been deployed
1 participant
Summary
_getCheckpointPathanddeleteCheckpointbuild the legacy fallback path withpath.join(geminiDir,+"checkpoint-${tag}.json"+)using the raw tag, andpath.joinnormalizes the..segments away. A tag such asx/../../secrettherefore resolves to a file outside the checkpoint directory:/chat delete <tag>unlinks that file and checkpoint load returns its contents. The new encoded path (_checkpointPath) is safe — only the legacy raw fallback is exposed.Details
_isInsideCheckpointDirhelper: the legacy raw path is only considered when it resolves inside the checkpoint directory, so a traversal tag simply misses with normal ENOENT semantics. Applied at both legacy uses: the load/exists fallback in_getCheckpointPathand the backward-compat unlink indeleteCheckpoint.isSubpath(path.relative-based, case-insensitive on win32/darwin) against both the raw and the realpath'd directory, so setups where a directory segment is a symlink (/var→/private/varon macOS) don't drop legitimate checkpoints. The candidate itself stays lexical —resolveToRealPathdecodes%XXsequences and the raw tag is user input, so it must be checked exactly as the fs calls will open it.Related Issues
Fixes #29191
How to Validate
main, the three traversal regression tests fail:deleteCheckpoint('x/../../outside-target')deletes a file next to the checkpoint directory and returnstrue, andloadCheckpoint/checkpointExistsreach the outside file.false/ empty history) and the outside file is untouched.Pre-Merge Checklist
npx vitest run src/core/logger.test.ts— 43/43 passingnpx eslint+tsc --noEmitclean on changed files