iOS: Breaking Change from 9.1.x forward, causing loss of data
dballance opened this issue · 17 comments
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: #711 (comment)
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?
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.
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.
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: 400f6b4eb6ebaea6936fdbaa0d2f16ef36576678I 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.