fix(cli): prevent path traversal vulnerabilities during skill install…

packages/cli/src/utils/skillUtils.test.ts

@@ -267,5 +267,136 @@ 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);
+    });
+
+    it('should allow installation if skill name starts with double dots but is safe (e.g. ..-foo or ...)', 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: ..-foo\ndescription: safe skill name starting with double dots\n---\nbody',
+      );
+
+      const installed = await installSkill(
+        mockSkillSourceDir,
+        'workspace',
+        undefined,
+        () => {},
+      );
+      expect(installed.length).toBe(1);
+      expect(installed[0].name).toBe('..-foo');
+
+      const destPath = installed[0].location;
+      const resolvedTarget = path.resolve(tempDir, '.gemini/skills');
+      expect(destPath.startsWith(resolvedTarget + path.sep)).toBe(true);
+    });
   });
 });

packages/cli/src/utils/skillUtils.ts

@@ -75,6 +75,18 @@ export function renderSkillActionFeedback(
   return `Skill "${skillName}" ${actionVerb} ${preposition} ${s} settings.`;
 }
 
+function isPathTraversal(relative: string): boolean {
+  return (
+    relative === '..' ||
+    relative.startsWith('..' + path.sep) ||
+    path.isAbsolute(relative)
+  );
+}
+
+function isInvalidSubpath(relative: string): boolean {
+  return relative === '' || isPathTraversal(relative);
+}
+
 /**
  * Central logic for installing a skill from a remote URL or local path.
  */
@@ -132,11 +144,12 @@ 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);
+      const relative = path.relative(resolvedTemp, sourcePath);
+      if (isPathTraversal(relative)) {
+        throw new Error('Invalid path: Directory traversal not allowed.');
+      }
     }
 
     onLog(`Searching for skills in ${sourcePath}...`);
@@ -159,16 +172,22 @@ 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);
 
-      const exists = await fs.stat(destPath).catch(() => null);
+      const relative = path.relative(resolvedTarget, destPath);
+      if (isInvalidSubpath(relative)) {
+        throw new Error('Invalid skill name: Path traversal detected.');
+      }
+
+      const exists = await fs.lstat(destPath).catch(() => null);
       if (exists) {
         onLog(`Skill "${skillName}" already exists. Overwriting...`);
         await fs.rm(destPath, { recursive: true, force: true });
@@ -231,14 +250,20 @@ 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);
+
+    const relative = path.relative(resolvedTarget, destPath);
+    if (isInvalidSubpath(relative)) {
+      throw new Error('Invalid skill name: Path traversal detected.');
+    }
 
     const exists = await fs.lstat(destPath).catch(() => null);
     if (exists) {
@@ -275,18 +300,21 @@ 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))) {
+    const relative = path.relative(resolvedTarget, skillPath);
+    if (isInvalidSubpath(relative)) {
       return null;
     }
 
@@ -300,7 +328,12 @@ export async function uninstallSkill(
     return { location: skillPath };
   }
 
-  const skillDir = path.dirname(skillToUninstall.location);
+  const skillDir = path.resolve(path.dirname(skillToUninstall.location));
+  const relative = path.relative(resolvedTarget, skillDir);
+  if (isInvalidSubpath(relative)) {
+    return null;
+  }
+
   await fs.rm(skillDir, { recursive: true, force: true });
   return { location: skillDir };
 }