Skip to content

fix(skills): prevent path traversal in skill install and uninstall#45

Merged
BlackHole1 merged 1 commit into
mainfrom
fix-sec
Mar 30, 2026
Merged

fix(skills): prevent path traversal in skill install and uninstall#45
BlackHole1 merged 1 commit into
mainfrom
fix-sec

Conversation

@BlackHole1
Copy link
Copy Markdown
Member

Skill names containing path components like ../.. could resolve outside the local skills root directories, allowing arbitrary directory removal on uninstall or arbitrary writes on install.

Add isPathWithinDirectory and isManagedSkillPathContained guards that reject any skill name whose resolved canonical or target path escapes the expected roots. Improve the unmanaged-skill error message to clarify that only oo-managed skills can be removed.

Skill names containing path components like `../..` could resolve
outside the local `skills` root directories, allowing arbitrary
directory removal on uninstall or arbitrary writes on install.

Add `isPathWithinDirectory` and `isManagedSkillPathContained` guards
that reject any skill name whose resolved canonical or target path
escapes the expected roots. Improve the unmanaged-skill error message
to clarify that only oo-managed skills can be removed.

Signed-off-by: Kevin Cui <bh@bugs.cc>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c482c032-4666-4d6b-b6c5-525385e98583

📥 Commits

Reviewing files that changed from the base of the PR and between 7a4e77c and af7ab93.

📒 Files selected for processing (8)
  • docs/commands.md
  • docs/commands.zh-CN.md
  • src/application/commands/skills/index.test.ts
  • src/application/commands/skills/managed-skill-paths.test.ts
  • src/application/commands/skills/managed-skill-paths.ts
  • src/application/commands/skills/registry-skill-install.ts
  • src/application/commands/skills/shared.ts
  • src/i18n/catalog.ts

Summary by CodeRabbit

  • Bug Fixes

    • Added path validation during skill installation and uninstallation to prevent escaping the designated skills directory.
    • Improved error messaging for attempts to remove unmanaged skills or skills with invalid paths.
  • Documentation

    • Updated skill management command documentation.

Walkthrough

This PR adds path validation rules to prevent skill names from escaping the local Codex skills root directories during installation and uninstallation. The changes introduce new utility functions (isManagedSkillPathContained, isPathWithinDirectory) in the managed-skill-paths module to validate path containment, integrate these validations into the registry skill installation and uninstallation flows, update documentation in English and Chinese versions describing the new validation behavior, add comprehensive test coverage for path traversal rejection scenarios, and introduce new i18n error messages for invalid paths and unmanaged skills.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

codex

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format <type>(<scope>): <subject> with type 'fix', scope 'skills', and a clear subject describing the main change to prevent path traversal in skill operations.
Description check ✅ Passed The description is directly related to the changeset, explaining the path traversal vulnerability and the guards added to prevent it, which aligns with all file modifications in the PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@BlackHole1 BlackHole1 merged commit 71b75fc into main Mar 30, 2026
5 checks passed
@BlackHole1 BlackHole1 deleted the fix-sec branch March 30, 2026 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant