diff --git a/packages/cli/src/utils/skillUtils.test.ts b/packages/cli/src/utils/skillUtils.test.ts index 449b61c5b2..fd2af254e3 100644 --- a/packages/cli/src/utils/skillUtils.test.ts +++ b/packages/cli/src/utils/skillUtils.test.ts @@ -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); + }); }); }); diff --git a/packages/cli/src/utils/skillUtils.ts b/packages/cli/src/utils/skillUtils.ts index ca4ed08c82..f170171881 100644 --- a/packages/cli/src/utils/skillUtils.ts +++ b/packages/cli/src/utils/skillUtils.ts @@ -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 }; }