Repository navigation
harden(app): keep native token + backup-safe secure storage - #12053
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2a6bdcad-78b6-418a-a4ca-35cd76e56775) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d493452700
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Codex P2: a failed secure write leaves the legacy prefs token; still use it. | ||
| if (_authTokenCache.isEmpty) { | ||
| _authTokenCache = _preferences?.getString('authToken') ?? ''; |
There was a problem hiding this comment.
When secure storage is inaccessible rather than merely rejecting one write—for example, an Android keystore key is invalid after restore—migrateAuthTokenFromPrefs() catches its first read/write exception, but the immediately following _readSecureAuthToken() throws again before this fallback executes. Because main() awaits SharedPreferencesUtil.init(), affected users still cannot start the app or retain their existing session. Keep the post-migration read in the same failure boundary and add a secure-storage-throws regression test.
AGENTS.md reference: AGENTS.md:L31-L32
Useful? React with 👍 / 👎.
|
|
||
| private fun buildRequest(url: String): Request? { | ||
| val token = stringPref("authToken") | ||
| val token = stringPref("nativeAuthToken").ifEmpty { stringPref("authToken") } |
There was a problem hiding this comment.
When an installation has already run the preceding secure-storage migration, authToken has been removed and nativeAuthToken does not exist yet. Android's BleCompanionService can start the foreground BLE service while Flutter is not running, so after updating, this lookup finds neither key and background audio frames cannot be authenticated until the user opens the app and SharedPreferencesUtil.init() creates the mirror. Preserve existing background sessions by providing a native-accessible migration/read path that does not require a Flutter startup.
AGENTS.md reference: app/AGENTS.md:L111-L115
Useful? React with 👍 / 👎.
| if (_authTokenCache.isEmpty) { | ||
| _authTokenCache = _preferences?.getString('authToken') ?? ''; | ||
| } | ||
| await _syncNativeAuthToken(_authTokenCache); |
There was a problem hiding this comment.
On every iOS startup and token update, this unconditional call writes the Firebase bearer token through shared_preferences, which maps to NSUserDefaults; the supposedly Android-only mirror therefore puts the token back into ordinary, backupable preferences and defeats the new Keychain storage on iOS. Gate the mirror and its cleanup to Android so non-Android platforms retain the documented secure-storage-only lifecycle.
AGENTS.md reference: app/AGENTS.md:L111-L115
Useful? React with 👍 / 👎.
| } else { | ||
| await _secureStorage?.delete(key: _authTokenSecureKey); | ||
| } | ||
| await _syncNativeAuthToken(''); |
There was a problem hiding this comment.
If authToken is cleared and then repopulated before the asynchronous secure-storage delete completes—for example, an expired-token transient failure followed promptly by a successful refresh—the new setter writes nativeAuthToken, but the older _deleteSecureAuthToken() later resumes here and removes that newer mirror. Secure storage then contains the fresh token while the native background streamer has none, so background transcription loses authentication until another refresh or app restart; serialize or revision-guard this state transition.
AGENTS.md reference: AGENTS.md:L44-L48
Useful? React with 👍 / 👎.
| await _secureStorage?.delete(key: _authTokenSecureKey); | ||
| } | ||
| await _syncNativeAuthToken(''); |
There was a problem hiding this comment.
When Keychain or encrypted-storage deletion throws during logout or terminal session invalidation, execution never reaches the native-mirror cleanup, leaving nativeAuthToken available to the Android foreground service after Dart has cleared its in-memory token. The service can therefore continue authenticating and uploading audio under the signed-out account until that bearer expires; make native cleanup unconditional, such as through a guarded finally, while still surfacing the secure-delete failure.
AGENTS.md reference: AGENTS.md:L44-L48
Useful? React with 👍 / 👎.
| <!-- Add android:stopWithTask option only when necessary. --> | ||
| <application | ||
| android:name=".MyApp" | ||
| android:allowBackup="false" |
There was a problem hiding this comment.
Because this app targets SDK 36, allowBackup="false" is not sufficient to reliably suppress Android 12+ OEM device-to-device migration. On affected devices the shared-preferences files—including the new plaintext native token and encrypted-storage metadata—can still be transferred without the source device's Keystore key, recreating the restore failure this hardening intends to prevent and potentially copying the bearer token. Add android:dataExtractionRules that explicitly excludes the relevant sharedpref data from both cloud backup and device transfer.
AGENTS.md reference: app/AGENTS.md:L111-L115
Useful? React with 👍 / 👎.
## Summary Crashlytics reports a fatal `PlatformException(Unexpected security result code, Code: -25299, Message: The specified item already exists in the keychain.)` from `SharedPreferencesUtil._writeSecureAuthToken` (preferences.dart:116). Two defects meet there. **The failure is fatal for no reason.** The `authToken` setter fires its secure-storage write unawaited, so a keychain rejection escapes as an unhandled async error and `main.dart` records it through `recordError(..., fatal: true)`. The app keeps running; only the crash report says otherwise. **-25299 is reachable in normal use.** `flutter_secure_storage` 9.2.4 writes with a check-then-add (`ios/Classes/FlutterSecureStorage.swift:172-222`): `containsKey` first, then `SecItemAdd` when the probe reports absent. The probe misses an entry that exists in two ways we hit: - Entries written between #11917 and #12053 carry the plugin default `kSecAttrAccessibleWhenUnlocked`; #12053 switched writes to `first_unlock`. A `WhenUnlocked` entry 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. - The setter is fire-and-forget and the plugin answers each method call on a background dispatch queue, so two overlapping writes both probe "absent" and both add. ## What changed - Keychain calls are serialized through one queue, so overlapping writes cannot both take the add path. - Failures are reported as a value and logged instead of thrown, so a keychain that refuses the write never becomes a fatal crash. The in-memory token keeps the session alive. - On -25299 the entry is deleted with no accessibility in the query — the only query that matches whatever accessibility it was stored under — and re-added. - Sign-out deletes use that same query. Previously they ran with `first_unlock` in the query, so a legacy `unlocked` entry survived logout in the keychain. - The migration checks whether the write landed before dropping the prefs copy, so a keychain that never accepts the entry still retries on the next launch instead of losing the token. 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 plaintext `NSUserDefaults` copy 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 a `FlutterSecureStorage` fake that can reject writes: - a duplicate-item write recovers by deleting the stale entry and rewriting; - overlapping token writes never run concurrently in the keychain (`maxConcurrent == 1`), the regression that produced the collision; - a keychain that rejects the write leaves the prefs token and the migration flag intact, so the next launch retries; - the plaintext mirror is cleared where no native reader consumes it, and a token rotation does not write it back. ## Verification **I could not run `flutter test` for this change.** The Flutter SDK path on this machine is unreadable from the agent sandbox (`Operation not permitted` on every file under it), so the suite and `dart format` did not run here. The deterministic manifest did: `make preflight` passes all 13 checks, and the push used the pre-push gate's documented `PRE_PUSH_SKIP_DART_FORMAT=1` hatch, so CI's `app-dart-format` is the first machine to see the formatting. The four tests above are written but unexecuted; please run: ``` cd app && flutter test test/unit/auth_token_secure_storage_migration_test.dart ``` 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 <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/BasedHardware/omi/pull/12420?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. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
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 BasedHardware#11917 and BasedHardware#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.
BasedHardware#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 BasedHardware#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.
1 participant
Summary
Follow-up to #11917. That squash landed the Flutter secure-storage move before the Codex items were on the branch.
Ports from #11352 (now closed) plus the Codex review:
authTokentonativeAuthTokenin prefs so Android BLE/WS can still read a token after the Flutter key is gone from SharedPreferences.OmiBackgroundAudioStreamerreadsnativeAuthToken, then falls back toauthToken.android:allowBackup="false"so encrypted prefs cannot restore without the keystore key.first_unlockso a locked BLE restore can still get the token.Test plan
nativeAuthToken.Note
Medium Risk
Touches session-token storage, Android backup, and iOS Keychain accessibility. Native still mirrors the token in SharedPreferences for background BLE/WS.
Overview
Follow-up to the Flutter secure-storage migration so native Android can still auth background transcription after
authTokenleaves SharedPreferences.Mirrors the live session into a dedicated
nativeAuthTokenprefs key on init, set, and delete.OmiBackgroundAudioStreamerreads that first, then falls back toauthToken. If a secure write fails, init still uses a leftover prefs token instead of logging the user out.Also sets
android:allowBackup="false"so encrypted prefs cannot restore without the keystore key, and uses iOS Keychainfirst_unlockso a locked-device BLE restore can still read the token. Migration tests now assert the native mirror.Reviewed by Cursor Bugbot for commit d493452. Configure here.