Skip to content

fix(security): reject symlinks when fetching skills from a git repo#96

Merged
albertodebortoli merged 1 commit into
mainfrom
security/reject-skill-symlinks
Jun 13, 2026
Merged

fix(security): reject symlinks when fetching skills from a git repo#96
albertodebortoli merged 1 commit into
mainfrom
security/reject-skill-symlinks

Conversation

@albertodebortoli

Copy link
Copy Markdown
Member

Description

  • What & why: GitRepositorySkillFetcher fetches skills by shallow-cloning a repository and reading files from the clone. It previously skipped only symlinks that resolved to directories, so a file symlink in a malicious repo (e.g. leak -> /Users/you/.ssh/id_rsa) would be enumerated and read, copying local file content outside the repository into the skill cache. downloadSkill also resolved symlinks before reading without verifying the result stayed inside the clone.
  • Fix: The fetcher now skips all symbolic links when enumerating skill paths, and downloadSkill refuses to read a file that is a symlink or whose resolved path escapes the clone directory. This matches GitHubSkillTreeClient, which already filters symlinks, keeping the two fetchers consistent.
  • Trade-offs: Skills that legitimately rely on symlinked files will have those entries skipped; this mirrors the existing GitHub-API fetcher's behaviour, so it is consistent across transports.

Type of Change

  • Bug fix
  • Feature
  • Maintenance / Refactor
  • Documentation
  • CI / Tooling
  • Other (specify)

How Has This Been Tested?

swift build and swift test locally on macOS (arm64).

  • Added / updated unit tests
  • Manually tested locally
  • Tested on macOS (arch: arm64)

New tests: a file symlink in a cloned repo is excluded from enumerated skill paths, and downloadSkill throws fileReadFailed when asked to read a symlinked path pointing outside the clone. The existing directory-symlink test still passes.

Checklist

  • Swift code builds locally (swift build)
  • Tests pass locally (swift test)
  • Code style / formatting respected
  • Documentation updated (comments on the symlink handling)

Breaking Changes?

  • No

Additional Notes

Part of a set of independent supply-chain hardening changes; this PR stands alone and can be merged on its own.

@albertodebortoli albertodebortoli added this to the 0.21.0 milestone Jun 13, 2026
@albertodebortoli albertodebortoli added the security Security fixes label Jun 13, 2026
@codecov

codecov Bot commented Jun 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...sitorySkillFetcher/GitRepositorySkillFetcher.swift 93.75% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

GitRepositorySkillFetcher now skips ALL symbolic links when enumerating skill
paths (previously only directory symlinks were skipped) and refuses to read a
skill file that is a symlink or resolves outside the clone directory. This
matches GitHubSkillTreeClient, which already filters symlinks, and prevents a
malicious repo from making Luca read files such as ~/.ssh/id_rsa.
@albertodebortoli albertodebortoli force-pushed the security/reject-skill-symlinks branch from 54eb0de to a1b6f8c Compare June 13, 2026 21:45
@albertodebortoli albertodebortoli marked this pull request as ready for review June 13, 2026 21:55
@albertodebortoli albertodebortoli merged commit d97bca5 into main Jun 13, 2026
3 checks passed
@albertodebortoli albertodebortoli deleted the security/reject-skill-symlinks branch June 13, 2026 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security Security fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant