Skip to content

PM-40727: feat: Add encryption layer for new KeystoreEncryptedSharedPreferences - #7191

Merged
david-livefront merged 1 commit into
mainfrom
PM-40727-keystore-encrypted-shared-preferences
Jul 24, 2026
Merged

PM-40727: feat: Add encryption layer for new KeystoreEncryptedSharedPreferences#7191
david-livefront merged 1 commit into
mainfrom
PM-40727-keystore-encrypted-shared-preferences

Conversation

@david-livefront

@david-livefront david-livefront commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

PM-40727

📔 Objective

This PR creates a new KeystoreEncryptedSharedPreferences that utilizes the AndroidKeystore to encrypt the data stored. A followup PR will migrate the data to this new storage location.

Notable changes:

  • The keys are stored as plaintext, whereas the old logic used AES_SIV for encryption. This encryption was not compliant with FedRamp requirements.
  • We currently only support the storing of Strings (this is the only type we need at the moment).

@david-livefront
david-livefront requested a review from a team as a code owner July 21, 2026 21:15
@david-livefront david-livefront added the ai-review-vnext Request a Claude code review using the vNext workflow label Jul 21, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context t:feature Change Type - Feature Development labels Jul 21, 2026
@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new KeystoreEncryptedSharedPreferences encryption layer, the supporting EncryptionManager/KeystoreManager (AES-256-GCM via the Android Keystore), their Hilt wiring, and the accompanying unit tests. The cryptographic construction is sound: a fresh random 12-byte IV is generated per encryption (no IV reuse), a 128-bit GCM tag is used consistently, key material stays in the AndroidKeyStore with aliases namespaced by application ID, and failures are surfaced through Result and logged via Timber. Test coverage is thorough across the encrypt/decrypt round trip, key generation/retrieval error paths, and the SharedPreferences contract.

Code Review Details

No new findings. The two open question threads on KeystoreEncryptedSharedPreferences.kt (write-path getOrThrow() throwing asymmetrically vs. the graceful read path, and getAll() value semantics) remain the appropriate place to continue that discussion and are not duplicated here.

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.19608% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.78%. Comparing base (f64a08d) to head (9dafd0d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...asource/disk/KeystoreEncryptedSharedPreferences.kt 77.77% 3 Missing and 3 partials ⚠️
...e/data/manager/encryption/EncryptionManagerImpl.kt 87.50% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7191      +/-   ##
==========================================
+ Coverage   86.20%   86.78%   +0.58%     
==========================================
  Files         884      873      -11     
  Lines       64657    64328     -329     
  Branches     9674     9684      +10     
==========================================
+ Hits        55737    55829      +92     
+ Misses       5456     5032     -424     
- Partials     3464     3467       +3     
Flag Coverage Δ
app-data 17.71% <0.00%> (-0.03%) ⬇️
app-ui-auth-tools 18.75% <0.00%> (-0.04%) ⬇️
app-ui-platform 16.53% <0.00%> (-0.03%) ⬇️
app-ui-vault 27.33% <0.00%> (-0.05%) ⬇️
authenticator 6.14% <0.00%> (+0.03%) ⬆️
lib-core-network-bridge 4.15% <69.60%> (+0.13%) ⬆️
lib-data-ui 1.19% <20.58%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@david-livefront
david-livefront force-pushed the PM-40727-keystore-encrypted-shared-preferences branch from 7735d8c to f4604a7 Compare July 21, 2026 21:35
@david-livefront
david-livefront force-pushed the PM-40727-keystore-encrypted-shared-preferences branch 2 times, most recently from 6cdef4e to 7434fc4 Compare July 21, 2026 21:55
@david-livefront
david-livefront force-pushed the PM-40727-keystore-encrypted-shared-preferences branch 3 times, most recently from 327cbe3 to ad62146 Compare July 23, 2026 17:54
@david-livefront
david-livefront force-pushed the PM-40727-keystore-encrypted-shared-preferences branch from ad62146 to 9dafd0d Compare July 23, 2026 18:03

@andrebispo5 andrebispo5 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed code and Claude findings, they were all addressed, looks good 👍

@david-livefront

Copy link
Copy Markdown
Collaborator Author

Thanks @andrebispo5

@david-livefront
david-livefront added this pull request to the merge queue Jul 24, 2026
Merged via the queue into main with commit 445f2dd Jul 24, 2026
47 checks passed
@david-livefront
david-livefront deleted the PM-40727-keystore-encrypted-shared-preferences branch July 24, 2026 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants