feat: sync modern opencode directories and portable extra paths#41
feat: sync modern opencode directories and portable extra paths#41lnilluv wants to merge 3 commits intoiHildy:mainfrom
Conversation
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 significantly enhances the OpenCode synchronization process by incorporating modern configuration directories into the default sync scope and improving the portability of extra paths. By transitioning to home-relative path storage for extra manifests and ensuring their proper expansion during application, the system becomes more robust and cross-platform compatible, reducing issues related to varying file system structures across different environments. 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
|
![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 introduces two main features: it adds support for syncing modern OpenCode directories and makes extra paths portable by storing them in a home-relative format. While these changes are well-tested, there is a suggestion for code clarity regarding a redundant function call. More critically, the changes highlight an existing security vulnerability where the manifest file from the sync repository is trusted blindly, allowing the repoPath to be manipulated to point to arbitrary locations on the local file system, which could lead to data exfiltration. It is recommended to add validation to ensure all repository paths are relative and confined within the repository root.
| ? entry.repoPath | ||
| : path.join(plan.repoRoot, entry.repoPath); | ||
| const localPath = entry.sourcePath; | ||
| const localPath = expandHome(entry.sourcePath, plan.homeDir); |
There was a problem hiding this comment.
The applyExtraPaths function trusts the repoPath provided in the manifest file from the repository. An attacker who controls the sync repository can modify the extra-manifest.json to set repoPath to an absolute path or use path traversal (e.g., ../../etc/passwd). During a syncRepoToLocal operation, this would cause the client to copy arbitrary files from the local system into the synced directories. If these directories are subsequently synced back to the repository (via syncLocalToRepo), sensitive local files could be exfiltrated to the attacker-controlled repository.
To remediate this, ensure that repoPath is a relative path and that the resolved path remains within the repository root.
There was a problem hiding this comment.
Fixed in 95a77c0. applyExtraPaths now validates manifest repoPath with resolveManifestRepoPath: absolute paths are rejected and any path that escapes plan.repoRoot is rejected. Added regression tests in src/sync/apply.test.ts for absolute and traversal cases.
src/sync/paths.ts
Outdated
| const entries = allowlist.map((sourcePath) => ({ | ||
| sourcePath, | ||
| repoPath: path.join(repoExtraDir, encodeExtraPath(sourcePath)), | ||
| sourcePath: toPortableSourcePath(sourcePath, locations.xdg.homeDir, platform), | ||
| repoPath: path.join( | ||
| repoExtraDir, | ||
| encodeExtraPath(toPortableSourcePath(sourcePath, locations.xdg.homeDir, platform)) | ||
| ), | ||
| })); |
There was a problem hiding this comment.
The function toPortableSourcePath is called twice with the same arguments. This is redundant. It's better to call it once, store the result in a variable, and then reuse it. This will improve both readability and avoid a repeated computation.
const entries = allowlist.map((sourcePath) => {
const portableSourcePath = toPortableSourcePath(sourcePath, locations.xdg.homeDir, platform);
return {
sourcePath: portableSourcePath,
repoPath: path.join(
repoExtraDir,
encodeExtraPath(portableSourcePath)
),
};
});There was a problem hiding this comment.
Fixed in 95a77c0. buildExtraPathPlan now computes portableSourcePath once per entry and reuses it for both sourcePath and encodeExtraPath(...).
1 participant
Summary
Validation