Repository navigation
fix(app): stop a duplicate keychain item from crashing the app - #12420
Merged
Merged
Conversation
The auth token setter fires its secure-storage write unawaited, so a PlatformException from the keychain escapes as an unhandled async error and main.dart records it as a fatal crash. iOS reported -25299 (errSecDuplicateItem): flutter_secure_storage writes with a check-then-add, and its existence probe misses an entry it cannot match - one stored under a different kSecAttrAccessible (entries written between #11917 and #12053 carry the plugin default rather than first_unlock) while the device is locked, or one added by a write racing this one, since the setter is fire-and-forget and the plugin answers each call on a background queue. Serialize keychain calls through one queue, report failures as a value instead of throwing, and on -25299 delete the entry with no accessibility in the query (the only query that matches whatever it was stored under) before re-adding. Sign-out deletes now use that same query, so a legacy entry no longer survives logout. The migration checks the write result before dropping the prefs copy, so a keychain that never accepts the entry still retries next launch. Failure-Class: none Verification: could not run `flutter test` in this environment - the Flutter SDK path is unreadable from the agent sandbox. Added three regression tests in app/test/unit/auth_token_secure_storage_migration_test.dart covering the duplicate-item recovery, write serialization, and the migration retry.
#12053 mirrors the live auth token into plain SharedPreferences so the Android background streaming service can authenticate, since Kotlin cannot reach the Flutter keychain wrapper (OmiBackgroundAudioStreamer.kt reads nativeAuthToken). The mirror runs on every platform, so on iOS and macOS #11917's move of the token into the Keychain is undone by a plaintext copy in NSUserDefaults that nothing reads. Mirror only where a native reader exists, and clear the key everywhere else so a copy an earlier build wrote is dropped on the next launch. The Android reader keeps its legacy authToken fallback, so a device that has not relaunched since the update still authenticates. The platform decision is injectable because the host running `flutter test` is never the Android device the mirror serves. Verification: could not run `flutter test` in this environment - the Flutter SDK path is unreadable from the agent sandbox. The mirroring tests now declare which side they exercise, and a new test covers the non-Android side: the pre-existing key is cleared at init and a token rotation does not write it back.
mdmohsin7
added a commit
that referenced
this pull request
Aug 30, 2026
Fixes #11694. ## Summary Prod `backend-listen` rejects live-transcription upgrades from clients presenting a token that expired **days** earlier: ``` WebSocket auth failed: code=4001 error=Token expired, 1786634034 < 1786900104 ``` I measured prod today, two weeks after the report: individual clients are still being rejected **35–44 times per 10 minutes** — a reconnect every ~15 seconds, per client, indefinitely. Those users capture nothing at all, and the retries land on the listen tier at times outnumbering real traffic. The client does refresh when it believes the token is expired. The belief is the bug — it is assembled from two local sources, and the one authoritative source is discarded. **It never asks the token.** `getAuthHeader` gates the refresh on `SharedPreferencesUtil().tokenExpirationTime`, a *separate stored copy* of the expiry. `refreshIdToken` writes that copy and the token in sequence, so any failure between them leaves the expiry advanced while the token is not — and the client is then certain a dead credential is fresh. That is not hypothetical: until #12420 landed this morning, the token write was fire-and-forget with no error handling, so a keychain failure lost it silently. **It trusts the device clock.** `expiry.isBefore(DateTime.now())` compares against the phone's own clock. A device running days behind considers a long-dead token valid and never refreshes. This repo already ships `ClockSkewDetector` because skew happens in this fleet — but it only reports; nothing feeds it into the auth decision. **And the server's verdict was thrown away.** Close code `4001` is mapped in `pure_socket.dart` to the string `'auth_token_refresh_required'` — for a log label. Nothing performed the refresh the constant names. `capture_controller.onClosed` handles `4002` (out of credits) and ignores `4001` entirely. So every input the client consults says "no refresh needed", and the one input that says otherwise is only printed. ## What changed - **`jwtExpiry()`** reads `exp` out of the token. A JWT carries its own expiry; a second copy beside it can only drift. - **`getAuthHeader` prefers it**, falling back to the cached value only when the token cannot be parsed — so an unreadable token is no worse off than today. - **`onClosed` acts on 4001**, refreshing before the next reconnect. This is the half that matters most: it is correct regardless of *why* the local view was wrong, including a skewed clock, which parsing `exp` alone cannot fix. Throttled to once per 30s so a persistently rejected credential cannot turn reconnects into a refresh storm. ## Verification **I could not run `flutter test`, `flutter analyze`, or the app** — the Flutter SDK path is unreadable from my sandbox, so the push used `PRE_PUSH_SKIP_DART_FORMAT=1`. Please run: ``` cd app && flutter test test/unit/jwt_expiry_test.dart ``` To avoid shipping parsing logic I could not execute, I reimplemented `jwtExpiry` in Python and ran every test vector through it: a valid `exp` decodes; a missing, non-integer, or zero `exp`, a non-JWT string, invalid base64, and an empty string all return null. The padding case is real rather than theoretical — Firebase strips `=`, and the encoded payload in that test has `len % 4 == 3`, which `base64Url.decode` rejects without the `normalize` call the implementation makes. Also checked by hand: `unawaited` and `Logger` were already imported in `capture_controller.dart`, `refreshIdToken` persists the token and expiry itself (so the handler adds no second write), and no changed line exceeds 120 characters. The push also used `PRE_PUSH_SKIP_FLUTTER_GENERATED=1`, which needs `flutter pub get` to have resolved app dependencies — blocked by the same missing SDK. It checks generated output is current; this diff adds no ARB keys and touches no generator input. `make preflight` passes. ## What this does not fix The 15 affected clients only get this when they update. If they are on an old build — the issue never established which versions, and the upgrade URL carries no app version — they keep looping until then. A server-side answer (closing with `4004` for a repeatedly-expired credential, or backing off the rejected upgrade) would cover them without a release, and is worth its own issue rather than being folded in here. Failure-Class: none <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/BasedHardware/omi/pull/12433?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Crashlytics reports a fatal
PlatformException(Unexpected security result code, Code: -25299, Message: The specified item already exists in the keychain.)fromSharedPreferencesUtil._writeSecureAuthToken(preferences.dart:116). Two defects meet there.The failure is fatal for no reason. The
authTokensetter fires its secure-storage write unawaited, so a keychain rejection escapes as an unhandled async error andmain.dartrecords it throughrecordError(..., fatal: true). The app keeps running; only the crash report says otherwise.-25299 is reachable in normal use.
flutter_secure_storage9.2.4 writes with a check-then-add (ios/Classes/FlutterSecureStorage.swift:172-222):containsKeyfirst, thenSecItemAddwhen the probe reports absent. The probe misses an entry that exists in two ways we hit:kSecAttrAccessibleWhenUnlocked; harden(app): keep native token + backup-safe secure storage #12053 switched writes tofirst_unlock. AWhenUnlockedentry cannot be matched while the device is locked — and the app refreshes tokens in the background during BLE streaming and CoreBluetooth restore — so the probe says absent and the add collides. The plugin's own repair path (delete at every accessibility, then add) only runs when the probe found the entry, so it never fires here.What changed
first_unlockin the query, so a legacyunlockedentry survived logout in the keychain.Second commit, adjacent defect in the same function: #12053's plaintext mirror (
nativeAuthToken) exists for the Android background streaming service, whose Kotlin cannot reach the Flutter keychain wrapper (OmiBackgroundAudioStreamer.kt:306). It runs on every platform, so on iOS and macOS #11917's move of the token into the Keychain was undone by a plaintextNSUserDefaultscopy that nothing reads. It is now written only where a native reader exists, and cleared everywhere else so an earlier build's copy is dropped on the next launch.If the keychain is genuinely locked since boot, the recovery delete cannot match either and the write still fails — now logged rather than fatal, and it self-heals through the plugin's own update path on the next write after unlock.
Tests
app/test/unit/auth_token_secure_storage_migration_test.dart, via aFlutterSecureStoragefake that can reject writes:maxConcurrent == 1), the regression that produced the collision;Verification
I could not run
flutter testfor this change. The Flutter SDK path on this machine is unreadable from the agent sandbox (Operation not permittedon every file under it), so the suite anddart formatdid not run here. The deterministic manifest did:make preflightpasses all 13 checks, and the push used the pre-push gate's documentedPRE_PUSH_SKIP_DART_FORMAT=1hatch, so CI'sapp-dart-formatis the first machine to see the formatting. The four tests above are written but unexecuted; please run:Device check worth doing before merge: sign out and back in on iOS, confirm the session survives a cold start, and confirm the crash stops appearing in Crashlytics for builds carrying this change.
Failure class
No existing class in
.github/failure-classes/describes this contract — a fire-and-forget platform write whose rejection escapes as a fatal error, over a store whose existence probe cannot distinguish absent from inaccessible. I did not mint one for a single instance; say the word if you want it registered.Failure-Class: none