[Shu-lin-Liu]

Summary

This PR closes #13687 by removing the additional encryption applied to secrets before storing them in the system keyring.

Proxy passwords, GitHub tokens, and API keys are now stored directly in the system keyring without application-level encryption, using separate locations:

• Proxy passwords: org.jabref.proxy / proxy
• GitHub tokens: org.jabref.github / github
• API keys: org.jabref.apikeys / fetcher name

To maintain compatibility with existing values, the migration follows the new-location approach that koppor approved in this PR. When reading a secret, JabRef first checks the new location. If no value is found, it checks the legacy location, decrypts the existing value using Password.decrypt(), stores the decrypted value in the new location, and removes the old entry.

If a legacy value cannot be decrypted, JabRef logs a warning and removes the unreadable entry. Only that secret is lost; all other secrets are still read normally.

The Password class is now retained only for decrypting legacy values during migration. Its encrypt() method has been removed, while the existing decryption algorithm and fixed IV remain for compatibility with previously encrypted values.

The existing API key preference migration has also been updated to store values directly in the new keyring location.

Steps to test

1. Run the following three test classes and verify that all tests pass:
   • PasswordTest: verifies that ciphertext encrypted by older JabRef versions can still be decrypted.
   • GuiPreferencesMigrationsTest: verifies that API keys are stored directly in the new keyring location.
   • KeyringMigrationTest: verifies reading from the new location, migrating legacy values, handling missing or undecryptable values, and deleting values from both locations.
2. Manually verify proxy password migration (the commands below are for macOS):
   • On main, configure a proxy password in Preferences → Network and enable "Persist password between sessions".
   • Save the preferences and confirm that the password is stored in the legacy keyring location: security find-generic-password -s org.jabref -a proxy -w prints the encrypted value.
   • Switch to this branch and start JabRef.
   • If macOS requests permission to access the keychain, click Allow.
   • Open Preferences → Network and verify that the proxy password is still present.
   • Close JabRef and check the keychain: security find-generic-password -s org.jabref -a proxy reports that the item could not be found, and security find-generic-password -s org.jabref.proxy -a proxy -w prints the password as plain text.
3. Verify the handling of an undecryptable legacy value:
   • Store an invalid encrypted value in the legacy keyring location: security add-generic-password -s org.jabref -a proxy -w 'invalid'
   • Start JabRef and open Preferences → Network.
   • Verify that a warning containing "Could not decrypt" appears in the log.
   • Verify that the invalid legacy entry is removed and JabRef continues running without crashing.

Not covered by tests yet

• Reading multiple API keys when one legacy value cannot be decrypted, to verify that the remaining keys are still read successfully.
• A separate migration test for GitHub tokens. GitHub tokens use the same migration implementation as proxy passwords, but do not have dedicated test coverage.

Related issues and pull requests

Closes #13687

AI usage

AI tool and model: Claude (claude-opus-5-5)

How AI was used:
I used Claude to explain the existing code and encryption concepts, troubleshoot development environment issues, analyse the issue requirements, and discuss possible migration approaches.
Claude also generated the code changes in both commits, including the modifications to Password.java and the regression test. Additionally, I used AI assistance to review the grammar and wording of this PR description.

My review and verification:
I reviewed the AI-generated code line by line and made sure I understood the changes. I also ran the tests and manually verified the behaviour in JabRef. I take full responsibility for the submitted code.

AI Influence Level: Code: AIL4, PR description: AIL2

AI CHECKLIST.md walkthrough

Checked against the final diff to main: JabRefCliPreferences.java, Password.java, PreferencesMigrations.java, PasswordTest.java, GuiPreferencesMigrationsTest.java, the new KeyringMigrationTest.java, and CHANGELOG.md.

1. Code self-review

Nullability and control flow

• No == null / != null checks — JSpecify annotations (@NullMarked, @Nullable, @NonNull) used instead.
• No Objects.requireNonNull(...) — nullability expressed via JSpecify annotations.
• New classes annotated with @NullMarked (org.jspecify.annotations.NullMarked).
• [/] New packages annotated with @NullMarked (org.jspecify.annotations.NullMarked).
• [/] Optional consumed with ifPresent / ifPresentOrElse / map / orElseThrow — never orElse(unusedValue) nor an isPresent() + get() block.
• StringUtil.isBlank(...) used instead of s == null || s.isBlank().

Exceptions

• No catch (Exception e) — only specific exceptions are caught.
• No throw new RuntimeException(...) / IllegalStateException(...) — these tear down the whole application.
• Logged exceptions are passed as the last logger argument (LOGGER.info("...", e)), not concatenated into the message string.

Style and idioms

