Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Resolve Incompatibility Between react-native-keychain Versions 8.2.0 and 9.0.0 for getGenericPassword #707

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bowlerr
Copy link

@Bowlerr Bowlerr commented Jan 16, 2025

Resolves [#704]

Description:

This PR addresses an incompatibility issue in react-native-keychain between versions 8.2.0 and 9+. The issue arises when passwords set using version 8.2.0 cannot be retrieved using version 9+ due to differences in Entity prefix construction during encryption and decryption.

Issue Summary:

  • When attempting to retrieve passwords set with version 8.2.0 using getGenericPassword in version 9+, the following error is encountered:
    Wrapped error: The message could not be decrypted successfully. It has either been tampered with or the wrong resource is being decrypted.
    

Root Cause:

  • A discrepancy in the Entity prefix construction caused a mismatch between how encrypted data was stored in version 8.2.0 and how it is retrieved in version 9+

Changes Made:

  1. Removed an unintentional space in FC's createPasswordEntity and createUsernameEntity methods in the Kotlin implementation to ensure consistent prefix generation with the Java implementation.
  2. Ensured the prefix format matches the version 8.2.0 logic:
    • Java: RN_KEYCHAIN:<alias>pass
    • Kotlin: RN_KEYCHAIN:<alias>pass (now consistent).

Impact:

  • Resolves the compatibility issue between versions 8.2.0 and future versions.
  • Ensures that passwords encrypted in older versions can still be decrypted successfully in newer versions.

Environment Details:

  • Devices Tested:
    • OnePlus 12 (Android 14)
    • Pixel 7a (Android 15)
  • React Native Keychain Version:
    • Password Set: 8.2.0
    • Password Retrieve: 9.2.2
  • React Native Version: 0.74.5

Checklist:

  • Verified encryption and decryption compatibility across versions.
  • Reviewed prefix generation logic in both Java and Kotlin.
  • Tested on relevant devices and Android versions.

…x to remove spacing to match previous Java implementation

- Updated createPasswordEntity and createUsernameEntity in CipherStorageFacebookConceal to remove the unnecessary space after the prefix. Now both match the Java implementation.

- Resolves: oblador#704

Co-authored-by: George Bell <[email protected]>
@Bowlerr Bowlerr changed the title Fix: Resolve Incompatibility Between react-native-keychain Versions 8.2.0 and 9+ for getGenericPassword Fix: Resolve Incompatibility Between react-native-keychain Versions 8.2.0 and 9.0.0 for getGenericPassword Jan 16, 2025
@Bowlerr
Copy link
Author

Bowlerr commented Jan 16, 2025

Checks failed from here

@DorianMazur
Copy link
Collaborator

DorianMazur commented Jan 17, 2025

@Bowlerr Great work 💯 These things are the easiest to miss.
I will test this PR and investigate why the Gradle build is failing. I suspect this change might affect users who have upgraded react-native-keychain to version 9.x.x and still use FacebookConceal. However, since FacebookConceal is deprecated and we are planning to remove it, I believe this should not be a major issue. Merging this PR will help ease the transition for users coming from older versions of react-native-keychain.

EDIT: To make sure this doesn't happen again I'll probably add a new scenarios to e2e for all ciphers

@Bowlerr
Copy link
Author

Bowlerr commented Jan 17, 2025

@DorianMazur that was a concern of mine too.
We could check both prefixes if one fails however I do not know if this will cause any issues with performance

@Bowlerr
Copy link
Author

Bowlerr commented Jan 17, 2025

@DorianMazur Managed to fix the gradle by updating kotlin and it built fine but I need to test it further
#709

@Bowlerr
Copy link
Author

Bowlerr commented Jan 17, 2025

@DorianMazur All tests pass locally after - #709

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.

2 participants