fix: keep extra manifest paths portable#39
Conversation
Summary of ChangesHello @young5lee, 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 an issue where Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of non-portable paths in extra-manifest.json by introducing a ~-based home directory representation for paths. However, a high-severity path traversal vulnerability was identified in applyExtraPaths. This vulnerability arises because the repoPath from the repository's manifest is trusted without validation, which could allow a malicious repository to exfiltrate arbitrary files from a user's machine. It is recommended to validate that all paths from the manifest are relative and stay within the expected repository boundaries. Additionally, there is one suggestion to improve code clarity in src/sync/paths.ts.
| ? entry.repoPath | ||
| : path.join(plan.repoRoot, entry.repoPath); | ||
| const localPath = entry.sourcePath; | ||
| const localPath = path.resolve(expandHome(entry.sourcePath, plan.homeDir)); |
There was a problem hiding this comment.
The applyExtraPaths function trusts the repoPath value from the extra-manifest.json file in the repository. If this value is an absolute path or contains path traversal sequences (e.g., ../../), it can point to arbitrary files on the user's system. Since this path is used as the source for a copy operation where the destination is an allowlisted local path, an attacker who controls the repository can trick the system into copying sensitive files (like /etc/shadow or SSH keys) into the sync directory. A subsequent sync-push would then upload these sensitive files to the repository, leading to data exfiltration. Additionally, the mode property from the manifest is used in chmod without validation, allowing an attacker to change permissions of allowlisted files.
| const allowlist = portablePaths.map((entry) => | ||
| normalizePath(entry, locations.xdg.homeDir, platform) | ||
| ); |
There was a problem hiding this comment.
For improved clarity, consider generating the allowlist directly from rawPaths. This would make the data flow more direct, as rawPaths would be the single source for both the runtime allowlist of absolute paths and the portablePaths for the manifest. This change improves readability without affecting functionality.
| const allowlist = portablePaths.map((entry) => | |
| normalizePath(entry, locations.xdg.homeDir, platform) | |
| ); | |
| const allowlist = rawPaths.map((entry) => | |
| normalizePath(entry, locations.xdg.homeDir, platform) | |
| ); |
Problem
When using
extraConfigPaths/extraSecretPaths,sync-pushwritesextra-manifest.jsonwith absolutesourcePathvalues (because paths are normalized viapath.resolve()/home expansion).Across machines/OSes this causes churn (
/home/...vs/Users/...) and can lead to extras being skipped on pull (allowlist mismatch), then removed on the next push.Fix
sourcePathinextra-manifest.jsonusing a portable~form when the path is under the user's home directory.~when copying/stat-ing extra paths so runtime behavior stays correct.Tests
sourcePathbehavior insrc/sync/paths.test.ts.