fix(security): skip symbolic links during skill updates#458
Conversation
Modified `src/skills/update.rs` to ensure symbolic links are not followed when updating a skill from a local source. This prevents potential information disclosure of sensitive host files. Consistent with the hardening previously applied to the installation path in `src/skills/install.rs`. Added regression test in `tests/test_update_security.rs`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR patches a symlink-handling vulnerability in skill updates where malicious symlinks in update sources could leak external files into skill destinations. The fix adds explicit symlink detection to skip symlinks during directory copy, is validated by a new security test, and is documented in the development journal. ChangesSymlink Vulnerability Fix and Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/skills/update.rs (1)
123-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore backup when copy fails.
At Line 123, a
copy_dir_allfailure returns immediately after Line 115 moved the current skill to backup, so failed updates can leave the skill absent/partial with only*.bakremaining.Proposed rollback fix for copy-step failures
- copy_dir_all(&local_dir, &skill_dir).map_err(SkillUpdateError::Io)?; + if let Err(e) = copy_dir_all(&local_dir, &skill_dir) { + let _ = fs::remove_dir_all(&skill_dir); + if backup_dir.exists() { + let _ = fs::rename(&backup_dir, &skill_dir); + } + return Err(SkillUpdateError::Io(e)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skills/update.rs` at line 123, After creating the backup (the earlier rename/move of existing skill to a *.bak), the failure returned from copy_dir_all(&local_dir, &skill_dir) must trigger a rollback that restores the backup to its original location; update the error path around copy_dir_all in update.rs so that on Err(e) you remove any partially copied skill_dir if present and then rename/move the backup back to skill_dir (using the same rename/rename semantics used when creating the backup), and propagate an appropriate SkillUpdateError::Io that includes both the original copy error and any restore error if the restore fails; reference copy_dir_all, local_dir, skill_dir, and SkillUpdateError::Io in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/journal/sentinel.md:
- Around line 35-36: Add a blank line immediately after the heading "##
2025-05-19 - Inconsistent Symlink Handling in Skill Updates" to satisfy MD022;
ensure there is an empty line between that heading and the following paragraph
so the markdown linter no longer flags the file .agents/journal/sentinel.md.
---
Outside diff comments:
In `@src/skills/update.rs`:
- Line 123: After creating the backup (the earlier rename/move of existing skill
to a *.bak), the failure returned from copy_dir_all(&local_dir, &skill_dir) must
trigger a rollback that restores the backup to its original location; update the
error path around copy_dir_all in update.rs so that on Err(e) you remove any
partially copied skill_dir if present and then rename/move the backup back to
skill_dir (using the same rename/rename semantics used when creating the
backup), and propagate an appropriate SkillUpdateError::Io that includes both
the original copy error and any restore error if the restore fails; reference
copy_dir_all, local_dir, skill_dir, and SkillUpdateError::Io in your changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 578e907c-33c0-4ad3-8234-938e7e45d456
📒 Files selected for processing (3)
.agents/journal/sentinel.mdsrc/skills/update.rstests/test_update_security.rs
| ## 2025-05-19 - Inconsistent Symlink Handling in Skill Updates | ||
| **Vulnerability:** While `src/skills/install.rs` was previously hardened against symlink following, `src/skills/update.rs` used a separate implementation of directory copying (`copy_dir_all`) that still followed symbolic links. This allowed information disclosure during local skill updates even after the installation path was secured. |
There was a problem hiding this comment.
Add a blank line below the heading to satisfy MD022.
Line 35 should be followed by an empty line before the paragraph on Line 36.
Markdown lint fix
## 2025-05-19 - Inconsistent Symlink Handling in Skill Updates
+
**Vulnerability:** While `src/skills/install.rs` was previously hardened against symlink following, `src/skills/update.rs` used a separate implementation of directory copying (`copy_dir_all`) that still followed symbolic links. This allowed information disclosure during local skill updates even after the installation path was secured.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2025-05-19 - Inconsistent Symlink Handling in Skill Updates | |
| **Vulnerability:** While `src/skills/install.rs` was previously hardened against symlink following, `src/skills/update.rs` used a separate implementation of directory copying (`copy_dir_all`) that still followed symbolic links. This allowed information disclosure during local skill updates even after the installation path was secured. | |
| ## 2025-05-19 - Inconsistent Symlink Handling in Skill Updates | |
| **Vulnerability:** While `src/skills/install.rs` was previously hardened against symlink following, `src/skills/update.rs` used a separate implementation of directory copying (`copy_dir_all`) that still followed symbolic links. This allowed information disclosure during local skill updates even after the installation path was secured. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 35-35: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/journal/sentinel.md around lines 35 - 36, Add a blank line
immediately after the heading "## 2025-05-19 - Inconsistent Symlink Handling in
Skill Updates" to satisfy MD022; ensure there is an empty line between that
heading and the following paragraph so the markdown linter no longer flags the
file .agents/journal/sentinel.md.
Source: Linters/SAST tools
1 participant
Severity: HIGH
Vulnerability: Information disclosure via symbolic link following in skill updates.
Impact: A malicious skill update source could contain symbolic links to sensitive host files (e.g.,
~/.ssh/id_rsa), causing their content to be copied into the project directory during an update.Fix: Modified the
copy_dir_allfunction insrc/skills/update.rsto explicitly skip symbolic links usingentry.file_type()?.is_symlink().Source:
src/skills/update.rs, functioncopy_dir_all.Verification: Added
tests/test_update_security.rswhich attempts to leak a sensitive file via a symlink during an update; the test fails without the fix and passes with it. Also ran the full test suite and clippy.PR created automatically by Jules for task 5657812799891626747 started by @yacosta738