fix: add validation for empty and blank API keys - #206
Conversation
Add strict validation to prevent saving empty or whitespace-only API keys. Implement provider-specific format validation using existing validateApiKey rules. Enhance error messages to guide users when invalid API keys are provided. Changes: - Enhanced saveProviderApiKey to validate against provider-specific formats - Added early validation check in config command handler - Improved error output with troubleshooting tips - Added comprehensive test suite for API key validation Fixes issue gunjanghate#153: API keys cannot be empty or contain only whitespace
|
@anshul23102 is attempting to deploy a commit to the Gunjan Ghate's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAPI key validation is now enforced strictly in the config command. Input is trimmed and validated via ChangesAPI Key Validation and Storage
API Key Validation Test Suite
Sequence DiagramsequenceDiagram
participant User
participant ConfigCmd as Config CLI Command
participant SaveFunc as saveProviderApiKey
participant Validator as validateApiKey
participant Keytar
participant ConfigFile as config.json
User->>ConfigCmd: Provide API key input
ConfigCmd->>SaveFunc: Pass raw input + provider
SaveFunc->>SaveFunc: Trim and check for empty/whitespace
SaveFunc->>Validator: Validate format for provider
alt Valid format
Validator-->>SaveFunc: Validation passed
SaveFunc->>Keytar: Store validated key with provider account
SaveFunc->>ConfigFile: Encrypt and persist validated key
SaveFunc-->>ConfigCmd: Success
else Invalid format
Validator-->>SaveFunc: Validation failed
SaveFunc-->>ConfigCmd: Error with provider-specific message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/commands/config.js`:
- Around line 585-594: The catch block that logs "Failed to save configuration."
currently falls through to the trailing process.exit(0), causing a success exit
code on failure; inside that catch block (the one that prints troubleshooting
tips) add a process.exit(1) after logging so the CLI returns a non‑zero exit
code on error and does not reach the final process.exit(0), ensuring failures
are signaled to callers.
In `@cli/tests/apiKeyValidation.test.js`:
- Around line 91-95: The summary log is placed after process.exit(1) so it never
prints on failure; move the console.log that prints `${passed} passed, ${failed}
failed` to before the conditional exit (or at least call it before invoking
process.exit) so the summary is always emitted when failed > 0; update the block
around the variables passed/failed and the process.exit(1) call accordingly to
log first then exit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2093604e-cbc1-42f4-92d8-84262306d0a2
📒 Files selected for processing (2)
cli/commands/config.jscli/tests/apiKeyValidation.test.js
| } catch (err) { | ||
| console.error(chalk.red('Failed to save configuration.')); | ||
| console.error(chalk.yellow(err.message)); | ||
| console.log(chalk.gray('')); | ||
| console.log(chalk.cyan('Troubleshooting tips:')); | ||
| console.log(chalk.gray(' 1. Ensure your API key is not empty or only whitespace')); | ||
| console.log(chalk.gray(' 2. Check that you are using the correct provider name')); | ||
| console.log(chalk.gray(' 3. Verify your API key format matches the expected pattern')); | ||
| } | ||
| process.exit(0); |
There was a problem hiding this comment.
Add process.exit(1) in the catch block to signal failure.
The catch block now correctly outputs troubleshooting tips, but the control flow falls through to process.exit(0) at line 594, causing the CLI to exit with a success code even when validation fails. This can mislead scripts or CI pipelines that check exit codes.
Proposed fix
console.log(chalk.gray(' 1. Ensure your API key is not empty or only whitespace'));
console.log(chalk.gray(' 2. Check that you are using the correct provider name'));
console.log(chalk.gray(' 3. Verify your API key format matches the expected pattern'));
+ process.exit(1);
}
- process.exit(0);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err) { | |
| console.error(chalk.red('Failed to save configuration.')); | |
| console.error(chalk.yellow(err.message)); | |
| console.log(chalk.gray('')); | |
| console.log(chalk.cyan('Troubleshooting tips:')); | |
| console.log(chalk.gray(' 1. Ensure your API key is not empty or only whitespace')); | |
| console.log(chalk.gray(' 2. Check that you are using the correct provider name')); | |
| console.log(chalk.gray(' 3. Verify your API key format matches the expected pattern')); | |
| } | |
| process.exit(0); | |
| } catch (err) { | |
| console.error(chalk.red('Failed to save configuration.')); | |
| console.error(chalk.yellow(err.message)); | |
| console.log(chalk.gray('')); | |
| console.log(chalk.cyan('Troubleshooting tips:')); | |
| console.log(chalk.gray(' 1. Ensure your API key is not empty or only whitespace')); | |
| console.log(chalk.gray(' 2. Check that you are using the correct provider name')); | |
| console.log(chalk.gray(' 3. Verify your API key format matches the expected pattern')); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/commands/config.js` around lines 585 - 594, The catch block that logs
"Failed to save configuration." currently falls through to the trailing
process.exit(0), causing a success exit code on failure; inside that catch block
(the one that prints troubleshooting tips) add a process.exit(1) after logging
so the CLI returns a non‑zero exit code on error and does not reach the final
process.exit(0), ensuring failures are signaled to callers.
| if (failed > 0) { | ||
| process.exit(1); | ||
| } | ||
|
|
||
| console.log(`\n${passed} passed, ${failed} failed`); |
There was a problem hiding this comment.
Move summary log before exit to ensure it prints on failure.
When tests fail, process.exit(1) on line 92 terminates immediately, so line 95 never executes and the summary is not printed. Users lose visibility into the total passed/failed counts when failures occur.
📊 Proposed fix to show summary before exit
+console.log(`\n${passed} passed, ${failed} failed`);
+
if (failed > 0) {
process.exit(1);
}
-console.log(`\n${passed} passed, ${failed} failed`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (failed > 0) { | |
| process.exit(1); | |
| } | |
| console.log(`\n${passed} passed, ${failed} failed`); | |
| console.log(`\n${passed} passed, ${failed} failed`); | |
| if (failed > 0) { | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/tests/apiKeyValidation.test.js` around lines 91 - 95, The summary log is
placed after process.exit(1) so it never prints on failure; move the console.log
that prints `${passed} passed, ${failed} failed` to before the conditional exit
(or at least call it before invoking process.exit) so the summary is always
emitted when failed > 0; update the block around the variables passed/failed and
the process.exit(1) call accordingly to log first then exit.
|
@anshul23102 add demo/screenshots of working |
2 participants
Fixes #153: gg config allows saving empty or blank string spaces as valid API Keys
Summary
This PR adds comprehensive validation to prevent saving empty or whitespace-only API keys in the configuration system. It validates against provider-specific format rules to ensure users can only save valid API keys.
Changes Made
Test Plan
gg config "" --provider geminigg config " " --provider geminigg config "AIzaSyValidKey..." --provider gemininode cli/tests/apiKeyValidation.test.jsAll 14 new validation tests pass successfully.
Summary by CodeRabbit
Release Notes