Skip to content

fix(skills): prevent symlinked install targets - #190

Open
levineam wants to merge 1 commit into
mainfrom
codex/propose-fix-for-symlink-vulnerability
Open

fix(skills): prevent symlinked install targets#190
levineam wants to merge 1 commit into
mainfrom
codex/propose-fix-for-symlink-vulnerability

Conversation

@levineam

Copy link
Copy Markdown
Owner

Motivation

  • Close a symlink-following and TOCTOU filesystem write vulnerability in the skill installer by ensuring installs cannot be redirected outside the intended destination root via symlinked path components or symlinked targets.

Description

  • Add validation to resolve and assert the installation root is a real directory, owned by the current user, and not group/other-writable via prepareInstallDirectory and assertSafeInstallDirectory in modules/jarvos-skills/src/index.js.
  • Reject any symbolic-link components inside the requested destinationDir with assertNoSymlinkComponents, and ensure per-skill directories are created and real-checked with prepareSkillDirectory to enforce containment under the resolved root.
  • Replace the previous naive fs.copyFileSync flow with copySkillFileSync which opens targets with O_NOFOLLOW and uses O_EXCL (unless force is set) to avoid races and prevent following target symlinks, and update installSkills to preflight and reject non-regular or symlinked targets before writing.
  • Use the resolved realpath of the destination for emitted events and add regression tests in modules/jarvos-skills/test/skills.test.js that verify symlinked directories and symlinked target files cannot redirect installation.

Testing

  • Ran the package test suite with npm test inside modules/jarvos-skills, and both @jarvos/skills bundle and projection tests passed.
  • Confirmed the new regression tests exercise symlinked-directory and symlinked-target scenarios and that installSkills rejects them as expected.
  • Performed git diff --check to ensure no whitespace or diff errors were introduced and found none.

Codex Task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant