Skip to content

fix(app): stop a duplicate keychain item from crashing the app - #12420

Merged
mdmohsin7 merged 3 commits into
mainfrom
fix/app-keychain-duplicate-item-crash
Aug 30, 2026
Merged

mdmohsin7 merged 3 commits into
mainfrom
fix/app-keychain-duplicate-item-crash

Conversation

@mdmohsin7

@mdmohsin7 mdmohsin7 commented Aug 30, 2026 •

Copy link
Copy Markdown
Member

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:

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

Review in cubic

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
mdmohsin7 merged commit b60cced into main Aug 30, 2026
25 checks passed
@mdmohsin7
mdmohsin7 deleted the fix/app-keychain-duplicate-item-crash branch August 30, 2026 13:15
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. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant