mogol / flutter_secure_storage

A Flutter plugin to store data in secure storage
https://pub.dartlang.org/packages/flutter_secure_storage
BSD 3-Clause "New" or "Revised" License
1.13k stars 376 forks source link

iOS: Breaking Change from 9.1.x forward, causing loss of data #762

Open dballance opened 3 months ago

dballance commented 3 months ago

On May 8th 2024, a version of this package with BREAKING CHANGES was published as a feature release on pub.dev.

Breaking Change

The package was changed from applying kSecAttrAccessible only on SecItemAdd calls to on all calls.

Previously, this value was only applied on writes (SecItemAdd) calls, and not on any other calls.

This change causes unnecessary narrowing of the search scope when reading (and possibly removing) values, as providing kSecAttrAccessible on reads will only match against keys stored with a matching kSecAttrAccessible.

For deletes, I also believe this would cause issues, as the query is still used to search the Keychain for a matching key before removing.

Compounding Factors

There are a few compounding factors that make this very difficult for consumers of this package to deal with.

No path back

It is impossible, as of the current latest release (9.2.2), to specify nil for kSecAttrAccessible, which would return the query for reads back to it's pre 9.1.0 state. Accessibility is defaulted to KeychainAccessibility.unlocked in Dart,

To illustrate, if I stored a value before 9.1.0 with an kSecAttrAccessible value other than the default (say KeychainAccessibility.first_unlock_this_device), I could read this value by simply calling read without specifying the accessibility.

After 9.1.0, I MUST call read and specify the same accessibility used to store the value, otherwise I get null as the response.

Apple only takes kSecAttrAccessible into account on the first write

Considering kSecAttrAccessible is only considered on the first write of the value to the keychain by Apple. If users of this package changed accessibility defaults after releasing a version into the field, they would likely lose all keychain data without realizing what changed, as keys stored with the default (kSecAttrAccessibleWhenUnlocked) would potentially be trying to fetch their keys with a different IOSOptions value for accessiblity, and this would ALWAYS fail in 9.1.x forward.

What should change

kSecAttrAccessible should be opt-in for all APIs, so that developers can use much like they would with the native APIs directly.

751 is likely the path to fixing this - but even it does not allow setting a null value for accessibility in IOSOptions on the Dart side.

Other OSS packages for keychain manipulation do NOT apply kSecAttr attributes on reads, only on writes. I would suggest that this package follow their lead.

I would encourage the package maintainers to publish a 10.x release, properly recognizing this as a breaking change. Further, I would potentially support some potential to unpublish or mark as deprecated 9.1.0, 9.1.1, 9.2.0, 9.2.1, and 9.2.2.

If I had publishing rights, I'd publish a 10.0.0 version from the 9.0.0 commit, then work forward from there to clean up the breaking change.

What can consumers do?

