Skip to content

Conversation

@jeanregisser
Copy link
Contributor

Fix annoying backup/restore behavior on Android

The problem that drove us crazy

You know that moment when your app gets reinstalled and suddenly the keychain acts weird? Here's what was happening:

  1. User uninstalls app → Android wipes the Keystore (normal)
  2. User reinstalls with allowBackup="true"RN_KEYCHAIN preferences get restored, but the actual encryption keys are gone
  3. App tries to read an old entry → library creates a brand new key, tries to decrypt old data with it, fails miserably
  4. Plot twist: that failed read just added a new alias to the Keystore, so now getAllGenericPasswordServices() suddenly shows that entry exists
  5. Developer confusion ensues 🤯

Basically, just trying to read an entry would change what the listing API returned. That's not supposed to happen.

What we tried first

Setting allowBackup="false" works around this by not restoring the preferences in the first place. But lots of apps already ship with backups enabled and can't easily change that.

The real fix

We made two key changes:

1. Stop creating keys during reads

  • Before: decrypt would auto-generate a missing key "just in case"
  • After: decrypt only uses existing keys, returns a clear error if key is missing
  • No more surprise mutations when you're just trying to read something

2. Handle the "stale data" case gracefully

  • When the key is missing: return false (not found) instead of throwing
  • When decryption fails because we're using a new key on old data: return false instead of throwing
  • Everything else still throws as before

Why this helps everyone

  • Fresh installs: Work exactly the same
  • Backup/restore scenarios: No more confusing behavior where reading changes what gets listed
  • Already affected apps: Even if you've already triggered the "create new key" behavior, those entries will now cleanly return false instead of erroring out
  • Reduces random crypto failures: This should also help with issues like #730 where users get unexpected CryptoFailedException errors, often caused by the same underlying backup/restore mismatch

The trade-off

We're now treating some decryption failures (like authentication tag mismatches) as "not found" instead of explicit errors. In 99% of cases this is what you want after a backup restore. In rare cases where you really need to know if data was corrupted vs just stale, you might miss that signal.

We added clear warning logs when this happens, and we could add a "strict mode" later if needed.

How to test

Quick test:

  1. Save some entries
  2. Uninstall/reinstall your app (keep allowBackup="true")
  3. Call getAllGenericPasswordServices() → should be empty and stay empty
  4. Try to read an old entry → should return false cleanly
  5. Call getAllGenericPasswordServices() again → should still be empty (this is the key improvement!)

Before this fix, step 5 would suddenly show the entry you tried to read in step 4, even though it couldn't actually be decrypted.

Manual backup simulation (for deeper testing):
You can manually simulate the backup/restore scenario using Android Studio's Device Explorer:

  1. Save entries, then use Device Explorer to copy /data/data/com.yourapp/files/datastore/RN_KEYCHAIN.preferences_pb to your computer
  2. Uninstall the app (this wipes both prefs and Keystore)
  3. Reinstall the app, then restore the RN_KEYCHAIN.preferences_pb file back to the same location (simulates backup restore - prefs restored, Keystore still empty)
  4. Test that reads return false without mutating the Keystore

Bottom line

Your keychain will now behave predictably after backup/restore, even if you can't change your backup settings. Reading entries won't have weird side effects anymore, and you'll see fewer random crypto exceptions in production.

Other inconsistency spotted (to address separately)

Note: There's still an inconsistency where hasGenericPassword checks preferences (stored) while getAllGenericPasswordServices checks Keystore (decryptable). This PR reduces the confusion by preventing reads from mutating Keystore state, but we should consider fixing that too.

…e/missing keys

- Avoid creating Keystore keys during decrypt across AES-GCM, AES-CBC, RSA by introducing `getExistingKeyOrNull` in `CipherStorageBase` and using it in decrypt paths.
- In `KeychainModule.getGenericPassword`, resolve `false` when the Keystore key is missing or decryption fails with tag/padding errors (e.g., `AEADBadTagException`, `BadPaddingException`), preserving explicit errors for other cases.
- Add inline comments documenting the reinstall + `allowBackup=true` scenario and the rationale for avoiding side effects on reads.
- Improves predictability after app reinstalls: listing no longer changes as a side effect of reads, and stale entries are treated gracefully.

Potential impact
- Calls that previously rejected on AEAD/BadPadding (or after an implicit key creation on read) now resolve `false`. Apps relying on those specific rejections should adjust handling.
github-merge-queue bot pushed a commit to divvi-xyz/divvi-mobile that referenced this pull request Sep 29, 2025
### Description

Use latest version of `@divvi/react-native-keychain` which contains
oblador/react-native-keychain#773

It's published from
https://github.com/mobilestack-xyz/react-native-keychain/commits/divvi-fork/

Note: I've chosen this versioning scheme to denote it's
react-native-keychain version 10.0.0 + divvi changes

### Test plan

- See oblador/react-native-keychain#773

### Related issues

- Part of ENG-636

### Backwards compatibility

Yes

### Network scalability

If a new NetworkId and/or Network are added in the future, the changes
in this PR will:

- [x] Continue to work without code changes, OR trigger a compilation
error (guaranteeing we find it when a new network is added)
github-merge-queue bot pushed a commit to divvi-xyz/divvi-mobile that referenced this pull request Sep 29, 2025
### Description

Use latest version of `@divvi/react-native-keychain` which contains
oblador/react-native-keychain#773

It's published from
https://github.com/mobilestack-xyz/react-native-keychain/commits/divvi-fork/

Note: I've chosen this versioning scheme to denote it's
react-native-keychain version 10.0.0 + divvi changes

### Test plan

- See oblador/react-native-keychain#773

### Related issues

- Part of ENG-636

### Backwards compatibility

Yes

### Network scalability

If a new NetworkId and/or Network are added in the future, the changes
in this PR will:

- [x] Continue to work without code changes, OR trigger a compilation
error (guaranteeing we find it when a new network is added)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant