fix(cli): prevent path traversal false positives on safe skill names

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);
+    });
   });
 });

packages/cli/src/utils/skillUtils.ts

@@ -1,306 +0,0 @@
-/**
- * @license
- * Copyright 2026 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-
-import { SettingScope } from '../config/settings.js';
-import type { SkillActionResult } from './skillSettings.js';
-import {
-  Storage,
-  loadSkillsFromDir,
-  type SkillDefinition,
-} from '@google/gemini-cli-core';
-import { cloneFromGit } from '../config/extensions/github.js';
-import extract from 'extract-zip';
-import * as fs from 'node:fs/promises';
-import * as path from 'node:path';
-import * as os from 'node:os';
-
-/**
- * Shared logic for building the core skill action message while allowing the
- * caller to control how each scope and its path are rendered (e.g., bolding or
- * dimming).
- *
- * This function ONLY returns the description of what happened. It is up to the
- * caller to append any interface-specific guidance (like "Use /skills reload"
- * or "Restart required").
- */
-export function renderSkillActionFeedback(
-  result: SkillActionResult,
-  formatScope: (label: string, path: string) => string,
-): string {
-  const { skillName, action, status, error } = result;
-
-  if (status === 'error') {
-    return (
-      error ||
-      `An error occurred while attempting to ${action} skill "${skillName}".`
-    );
-  }
-
-  if (status === 'no-op') {
-    return `Skill "${skillName}" is already ${action === 'enable' ? 'enabled' : 'disabled'}.`;
-  }
-
-  const isEnable = action === 'enable';
-  const actionVerb = isEnable ? 'enabled' : 'disabled';
-  const preposition = isEnable
-    ? 'by removing it from the disabled list in'
-    : 'by adding it to the disabled list in';
-
-  const formatScopeItem = (s: { scope: SettingScope; path: string }) => {
-    const label =
-      s.scope === SettingScope.Workspace ? 'workspace' : s.scope.toLowerCase();
-    return formatScope(label, s.path);
-  };
-
-  const totalAffectedScopes = [
-    ...result.modifiedScopes,
-    ...result.alreadyInStateScopes,
-  ];
-
-  if (totalAffectedScopes.length === 2) {
-    const s1 = formatScopeItem(totalAffectedScopes[0]);
-    const s2 = formatScopeItem(totalAffectedScopes[1]);
-
-    if (isEnable) {
-      return `Skill "${skillName}" ${actionVerb} ${preposition} ${s1} and ${s2} settings.`;
-    } else {
-      return `Skill "${skillName}" is now disabled in both ${s1} and ${s2} settings.`;
-    }
-  }
-
-  const s = formatScopeItem(totalAffectedScopes[0]);
-  return `Skill "${skillName}" ${actionVerb} ${preposition} ${s} settings.`;
-}
-
-/**
- * Central logic for installing a skill from a remote URL or local path.
- */
-export async function installSkill(
-  source: string,
-  scope: 'user' | 'workspace',
-  subpath: string | undefined,
-  onLog: (msg: string) => void,
-  requestConsent: (
-    skills: SkillDefinition[],
-    targetDir: string,
-  ) => Promise<boolean> = () => Promise.resolve(true),
-): Promise<Array<{ name: string; location: string }>> {
-  let sourcePath = source;
-  let tempDirToClean: string | undefined = undefined;
-
-  const isGitUrl =
-    source.startsWith('git@') ||
-    source.startsWith('http://') ||
-    source.startsWith('https://');
-
-  const isSkillFile = source.toLowerCase().endsWith('.skill');
-
-  try {
-    if (isGitUrl) {
-      tempDirToClean = await fs.mkdtemp(
-        path.join(os.tmpdir(), 'gemini-skill-'),
-      );
-      sourcePath = tempDirToClean;
-
-      onLog(`Cloning skill from ${source}...`);
-      // Reuse existing robust git cloning utility from extension manager.
-      await cloneFromGit(
-        {
-          source,
-          type: 'git',
-        },
-        tempDirToClean,
-      );
-    } else if (isSkillFile) {
-      tempDirToClean = await fs.mkdtemp(
-        path.join(os.tmpdir(), 'gemini-skill-'),
-      );
-      sourcePath = tempDirToClean;
-
-      onLog(`Extracting skill from ${source}...`);
-      await extract(path.resolve(source), { dir: tempDirToClean });
-    }
-
-    // If a subpath is provided, resolve it against the cloned/local root.
-    if (subpath) {
-      sourcePath = path.join(sourcePath, subpath);
-    }
-
-    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.');
-    }
-
-    onLog(`Searching for skills in ${sourcePath}...`);
-    const skills = await loadSkillsFromDir(sourcePath);
-
-    if (skills.length === 0) {
-      throw new Error(
-        `No valid skills found in ${source}${subpath ? ` at path "${subpath}"` : ''}. Ensure a SKILL.md file exists with valid frontmatter.`,
-      );
-    }
-
-    const workspaceDir = process.cwd();
-    const storage = new Storage(workspaceDir);
-    const targetDir =
-      scope === 'workspace'
-        ? storage.getProjectSkillsDir()
-        : Storage.getUserSkillsDir();
-
-    if (!(await requestConsent(skills, targetDir))) {
-      throw new Error('Skill installation cancelled by user.');
-    }
-
-    await fs.mkdir(targetDir, { 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 exists = await fs.stat(destPath).catch(() => null);
-      if (exists) {
-        onLog(`Skill "${skillName}" already exists. Overwriting...`);
-        await fs.rm(destPath, { recursive: true, force: true });
-      }
-
-      await fs.cp(skillDir, destPath, { recursive: true });
-      installedSkills.push({ name: skillName, location: destPath });
-    }
-
-    return installedSkills;
-  } finally {
-    if (tempDirToClean) {
-      await fs.rm(tempDirToClean, { recursive: true, force: true });
-    }
-  }
-}
-
-/**
- * Central logic for linking a skill from a local path via symlink.
- */
-export async function linkSkill(
-  source: string,
-  scope: 'user' | 'workspace',
-  onLog: (msg: string) => void,
-  requestConsent: (
-    skills: SkillDefinition[],
-    targetDir: string,
-  ) => Promise<boolean> = () => Promise.resolve(true),
-): Promise<Array<{ name: string; location: string }>> {
-  const sourcePath = path.resolve(source);
-
-  onLog(`Searching for skills in ${sourcePath}...`);
-  const skills = await loadSkillsFromDir(sourcePath);
-
-  if (skills.length === 0) {
-    throw new Error(
-      `No valid skills found in "${sourcePath}". Ensure a SKILL.md file exists with valid frontmatter.`,
-    );
-  }
-
-  // Check for internal name collisions
-  const seenNames = new Map<string, string>();
-  for (const skill of skills) {
-    if (seenNames.has(skill.name)) {
-      throw new Error(
-        `Duplicate skill name "${skill.name}" found at multiple locations:\n  - ${seenNames.get(skill.name)}\n  - ${skill.location}`,
-      );
-    }
-    seenNames.set(skill.name, skill.location);
-  }
-
-  const workspaceDir = process.cwd();
-  const storage = new Storage(workspaceDir);
-  const targetDir =
-    scope === 'workspace'
-      ? storage.getProjectSkillsDir()
-      : Storage.getUserSkillsDir();
-
-  if (!(await requestConsent(skills, targetDir))) {
-    throw new Error('Skill linking cancelled by user.');
-  }
-
-  await fs.mkdir(targetDir, { 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 exists = await fs.lstat(destPath).catch(() => null);
-    if (exists) {
-      onLog(
-        `Skill "${skillName}" already exists at destination. Overwriting...`,
-      );
-      await fs.rm(destPath, { recursive: true, force: true });
-    }
-
-    // Use 'junction' on Windows to avoid EPERM errors — junctions don't
-    // require elevated privileges or Developer Mode (fixes #24816)
-    await fs.symlink(
-      skillSourceDir,
-      destPath,
-      process.platform === 'win32' ? 'junction' : 'dir',
-    );
-    linkedSkills.push({ name: skillName, location: destPath });
-  }
-
-  return linkedSkills;
-}
-
-/**
- * Central logic for uninstalling a skill by name.
- */
-export async function uninstallSkill(
-  name: string,
-  scope: 'user' | 'workspace',
-): Promise<{ location: string } | null> {
-  const workspaceDir = process.cwd();
-  const storage = new Storage(workspaceDir);
-  const targetDir =
-    scope === 'workspace'
-      ? storage.getProjectSkillsDir()
-      : Storage.getUserSkillsDir();
-
-  // Load all skills in the target directory to find the one with the matching name
-  const discoveredSkills = await loadSkillsFromDir(targetDir);
-  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);
-
-    // Security check: ensure the resolved path is within the target directory to prevent path traversal
-    if (!skillPath.startsWith(path.resolve(targetDir))) {
-      return null;
-    }
-
-    const exists = await fs.lstat(skillPath).catch(() => null);
-
-    if (!exists) {
-      return null;
-    }
-
-    await fs.rm(skillPath, { recursive: true, force: true });
-    return { location: skillPath };
-  }
-
-  const skillDir = path.dirname(skillToUninstall.location);
-  await fs.rm(skillDir, { recursive: true, force: true });
-  return { location: skillDir };
-}