Validate and restrict git skill sync paths to prevent arbitrary overwrite#211
Validate and restrict git skill sync paths to prevent arbitrary overwrite#211AkaraChen wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 28 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5bb29f88b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| )); | ||
| } | ||
|
|
||
| Ok(target_dir) |
There was a problem hiding this comment.
When source_paths contains a symlink that points at an allowed installed skill directory, canonical_target passes the allow-list check, but the function returns the original target_dir; the later remove_dir_all/copy therefore operates on the symlink path outside the allow-list rather than on the installed skill. Since source_paths is client supplied, a caller can create a symlink to any installed skill and cause sync to replace that symlink path with cloned repo contents, bypassing the intended restriction to installed skill directories. Returning the canonical target, or requiring the supplied path itself to be an allowed installed path, keeps the mutation in-scope.
Useful? React with 👍 / 👎.
| for resources in load_all_agents(resource_scope, project_root) { | ||
| for skill in resources.skills { |
There was a problem hiding this comment.
When scope is all with a project root and the same skill name is installed for the same agent in both project and global scopes, this allow-list misses the global installation because load_all_agents(ResourceScope::Both, ...) delegates to load_both_annotated, which records project skill names first and then skips matching global names in crates/core/src/manager/mod.rs:128-159. A request that is meant to sync both installations will therefore validate the project path but reject the global source_path as INVALID_SYNC_TARGET; build the allow-list from each scope separately so all really includes both directories.
Useful? React with 👍 / 👎.
1 participant
Motivation
source_paths, which allowed deletion/replacement of arbitrary user directories.\Description
scopeand optionalproject_roottoGitSyncRequestso the server can validate which installation locations are in-scope before mutating them (crates/api/src/dto/skill.rs).\..traversal and requiring the cloned path to parse as a skill and remain under the repo temp dir (resolve_cloned_skill_dir,reject_unsafe_relative_path).\source_pathagainst that set before anyremove_dir_allor copy occurs (allowed_git_sync_target_dirs,validate_git_sync_target_dir).\git_sync_skillroute to use these helpers and only perform delete-and-copy on validated target directories; parsing of the cloned skill is now required.\allhandling for sync scope (global/project/both) so clients can request validation across both scopes when appropriate.\scope: "all"andproject_rootwhere applicable) so the backend can perform validation.\crates/api/src/routes/skills.rstests).\Testing
cargo fmt --all --check— succeeded.\cd crates/desktop && bunx prettier --check src/components/sync-github-skill-dialog.tsx src/generated/dto/GitSyncRequest.ts— succeeded.\cargo test -p aghub-apiin this environment hit network/caching limitations and could not complete (crate downloads blocked), so backend tests were not executed here.\cd crates/desktop && bun run typecheck) which failed in this environment because node dependencies are not installed, so full TS typechecking was not run here.Codex Task