• [/] New BibEntry objects built with withers (withField, not setField).
• [/] Modern Java used: List.of() / Map.of() / Set.of(), Path.of(), SequencedCollection / SequencedSet, text blocks.
• [/] Regexes use a precompiled Pattern.compile(...) constant, not String.matches(...).
• [/] Keep alphabetical ordering when adding variables or enum items.
• [/] Background work uses org.jabref.logic.util.BackgroundTask, not new Thread().
• No commented-out code, no trivial comments restating the code, no AI-disclosure comments in source.
• Markdown Javadoc (///) uses Markdown syntax, not JavaDoc inline tags: `code` instead of {@code}, [ClassName] instead of {@link}.

User-facing text

• [/] All user-facing text localized (Localization.lang in Java, % prefix in FXML).
• [/] Sentence case (not Title Case); no trailing !; labels do not end with :.
• [/] Variance expressed with placeholders ("...: %0"), not string concatenation.

Security

• [/] User-controlled data (request params, entry fields, file contents) is HTML-escaped before being written into any text/html response — including exception/error messages, not just the success body (XSS).

Tests

• [/] In case issue lists an example, this concrete example is covered by a test case.
• Behavior changes in org.jabref.model / org.jabref.logic have added or updated tests.
• Tests assert object contents (assertEquals), use plain JUnit asserts (not AssertJ), have no @DisplayName, do not catch exceptions (let them propagate so JUnit reports setup/teardown failures directly), and use @TempDir instead of manual temp directories.
• [/] Fetcher tests hit the live endpoints — the remote API is not mocked or stubbed (automated-review suggestions to mock it are rejected on purpose).

2. Verification commands

• ./gradlew :jablib:check (or ./gradlew check for all modules).
• ./gradlew checkstyleMain checkstyleTest checkstyleJmh.
• ./gradlew modernizer.
• ./gradlew --no-configuration-cache :rewriteDryRun reports no changes (run ./gradlew rewriteRun to fix).
• ./gradlew javadoc.
• npx markdownlint-cli2 "docs/**/*.md" "*.md" (only if Markdown changed).
• npm ci && npm run textlint reports no misspellings (only if Markdown changed).
• [/] Only if formatting is still off after rewriteRun: docker run -v $(pwd):/github/workspace ghcr.io/leventebajczi/intellij-format:master "*.java" "" ".idea/codeStyles/Project.xml".

3. Documentation

• CHANGELOG.md entry added if the change is visible to the user (end-user wording, no extra blank lines, sorted in next to existing entries about the same component/feature). Link the issue if one exists; link the PR only when no issue exists. Use TODO as the placeholder when neither is known yet — never a fake number. No entry for fixes to changes that were themselves introduced after the last release (feature only in ## [Unreleased]) — update the existing unreleased entry instead if needed.
• Searched jabref/issues and jabref-koppor/issues for a related issue; linked only on a confident match, otherwise kept TODO (no closes/fixes for merely-similar issues).
• [/] Requirement added to docs/requirements/<area>.md if the change is a new feature or significant bug fix (skip for refactors, minor fixes, and internal changes).
• [/] Developer documentation under docs/ updated if behavior or architecture changed.

4. Pull request

• PR body built from .github/PULL_REQUEST_TEMPLATE.md, every section filled.
• All checklist items kept and marked [x], [ ], or [/].
• "Steps to test" is a numbered list with a cropped screenshot of the result for every visible change; no video (only allowed when another program is involved, e.g. drag and drop or push to an external application).
• All HTML comments removed from the PR body.
• [/] PR created with gh pr create --body-file <file> (not --body).
• [/] If CHANGELOG.md used a TODO placeholder (no issue confidently identified yet — an existing issue link always stays), the PR was opened as draft, the placeholder was replaced with the real PR-number link after PR creation, committed and pushed, and only then was the PR marked ready for review. If an issue is identified or created later, the link is switched to the issue.

Checklist

• I own the copyright of the code submitted and I license it under the MIT license
• If AI tools were used, I disclosed them in the "AI usage" section and reviewed, understood, and take full ownership of all AI-generated code
• I manually tested my changes in running JabRef (always required)
• I added JUnit tests for changes (if applicable)
• [/] I added screenshots in the PR description (if change is visible to the user)
• I added one sentence (max 20 words) to CHANGELOG.md describing the change from the user's point of view (if the change is visible to the user)
• [/] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

[github-actions]

Hey @Shu-lin-Liu! 👋

Thank you for contributing to JabRef!

We have automated checks in place, based on which you will soon get feedback if any of them are failing. We also use Qodo for review assistance. It will update your pull request description with a review help and offer suggestions to improve the pull request.

After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. You can read about the whole pull request process in our contribution guide.

Please ensure that your pull request is in line with our AI Usage Policy and make necessary disclosures.

[koppor]

New location seems to be good. Then in V7 we might remove the encryption totally.

[Shu-lin-Liu]

Thanks for the clarification! I’ll follow the approach of using the new location and keep only the decryption functionality in ‘Password’ until it can be removed in v7.

I also noticed a small unrelated issue during testing: if a proxy password is saved first and “Persist password between sessions” is enabled afterwards without changing the password, the password is not stored in the keyring. It seems that ‘bindToKeyring’ in ‘JabRefCliPreferences’ only updates the keyring when the password value changes. Would you prefer me to open a separate issue for this?

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit b0a4ae6

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 29b504f

[github-actions]

Do not mark a PR as ready-for-review while the CI or the automatic review is running. They need to be completed and passed.

[qodo-free-for-open-source-projects]

2. Api migration tests use uppercase locals 📘 Rule violation ⚙ Maintainability

The new migration tests name method-local variables V5_9_FETCHER_CUSTOM_KEY_NAMES and
V5_9_FETCHER_CUSTOM_KEYS as though they were constants. These locals appear in each added
migration test, so maintainers must distinguish inconsistent naming from the class-level constant
convention when updating the tests.

Agent Prompt

## Issue description
The added migration tests use uppercase constant-style names for method-local variables, which violates the project's Java naming convention.

## Fix Focus Areas
- jabgui/src/test/java/org/jabref/migrations/GuiPreferencesMigrationsTest.java[290-291]
- jabgui/src/test/java/org/jabref/migrations/GuiPreferencesMigrationsTest.java[313-314]

## Recommended Fix
Rename the method-local variables to descriptive lowerCamelCase names, such as `v5_9FetcherCustomKeyNames` and `v5_9FetcherCustomKeys`, and update their usages in the affected tests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 29b504f

[Shu-lin-Liu]

All CI checks have passed and all Qodo findings are resolved. However, each time I mark the PR as ready for review, Qodo starts a new review and posts “Qodo is busy working”, which makes the guard turn the PR back into a draft. Could a maintainer please mark it as ready for review? Thank you!