I would encourage all consumers of this package to pin to 9.0.0 in your pubspec files, and do not upgrade (especially don't upgrade and release an app version 😬 ) until this is resolved.

Related Changesets

This change was merged for #602 and #628 for iOS and MacOS.

Related Issues

This change caused a flurry of issues, and related changesets to fix them, without recognizing the underlying cause.

709, #711, #720, #727 (Likely), #752, #760

Important comments from the community

The first instance of this issue being noted by the community is here: https://github.com/mogol/flutter_secure_storage/issues/711#issuecomment-2112145552

image

This was noted 7 days after release, and correctly identified the issue with the package.

Package Maintenance

@juliansteenbakker - If you're unable to maintain this package - can the publishing rights be given over to another that could?

paulking86 commented 3 months ago

It's worth mentioning that due to the lack of iOS Privacy Manifest in version 9.0.0, it would be impossible to release an app on the App Store using your suggestion to downgrade. The manifest was added in 9.1.0, which is the same release which introduces the breaking change.

dballance commented 3 months ago

It's worth mentioning that due to the lack of iOS Privacy Manifest in version 9.0.0, it would be impossible to release an app on the App Store using your suggestion to downgrade. The manifest was added in 9.1.0, which is the same release which introduces the breaking change.

This makes it even worse. I was under the understanding that they're not aggressively enforcing the privacy manifests yet (As the warnings suddenly disappeared in April), but wouldn't surprise me.

I guess the only real path forward is a fork.

dballance commented 2 months ago

If anyone needs a functioning branch and doesn't want to fork themselves, add the below to your pubspec file.

This pulls in #751 and adds another changeset to make accessibility nullable on the dart side.

dependency_overrides:
  flutter_secure_storage:
    git: 
      url: https://github.com/Tango-Tango/flutter_secure_storage.git
      path: flutter_secure_storage
      ref: 400f6b4eb6ebaea6936fdbaa0d2f16ef36576678
  flutter_secure_storage_linux:
    git: 
      url: https://github.com/Tango-Tango/flutter_secure_storage.git
      path: flutter_secure_storage_linux
      ref: 400f6b4eb6ebaea6936fdbaa0d2f16ef36576678
  flutter_secure_storage_macos:
    git: 
      url: https://github.com/Tango-Tango/flutter_secure_storage.git
      path: flutter_secure_storage_macos
      ref: 400f6b4eb6ebaea6936fdbaa0d2f16ef36576678
  flutter_secure_storage_platform_interface:
    git: 
      url: https://github.com/Tango-Tango/flutter_secure_storage.git
      path: flutter_secure_storage_platform_interface
      ref: 400f6b4eb6ebaea6936fdbaa0d2f16ef36576678
  flutter_secure_storage_web:
    git: 
      url: https://github.com/Tango-Tango/flutter_secure_storage.git
      path: flutter_secure_storage_web
      ref: 400f6b4eb6ebaea6936fdbaa0d2f16ef36576678
  flutter_secure_storage_windows:
    git: 
      url: https://github.com/Tango-Tango/flutter_secure_storage.git
      path: flutter_secure_storage_windows
      ref: 400f6b4eb6ebaea6936fdbaa0d2f16ef36576678
juliansteenbakker commented 2 months ago

I would like to apologize for my disappearance the pas months, i have however checked and merged all outstanding PR's, and will release a new version after some testing tomorrow. I think this will fix all issues mentioned in this issue, since most of the code from the fork is merged into the main repository.

faaqi commented 2 months ago

@juliansteenbakker when will the new version gonna release? Facing this as well

gonzalogauto commented 2 months ago

Any updates on this?

mark8044 commented 2 months ago

@juliansteenbakker I think we need a fix, the problem seems to be increasing for whatever reason

eldarkk commented 2 months ago

Guys any workarounds to fix this ?

Except downgrading to 8.0.0

dballance commented 2 months ago

There is also now a dep issue with win32 that should be fixed, as the package is unusable in it's current state in the latest released Flutter version.

Would encourage a release of 10.x ASAP, including the win32 fix. There is a PR open already: https://github.com/mogol/flutter_secure_storage/pull/780

mark8044 commented 1 month ago

@dballance Is there any suggested way forward at this point? seems like alot of things are breaking all at once

dballance commented 1 month ago

The only suggestion I have for now is to ship your own fork. There could be some community support for shipping a fork to pub.dev.

dballance commented 1 month ago

I have updated our fork to pull in the win32 change, just update the ref in https://github.com/mogol/flutter_secure_storage/issues/762#issuecomment-2276036809 to: c54719f367d836d6f643176296468e9c0a70157e

dballance commented 1 month ago

There is now another blocker potentially for some, since this package (flutter_secure_storage_web) pins web versions to <1.0.0.

web 1.0.0 was released 2 months ago, and other packages have upgraded, but because this pins <1.0.0 it will potentially clash with other packages that have upgraded and prevent resolving deps.

This is also mitigated in the fork we're using, with hash: 92cd044dbad231e52902568d3b5ae1a5e470bb0a

amrLLSE commented 1 month ago

Any updates on this?

doshpin commented 3 weeks ago

the only solution for me is setting exactly version 9.0.0:

  flutter_secure_storage: 9.0.0
acacioveit commented 3 weeks ago

@juliansteenbakker it passed 2 months since your last comment about it, do you have any news about it? This package is used a lot from the flutter community, we need assistance!

SametSahin10 commented 2 weeks ago

Any updates here?