fix(cli): prevent path traversal during skill install, link, and uninstall

This commit is contained in:
Om Patel
2026-06-03 15:57:04 -04:00
parent 4196596f7f
commit 22c0a97f8a
2 changed files with 140 additions and 13 deletions
+108
View File
@@ -267,5 +267,113 @@ describe('skillUtils', () => {
const exists = await fs.stat(skillDir).catch(() => null);
expect(exists).toBeNull();
});
it('should prevent path traversal in fallback uninstallation (e.g. sibling directories)', async () => {
const skillsDir = path.join(tempDir, '.gemini/skills');
await fs.mkdir(skillsDir, { recursive: true });
const siblingDir = path.join(tempDir, '.gemini/skills-attacker');
await fs.mkdir(siblingDir, { recursive: true });
// Attempt to uninstall the sibling directory using path traversal
const result = await uninstallSkill('../skills-attacker', 'user');
expect(result).toBeNull();
// Verify sibling directory is NOT deleted
const exists = await fs.stat(siblingDir).catch(() => null);
expect(exists).not.toBeNull();
});
it('should prevent path traversal in fallback uninstallation with dot or dot dot', async () => {
expect(await uninstallSkill('..', 'user')).toBeNull();
expect(await uninstallSkill('.', 'user')).toBeNull();
expect(await uninstallSkill('', 'user')).toBeNull();
});
});
describe('path traversal prevention', () => {
it('should throw error during installation if skill name is dot dot or dot', async () => {
const mockSkillSourceDir = path.join(tempDir, 'mock-skill-source');
const skillSubDir = path.join(mockSkillSourceDir, 'test-skill');
await fs.mkdir(skillSubDir, { recursive: true });
await fs.writeFile(
path.join(skillSubDir, 'SKILL.md'),
'---\nname: ..\ndescription: exploit\n---\nbody',
);
await expect(
installSkill(mockSkillSourceDir, 'workspace', undefined, () => {}),
).rejects.toThrow('Invalid skill name: Path traversal detected.');
});
it('should throw error during linking if skill name is dot dot or dot', async () => {
const mockSkillSourceDir = path.join(tempDir, 'mock-skill-source');
const skillSubDir = path.join(mockSkillSourceDir, 'test-skill');
await fs.mkdir(skillSubDir, { recursive: true });
await fs.writeFile(
path.join(skillSubDir, 'SKILL.md'),
'---\nname: ..\ndescription: exploit\n---\nbody',
);
await expect(
linkSkill(mockSkillSourceDir, 'workspace', () => {}),
).rejects.toThrow('Invalid skill name: Path traversal detected.');
});
it('should throw error during installation if subpath escapes temp directory', async () => {
const skillPath = path.join(projectRoot, 'weather-skill.skill');
const exists = await fs.stat(skillPath).catch(() => null);
if (!exists) return;
await expect(
installSkill(skillPath, 'workspace', '../escape', () => {}),
).rejects.toThrow('Invalid path: Directory traversal not allowed.');
});
it('should sanitize absolute path names and install them safely within the target directory', async () => {
const mockSkillSourceDir = path.join(tempDir, 'mock-skill-source');
const skillSubDir = path.join(mockSkillSourceDir, 'test-skill');
await fs.mkdir(skillSubDir, { recursive: true });
await fs.writeFile(
path.join(skillSubDir, 'SKILL.md'),
'---\nname: /tmp/exploit\ndescription: exploit\n---\nbody',
);
const installed = await installSkill(
mockSkillSourceDir,
'workspace',
undefined,
() => {},
);
expect(installed.length).toBe(1);
expect(installed[0].name).toBe('-tmp-exploit');
const destPath = installed[0].location;
const resolvedTarget = path.resolve(tempDir, '.gemini/skills');
expect(destPath.startsWith(resolvedTarget + path.sep)).toBe(true);
});
it('should sanitize traversal names with spaces and install them safely within the target directory', async () => {
const mockSkillSourceDir = path.join(tempDir, 'mock-skill-source');
const skillSubDir = path.join(mockSkillSourceDir, 'test-skill');
await fs.mkdir(skillSubDir, { recursive: true });
await fs.writeFile(
path.join(skillSubDir, 'SKILL.md'),
'---\nname: " ../../exploit "\ndescription: exploit\n---\nbody',
);
const installed = await installSkill(
mockSkillSourceDir,
'workspace',
undefined,
() => {},
);
expect(installed.length).toBe(1);
expect(installed[0].name).toBe(' ..-..-exploit ');
const destPath = installed[0].location;
const resolvedTarget = path.resolve(tempDir, '.gemini/skills');
expect(destPath.startsWith(resolvedTarget + path.sep)).toBe(true);
});
});
});
+32 -13
View File
@@ -132,11 +132,14 @@ export async function installSkill(
sourcePath = path.resolve(sourcePath);
// Quick security check to prevent directory traversal out of temp dir when cloning
if (
tempDirToClean &&
!sourcePath.startsWith(path.resolve(tempDirToClean))
) {
throw new Error('Invalid path: Directory traversal not allowed.');
if (tempDirToClean) {
const resolvedTemp = path.resolve(tempDirToClean);
if (
sourcePath !== resolvedTemp &&
!sourcePath.startsWith(resolvedTemp + path.sep)
) {
throw new Error('Invalid path: Directory traversal not allowed.');
}
}
onLog(`Searching for skills in ${sourcePath}...`);
@@ -159,14 +162,19 @@ export async function installSkill(
throw new Error('Skill installation cancelled by user.');
}
await fs.mkdir(targetDir, { recursive: true });
const resolvedTarget = path.resolve(targetDir);
await fs.mkdir(resolvedTarget, { recursive: true });
const installedSkills: Array<{ name: string; location: string }> = [];
for (const skill of skills) {
const skillName = skill.name;
const skillDir = path.dirname(skill.location);
const destPath = path.join(targetDir, skillName);
const destPath = path.resolve(resolvedTarget, skillName);
if (!destPath.startsWith(resolvedTarget + path.sep)) {
throw new Error('Invalid skill name: Path traversal detected.');
}
const exists = await fs.stat(destPath).catch(() => null);
if (exists) {
@@ -231,14 +239,19 @@ export async function linkSkill(
throw new Error('Skill linking cancelled by user.');
}
await fs.mkdir(targetDir, { recursive: true });
const resolvedTarget = path.resolve(targetDir);
await fs.mkdir(resolvedTarget, { recursive: true });
const linkedSkills: Array<{ name: string; location: string }> = [];
for (const skill of skills) {
const skillName = skill.name;
const skillSourceDir = path.dirname(skill.location);
const destPath = path.join(targetDir, skillName);
const destPath = path.resolve(resolvedTarget, skillName);
if (!destPath.startsWith(resolvedTarget + path.sep)) {
throw new Error('Invalid skill name: Path traversal detected.');
}
const exists = await fs.lstat(destPath).catch(() => null);
if (exists) {
@@ -275,18 +288,20 @@ export async function uninstallSkill(
? storage.getProjectSkillsDir()
: Storage.getUserSkillsDir();
const resolvedTarget = path.resolve(targetDir);
// Load all skills in the target directory to find the one with the matching name
const discoveredSkills = await loadSkillsFromDir(targetDir);
const discoveredSkills = await loadSkillsFromDir(resolvedTarget);
const skillToUninstall = discoveredSkills.find((s) => s.name === name);
if (!skillToUninstall) {
// Fallback: Check if a directory with the given name exists.
// This maintains backward compatibility for cases where the metadata might be missing or corrupted
// but the directory name matches the user's request.
const skillPath = path.resolve(targetDir, name);
const skillPath = path.resolve(resolvedTarget, name);
// Security check: ensure the resolved path is within the target directory to prevent path traversal
if (!skillPath.startsWith(path.resolve(targetDir))) {
if (!skillPath.startsWith(resolvedTarget + path.sep)) {
return null;
}
@@ -300,7 +315,11 @@ export async function uninstallSkill(
return { location: skillPath };
}
const skillDir = path.dirname(skillToUninstall.location);
const skillDir = path.resolve(path.dirname(skillToUninstall.location));
if (!skillDir.startsWith(resolvedTarget + path.sep)) {
return null;
}
await fs.rm(skillDir, { recursive: true, force: true });
return { location: skillDir };
}