Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,22 @@ actor GitRepositorySkillFetcher: SkillRepositoryFetching {

func downloadSkill(repository: String, path: String, ref: String?) async throws -> Data {
let cloneDir = try await clone(repository, ref: ref)
let fileURL = cloneDir.resolvingSymlinksInPath().appendingPathComponent(path)
let baseDir = cloneDir.resolvingSymlinksInPath()
let fileURL = baseDir.appendingPathComponent(path)

// A malicious skill repository must not be able to make Luca read files outside the clone
// (e.g. a symlink pointing at ~/.ssh/id_rsa). Reject the read if the final component is a
// symbolic link, or if resolving the path escapes the clone directory.
let resourceValues = try? fileURL.resourceValues(forKeys: [.isSymbolicLinkKey])
if resourceValues?.isSymbolicLink == true {
throw GitRepositorySkillFetcherError.fileReadFailed(path: path)
}
let resolvedPath = fileURL.resolvingSymlinksInPath().standardizedFileURL.path
let basePath = baseDir.standardizedFileURL.path
guard resolvedPath == basePath || resolvedPath.hasPrefix(basePath + "/") else {
throw GitRepositorySkillFetcherError.fileReadFailed(path: path)
}

do {
return try Data(contentsOf: fileURL)
} catch {
Expand Down Expand Up @@ -184,14 +199,11 @@ actor GitRepositorySkillFetcher: SkillRepositoryFetching {
let isDir = resourceValues?.isDirectory ?? false
if isDir { continue }

// Skip symbolic links that point to directories, since they cannot be read as files
// and we rely on deduplication to find the original directory if it's within the skill scope.
// Skip all symbolic links. Following them risks reading files outside the cloned
// repository (e.g. a link pointing at ~/.ssh), and the GitHub tree client excludes
// symlinks too, so this keeps both fetchers consistent.
let isSymlink = resourceValues?.isSymbolicLink ?? false
if isSymlink {
let resolved = fileURL.resolvingSymlinksInPath()
let isResolvedDir = (try? resolved.resourceValues(forKeys: [.isDirectoryKey]))?.isDirectory ?? false
if isResolvedDir { continue }
}
if isSymlink { continue }

let fileComponents = fileURL.resolvingSymlinksInPath().pathComponents
let relativePath = fileComponents.dropFirst(baseComponents.count).joined(separator: "/")
Expand Down
33 changes: 33 additions & 0 deletions Tests/Core/GitRepositorySkillFetcherTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,39 @@ struct GitRepositorySkillFetcherTests {
#expect(!paths.contains("skills/foo/link-to-dir"))
}

// MARK: - test_skillPaths_skipsFileSymlinks

@Test
func test_skillPaths_skipsFileSymlinks() async throws {
let runner = SeedingSubprocessRunnerMock()
runner.exitCodes = [0]
runner.filesToCreate = ["skills/foo/SKILL.md": "# Foo"]
// Symlink inside the skill directory that resolves to a file outside the clone.
runner.symlinksToCreate = ["skills/foo/leak": "/etc/passwd"]
let sut = GitRepositorySkillFetcher(subprocessRunner: runner)

let paths = try await sut.skillPaths(repository: "owner/repo", ref: nil)

#expect(paths.contains("skills/foo/SKILL.md"))
#expect(!paths.contains("skills/foo/leak"))
}

// MARK: - test_downloadSkill_symlink_throwsFileReadFailed

@Test
func test_downloadSkill_symlink_throwsFileReadFailed() async throws {
let runner = SeedingSubprocessRunnerMock()
runner.exitCodes = [0]
runner.filesToCreate = ["skills/foo/SKILL.md": "# Foo"]
// A symlink pointing outside the clone must not be readable through downloadSkill.
runner.symlinksToCreate = ["skills/foo/leak": "/etc/passwd"]
let sut = GitRepositorySkillFetcher(subprocessRunner: runner)

await #expect(throws: GitRepositorySkillFetcherError.fileReadFailed(path: "skills/foo/leak")) {
try await sut.downloadSkill(repository: "owner/repo", path: "skills/foo/leak", ref: nil)
}
}

// MARK: - test_skillPaths_sameRepoDifferentRef_clonesAgain

@Test
Expand Down