Conversation
🦋 Changeset detectedLatest commit: bf7e2e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, 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 security posture of credential storage by preventing the unnecessary persistence of encryption keys to local files when a more secure OS keyring is present. It introduces a robust migration path for existing installations, automatically moving previously file-stored keys to the keyring and cleaning up the old files. The changes streamline key management, making the system more secure by default and reducing the attack surface for sensitive data. 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
|
There was a problem hiding this comment.
Code Review
This pull request improves security by prioritizing the OS keyring for storing the encryption key and only using a file as a fallback. The changes also include logic to migrate existing file-based keys to the keyring. This is a great improvement. However, I've found a critical issue where I/O errors during the key file saving process are ignored, which could lead to data loss if the key is not persisted correctly. My review includes suggestions to fix this by propagating errors.
Note: Security Review is unavailable for this PR.
|
Addressed the review feedback from @gemini-code-assist: |
|
Thank you for addressing the feedback, @jpoehnelt! I've reviewed the changes and confirmed that |
|
/gemini review |
1 similar comment
|
/gemini review |
0db5a76 to
59eee6f
Compare
|
/gemini review |
59eee6f to
e094e69
Compare
|
/gemini review |
e094e69 to
8c4930d
Compare
|
/gemini review |
…ng is available Previously, get_or_create_key() unconditionally wrote the encryption key to ~/.config/gws/.encryption_key on first run, even when the OS keyring was available. This left the key material on disk as a plain file, making credentials portable by copying the config directory. Changes: - Extract save_key_file() helper to deduplicate file-writing logic - On keyring read success: delete stale .encryption_key (migration) - On NoEntry + existing file: migrate key into keyring, then delete file - On NoEntry + new key: try keyring first, only write file as fallback Fixes #344
8c4930d to
bf7e2e6
Compare
|
/gemini review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #345 +/- ##
==========================================
- Coverage 61.78% 61.74% -0.05%
==========================================
Files 38 38
Lines 14607 14603 -4
==========================================
- Hits 9025 9016 -9
- Misses 5582 5587 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
get_or_create_key()unconditionally wrote the encryption key to~/.config/gws/.encryption_keyon first run, even when the OS keyring was available. This left key material on disk as a plain file (permissions0600), making credentials fully portable by copying the config directory.Changes
save_key_file()helper — deduplicates file-writing logic with secure permissions.encryption_keyif it exists (migrates existing installs on upgrade).encryption_keyas fallback if keyring store failsMigration
Existing users with a stale
.encryption_keyfile will have it automatically removed on their next CLI invocation, as long as their OS keyring is working. No manual steps required.Testing
All 499 tests pass. The 8
credential_storetests specifically validate encrypt/decrypt round-trips and key determinism.Fixes #344