Keychain not always ready to read encrypted key on startup #246
Comments
|
About one week later, stats on this issue are: |
|
Are there any other workarounds that might work better? |
|
Hey @JoniVR Another way would be to not block the main thread and instead wait asynchronously in JS using a simple const MMKV = MMKV.storage.initialize();
const PersistStorage = {
setItem:MMKV.setItem,
removeItem:MMKV.removeItem,
getAllKeys: MMKV.indexer.getKeys,
getItem:async (key) => {
return new Promise(resolve => {
// Fetch the value from storage
const value = MMKV.getItem(key);
// Check if value is undefined
if (value === undefined) {
// Set a timeout and retry getting the key after a delay
setTimeout(() => {
// Add some logging here if needed to check instance is ready etc as we did earlier.
resolve(PersistStorage.getItem(key))
},500);
} else {
// Value is found, resolve promise.
resolve(value);
}
})
}
}Let me know if this looks ok |
|
@ammarahm-ed Looks okay to me, will be patching out the native fix for now... edit: seems like I needed to tweak it a bit: const PersistStorage = {
setItem: encryptedStorage.setItem,
removeItem: encryptedStorage.removeItem,
getAllKeys: encryptedStorage.indexer.getKeys,
setStringAsync: encryptedStorage.setStringAsync,
setString: encryptedStorage.setString,
getItem: async (key: any) => {
return new Promise(resolve => {
// Fetch the value from storage
const value = encryptedStorage.getItem(key);
// Check if value is undefined
if (value === undefined) {
// Set a timeout and retry getting the key after a delay
setTimeout(() => {
// Add some logging here if needed to check instance is ready etc as we did earlier.
resolve(PersistStorage.getItem(key));
}, 200);
} else {
// Value is found, resolve promise.
resolve(value);
}
});
},
}; |
Any new reports |
|
@jaltin Good! We're currently not in beta testing phase yet for the next version so I can only start testing in a few weeks unfortunately.. |
Did you also have a native fix in the pipeline for this or is the js fix the way to go long term? |
|
I personally patched out the native fix since it wasn't fully solving the issue anyways and the JS method should do something similar (hopefully better). Here's my patch file contents: diff --git a/node_modules/react-native-mmkv-storage/ios/SecureStorage.m b/node_modules/react-native-mmkv-storage/ios/SecureStorage.m
index 1c4e1c2..92ec0e7 100644
--- a/node_modules/react-native-mmkv-storage/ios/SecureStorage.m
+++ b/node_modules/react-native-mmkv-storage/ios/SecureStorage.m
@@ -45,14 +45,6 @@ - (NSString *) getSecureKey:(NSString *)key
@try {
[self handleAppUninstallation];
NSString *value = [self searchKeychainCopyMatching:key];
- dispatch_sync(dispatch_get_main_queue(), ^{
- int readAttempts = 0;
- // See: https://github.com/ammarahm-ed/react-native-mmkv-storage/issues/195
- while (![[UIApplication sharedApplication] isProtectedDataAvailable] && readAttempts++ < 100) {
- // sleep 25ms before another attempt
- usleep(25000);
- }
- });
if (value == nil) {
NSString* errorMessage = @"key does not present";
You could also revert to the version before the fix, might be easier. My thinking was the reason the native fix might not work could be because we're blocking the main thread (and thus all app execution) which prevents the protected data from becoming available too.. Though I'm not 100% sure that's the issue, just my best guess. Could also be that it's really an Apple related bug where the protected data just doesn't become available at all.... in which case the JS way of patching it currently will let the user wait forever, so perhaps it still needs some sort of max attempt limit. Or would redux-persist time-out in this case? |
We thought the same, so we are waiting 200 ms between tries and only tries 5 times. If it still fails we show an alert asking the user to restart the app (which often solves the issue for us). Dirty hack but hoping it will duct tape the issue for now. |
|
Yes, that sounds like a pretty good temporary mitigation, usually restarting will resolve the issue.. might actually implement that too for the time being :) |
|
@JoniVR , an update on the fix we have applied. We released it in production a few days ago and we are seeing the fix being applied for some users. For some it works ok to wait 200 ms a few times, but for others it still doesn't work (so they get the alert asking them to restart the app). @ammarahm-ed do you have any idea if this is fixable in the native part? |
|
@jaltin The previous fix did the same thing in the native side. I'm not sure what's going wrong. Seems weird to have to wait 200ms in any case. The native fix in some cases waited the full 10s before still failing. Could be that the value simply doesn't become available at all. This feels like an Apple bug to me, unless I'm missing something. |
|
I have a new theory. I think the reason we're still seeing errors despite the workaround might be due to the fact that I haven't had any actual manual user reports on the issues since the fix (though it could just be that nobody bothered to report it yet). I've added an extra log line to the next beta build to log the @jaltin, does your app also happen to use/support background app refresh? |
|
@JoniVR That could be the case. What if the app enters from background to foreground? at the point is there a way to make protected data available again to the user? But yes, if for example that app wakes up in background through a silent notification or something and it checks for the info in key chain to send a request to the API, that won't work since that data can't be accessed in background state. |
|
@ammarahm-ed Yes, I'm still collecting more logging from beta users but from the 5 reports I got in latest build, 4 recovered the token correctly, example below: And one failed recovering the token: In all 5 cases, app state was The app is launched in the background (can indeed be multiple reasons: notification, background app refresh, ...) and then the store is instantiated and key is read, which then fails. What probably happens after is that the app remains "frozen" in the background until the user opens it, the store is already instantiated but without key, so re-hydration fails? You can see in the second example the Any ideas or suggestions on how we can work around this? An extra internal variable in the native side that remembers the edit: what does confuse me though, by default the code seems to use |
|
@ammarahm-ed Perhaps we can detect in the app if:
Or just to simplify listen to the Doesn't fix the issue for non redux-persist cases but those can probably be worked around in a similar way. |
|
@ammarahm-ed I think I may have found something, but I'm not sure and I'd really like a confirmation: Is it possible that the JS side instantiates storage with I'm asking this because this could be why (when the app is launched in the background due to a notification), the storage can't be read. It should be using At first I thought (based on the native iOS code, which defaults no Accessibility value to Am I correct in assuming that if this value is defaulted incorrectly, I should change my storage instantiation to: export const encryptedStorage = new MMKVLoader()
.setAccessibleIOS(IOSAccessibleStates.AFTER_FIRST_UNLOCK)
.withInstanceID('encrypted') // different name causes storage to be recreated with correct AccessibleState
.withEncryption()
.initialize(); |
Follow-up issue from #231.
I left in the automatic detection of this issue and I'm still getting this sometimes in production. This automatic detection in production also gives a better sense of the amount of times the issue was actually occurring.
I've received 18 reports from 11 different users (with around 1400 iOS users in total) just today and yesterday combined.
Some of these reports clearly showcase the workaround working, for example:
Notice how there's a 6s delay between line 1 -> 2 => indicates the delay worked
Other reports don't show a delay and also seem to still log the user out:
So, for some reason the delay isn't always happening.. any ideas as to why?
edit: a third kind of log:
Here the timing between the "read" and actual re-hydration seems to be +2hrs, which I think indicates that the app launched in the background?
Originally posted by @JoniVR in #195 (comment)
The text was updated successfully, but these errors were encountered: