feat: Accept defaultValue from settings when installing an extension#18448
feat: Accept defaultValue from settings when installing an extension#18448dandye wants to merge 2 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @dandye, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for configuring extensions by integrating default values into the settings configuration process. Users will now see suggested default values directly in the prompts, with sensitive data appropriately masked. Furthermore, the system will automatically apply these defaults if no explicit input is provided, streamlining the setup of extensions and reducing the need for manual input for common settings. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for "defaultValue" in extension settings. While aiming to enhance user experience, the current implementation for sensitive settings presents a significant security risk across both CLI and UI components: masking or hiding default values allows malicious extensions to inject dangerous payloads without user awareness. It is crucial to either disallow default values for sensitive settings or ensure full transparency during setup. Additionally, a duplicated and inconsistent "promptForSetting" function in "packages/cli/src/config/extensions/extensionSettings.ts" needs to be addressed for code consistency and to prevent confusing user behavior, aligning with the principle that functionally similar components should be consistent.
| type: setting.sensitive ? 'password' : 'text', | ||
| name: 'value', | ||
| message: `${setting.name}\n${setting.description}`, | ||
| initial: setting.defaultValue, |
There was a problem hiding this comment.
This "promptForSetting" function presents a security vulnerability: it uses "defaultValue" as the "initial" prompt value without displaying it, particularly for sensitive settings. This allows malicious extensions to inject unintended values if a user accepts the default without awareness. Furthermore, this function is inconsistent with the version in "packages/cli/src/commands/extensions/utils.ts". It should either be removed if it's a leftover from refactoring or updated to ensure default values are clearly visible and consistent with other implementations.
export async function promptForSetting(
setting: ExtensionSetting,
): Promise<string> {
let description = setting.description;
if (setting.defaultValue !== undefined) {
const displayValue = setting.sensitive ? '******' : setting.defaultValue;
description += ` [default: ${displayValue}]`;
}
const response = await prompts({
type: setting.sensitive ? 'password' : 'text',
name: 'value',
message: `${setting.name}\n${description}`,
initial: setting.defaultValue,
});
return response.value;
}References
- Functionally similar prompt sections like 'planningWorkflow' and 'primaryWorkflows' should be located in the same spot for consistency, as security or other concerns for one likely apply to both.
| const displayValue = setting.sensitive ? '******' : setting.defaultValue; | ||
| description += ` [default: ${displayValue}]`; |
There was a problem hiding this comment.
Masking the defaultValue for sensitive settings in the prompt message prevents users from verifying the value they are about to accept. If a malicious extension provides a default value containing command injection sequences (e.g., ; malicious_command), a user might unknowingly execute it by simply pressing Enter. Sensitive settings should either not have default values, or the default values should be transparently displayed during the initial configuration prompt.
| {state.setting.defaultValue !== undefined && ( | ||
| <Text color={theme.text.secondary}> | ||
| {' '} | ||
| [default:{' '} | ||
| {state.setting.sensitive ? '******' : state.setting.defaultValue}] | ||
| </Text> | ||
| )} | ||
| </Text> | ||
| <Box flexDirection="row" marginTop={1}> | ||
| <Text color={theme.text.accent}>{'> '}</Text> | ||
| <TextInput | ||
| buffer={settingBuffer} | ||
| onSubmit={handleSettingSubmit} | ||
| onSubmit={(val) => { | ||
| if (val === '' && state.setting.defaultValue !== undefined) { | ||
| handleSettingSubmit(state.setting.defaultValue); | ||
| } else { | ||
| handleSettingSubmit(val); | ||
| } | ||
| }} |
There was a problem hiding this comment.
In the interactive UI, masking the defaultValue for sensitive settings (line 273) and then falling back to it if the user submits an empty string (line 283) creates a security risk. A user cannot see the value they are accepting, which could allow a malicious extension to smuggle commands or other malicious data into the configuration. Consider disabling default values for sensitive settings or ensuring they are visible during the first-time configuration.
gemini-cli
bot
added
the
priority/p1
1 participant
Summary
Implemented support for
defaultValuein extension settings configuration. This ensures a smoother user experience by pre-filling reasonable defaults (masked if sensitive) and utilizing them when the user provides no input.Details
defaultValueproperty to theExtensionSettinginterface inextensionSettings.ts.ConfigExtensionDialogto:[default: ******]for sensitive,[default: context]for others).TextInputto handle masking correctly when a default value is present but not yet accepted.defaultValueif the user submits an empty value.ConfigExtensionDialog.test.tsxcovering rendering, masking, and fallback logic.Related Issues
Fixes #18447
How to Validate
Run the tests to verify the logic:
npm test -w packages/cli -- src/ui/components/ConfigExtensionDialog.test.tsxIntegration test
Test Evidence
Pre-Merge Checklist
Screenshots
Before
After
Pressing enter writes the default to: