diff --git a/docs/extensions/index.md b/docs/extensions/index.md index b17f999863f..9d4d6c63cce 100644 --- a/docs/extensions/index.md +++ b/docs/extensions/index.md @@ -232,9 +232,12 @@ gemini extensions settings list and you can update a given setting using: ``` -gemini extensions settings set +gemini extensions settings set [--scope ] ``` +- `--scope`: The scope to set the setting in (`user` or `workspace`). This is + optional and will default to `user`. + ### Custom commands Extensions can provide [custom commands](../cli/custom-commands.md) by placing diff --git a/packages/cli/src/commands/extensions/settings.ts b/packages/cli/src/commands/extensions/settings.ts index 7006dd76f59..e695054c11d 100644 --- a/packages/cli/src/commands/extensions/settings.ts +++ b/packages/cli/src/commands/extensions/settings.ts @@ -6,9 +6,10 @@ import type { CommandModule } from 'yargs'; import { - getEnvContents, updateSetting, promptForSetting, + ExtensionSettingScope, + getScopedEnvContents, } from '../../config/extensions/extensionSettings.js'; import { getExtensionAndManager } from './utils.js'; import { debugLogger } from '@google/gemini-cli-core'; @@ -18,10 +19,11 @@ import { exitCli } from '../utils.js'; interface SetArgs { name: string; setting: string; + scope: string; } const setCommand: CommandModule = { - command: 'set ', + command: 'set [--scope] ', describe: 'Set a specific setting for an extension.', builder: (yargs) => yargs @@ -34,9 +36,15 @@ const setCommand: CommandModule = { describe: 'The setting to configure (name or env var).', type: 'string', demandOption: true, + }) + .option('scope', { + describe: 'The scope to set the setting in.', + type: 'string', + choices: ['user', 'workspace'], + default: 'user', }), handler: async (args) => { - const { name, setting } = args; + const { name, setting, scope } = args; const { extension, extensionManager } = await getExtensionAndManager(name); if (!extension || !extensionManager) { return; @@ -55,6 +63,7 @@ const setCommand: CommandModule = { extension.id, setting, promptForSetting, + scope as ExtensionSettingScope, ); await exitCli(); }, @@ -92,12 +101,30 @@ const listCommand: CommandModule = { return; } - const currentSettings = await getEnvContents(extensionConfig, extension.id); + const userSettings = await getScopedEnvContents( + extensionConfig, + extension.id, + ExtensionSettingScope.USER, + ); + const workspaceSettings = await getScopedEnvContents( + extensionConfig, + extension.id, + ExtensionSettingScope.WORKSPACE, + ); + const mergedSettings = { ...userSettings, ...workspaceSettings }; debugLogger.log(`Settings for "${name}":`); for (const setting of extensionConfig.settings) { - const value = currentSettings[setting.envVar]; + const value = mergedSettings[setting.envVar]; let displayValue: string; + let scopeInfo = ''; + + if (workspaceSettings[setting.envVar] !== undefined) { + scopeInfo = ' (workspace)'; + } else if (userSettings[setting.envVar] !== undefined) { + scopeInfo = ' (user)'; + } + if (value === undefined) { displayValue = '[not set]'; } else if (setting.sensitive) { @@ -108,7 +135,7 @@ const listCommand: CommandModule = { debugLogger.log(` - ${setting.name} (${setting.envVar})`); debugLogger.log(` Description: ${setting.description}`); - debugLogger.log(` Value: ${displayValue}`); + debugLogger.log(` Value: ${displayValue}${scopeInfo}`); } await exitCli(); }, diff --git a/packages/cli/src/config/extensions/extensionSettings.test.ts b/packages/cli/src/config/extensions/extensionSettings.test.ts index 55af343c0fd..539577d915c 100644 --- a/packages/cli/src/config/extensions/extensionSettings.test.ts +++ b/packages/cli/src/config/extensions/extensionSettings.test.ts @@ -13,6 +13,8 @@ import { promptForSetting, type ExtensionSetting, updateSetting, + ExtensionSettingScope, + getScopedEnvContents, } from './extensionSettings.js'; import type { ExtensionConfig } from '../extension.js'; import { ExtensionStorage } from './storage.js'; @@ -20,6 +22,7 @@ import prompts from 'prompts'; import * as fsPromises from 'node:fs/promises'; import * as fs from 'node:fs'; import { KeychainTokenStorage } from '@google/gemini-cli-core'; +import { EXTENSION_SETTINGS_FILENAME } from './variables.js'; vi.mock('prompts'); vi.mock('os', async (importOriginal) => { @@ -35,67 +38,66 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { await importOriginal(); return { ...actual, - KeychainTokenStorage: vi.fn().mockImplementation(() => ({ - getSecret: vi.fn(), - setSecret: vi.fn(), - deleteSecret: vi.fn(), - listSecrets: vi.fn(), - isAvailable: vi.fn().mockResolvedValue(true), - })), + KeychainTokenStorage: vi.fn(), }; }); -interface MockKeychainStorage { - getSecret: ReturnType; - setSecret: ReturnType; - deleteSecret: ReturnType; - listSecrets: ReturnType; - isAvailable: ReturnType; -} - describe('extensionSettings', () => { let tempHomeDir: string; + let tempWorkspaceDir: string; let extensionDir: string; - let mockKeychainStorage: MockKeychainStorage; - let keychainData: Record; + let mockKeychainData: Record>; beforeEach(() => { vi.clearAllMocks(); - keychainData = {}; - mockKeychainStorage = { - getSecret: vi - .fn() - .mockImplementation(async (key: string) => keychainData[key] || null), - setSecret: vi - .fn() - .mockImplementation(async (key: string, value: string) => { - keychainData[key] = value; - }), - deleteSecret: vi.fn().mockImplementation(async (key: string) => { - delete keychainData[key]; - }), - listSecrets: vi - .fn() - .mockImplementation(async () => Object.keys(keychainData)), - isAvailable: vi.fn().mockResolvedValue(true), - }; - ( - KeychainTokenStorage as unknown as ReturnType - ).mockImplementation(() => mockKeychainStorage); - + mockKeychainData = {}; + vi.mocked(KeychainTokenStorage).mockImplementation( + (serviceName: string) => { + if (!mockKeychainData[serviceName]) { + mockKeychainData[serviceName] = {}; + } + const keychainData = mockKeychainData[serviceName]; + return { + getSecret: vi + .fn() + .mockImplementation( + async (key: string) => keychainData[key] || null, + ), + setSecret: vi + .fn() + .mockImplementation(async (key: string, value: string) => { + keychainData[key] = value; + }), + deleteSecret: vi.fn().mockImplementation(async (key: string) => { + delete keychainData[key]; + }), + listSecrets: vi + .fn() + .mockImplementation(async () => Object.keys(keychainData)), + isAvailable: vi.fn().mockResolvedValue(true), + } as unknown as KeychainTokenStorage; + }, + ); tempHomeDir = os.tmpdir() + path.sep + `gemini-cli-test-home-${Date.now()}`; + tempWorkspaceDir = path.join( + os.tmpdir(), + `gemini-cli-test-workspace-${Date.now()}`, + ); extensionDir = path.join(tempHomeDir, '.gemini', 'extensions', 'test-ext'); // Spy and mock the method, but also create the directory so we can write to it. vi.spyOn(ExtensionStorage.prototype, 'getExtensionDir').mockReturnValue( extensionDir, ); fs.mkdirSync(extensionDir, { recursive: true }); + fs.mkdirSync(tempWorkspaceDir, { recursive: true }); vi.mocked(os.homedir).mockReturnValue(tempHomeDir); + vi.spyOn(process, 'cwd').mockReturnValue(tempWorkspaceDir); vi.mocked(prompts).mockClear(); }); afterEach(() => { fs.rmSync(tempHomeDir, { recursive: true, force: true }); + fs.rmSync(tempWorkspaceDir, { recursive: true, force: true }); vi.restoreAllMocks(); }); @@ -213,7 +215,10 @@ describe('extensionSettings', () => { VAR1: 'previous-VAR1', SENSITIVE_VAR: 'secret', }; - keychainData['SENSITIVE_VAR'] = 'secret'; + const userKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext 12345`, + ); + await userKeychain.setSecret('SENSITIVE_VAR', 'secret'); const envPath = path.join(extensionDir, '.env'); await fsPromises.writeFile(envPath, 'VAR1=previous-VAR1'); @@ -228,9 +233,7 @@ describe('extensionSettings', () => { expect(mockRequestSetting).not.toHaveBeenCalled(); const actualContent = await fsPromises.readFile(envPath, 'utf-8'); expect(actualContent).toBe(''); - expect(mockKeychainStorage.deleteSecret).toHaveBeenCalledWith( - 'SENSITIVE_VAR', - ); + expect(await userKeychain.getSecret('SENSITIVE_VAR')).toBeNull(); }); it('should remove sensitive settings from keychain', async () => { @@ -252,7 +255,10 @@ describe('extensionSettings', () => { settings: [], }; const previousSettings = { SENSITIVE_VAR: 'secret' }; - keychainData['SENSITIVE_VAR'] = 'secret'; + const userKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext 12345`, + ); + await userKeychain.setSecret('SENSITIVE_VAR', 'secret'); await maybePromptForSettings( newConfig, @@ -262,9 +268,7 @@ describe('extensionSettings', () => { previousSettings, ); - expect(mockKeychainStorage.deleteSecret).toHaveBeenCalledWith( - 'SENSITIVE_VAR', - ); + expect(await userKeychain.getSecret('SENSITIVE_VAR')).toBeNull(); }); it('should remove settings that are no longer in the config', async () => { @@ -455,7 +459,7 @@ describe('extensionSettings', () => { }); }); - describe('getEnvContents', () => { + describe('getScopedEnvContents', () => { const config: ExtensionConfig = { name: 'test-ext', version: '1.0.0', @@ -469,39 +473,94 @@ describe('extensionSettings', () => { }, ], }; + const extensionId = '12345'; - it('should return combined contents from .env and keychain', async () => { - const envPath = path.join(extensionDir, '.env'); - await fsPromises.writeFile(envPath, 'VAR1=value1'); - keychainData['SENSITIVE_VAR'] = 'secret'; + it('should return combined contents from user .env and keychain for USER scope', async () => { + const userEnvPath = path.join(extensionDir, EXTENSION_SETTINGS_FILENAME); + await fsPromises.writeFile(userEnvPath, 'VAR1=user-value1'); + const userKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext 12345`, + ); + await userKeychain.setSecret('SENSITIVE_VAR', 'user-secret'); - const contents = await getEnvContents(config, '12345'); + const contents = await getScopedEnvContents( + config, + extensionId, + ExtensionSettingScope.USER, + ); expect(contents).toEqual({ - VAR1: 'value1', - SENSITIVE_VAR: 'secret', + VAR1: 'user-value1', + SENSITIVE_VAR: 'user-secret', }); }); - it('should return an empty object if no settings are defined', async () => { - const contents = await getEnvContents( - { name: 'test-ext', version: '1.0.0' }, - '12345', + it('should return combined contents from workspace .env and keychain for WORKSPACE scope', async () => { + const workspaceEnvPath = path.join( + tempWorkspaceDir, + EXTENSION_SETTINGS_FILENAME, ); - expect(contents).toEqual({}); - }); + await fsPromises.writeFile(workspaceEnvPath, 'VAR1=workspace-value1'); + const workspaceKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext 12345 ${tempWorkspaceDir}`, + ); + await workspaceKeychain.setSecret('SENSITIVE_VAR', 'workspace-secret'); - it('should return only keychain contents if .env file does not exist', async () => { - keychainData['SENSITIVE_VAR'] = 'secret'; - const contents = await getEnvContents(config, '12345'); - expect(contents).toEqual({ SENSITIVE_VAR: 'secret' }); + const contents = await getScopedEnvContents( + config, + extensionId, + ExtensionSettingScope.WORKSPACE, + ); + + expect(contents).toEqual({ + VAR1: 'workspace-value1', + SENSITIVE_VAR: 'workspace-secret', + }); }); + }); - it('should return only .env contents if keychain is empty', async () => { - const envPath = path.join(extensionDir, '.env'); - await fsPromises.writeFile(envPath, 'VAR1=value1'); - const contents = await getEnvContents(config, '12345'); - expect(contents).toEqual({ VAR1: 'value1' }); + describe('getEnvContents (merged)', () => { + const config: ExtensionConfig = { + name: 'test-ext', + version: '1.0.0', + settings: [ + { name: 's1', description: 'd1', envVar: 'VAR1' }, + { name: 's2', description: 'd2', envVar: 'VAR2', sensitive: true }, + { name: 's3', description: 'd3', envVar: 'VAR3' }, + ], + }; + const extensionId = '12345'; + + it('should merge user and workspace settings, with workspace taking precedence', async () => { + // User settings + const userEnvPath = path.join(extensionDir, EXTENSION_SETTINGS_FILENAME); + await fsPromises.writeFile( + userEnvPath, + 'VAR1=user-value1\nVAR3=user-value3', + ); + const userKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext ${extensionId}`, + ); + await userKeychain.setSecret('VAR2', 'user-secret2'); + + // Workspace settings + const workspaceEnvPath = path.join( + tempWorkspaceDir, + EXTENSION_SETTINGS_FILENAME, + ); + await fsPromises.writeFile(workspaceEnvPath, 'VAR1=workspace-value1'); + const workspaceKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext ${extensionId} ${tempWorkspaceDir}`, + ); + await workspaceKeychain.setSecret('VAR2', 'workspace-secret2'); + + const contents = await getEnvContents(config, extensionId); + + expect(contents).toEqual({ + VAR1: 'workspace-value1', + VAR2: 'workspace-secret2', + VAR3: 'user-value3', + }); }); }); @@ -517,87 +576,114 @@ describe('extensionSettings', () => { const mockRequestSetting = vi.fn(); beforeEach(async () => { - // Pre-populate settings - const envContent = 'VAR1=value1\n'; - const envPath = path.join(extensionDir, '.env'); - await fsPromises.writeFile(envPath, envContent); - keychainData['VAR2'] = 'value2'; + const userEnvPath = path.join(extensionDir, '.env'); + await fsPromises.writeFile(userEnvPath, 'VAR1=value1\n'); + const userKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext 12345`, + ); + await userKeychain.setSecret('VAR2', 'value2'); mockRequestSetting.mockClear(); }); - it('should update a non-sensitive setting', async () => { + it('should update a non-sensitive setting in USER scope', async () => { mockRequestSetting.mockResolvedValue('new-value1'); - await updateSetting(config, '12345', 'VAR1', mockRequestSetting); + await updateSetting( + config, + '12345', + 'VAR1', + mockRequestSetting, + ExtensionSettingScope.USER, + ); - expect(mockRequestSetting).toHaveBeenCalledWith(config.settings![0]); const expectedEnvPath = path.join(extensionDir, '.env'); const actualContent = await fsPromises.readFile(expectedEnvPath, 'utf-8'); expect(actualContent).toContain('VAR1=new-value1'); - expect(mockKeychainStorage.setSecret).not.toHaveBeenCalled(); }); - it('should update a non-sensitive setting by name', async () => { - mockRequestSetting.mockResolvedValue('new-value-by-name'); + it('should update a non-sensitive setting in WORKSPACE scope', async () => { + mockRequestSetting.mockResolvedValue('new-workspace-value'); - await updateSetting(config, '12345', 's1', mockRequestSetting); + await updateSetting( + config, + '12345', + 'VAR1', + mockRequestSetting, + ExtensionSettingScope.WORKSPACE, + ); - expect(mockRequestSetting).toHaveBeenCalledWith(config.settings![0]); - const expectedEnvPath = path.join(extensionDir, '.env'); + const expectedEnvPath = path.join(tempWorkspaceDir, '.env'); const actualContent = await fsPromises.readFile(expectedEnvPath, 'utf-8'); - expect(actualContent).toContain('VAR1=new-value-by-name'); - expect(mockKeychainStorage.setSecret).not.toHaveBeenCalled(); + expect(actualContent).toContain('VAR1=new-workspace-value'); }); - it('should update a sensitive setting', async () => { + it('should update a sensitive setting in USER scope', async () => { mockRequestSetting.mockResolvedValue('new-value2'); - await updateSetting(config, '12345', 'VAR2', mockRequestSetting); - - expect(mockRequestSetting).toHaveBeenCalledWith(config.settings![1]); - expect(mockKeychainStorage.setSecret).toHaveBeenCalledWith( + await updateSetting( + config, + '12345', 'VAR2', - 'new-value2', + mockRequestSetting, + ExtensionSettingScope.USER, ); - const expectedEnvPath = path.join(extensionDir, '.env'); - const actualContent = await fsPromises.readFile(expectedEnvPath, 'utf-8'); - expect(actualContent).not.toContain('VAR2=new-value2'); - }); - it('should update a sensitive setting by name', async () => { - mockRequestSetting.mockResolvedValue('new-sensitive-by-name'); + const userKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext 12345`, + ); + expect(await userKeychain.getSecret('VAR2')).toBe('new-value2'); + }); - await updateSetting(config, '12345', 's2', mockRequestSetting); + it('should update a sensitive setting in WORKSPACE scope', async () => { + mockRequestSetting.mockResolvedValue('new-workspace-secret'); - expect(mockRequestSetting).toHaveBeenCalledWith(config.settings![1]); - expect(mockKeychainStorage.setSecret).toHaveBeenCalledWith( + await updateSetting( + config, + '12345', 'VAR2', - 'new-sensitive-by-name', + mockRequestSetting, + ExtensionSettingScope.WORKSPACE, ); - }); - it('should do nothing if the setting does not exist', async () => { - await updateSetting(config, '12345', 'VAR3', mockRequestSetting); - expect(mockRequestSetting).not.toHaveBeenCalled(); + const workspaceKeychain = new KeychainTokenStorage( + `Gemini CLI Extensions test-ext 12345 ${tempWorkspaceDir}`, + ); + expect(await workspaceKeychain.getSecret('VAR2')).toBe( + 'new-workspace-secret', + ); }); - it('should do nothing if there are no settings', async () => { - const emptyConfig: ExtensionConfig = { - name: 'test-ext', - version: '1.0.0', - }; - await updateSetting(emptyConfig, '12345', 'VAR1', mockRequestSetting); - expect(mockRequestSetting).not.toHaveBeenCalled(); - }); + it('should leave existing, unmanaged .env variables intact when updating in WORKSPACE scope', async () => { + // Setup a pre-existing .env file in the workspace with unmanaged variables + const workspaceEnvPath = path.join(tempWorkspaceDir, '.env'); + const originalEnvContent = + 'PROJECT_VAR_1=value_1\nPROJECT_VAR_2=value_2\nVAR1=original-value'; // VAR1 is managed by extension + await fsPromises.writeFile(workspaceEnvPath, originalEnvContent); - it('should wrap values with spaces in quotes', async () => { - mockRequestSetting.mockResolvedValue('a value with spaces'); + // Simulate updating an extension-managed non-sensitive setting + mockRequestSetting.mockResolvedValue('updated-value'); + await updateSetting( + config, + '12345', + 'VAR1', + mockRequestSetting, + ExtensionSettingScope.WORKSPACE, + ); - await updateSetting(config, '12345', 'VAR1', mockRequestSetting); + // Read the .env file after update + const actualContent = await fsPromises.readFile( + workspaceEnvPath, + 'utf-8', + ); - const expectedEnvPath = path.join(extensionDir, '.env'); - const actualContent = await fsPromises.readFile(expectedEnvPath, 'utf-8'); - expect(actualContent).toContain('VAR1="a value with spaces"'); + // Assert that original variables are intact and extension variable is updated + expect(actualContent).toContain('PROJECT_VAR_1=value_1'); + expect(actualContent).toContain('PROJECT_VAR_2=value_2'); + expect(actualContent).toContain('VAR1=updated-value'); + + // Ensure no other unexpected changes or deletions + const lines = actualContent.split('\n').filter((line) => line.length > 0); + expect(lines).toHaveLength(3); // Should only have the three variables }); }); }); diff --git a/packages/cli/src/config/extensions/extensionSettings.ts b/packages/cli/src/config/extensions/extensionSettings.ts index 8d61f377992..1c39ca048bd 100644 --- a/packages/cli/src/config/extensions/extensionSettings.ts +++ b/packages/cli/src/config/extensions/extensionSettings.ts @@ -7,12 +7,19 @@ import * as fs from 'node:fs/promises'; import * as fsSync from 'node:fs'; import * as dotenv from 'dotenv'; +import * as path from 'node:path'; import { ExtensionStorage } from './storage.js'; import type { ExtensionConfig } from '../extension.js'; import prompts from 'prompts'; import { debugLogger, KeychainTokenStorage } from '@google/gemini-cli-core'; +import { EXTENSION_SETTINGS_FILENAME } from './variables.js'; + +export enum ExtensionSettingScope { + USER = 'user', + WORKSPACE = 'workspace', +} export interface ExtensionSetting { name: string; @@ -25,7 +32,24 @@ export interface ExtensionSetting { const getKeychainStorageName = ( extensionName: string, extensionId: string, -): string => `Gemini CLI Extensions ${extensionName} ${extensionId}`; + scope: ExtensionSettingScope, +): string => { + const base = `Gemini CLI Extensions ${extensionName} ${extensionId}`; + if (scope === ExtensionSettingScope.WORKSPACE) { + return `${base} ${process.cwd()}`; + } + return base; +}; + +const getEnvFilePath = ( + extensionName: string, + scope: ExtensionSettingScope, +): string => { + if (scope === ExtensionSettingScope.WORKSPACE) { + return path.join(process.cwd(), EXTENSION_SETTINGS_FILENAME); + } + return new ExtensionStorage(extensionName).getEnvFilePath(); +}; export async function maybePromptForSettings( extensionConfig: ExtensionConfig, @@ -42,9 +66,12 @@ export async function maybePromptForSettings( ) { return; } - const envFilePath = new ExtensionStorage(extensionName).getEnvFilePath(); + // We assume user scope here because we don't have a way to ask the user for scope during the initial setup. + // The user can change the scope later using the `settings set` command. + const scope = ExtensionSettingScope.USER; + const envFilePath = getEnvFilePath(extensionName, scope); const keychain = new KeychainTokenStorage( - getKeychainStorageName(extensionName, extensionId), + getKeychainStorageName(extensionName, extensionId, scope), ); if (!settings || settings.length === 0) { @@ -112,23 +139,19 @@ export async function promptForSetting( return response.value; } -export async function getEnvContents( +export async function getScopedEnvContents( extensionConfig: ExtensionConfig, extensionId: string, + scope: ExtensionSettingScope, ): Promise> { - if (!extensionConfig.settings || extensionConfig.settings.length === 0) { - return Promise.resolve({}); - } - const extensionStorage = new ExtensionStorage(extensionConfig.name); + const { name: extensionName } = extensionConfig; const keychain = new KeychainTokenStorage( - getKeychainStorageName(extensionConfig.name, extensionId), + getKeychainStorageName(extensionName, extensionId, scope), ); + const envFilePath = getEnvFilePath(extensionName, scope); let customEnv: Record = {}; - if (fsSync.existsSync(extensionStorage.getEnvFilePath())) { - const envFile = fsSync.readFileSync( - extensionStorage.getEnvFilePath(), - 'utf-8', - ); + if (fsSync.existsSync(envFilePath)) { + const envFile = fsSync.readFileSync(envFilePath, 'utf-8'); customEnv = dotenv.parse(envFile); } @@ -145,11 +168,34 @@ export async function getEnvContents( return customEnv; } +export async function getEnvContents( + extensionConfig: ExtensionConfig, + extensionId: string, +): Promise> { + if (!extensionConfig.settings || extensionConfig.settings.length === 0) { + return Promise.resolve({}); + } + + const userSettings = await getScopedEnvContents( + extensionConfig, + extensionId, + ExtensionSettingScope.USER, + ); + const workspaceSettings = await getScopedEnvContents( + extensionConfig, + extensionId, + ExtensionSettingScope.WORKSPACE, + ); + + return { ...userSettings, ...workspaceSettings }; +} + export async function updateSetting( extensionConfig: ExtensionConfig, extensionId: string, settingKey: string, requestSetting: (setting: ExtensionSetting) => Promise, + scope: ExtensionSettingScope, ): Promise { const { name: extensionName, settings } = extensionConfig; if (!settings || settings.length === 0) { @@ -168,7 +214,7 @@ export async function updateSetting( const newValue = await requestSetting(settingToUpdate); const keychain = new KeychainTokenStorage( - getKeychainStorageName(extensionName, extensionId), + getKeychainStorageName(extensionName, extensionId, scope), ); if (settingToUpdate.sensitive) { @@ -177,27 +223,29 @@ export async function updateSetting( } // For non-sensitive settings, we need to read the existing .env file, - // update the value, and write it back. - const allSettings = await getEnvContents(extensionConfig, extensionId); - allSettings[settingToUpdate.envVar] = newValue; + // update the value, and write it back, preserving any other values. + const envFilePath = getEnvFilePath(extensionName, scope); + let envContent = ''; + if (fsSync.existsSync(envFilePath)) { + envContent = await fs.readFile(envFilePath, 'utf-8'); + } - const envFilePath = new ExtensionStorage(extensionName).getEnvFilePath(); + const parsedEnv = dotenv.parse(envContent); + parsedEnv[settingToUpdate.envVar] = newValue; + // We only want to write back the variables that are not sensitive. const nonSensitiveSettings: Record = {}; - for (const setting of settings) { - // We only care about non-sensitive settings for the .env file. - if (setting.sensitive) { - continue; - } - const value = allSettings[setting.envVar]; - if (value !== undefined) { - nonSensitiveSettings[setting.envVar] = value; + const sensitiveEnvVars = new Set( + settings.filter((s) => s.sensitive).map((s) => s.envVar), + ); + for (const [key, value] of Object.entries(parsedEnv)) { + if (!sensitiveEnvVars.has(key)) { + nonSensitiveSettings[key] = value; } } - const envContent = formatEnvContent(nonSensitiveSettings); - - await fs.writeFile(envFilePath, envContent); + const newEnvContent = formatEnvContent(nonSensitiveSettings); + await fs.writeFile(envFilePath, newEnvContent); } interface settingsChanges { diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 1d970fd2629..84f41b43fea 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -4,7 +4,15 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { vi, describe, it, expect, beforeEach, type Mock } from 'vitest'; +import { + vi, + describe, + it, + expect, + beforeEach, + afterEach, + type Mock, +} from 'vitest'; import EventEmitter from 'node:events'; import type { Readable } from 'node:stream'; import { type ChildProcess } from 'node:child_process'; @@ -1284,3 +1292,170 @@ describe('ShellExecutionService execution method selection', () => { expect(result.executionMethod).toBe('child_process'); }); }); + +describe('ShellExecutionService environment variables', () => { + let mockPtyProcess: EventEmitter & { + pid: number; + kill: Mock; + onData: Mock; + onExit: Mock; + write: Mock; + resize: Mock; + }; + let mockChildProcess: EventEmitter & Partial; + + beforeEach(() => { + vi.clearAllMocks(); + vi.resetModules(); // Reset modules to ensure process.env changes are fresh + + // Mock for pty + mockPtyProcess = new EventEmitter() as EventEmitter & { + pid: number; + kill: Mock; + onData: Mock; + onExit: Mock; + write: Mock; + resize: Mock; + }; + mockPtyProcess.pid = 12345; + mockPtyProcess.kill = vi.fn(); + mockPtyProcess.onData = vi.fn(); + mockPtyProcess.onExit = vi.fn(); + mockPtyProcess.write = vi.fn(); + mockPtyProcess.resize = vi.fn(); + + mockPtySpawn.mockReturnValue(mockPtyProcess); + mockGetPty.mockResolvedValue({ + module: { spawn: mockPtySpawn }, + name: 'mock-pty', + }); + + // Mock for child_process + mockChildProcess = new EventEmitter() as EventEmitter & + Partial; + mockChildProcess.stdout = new EventEmitter() as Readable; + mockChildProcess.stderr = new EventEmitter() as Readable; + mockChildProcess.kill = vi.fn(); + Object.defineProperty(mockChildProcess, 'pid', { + value: 54321, + configurable: true, + }); + mockCpSpawn.mockReturnValue(mockChildProcess); + + // Default exit behavior for mocks + mockPtyProcess.onExit.mockImplementationOnce(({ exitCode, signal }) => { + // Small delay to allow async ops to complete + setTimeout(() => mockPtyProcess.emit('exit', { exitCode, signal }), 0); + }); + mockChildProcess.on('exit', (code, signal) => { + // Small delay to allow async ops to complete + setTimeout(() => mockChildProcess.emit('close', code, signal), 0); + }); + }); + + afterEach(() => { + // Clean up process.env after each test + vi.unstubAllEnvs(); + }); + + it('should use a sanitized environment when in a GitHub run', async () => { + // Mock the environment to simulate a GitHub Actions run + vi.stubEnv('GITHUB_SHA', 'test-sha'); + vi.stubEnv('MY_SENSITIVE_VAR', 'secret-value'); // This should be stripped out + vi.stubEnv('PATH', '/test/path'); // An essential var that should be kept + vi.stubEnv('GEMINI_CLI_TEST_VAR', 'test-value'); // A test var that should be kept + + vi.resetModules(); + const { ShellExecutionService } = await import( + './shellExecutionService.js' + ); + + // Test pty path + await ShellExecutionService.execute( + 'test-pty-command', + '/', + vi.fn(), + new AbortController().signal, + true, + shellExecutionConfig, + ); + + const ptyEnv = mockPtySpawn.mock.calls[0][2].env; + expect(ptyEnv).not.toHaveProperty('MY_SENSITIVE_VAR'); + expect(ptyEnv).toHaveProperty('PATH', '/test/path'); + expect(ptyEnv).toHaveProperty('GEMINI_CLI_TEST_VAR', 'test-value'); + + // Ensure pty process exits for next test + mockPtyProcess.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + await new Promise(process.nextTick); + + // Test child_process path + mockGetPty.mockResolvedValue(null); // Force fallback + await ShellExecutionService.execute( + 'test-cp-command', + '/', + vi.fn(), + new AbortController().signal, + true, + shellExecutionConfig, + ); + + const cpEnv = mockCpSpawn.mock.calls[0][2].env; + expect(cpEnv).not.toHaveProperty('MY_SENSITIVE_VAR'); + expect(cpEnv).toHaveProperty('PATH', '/test/path'); + expect(cpEnv).toHaveProperty('GEMINI_CLI_TEST_VAR', 'test-value'); + + // Ensure child_process exits + mockChildProcess.emit('exit', 0, null); + mockChildProcess.emit('close', 0, null); + await new Promise(process.nextTick); + }); + + it('should include the full process.env when not in a GitHub run', async () => { + vi.stubEnv('MY_TEST_VAR', 'test-value'); + vi.stubEnv('GITHUB_SHA', ''); + vi.stubEnv('SURFACE', ''); + vi.resetModules(); + const { ShellExecutionService } = await import( + './shellExecutionService.js' + ); + + // Test pty path + await ShellExecutionService.execute( + 'test-pty-command-no-github', + '/', + vi.fn(), + new AbortController().signal, + true, + shellExecutionConfig, + ); + expect(mockPtySpawn).toHaveBeenCalled(); + const ptyEnv = mockPtySpawn.mock.calls[0][2].env; + expect(ptyEnv).toHaveProperty('MY_TEST_VAR', 'test-value'); + expect(ptyEnv).toHaveProperty('GEMINI_CLI', '1'); + + // Ensure pty process exits + mockPtyProcess.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + await new Promise(process.nextTick); + + // Test child_process path (forcing fallback by making pty unavailable) + mockGetPty.mockResolvedValue(null); + await ShellExecutionService.execute( + 'test-cp-command-no-github', + '/', + vi.fn(), + new AbortController().signal, + true, // Still tries pty, but it will fall back + shellExecutionConfig, + ); + expect(mockCpSpawn).toHaveBeenCalled(); + const cpEnv = mockCpSpawn.mock.calls[0][2].env; + expect(cpEnv).toHaveProperty('MY_TEST_VAR', 'test-value'); + expect(cpEnv).toHaveProperty('GEMINI_CLI', '1'); + + // Ensure child_process exits + mockChildProcess.emit('exit', 0, null); + mockChildProcess.emit('close', 0, null); + await new Promise(process.nextTick); + }); +}); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index be43fc14744..f917a2999be 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -148,6 +148,58 @@ const getFullBufferText = (terminal: pkg.Terminal): string => { return lines.join('\n'); }; +function getSanitizedEnv(): NodeJS.ProcessEnv { + const isRunningInGithub = + process.env['GITHUB_SHA'] || process.env['SURFACE'] === 'Github'; + + if (!isRunningInGithub) { + // For local runs, we want to preserve the user's full environment. + return { ...process.env }; + } + + // For CI runs (GitHub), we sanitize the environment for security. + const env: NodeJS.ProcessEnv = {}; + const essentialVars = [ + // Cross-platform + 'PATH', + // Windows specific + 'Path', + 'SYSTEMROOT', + 'SystemRoot', + 'COMSPEC', + 'ComSpec', + 'PATHEXT', + 'WINDIR', + 'TEMP', + 'TMP', + 'USERPROFILE', + 'SYSTEMDRIVE', + 'SystemDrive', + // Unix/Linux/macOS specific + 'HOME', + 'LANG', + 'SHELL', + 'TMPDIR', + 'USER', + 'LOGNAME', + ]; + + for (const key of essentialVars) { + if (process.env[key] !== undefined) { + env[key] = process.env[key]; + } + } + + // Always carry over test-specific variables for our own integration tests. + for (const key in process.env) { + if (key.startsWith('GEMINI_CLI_TEST')) { + env[key] = process.env[key]; + } + } + + return env; +} + /** * A centralized service for executing shell commands with robust process * management, cross-platform compatibility, and streaming output capabilities. @@ -249,7 +301,7 @@ export class ShellExecutionService { shell: false, detached: !isWindows, env: { - ...process.env, + ...getSanitizedEnv(), GEMINI_CLI: '1', TERM: 'xterm-256color', PAGER: 'cat', @@ -463,7 +515,7 @@ export class ShellExecutionService { cols, rows, env: { - ...process.env, + ...getSanitizedEnv(), GEMINI_CLI: '1', TERM: 'xterm-256color', PAGER: shellExecutionConfig.pager ?? 'cat',