From 63388a917859c5402103ea80e932ac1253b97da4 Mon Sep 17 00:00:00 2001 From: Williams Date: Tue, 6 Jan 2026 21:19:21 +0000 Subject: [PATCH 1/2] use platform path separator for windows compatibility --- src/commands/install.ts | 11 ++++++----- tests/commands/install.test.ts | 34 +++++++++++++++++++++------------- tests/integration/e2e.test.ts | 2 +- tests/utils/skills.test.ts | 4 ++-- 4 files changed, 30 insertions(+), 21 deletions(-) diff --git a/src/commands/install.ts b/src/commands/install.ts index dc35b18..6b71a3b 100644 --- a/src/commands/install.ts +++ b/src/commands/install.ts @@ -1,5 +1,5 @@ import { readFileSync, readdirSync, existsSync, mkdirSync, rmSync, cpSync, statSync } from 'fs'; -import { join, basename, resolve } from 'path'; +import { join, basename, resolve, sep } from 'path'; import { homedir } from 'os'; import { execSync } from 'child_process'; import chalk from 'chalk'; @@ -18,7 +18,8 @@ function isLocalPath(source: string): boolean { source.startsWith('/') || source.startsWith('./') || source.startsWith('../') || - source.startsWith('~/') + source.startsWith('~/') || + /^[a-zA-Z]:[\\/]/.test(source) ); } @@ -194,7 +195,7 @@ async function installSingleLocalSkill( // Security: ensure target path stays within target directory const resolvedTargetPath = resolve(targetPath); const resolvedTargetDir = resolve(targetDir); - if (!resolvedTargetPath.startsWith(resolvedTargetDir + '/')) { + if (!resolvedTargetPath.startsWith(resolvedTargetDir + sep)) { console.error(chalk.red(`Security error: Installation path outside target directory`)); process.exit(1); } @@ -244,7 +245,7 @@ async function installSpecificSkill( // Security: ensure target path stays within target directory const resolvedTargetPath = resolve(targetPath); const resolvedTargetDir = resolve(targetDir); - if (!resolvedTargetPath.startsWith(resolvedTargetDir + '/')) { + if (!resolvedTargetPath.startsWith(resolvedTargetDir + sep)) { console.error(chalk.red(`Security error: Installation path outside target directory`)); process.exit(1); } @@ -370,7 +371,7 @@ async function installFromRepo( // Security: ensure target path stays within target directory const resolvedTargetPath = resolve(info.targetPath); const resolvedTargetDir = resolve(targetDir); - if (!resolvedTargetPath.startsWith(resolvedTargetDir + '/')) { + if (!resolvedTargetPath.startsWith(resolvedTargetDir + sep)) { console.error(chalk.red(`Security error: Installation path outside target directory`)); continue; } diff --git a/tests/commands/install.test.ts b/tests/commands/install.test.ts index 586b9e5..83e83d2 100644 --- a/tests/commands/install.test.ts +++ b/tests/commands/install.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { resolve, join } from 'path'; +import { resolve, join, sep } from 'path'; import { homedir } from 'os'; // We need to test the helper functions, but they're not exported @@ -14,13 +14,16 @@ describe('install.ts helper functions', () => { source.startsWith('/') || source.startsWith('./') || source.startsWith('../') || - source.startsWith('~/') + source.startsWith('~/') || + /^[a-zA-Z]:\\/.test(source) // Windows absolute path ); }; - it('should detect absolute paths starting with /', () => { + it('should detect absolute paths starting with / or drive letter', () => { expect(isLocalPath('/absolute/path/to/skill')).toBe(true); - expect(isLocalPath('/Users/test/skills')).toBe(true); + if (process.platform === 'win32') { + expect(isLocalPath('C:\\Users\\test\\skills')).toBe(true); + } }); it('should detect relative paths starting with ./', () => { @@ -130,8 +133,9 @@ describe('install.ts helper functions', () => { }); it('should keep absolute paths as-is (resolved)', () => { - const expanded = expandPath('/absolute/path'); - expect(expanded).toBe('/absolute/path'); + const absPath = resolve('/absolute/path'); + const expanded = expandPath(absPath); + expect(expanded).toBe(absPath); }); }); @@ -140,28 +144,32 @@ describe('install.ts helper functions', () => { const isPathSafe = (targetPath: string, targetDir: string): boolean => { const resolvedTargetPath = resolve(targetPath); const resolvedTargetDir = resolve(targetDir); - return resolvedTargetPath.startsWith(resolvedTargetDir + '/'); + return resolvedTargetPath.startsWith(resolvedTargetDir + sep); }; it('should allow normal skill paths within target directory', () => { - expect(isPathSafe('/home/user/.claude/skills/my-skill', '/home/user/.claude/skills')).toBe(true); + const base = resolve('/home/user/.claude/skills'); + expect(isPathSafe(join(base, 'my-skill'), base)).toBe(true); }); it('should block path traversal attempts with ../', () => { - expect(isPathSafe('/home/user/.claude/skills/../../../etc/passwd', '/home/user/.claude/skills')).toBe(false); + const base = resolve('/home/user/.claude/skills'); + expect(isPathSafe(join(base, '../../../etc/passwd'), base)).toBe(false); }); it('should block paths outside target directory', () => { - expect(isPathSafe('/etc/passwd', '/home/user/.claude/skills')).toBe(false); + const base = resolve('/home/user/.claude/skills'); + expect(isPathSafe(resolve('/etc/passwd'), base)).toBe(false); }); it('should block paths that are prefix but not subdirectory', () => { - // /home/user/.claude/skills-evil should NOT be allowed when target is /home/user/.claude/skills - expect(isPathSafe('/home/user/.claude/skills-evil', '/home/user/.claude/skills')).toBe(false); + const base = resolve('/home/user/.claude/skills'); + expect(isPathSafe(resolve('/home/user/.claude/skills-evil'), base)).toBe(false); }); it('should allow nested subdirectories', () => { - expect(isPathSafe('/home/user/.claude/skills/category/my-skill', '/home/user/.claude/skills')).toBe(true); + const base = resolve('/home/user/.claude/skills'); + expect(isPathSafe(join(base, 'category/my-skill'), base)).toBe(true); }); }); }); diff --git a/tests/integration/e2e.test.ts b/tests/integration/e2e.test.ts index b4d328d..e5d86a5 100644 --- a/tests/integration/e2e.test.ts +++ b/tests/integration/e2e.test.ts @@ -180,7 +180,7 @@ describe('End-to-end CLI tests', () => { }); it('should error for non-existent local path', () => { - const result = runCli(`install /non/existent/path -y`); + const result = runCli(`install ${join(testTempDir, 'non-existent')} -y`); expect(result.exitCode).toBe(1); expect(result.stderr).toContain('does not exist'); diff --git a/tests/utils/skills.test.ts b/tests/utils/skills.test.ts index b460361..7300e1b 100644 --- a/tests/utils/skills.test.ts +++ b/tests/utils/skills.test.ts @@ -181,7 +181,7 @@ describe('skills.ts', () => { const skill = findSkill('my-skill'); expect(skill).not.toBeNull(); - expect(skill?.path).toContain('my-skill/SKILL.md'); + expect(skill?.path).toContain(join('my-skill', 'SKILL.md')); expect(skill?.baseDir).toContain('my-skill'); }); @@ -191,7 +191,7 @@ describe('skills.ts', () => { const skill = findSkill('linked-skill'); expect(skill).not.toBeNull(); - expect(skill?.path).toContain('linked-skill/SKILL.md'); + expect(skill?.path).toContain(join('linked-skill', 'SKILL.md')); }); it('should return null for non-existent skill', () => { From 4b5b07af4c74695711fc5bd25937426dd8ee3716 Mon Sep 17 00:00:00 2001 From: Williams Date: Tue, 6 Jan 2026 21:49:09 +0000 Subject: [PATCH 2/2] use join for nested path test --- tests/commands/install.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/commands/install.test.ts b/tests/commands/install.test.ts index 83e83d2..931e5cf 100644 --- a/tests/commands/install.test.ts +++ b/tests/commands/install.test.ts @@ -169,7 +169,7 @@ describe('install.ts helper functions', () => { it('should allow nested subdirectories', () => { const base = resolve('/home/user/.claude/skills'); - expect(isPathSafe(join(base, 'category/my-skill'), base)).toBe(true); + expect(isPathSafe(join(base, 'category', 'my-skill'), base)).toBe(true); }); }); });