martinkasa / capacitor-secure-storage-plugin

Capacitor plugin for storing string values securly on iOS and Android.
MIT License
152 stars 53 forks source link

Storage.set() throws an error on 0.6.4 and 0.7.0 on iOS #54

Closed mic345 closed 2 years ago

mic345 commented 2 years ago

Summary

Calling Storage.set() throws an error if called on iPhones (checked on iphone 12 and iphone XR). Note that it works well on the web version.

Steps to reproduce

  1. Start an hello world ionic app.
  2. Add the capacitor-secure-storage-plugin@0.6.2 plugin (maybe this step is not required, in our case the bug occurred after an upgrade).
  3. Call SecureStoragePlugin.set( { key: 'credentials', value: JSON.stringify( { token: '123' } ) } );
  4. Upgrade to capacitor-secure-storage-plugin@0.6.4
  5. Call SecureStoragePlugin.set( { key: 'credentials', value: JSON.stringify( { token: '123' } ) } );
  6. Bug: an error is thrown.

Environment

Device: iPhone XR, iOS 15.4.1

Plugin version: 0.6.4 and 0.7.0 (0.6.2 works well)

Ionic:

   Ionic CLI                     : 6.18.0 (/home/***/.config/yarn/global/node_modules/@ionic/cli)
   Ionic Framework               : @ionic/angular 6.1.6
   @angular-devkit/build-angular : 13.2.6
   @angular-devkit/schematics    : 13.2.6
   @angular/cli                  : 13.2.6
   @ionic/angular-toolkit        : 6.1.0

Capacitor:

   Capacitor CLI      : 3.5.1
   @capacitor/android : not installed
   @capacitor/core    : 3.5.1
   @capacitor/ios     : 3.5.1

Utility:

   cordova-res : not installed globally
   native-run  : 1.6.0

System:

   NodeJS : v16.13.0 (/home/***/.nvm/versions/node/v16.13.0/bin/node)
   npm    : 8.1.0
iAmMrHands commented 2 years ago

Seems like adding accessibility option .always to the keychainwrapper call caused this, as .always is deprecated since iOS 12.

let saveSuccessful: Bool = keychainwrapper.set(value, forKey: key, withAccessibility: .always)

martinkasa commented 2 years ago

PR by @iAmMrHands was merged and published as v0.7.1. If the problem is still there, @mic345 please reopen this issue.

gerritvanaaken commented 2 years ago

Bug still present. Did an upgrade to 0.7.1 but still having problems setting values on iOS:

⚡️  To Native ->  SecureStoragePlugin set 50656315
ERROR MESSAGE:  {"errorMessage":"error","message":"error"}
⚡️  [error] - {"errorMessage":"error","message":"error"}
martinkasa commented 2 years ago

There is test-app included in the repo. try to run it and play with it a bit. For me it works as expected, no errors.

mic345 commented 2 years ago

Hi guys and good morning 😊

We too are still experiencing the issue on 0.7.1 when we are testing on a real device running iOS 15.5 (connected via xcode). The information is not saved and you get the error message on xcode console.

Any suggestions?

mfrey-WELL commented 2 years ago

To anyone still struggling with this: If you have upgraded from a previous version to 0.7.0 or 0.7.1 you will need to reset your storage. Just get() the value, remove() it and then set() it again. If you upgrade from 0.7.0 to 0.7.1 it's the same story. This will happen every time this plugin changes the 'withAccessibility' option.

This is actually documented here https://github.com/jrendel/SwiftKeychainWrapper#synchronizable-option

martinkasa commented 2 years ago

Hi @mfrey-WELL, thanks for explanation. Just for All of you to understand, it was changed to "always" to be able to access the storage when app runs on background, but then we found out that always is deprecated and also considered AS security problem, so we decided to change it to "after first unlock". Sorry for inconvenience.

gerritvanaaken commented 2 years ago

So it is considered a good idea to "recreate" the storage on every app boot-up, so we are on the safe side?

mic345 commented 2 years ago

To anyone still struggling with this: If you have upgraded from a previous version to 0.7.0 or 0.7.1 you will need to reset your storage. Just get() the value, remove() it and then set() it again. If you upgrade from 0.7.0 to 0.7.1 it's the same story. This will happen every time this plugin changes the 'withAccessibility' option.

This is actually documented here https://github.com/jrendel/SwiftKeychainWrapper#synchronizable-option

Thanks for the quick response, indeed this was the issue 👍

A suggestion -- why don't you add a protocol: number property to the saved information and when the data is not backward compatible (e.g. a protocol upgrade), make the data refresh you suggested from within the plugin?

This would save plugin users from doing it, each by themselves (code reuse) and save you the time of answering such future questions.

Anyways, thanks for the help 😊 Mic.