fix: migrate API keys from plaintext XML to IntelliJ PasswordSafe#1046
Open
stephanj wants to merge 2 commits into
Open
fix: migrate API keys from plaintext XML to IntelliJ PasswordSafe#1046stephanj wants to merge 2 commits into
stephanj wants to merge 2 commits into
Conversation
Move all 19 credential fields (OpenAI, Anthropic, Mistral, Groq, Gemini, DeepSeek, OpenRouter, Grok, Kimi, GLM, Azure OpenAI, AWS access/secret/ bearer, CustomOpenAI, Google Search, Google CSE, Tavily) out of the plaintext DevoxxGenieSettingsPlugin.xml and into IntelliJ's PasswordSafe (OS keychain / KeePass / KWallet). Changes: - New service/credentials package: CredentialKey (19 enum constants), CredentialService interface, PasswordSafeCredentialService impl with in-memory fallback for headless/test environments. - DevoxxGenieStateService: Lombok accessors disabled on the 19 credential fields; hand-written @transient accessors delegate to CredentialService; one-shot migration in loadState reads legacy plaintext values, writes them to PasswordSafe, and wipes the XML fields. - CredentialMigrationStartupActivity: belt-and-braces retry on every project open; idempotent via the credentialsMigratedV1 flag. - isAwsEnabled / isAzureOpenAIEnabled now read through getters so they see the PasswordSafe-backed values instead of empty legacy fields. - DevoxxGenieSettingsService interface gap fix: add missing getter/setter pairs for grokKey, kimiKey, glmKey, customOpenAIApiKey. - plugin.xml: register CredentialService and the new startup activity. Backward compatibility: - Public getter/setter signatures on DevoxxGenieStateService are preserved; LLMProviderService, BedrockAuthResolver, the settings UI panels and ~20 test classes need no code change. - Existing keys are auto-migrated on first launch after upgrade. - Downgrading to an older plugin version hides the keys (they live in PasswordSafe only) so the user has to re-enter them. Tested: ./gradlew test (2687 tests, 0 failures) and ./gradlew buildPlugin both green.
…safety Address reviewer feedback on PR devoxx#1046: Bug devoxx#1 (CRITICAL): legacy XML keys silently lost on upgrade. IntelliJ's BeanBinding/XmlSerializer treats @transient on the getter as "skip this property in both directions". With the previous setup (private field + @transient on hand-written accessor + no store annotation on the field) BOTH bindings were dropped, so legacy <option name="openAIKey" value="sk-..."/> entries were silently ignored and the migration read empty values. The reviewer's proposed fix (override getState() to null the fields on a copy) does not actually work for this codebase, because XmlSerializer uses the hand-written getter (which reads from PasswordSafe) regardless of the field value. Verified by reading the bytecode of com.intellij.serialization.PropertyCollector and com.intellij.util.xmlb.SkipDefaultsSerializationFilter. Correct fix: keep @transient on the 19 hand-written accessor pairs (so they are removed from the property map in PropertyCollector#isAcceptableProperty), AND add @OptionTag to each underlying field (a 'store annotation' which forces PropertyCollector#doCollectOwnFields to register the private field even with collectPrivateFields=false). With the accessor pair gone from the map, the field escapes the dedup loop and becomes the sole binding. XmlSerializer then reads & writes only the field; the accessors continue to route every user-facing get/set through PasswordSafe. After migration the wiped field equals the default-bean's empty value so the SkipDefaultsSerializationFilter omits it from the XML \u2014 no re-leak on save. Bug devoxx#2 (CRITICAL): credentialsMigratedV1 set even on partial failure. Now gates the flag (and saveSettings()) on failed == 0; on any per-key failure the plaintext fields and the flag are left untouched so the next startup retries. Bug devoxx#3 (HIGH): migrateCredentialsToPasswordSafe() not thread-safe. Marked synchronized. Bug devoxx#4 (HIGH): PasswordSafeCredentialService#getCredential not synchronized while the setters are. Marked synchronized. Medium devoxx#7: CredentialAttributes inconsistency. setCredential now passes null userName to new Credentials(\u2026), matching the null userName in CredentialKey#attributes(). Round-trip is stable across keychain backends. Tested: ./gradlew test \u2014 2687 tests, 0 failures.
Collaborator
Author
|
Pushed fixes for all 4 blocking + 2 medium issues from the review. Deviation from reviewer's Bug #1 fix — worth a second look: The reviewer proposed "remove
The fix that does work (and which I shipped):
Net result:
No Tests: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move all 19 credential fields (OpenAI, Anthropic, Mistral, Groq, Gemini, DeepSeek, OpenRouter, Grok, Kimi, GLM, Azure OpenAI, AWS access/secret/bearer, CustomOpenAI, Google Search, Google CSE, Tavily) from plaintext
DevoxxGenieSettingsPlugin.xmlinto IntelliJ's PasswordSafe (OS keychain / KeePass / KWallet).Changes
service/credentialspackage:CredentialKeyenum (19 constants),CredentialServiceinterface,PasswordSafeCredentialServiceimpl with in-memory fallback for headless / test environments.DevoxxGenieStateService: Lombok accessors disabled on the 19 credential fields; hand-written@Transientaccessors delegate toCredentialService; one-shot migration inloadStatereads legacy plaintext from the freshly-deserialised state, writes it to PasswordSafe, and wipes the XML fields.isAwsEnabled/isAzureOpenAIEnabledswitched to use getters so they see the PasswordSafe-backed values.CredentialMigrationStartupActivity: belt-and-braces retry on every project open; idempotent via the new `credentialsMigratedV1` flag.Security
Credentials no longer hit `DevoxxGenieSettingsPlugin.xml`. Existing installs are auto-migrated on first launch. Downgrading to an older plugin hides the keys (they live in PasswordSafe only) — documented caveat.
Compatibility
All public getter/setter signatures on `DevoxxGenieStateService` are preserved. `LLMProviderService`, `BedrockAuthResolver`, all settings UI panels, and the ~20 test classes that mock the key getters need no code change.
Testing
`./gradlew test` — 2687 tests, 0 failures. `./gradlew buildPlugin` — green.