From a81500a92979b25d4520781cf5f5002bfb2d243c Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Wed, 14 Jan 2026 17:47:02 -0800 Subject: [PATCH] feat(cli): add security consent prompts for skill installation (#16549) --- .../cli/src/commands/skills/install.test.ts | 59 ++++++- packages/cli/src/commands/skills/install.ts | 31 +++- packages/cli/src/config/extension.test.ts | 5 +- .../cli/src/config/extensions/consent.test.ts | 52 +++++- packages/cli/src/config/extensions/consent.ts | 64 +++++-- packages/cli/src/utils/skillUtils.test.ts | 29 ++++ packages/cli/src/utils/skillUtils.ts | 157 ++++++++++-------- 7 files changed, 296 insertions(+), 101 deletions(-) diff --git a/packages/cli/src/commands/skills/install.test.ts b/packages/cli/src/commands/skills/install.test.ts index e0621c8028..9fd05affcd 100644 --- a/packages/cli/src/commands/skills/install.test.ts +++ b/packages/cli/src/commands/skills/install.test.ts @@ -7,11 +7,18 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const mockInstallSkill = vi.hoisted(() => vi.fn()); +const mockRequestConsentNonInteractive = vi.hoisted(() => vi.fn()); +const mockSkillsConsentString = vi.hoisted(() => vi.fn()); vi.mock('../../utils/skillUtils.js', () => ({ installSkill: mockInstallSkill, })); +vi.mock('../../config/extensions/consent.js', () => ({ + requestConsentNonInteractive: mockRequestConsentNonInteractive, + skillsConsentString: mockSkillsConsentString, +})); + vi.mock('@google/gemini-cli-core', () => ({ debugLogger: { log: vi.fn(), error: vi.fn() }, })); @@ -23,6 +30,8 @@ describe('skill install command', () => { beforeEach(() => { vi.clearAllMocks(); vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); + mockSkillsConsentString.mockResolvedValue('Mock Consent String'); + mockRequestConsentNonInteractive.mockResolvedValue(true); }); describe('installCommand', () => { @@ -37,9 +46,10 @@ describe('skill install command', () => { }); it('should call installSkill with correct arguments for user scope', async () => { - mockInstallSkill.mockResolvedValue([ - { name: 'test-skill', location: '/mock/user/skills/test-skill' }, - ]); + mockInstallSkill.mockImplementation(async (_s, _sc, _p, _ol, rc) => { + await rc([]); + return [{ name: 'test-skill', location: '/mock/user/skills/test-skill' }]; + }); await handleInstall({ source: 'https://example.com/repo.git', @@ -51,6 +61,7 @@ describe('skill install command', () => { 'user', undefined, expect.any(Function), + expect.any(Function), ); expect(debugLogger.log).toHaveBeenCalledWith( expect.stringContaining('Successfully installed skill: test-skill'), @@ -58,6 +69,47 @@ describe('skill install command', () => { expect(debugLogger.log).toHaveBeenCalledWith( expect.stringContaining('location: /mock/user/skills/test-skill'), ); + expect(mockRequestConsentNonInteractive).toHaveBeenCalledWith( + 'Mock Consent String', + ); + }); + + it('should skip prompt and log consent when --consent is provided', async () => { + mockInstallSkill.mockImplementation(async (_s, _sc, _p, _ol, rc) => { + await rc([]); + return [{ name: 'test-skill', location: '/mock/user/skills/test-skill' }]; + }); + + await handleInstall({ + source: 'https://example.com/repo.git', + consent: true, + }); + + expect(mockRequestConsentNonInteractive).not.toHaveBeenCalled(); + expect(debugLogger.log).toHaveBeenCalledWith( + 'You have consented to the following:', + ); + expect(debugLogger.log).toHaveBeenCalledWith('Mock Consent String'); + expect(mockInstallSkill).toHaveBeenCalled(); + }); + + it('should abort installation if consent is denied', async () => { + mockRequestConsentNonInteractive.mockResolvedValue(false); + mockInstallSkill.mockImplementation(async (_s, _sc, _p, _ol, rc) => { + if (!(await rc([]))) { + throw new Error('Skill installation cancelled by user.'); + } + return []; + }); + + await handleInstall({ + source: 'https://example.com/repo.git', + }); + + expect(debugLogger.error).toHaveBeenCalledWith( + 'Skill installation cancelled by user.', + ); + expect(process.exit).toHaveBeenCalledWith(1); }); it('should call installSkill with correct arguments for workspace scope and subpath', async () => { @@ -76,6 +128,7 @@ describe('skill install command', () => { 'workspace', 'my-skills-dir', expect.any(Function), + expect.any(Function), ); }); diff --git a/packages/cli/src/commands/skills/install.ts b/packages/cli/src/commands/skills/install.ts index bdf8402de7..f0701d39b6 100644 --- a/packages/cli/src/commands/skills/install.ts +++ b/packages/cli/src/commands/skills/install.ts @@ -5,24 +5,43 @@ */ import type { CommandModule } from 'yargs'; -import { debugLogger } from '@google/gemini-cli-core'; +import { debugLogger, type SkillDefinition } from '@google/gemini-cli-core'; import { getErrorMessage } from '../../utils/errors.js'; import { exitCli } from '../utils.js'; import { installSkill } from '../../utils/skillUtils.js'; import chalk from 'chalk'; +import { + requestConsentNonInteractive, + skillsConsentString, +} from '../../config/extensions/consent.js'; interface InstallArgs { source: string; scope?: 'user' | 'workspace'; path?: string; + consent?: boolean; } export async function handleInstall(args: InstallArgs) { try { - const { source } = args; + const { source, consent } = args; const scope = args.scope ?? 'user'; const subpath = args.path; + const requestConsent = async ( + skills: SkillDefinition[], + targetDir: string, + ) => { + if (consent) { + debugLogger.log('You have consented to the following:'); + debugLogger.log(await skillsConsentString(skills, source, targetDir)); + return true; + } + return requestConsentNonInteractive( + await skillsConsentString(skills, source, targetDir), + ); + }; + const installedSkills = await installSkill( source, scope, @@ -30,6 +49,7 @@ export async function handleInstall(args: InstallArgs) { (msg) => { debugLogger.log(msg); }, + requestConsent, ); for (const skill of installedSkills) { @@ -68,6 +88,12 @@ export const installCommand: CommandModule = { 'Sub-path within the repository to install from (only used for git repository sources).', type: 'string', }) + .option('consent', { + describe: + 'Acknowledge the security risks of installing a skill and skip the confirmation prompt.', + type: 'boolean', + default: false, + }) .check((argv) => { if (!argv.source) { throw new Error('The source argument must be provided.'); @@ -79,6 +105,7 @@ export const installCommand: CommandModule = { source: argv['source'] as string, scope: argv['scope'] as 'user' | 'workspace', path: argv['path'] as string | undefined, + consent: argv['consent'] as boolean | undefined, }); await exitCli(); }, diff --git a/packages/cli/src/config/extension.test.ts b/packages/cli/src/config/extension.test.ts index 6619befc66..a3230058f7 100644 --- a/packages/cli/src/config/extension.test.ts +++ b/packages/cli/src/config/extension.test.ts @@ -1298,10 +1298,11 @@ describe('extension tests', () => { expect(mockRequestConsent).toHaveBeenCalledWith( `Installing extension "my-local-extension". -${INSTALL_WARNING_MESSAGE} This extension will run the following MCP servers: * test-server (local): node dobadthing \\u001b[12D\\u001b[K server.js - * test-server-2 (remote): https://google.com`, + * test-server-2 (remote): https://google.com + +${INSTALL_WARNING_MESSAGE}`, ); }); diff --git a/packages/cli/src/config/extensions/consent.test.ts b/packages/cli/src/config/extensions/consent.test.ts index ccb569f43e..4180a72b16 100644 --- a/packages/cli/src/config/extensions/consent.test.ts +++ b/packages/cli/src/config/extensions/consent.test.ts @@ -191,12 +191,13 @@ describe('consent', () => { const expectedConsentString = [ 'Installing extension "test-ext".', - INSTALL_WARNING_MESSAGE, 'This extension will run the following MCP servers:', ' * server1 (local): npm start', ' * server2 (remote): https://remote.com', 'This extension will append info to your gemini.md context using my-context.md', 'This extension will exclude the following core tools: tool1,tool2', + '', + INSTALL_WARNING_MESSAGE, ].join('\n'); expect(requestConsent).toHaveBeenCalledWith(expectedConsentString); @@ -324,7 +325,6 @@ describe('consent', () => { const expectedConsentString = [ 'Installing extension "test-ext".', - INSTALL_WARNING_MESSAGE, 'This extension will run the following MCP servers:', ' * server1 (local): npm start', ' * server2 (remote): https://remote.com', @@ -332,13 +332,17 @@ describe('consent', () => { 'This extension will exclude the following core tools: tool1,tool2', '', chalk.bold('Agent Skills:'), - SKILLS_WARNING_MESSAGE, - 'This extension will install the following agent skills:', + '\nThis extension will install the following agent skills:\n', ` * ${chalk.bold('skill1')}: desc1`, - ` (Location: ${skill1.location}) (2 items in directory)`, - ` * ${chalk.bold('skill2')}: desc2`, - ` (Location: ${skill2.location}) (1 items in directory)`, + chalk.dim(` (Source: ${skill1.location}) (2 items in directory)`), '', + ` * ${chalk.bold('skill2')}: desc2`, + chalk.dim(` (Source: ${skill2.location}) (1 items in directory)`), + '', + '', + INSTALL_WARNING_MESSAGE, + '', + SKILLS_WARNING_MESSAGE, ].join('\n'); expect(requestConsent).toHaveBeenCalledWith(expectedConsentString); @@ -375,10 +379,42 @@ describe('consent', () => { expect(requestConsent).toHaveBeenCalledWith( expect.stringContaining( - ` (Location: ${skill.location}) ${chalk.red('⚠️ (Could not count items in directory)')}`, + ` (Source: ${skill.location}) ${chalk.red('⚠️ (Could not count items in directory)')}`, ), ); }); }); }); + + describe('skillsConsentString', () => { + it('should generate a consent string for skills', async () => { + const skill1Dir = path.join(tempDir, 'skill1'); + await fs.mkdir(skill1Dir, { recursive: true }); + await fs.writeFile(path.join(skill1Dir, 'SKILL.md'), 'body1'); + + const skill1: SkillDefinition = { + name: 'skill1', + description: 'desc1', + location: path.join(skill1Dir, 'SKILL.md'), + body: 'body1', + }; + + const { skillsConsentString } = await import('./consent.js'); + const consentString = await skillsConsentString( + [skill1], + 'https://example.com/repo.git', + '/mock/target/dir', + ); + + expect(consentString).toContain( + 'Installing agent skill(s) from "https://example.com/repo.git".', + ); + expect(consentString).toContain('Install Destination: /mock/target/dir'); + expect(consentString).toContain('\n' + SKILLS_WARNING_MESSAGE); + expect(consentString).toContain(` * ${chalk.bold('skill1')}: desc1`); + expect(consentString).toContain( + chalk.dim(`(Source: ${skill1.location}) (1 items in directory)`), + ); + }); + }); }); diff --git a/packages/cli/src/config/extensions/consent.ts b/packages/cli/src/config/extensions/consent.ts index 47391fd9e6..27b8e9a904 100644 --- a/packages/cli/src/config/extensions/consent.ts +++ b/packages/cli/src/config/extensions/consent.ts @@ -21,6 +21,27 @@ export const SKILLS_WARNING_MESSAGE = chalk.yellow( "Agent skills inject specialized instructions and domain-specific knowledge into the agent's system prompt. This can change how the agent interprets your requests and interacts with your environment. Review the skill definitions at the location(s) provided below to ensure they meet your security standards.", ); +/** + * Builds a consent string for installing agent skills. + */ +export async function skillsConsentString( + skills: SkillDefinition[], + source: string, + targetDir?: string, +): Promise { + const output: string[] = []; + output.push(`Installing agent skill(s) from "${source}".`); + output.push('\nThe following agent skill(s) will be installed:\n'); + output.push(...(await renderSkillsList(skills))); + + if (targetDir) { + output.push(`Install Destination: ${targetDir}`); + } + output.push('\n' + SKILLS_WARNING_MESSAGE); + + return output.join('\n'); +} + /** * Requests consent from the user to perform an action, by reading a Y/n * character from stdin. @@ -120,7 +141,6 @@ async function extensionConsentString( const output: string[] = []; const mcpServerEntries = Object.entries(sanitizedConfig.mcpServers || {}); output.push(`Installing extension "${sanitizedConfig.name}".`); - output.push(INSTALL_WARNING_MESSAGE); if (mcpServerEntries.length) { output.push('This extension will run the following MCP servers:'); @@ -149,23 +169,37 @@ async function extensionConsentString( } if (skills.length > 0) { output.push(`\n${chalk.bold('Agent Skills:')}`); - output.push(SKILLS_WARNING_MESSAGE); - output.push('This extension will install the following agent skills:'); - for (const skill of skills) { - output.push(` * ${chalk.bold(skill.name)}: ${skill.description}`); - const skillDir = path.dirname(skill.location); - let fileCountStr = ''; - try { - const skillDirItems = await fs.readdir(skillDir); - fileCountStr = ` (${skillDirItems.length} items in directory)`; - } catch { - fileCountStr = ` ${chalk.red('⚠️ (Could not count items in directory)')}`; - } - output.push(` (Location: ${skill.location})${fileCountStr}`); + output.push('\nThis extension will install the following agent skills:\n'); + output.push(...(await renderSkillsList(skills))); + } + + output.push('\n' + INSTALL_WARNING_MESSAGE); + if (skills.length > 0) { + output.push('\n' + SKILLS_WARNING_MESSAGE); + } + + return output.join('\n'); +} + +/** + * Shared logic for formatting a list of agent skills for a consent prompt. + */ +async function renderSkillsList(skills: SkillDefinition[]): Promise { + const output: string[] = []; + for (const skill of skills) { + output.push(` * ${chalk.bold(skill.name)}: ${skill.description}`); + const skillDir = path.dirname(skill.location); + let fileCountStr = ''; + try { + const skillDirItems = await fs.readdir(skillDir); + fileCountStr = ` (${skillDirItems.length} items in directory)`; + } catch { + fileCountStr = ` ${chalk.red('⚠️ (Could not count items in directory)')}`; } + output.push(chalk.dim(` (Source: ${skill.location})${fileCountStr}`)); output.push(''); } - return output.join('\n'); + return output; } /** diff --git a/packages/cli/src/utils/skillUtils.test.ts b/packages/cli/src/utils/skillUtils.test.ts index 9dfe8560a6..5f98471112 100644 --- a/packages/cli/src/utils/skillUtils.test.ts +++ b/packages/cli/src/utils/skillUtils.test.ts @@ -78,4 +78,33 @@ describe('skillUtils', () => { const installedExists = await fs.stat(installedPath).catch(() => null); expect(installedExists?.isDirectory()).toBe(true); }); + + it('should abort installation if consent is rejected', async () => { + const mockSkillDir = path.join(tempDir, 'mock-skill-source'); + const skillSubDir = path.join(mockSkillDir, 'test-skill'); + await fs.mkdir(skillSubDir, { recursive: true }); + await fs.writeFile( + path.join(skillSubDir, 'SKILL.md'), + '---\nname: test-skill\ndescription: test\n---\nbody', + ); + + const requestConsent = vi.fn().mockResolvedValue(false); + + await expect( + installSkill( + mockSkillDir, + 'workspace', + undefined, + () => {}, + requestConsent, + ), + ).rejects.toThrow('Skill installation cancelled by user.'); + + expect(requestConsent).toHaveBeenCalled(); + + // Verify it was NOT copied + const installedPath = path.join(tempDir, '.gemini/skills', 'test-skill'); + const installedExists = await fs.stat(installedPath).catch(() => null); + expect(installedExists).toBeNull(); + }); }); diff --git a/packages/cli/src/utils/skillUtils.ts b/packages/cli/src/utils/skillUtils.ts index 75acbae87d..43cae2733c 100644 --- a/packages/cli/src/utils/skillUtils.ts +++ b/packages/cli/src/utils/skillUtils.ts @@ -6,7 +6,11 @@ import { SettingScope } from '../config/settings.js'; import type { SkillActionResult } from './skillSettings.js'; -import { Storage, loadSkillsFromDir } from '@google/gemini-cli-core'; +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'; @@ -79,6 +83,10 @@ export async function installSkill( scope: 'user' | 'workspace', subpath: string | undefined, onLog: (msg: string) => void, + requestConsent: ( + skills: SkillDefinition[], + targetDir: string, + ) => Promise = () => Promise.resolve(true), ): Promise> { let sourcePath = source; let tempDirToClean: string | undefined = undefined; @@ -90,85 +98,92 @@ export async function installSkill( const isSkillFile = source.toLowerCase().endsWith('.skill'); - if (isGitUrl) { - tempDirToClean = await fs.mkdtemp(path.join(os.tmpdir(), 'gemini-skill-')); - sourcePath = tempDirToClean; + 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(`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 }); - } + 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); - } + // If a subpath is provided, resolve it against the cloned/local root. + if (subpath) { + sourcePath = path.join(sourcePath, subpath); + } - sourcePath = path.resolve(sourcePath); + sourcePath = path.resolve(sourcePath); - // Quick security check to prevent directory traversal out of temp dir when cloning - if (tempDirToClean && !sourcePath.startsWith(path.resolve(tempDirToClean))) { + // 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 }); } - throw new Error('Invalid path: Directory traversal not allowed.'); } - - onLog(`Searching for skills in ${sourcePath}...`); - const skills = await loadSkillsFromDir(sourcePath); - - if (skills.length === 0) { - if (tempDirToClean) { - await fs.rm(tempDirToClean, { recursive: true, force: true }); - } - 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(); - - 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 }); - } - - if (tempDirToClean) { - await fs.rm(tempDirToClean, { recursive: true, force: true }); - } - - return installedSkills; } /**