Skip to content
Closed
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
11 changes: 6 additions & 5 deletions src/commands/install.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -18,7 +18,8 @@ function isLocalPath(source: string): boolean {
source.startsWith('/') ||
source.startsWith('./') ||
source.startsWith('../') ||
source.startsWith('~/')
source.startsWith('~/') ||
/^[a-zA-Z]:[\\/]/.test(source)
);
}

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
}
Expand Down
34 changes: 21 additions & 13 deletions tests/commands/install.test.ts
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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 ./', () => {
Expand Down Expand Up @@ -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);
});
});

Expand All @@ -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);
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
4 changes: 2 additions & 2 deletions tests/utils/skills.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});

Expand All @@ -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', () => {
Expand Down