aparajita / capacitor-secure-storage

Secure, flexible storage for Capacitor apps using iOS Keychain and Android Keystore.
MIT License
107 stars 16 forks source link

bug: Empty error when decryptString was called #10

Closed c01nd01r closed 11 months ago

c01nd01r commented 11 months ago

Bug report

Capacitor version

💊   Capacitor Doctor  💊 

Latest Dependencies:

  @capacitor/cli: 5.5.1
  @capacitor/core: 5.5.1
  @capacitor/android: 5.5.1
  @capacitor/ios: 5.5.1

Installed Dependencies:

  @capacitor/cli: 5.5.0
  @capacitor/core: 5.5.0
  @capacitor/android: 5.5.0
  @capacitor/ios: 5.5.0

Platform(s)

Android 13 iOS(?)

Current behavior

The plugin returns an error object that contains empty strings for the code and message fields.

Expected behavior

The plugin should return null if it is unable to retrieve data for the specified key.

Code reproduction

Hard to reproduce

Other technical details

npm --version output: 10.1.0

node --version output: v20.8.1

pod --version output (iOS issues only): 1.13.0

Additional context

During the development of the application, after numerous installations and uninstallations of the app on a real Android device, I found that when attempting to retrieve data by key, I receive an empty error message from Capacitor native platform.

I decided to investigate the issue and found that the error occurs in the call to the decryptString() method, specifically in the condition check that triggers the throwing of the KeyStoreException(KeyStoreException.ErrorKind.notFound, prefixedKey) exception (lines 279-281 in the SecureStorage.java file).

https://github.com/aparajita/capacitor-secure-storage/blob/7ece5065831e8e4fb58f4aefde46993d33906456/android/src/main/java/com/aparajita/capacitor/securestorage/SecureStorage.java#L279-L281

According to the commit history in the repository, the error of type ErrorKind.notFound was replaced with a null return value (commit 0f2491a, feat: no blowfish, return null).

capacitor-secure-storage/android/src/main/java/com/aparajita/capacitor/securestorage/KeyStoreException.java

 if (data != null) {
      return decryptString(data, prefixedKey);
    } else {
-      throw new KeyStoreException(KeyStoreException.ErrorKind.notFound, prefixedKey);
+      return null;
    }

However, in my situation, an exception was thrown in a different part of the source code. As I understand it, the empty error is returned because the error description corresponding to the ErrorKind.notFound type was not found in the errorMap field of the KeyStoreException object.

https://github.com/aparajita/capacitor-secure-storage/blob/7ece5065831e8e4fb58f4aefde46993d33906456/android/src/main/java/com/aparajita/capacitor/securestorage/KeyStoreException.java#L8-L17

This description was removed in the commit 0f2491a.

And one more thing. I noticed that in the constructor init of the KeyStoreException error object, there is a call to the toString method on ErrorKind. I may be mistaken (I'm not a Java developer), but according to the documentation, calling toString on an enum should return the name of the field (for example, notFound), not the numeric code of the specified enum.

https://github.com/aparajita/capacitor-secure-storage/blob/7ece5065831e8e4fb58f4aefde46993d33906456/android/src/main/java/com/aparajita/capacitor/securestorage/KeyStoreException.java#L63

I also noticed that in the plugin code for iOS, there is also the usage of ErrorKind.notFound. Perhaps this issue is relevant for iOS as well? Unfortunately, I cannot confirm this.

https://github.com/aparajita/capacitor-secure-storage/blob/7ece5065831e8e4fb58f4aefde46993d33906456/ios/Plugin/KeychainError.swift#L12

aparajita commented 11 months ago

Thanks for the report, you uncovered a few bugs, although the actual bugs are not quite what you pointed out. Working on a fix.

aparajita commented 11 months ago

v5.0.0 has just been released. Actually the only actual bug was that decryptString was throwing an error instead of returning null. The leftover notFound stuff was not actually a bug, just unused code that has been removed.

I changed the plugin to throw an actual instance of StorageError, and StorageErrorType is now a string enum, hence the bump to v5.

c01nd01r commented 11 months ago

Awesome! Thank you so much!