fix: use encrypted storage for API keys (CWE-312) #549
Conversation
📝 WalkthroughWalkthroughAdds encrypted storage support for API keys: detection now checks secure encrypted storage before other sources; first-run wizard persists keys to encrypted storage via an environment manager with fallbacks when encryption support is unavailable. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as FirstRunWizard
participant Detector as APIKeyDetector
participant EnvMgr as EnvManager (encrypted store)
participant Session as In-Memory Session
CLI->>EnvMgr: set_variable(app="cortex", key=name, value, encrypt=true, desc)
EnvMgr-->>CLI: success / error
CLI->>Session: set session env var (always)
Detector->>EnvMgr: read variable for provider (decrypt)
EnvMgr-->>Detector: value / not found / error
alt decrypted value returned
Detector->>Session: set session env var
Detector-->>CLI: return (found, provider, key, source="encrypted_storage")
else not found or crypto missing
Detector->>Detector: continue other checks (cached, saved provider, env, etc.)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
CLA Verification PassedAll contributors have signed the CLA.
|
Summary of ChangesHello @Anshgrover23, 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 enhances the security of API key storage by replacing plain-text storage in shell configs with Fernet-encrypted storage. It modifies the API key detection logic to prioritize the new encrypted storage location and gracefully handle cases where the cryptography package is not available. The first-run wizard now saves API keys to the encrypted storage, improving the overall security posture of the application. Highlights
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 is a great security improvement, replacing plaintext API key storage in shell configuration files with Fernet-encrypted storage. The changes in api_key_detector.py and first_run_wizard.py correctly implement reading from and writing to the new secure storage. The fallback mechanism for when the cryptography package is not installed is also well-handled. I've provided a couple of suggestions to improve maintainability by reducing code duplication and removing hardcoded strings. These changes will make the code cleaner and easier to manage in the future.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cortex/api_key_detector.py (1)
133-159: Consider extracting the hardcoded app name to a constant.The app name
"cortex"is hardcoded on line 146, whilefirst_run_wizard.pydefinesCORTEX_APP_NAME = "cortex"as a constant. For consistency and maintainability, consider defining this constant in api_key_detector.py at the module level alongside other app-related constants.♻️ Suggested refactor
Add a module-level constant after line 42 with other constants:
CORTEX_CACHE_FILE = ".api_key_cache" + +# Application name for encrypted storage +CORTEX_APP_NAME = "cortex"Then use it in the method:
- value = env_mgr.get_variable(app="cortex", key=env_var, decrypt=True) + value = env_mgr.get_variable(app=CORTEX_APP_NAME, key=env_var, decrypt=True)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/api_key_detector.pycortex/first_run_wizard.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/api_key_detector.pycortex/first_run_wizard.py
🧬 Code graph analysis (1)
cortex/first_run_wizard.py (2)
tests/test_env_manager.py (1)
env_manager(63-67)cortex/env_manager.py (3)
get_env_manager(1108-1110)set_variable(689-738)encrypt(502-514)
🔇 Additional comments (7)
cortex/api_key_detector.py (3)
26-26: Good addition of logging support.The logging setup follows Python best practices and will be useful for debugging the new encrypted storage detection path.
Also applies to: 34-34
10-16: Documentation accurately reflects the new detection order.The docstrings clearly document that encrypted storage is checked as the third-highest priority source, and the implementation matches this specification.
Also applies to: 81-87
105-108: Correct integration of encrypted storage check in detection flow.The encrypted storage check is properly positioned in the detection priority order, running after environment variables but before cached keys. This ensures that securely stored keys are used when available.
cortex/first_run_wizard.py (4)
22-22: Good addition of constant for app name.Defining
CORTEX_APP_NAMEas a module-level constant is a best practice and improves maintainability. The import ofget_env_manageris correctly placed at the module level.Also applies to: 26-27
793-823: Excellent error handling and graceful degradation.The method properly handles the case where the
cryptographypackage is not installed (ImportError) and other potential failures, always falling back to setting the variable in the current session. This ensures the wizard doesn't break even if encrypted storage is unavailable.The implementation correctly uses the
EnvironmentManagerAPI and sets appropriate encryption and metadata parameters.
327-327: Call sites remain unchanged due to backward-compatible method signature.The refactored
_save_env_varmethod maintains the same signature, so existing calls continue to work without modification. This is good design for maintainability.Also applies to: 348-348
807-807: Minor issue with description string formatting.The description generation on line 807 produces slightly odd capitalization. For example,
"ANTHROPIC_API_KEY"becomes"API key for Anthropic Api Key"where "Api Key" is incorrectly title-cased.🐛 Proposed fix for description formatting
- description=f"API key for {name.replace('_API_KEY', '').replace('_', ' ').title()}", + description=f"API key for {name.replace('_API_KEY', '').replace('_', ' ').lower().capitalize()}",Or for better formatting:
- description=f"API key for {name.replace('_API_KEY', '').replace('_', ' ').title()}", + provider_name = name.replace('_API_KEY', '').lower().replace('_', ' ').title() + description=f"API key for {provider_name}",This would produce "API key for Anthropic" instead of "API key for Anthropic Api Key".
Likely an incorrect or invalid review comment.
|
@Suyashd999 Can I get a review on this one ? |
|
4 participants
Related Issue
Closes #
Summary
~/.cortex/environments/cortex.jsonAI Disclosure
Used Claude Code to implement the security fix.
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/)Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.