Six pre-existing bugs found reviewing 6.0.0-dev.1 by hpoul · Pull Request #151 · authpass/biometric_storage

hpoul · 2026-08-25T14:56:18Z

Six pre-existing bugs, all found while reviewing #150 and none introduced by it. They were deliberately kept out of that PR to keep it reviewable; this is the follow-up it promised. Targets 6.0.0-dev.2.

Three of the six change observable behavior and are marked breaking in the changelog. 6.0.0 has not been published, so now is the moment.

1. Android used a destroyed `Activity` after a configuration change

onReattachedToActivityForConfigChanges and onDetachedFromActivityForConfigChanges were both empty bodies. ActivityAware in the engine is explicit about what that costs:

By the end of this method, the Activity … is no longer valid. Any references … should be cleared.

and, of the reattach binding:

binding includes a reference to the new instance of the Activity.

So the plugin never released the dead reference and never adopted the new one: when the Activity is recreated for a configuration change, attachedActivity goes on pointing at the destroyed one and every authenticated read or write hands BiometricPrompt a dead FragmentActivity.

Who this actually affects is narrower than "anyone who rotates", and I got that wrong at first. Emulator testing (API 36) established two gates:

The flutter create manifest declares configChanges="orientation|screenSize|…", so plain rotation does not recreate the Activity. I confirmed this: rotating with the stock manifest fires no lifecycle callback at all. Removing those entries makes a config change recreate it.
More decisively, an app whose FlutterActivity creates its own engine destroys that engine with the host (FlutterActivityAndFragmentDelegate.onDetach → flutterEngine.destroy(), since shouldDestroyEngineWithHost defaults true for self-created engines). The plugin instance dies with it and a fresh instance attaches via onAttachedToActivity. I observed exactly that: onDetachedFromActivityForConfigChanges followed by Attached to new activity rather than Reattached….

So the dangling reference requires the engine to outlive the Activity — a cached FlutterEngine, or add-to-app. For those apps it is a real dangling reference; for a default flutter create app the path is unreachable today.

Reproduced and cured, on an emulator

Built a cached-engine harness in the example (pre-warmed FlutterEngine in an Application, MainActivity overriding getCachedEngineId() and shouldDestroyEngineWithHost(), configChanges stripped) so the plugin instance survives Activity recreation. API 36 emulator, device-credential PIN, authenticating through the "Custom Prompts" store. Identical steps for both builds; the only difference is whether the two hooks have bodies. The harness is not part of this PR — it was reverted.

	with the fix	hooks reverted
callbacks fire on recreation	`onDetachedFromActivityForConfigChanges` + `Reattached…`	none
`authenticate()` reached	yes	yes
prompt drawn	yes	no
outcome	`read: { … Lorem Ipsum }`	never completes

Both builds write successfully before the configuration change, so the difference is entirely the recreation.

The failure mode is a silent hang, not a crash, and the mechanism is androidx's own guard. The reverted run logs it at the exact moment the read stops:

E/BiometricPromptCompat: Unable to start authentication. Called after onSaveInstanceState().

Zero occurrences in the fixed run. BiometricPrompt refuses to start against a state-saved host and returns without invoking any callback, so the pending Flutter result never completes — no exception on either side. It is also why the plugin's own "We are not attached to an activity" guard never fires: attachedActivity is not null, it is merely dead.

On the issues: a hung Dart future does not block the main thread, so this cannot by itself raise a literal system ANR dialog — it produces "the app stopped responding to my auth request", which is what users tend to report as an ANR. So #142 is consistent with the reports but not with a strict ANR, and #143 (crash) fits an older androidx that threw rather than guarded. Both are worth re-testing against dev.2, both need a cached engine or add-to-app, and I am closing neither.

1b. Android hung forever when the activity could not host a dialog

Found while verifying the above, and reachable independently of it. The same androidx guard fires whenever the app is merely backgrounded — onSaveInstanceState has run, the activity reference is perfectly valid, and authenticate() still returns without a callback, hanging the Dart future permanently.

So being attached is not the same as being able to show a dialog. authenticate() now checks isFinishing, isDestroyed and supportFragmentManager.isStateSaved before constructing the prompt, and reports a clean AuthException instead. That converts a permanent silent hang — undebuggable from Dart — into a catchable error, and it applies even once the lifecycle handling above is correct.

2. Android delivered an error message as the error code

result.error("Storage $name was not initialized.", null, null) — the signature is error(code, message, details), so a whole English sentence arrived as PlatformException.code. That field is matched on, not just displayed: Dart's own _transformErrors branches on code.startsWith('AuthError:'). Now NotInitialized, with the sentence as the message.

3. Darwin `init` replied twice

The requiredArg helper replies with the error and returns; the unconditional result(true) after the block then ran anyway. Flutter discards the second reply and logs "Reply already submitted" — so a caller that passed a missing or mistyped argument was told init had succeeded. The success reply now sits on the success path.

4. Darwin reported a recoverable lockout as "plugin unsupported" — breaking

canAuthenticate mapped every unrecognised LAError to "ErrorUnknown", which Dart reads as CanAuthenticateResponse.unsupported — documented as "Plugin does not support platform." So biometryLockout, which means "too many attempts, try again later", told callers the plugin did not work on iOS at all. It now reports ErrorStatusUnknown, matching what Android returns for a code it does not recognise, and logs the raw value.

This is a real behavior change, not a relabelling: callers commonly treat statusUnknown as usable — example/lib/main.dart does — and will now attempt authentication where they previously gave up. That is the intended direction, since failing at the prompt with a real reason beats disabling the feature outright, but it is why the changelog calls it breaking.

5. `forceInit` was implemented on Android only — breaking

The Dart API documents "if forceInit is true, will throw an exception if the store was already created in this runtime". Windows, web, iOS and macOS all accepted the flag and dropped it. All four honour it now, including the case Android already got right: without forceInit, a repeat init is a no-op reporting false. Windows and web have no native side holding per-store handles, so each tracks the names it has handed out.

6. win32 `read()` hid credential-store failures — breaking

It returned null for every CredRead failure, not just ERROR_NOT_FOUND. null is the documented answer to "nothing stored there", so a failing credential store was indistinguishable from an empty one — it read as silent data loss. Genuine errors now throw BiometricStorageException; not-found still returns null. Non-not-found errors were already logged at warning level, which is why this was lossy rather than truly silent.

Verification

flutter analyze --fatal-infos, flutter test and dart format clean in both the package and example/.
Android build for the Kotlin changes; iOS build for the Swift ones — and the new Swift strings confirmed present in the linked binary rather than trusting a quiet build.
The win32 suite covers the new forceInit path and runs on the Windows CI job.
Emulator (API 36, device-credential PIN): the authenticated write/read path prompts and succeeds; the configuration-change callbacks were observed firing. See the narrowing in item 1 — that testing is what established the two gates, and corrected this PR's original claim that plain rotation was enough.
Emulator A/B for item 1 — see the table above. The buggy build hangs, the fixed build completes, same harness and same steps.

🤖 Generated with Claude Code

onReattachedToActivityForConfigChanges and onDetachedFromActivityForConfigChanges were both empty bodies. ActivityAware's own documentation is explicit about what that costs: "By the end of this method, the Activity ... is no longer valid. Any references ... should be cleared", and the reattach binding "includes a reference to the new instance of the Activity". So after a rotation `attachedActivity` went on pointing at the destroyed Activity, and every authenticated read or write handed BiometricPrompt a dead FragmentActivity. Worth checking against #142 (ANR) and #143 (crash) — both are reports of the prompt misbehaving on real devices, and both predate any of this. Also: the storage-not-initialized error passed its message as the error *code*. `error(code, message, details)`, and the code is what callers match on — Dart's own _transformErrors branches on `code.startsWith('AuthError:')`. It is "NotInitialized" now, with the sentence as the message. Verified by building the example for Android. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Three things in the shared implementation, none of them new. `init` replied twice on a bad argument. The requiredArg helper replies with the error and returns, then the unconditional result(true) ran anyway. Flutter discards the second reply and logs "Reply already submitted", so the caller was told init had succeeded when it had not. The success reply now sits on the success path. forceInit was never implemented here — the flag crossed the channel and was ignored, while the Dart API documents that it throws if the store was already created in this runtime. Mirrors Android now, including the case it gets right: without forceInit a repeat init is a no-op that reports false. canAuthenticate mapped every unrecognised LAError to "ErrorUnknown", which the Dart side reads as CanAuthenticateResponse.unsupported — documented as "Plugin does not support platform". So a biometryLockout, which is recoverable and means the user should try again later, told callers the plugin did not work on iOS at all. It reports ErrorStatusUnknown now, the same as Android does for a code it does not recognise, and logs the raw value. That last one is a behaviour change, not just a relabelling: callers commonly treat statusUnknown as usable and will now attempt authentication where they previously gave up. That is the intended direction — failing at the prompt with a real reason beats disabling the feature outright — but it is why this is called out as breaking in the changelog. Verified by building the example for iOS, and by confirming the new strings are in the linked binary rather than trusting a quiet build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

getStorage accepted forceInit on both and dropped it, so the documented "will throw an exception if the store was already created in this runtime" held on Android alone. Neither platform has a native side keeping per-store handles, so each tracks the names it has handed out. win32 read() also returned null for every CredRead failure, not just ERROR_NOT_FOUND. null is the documented answer to "nothing stored there", so a credential store that was failing was indistinguishable from an empty one — it read as silent data loss. Genuine errors throw BiometricStorageException now; not-found still returns null. The warning-level log was already there, which is why this was lossy rather than silent. The win32 suite covers the forceInit path; it runs on the Windows CI job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

None of these are defects in the fixes themselves; they are places where the fixes stopped short of their own reasoning. canAuthenticate: the nil-error branch still answered "ErrorUnknown", which is the exact mapping the commit before this one rejected — "cannot tell" is not "this plugin does not work here". And biometryLockout, the one unmapped code that is actually common, now has its own arm, so the log in the default arm keeps meaning "something we have not seen" instead of firing on the ordinary case. forceInit raised PlatformException(code: 'AlreadyInitialized') on the method-channel platforms and BiometricStorageException on the two implemented in Dart, so a caller needed two catch clauses for one failure. The channel path translates now; every platform throws BiometricStorageException. win32 delete() still collapsed a failing credential store into `false`, which is its answer for "there was nothing to delete" — the same conflation this branch just removed from read(). The changelog was also wrong by omission in a way that mattered more than any of the above: moving the darwin init reply onto the success path also stopped a repeat getStorage() rebuilding the store. That used to discard the cached LAContext, so any darwinTouchIDAuthenticationForceReuseContextDuration in flight, and silently adopt the second call's options. First call wins now, as on Android. That is a behaviour change existing iOS users can hit without touching forceInit, and it now has its own entry. The forceInit entry also says that Linux still ignores the flag, rather than leaving "Windows, web, iOS and macOS" to be read as "everywhere". Verified: analyze, format and tests clean; iOS build with ErrorStatusUnknown confirmed in the linked binary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR rolls up a set of cross-platform bug fixes (Android, Darwin, Windows, Web, and Dart surface) that were discovered during the 6.0.0-dev.1 review, and bumps the package to 6.0.0-dev.2. Several fixes intentionally change observable behavior (documented as breaking) to align runtime behavior with the public API contract.

Changes:

Android: fix incorrect PlatformException.code for “not initialized” and correctly refresh/clear Activity references across configuration-change reattachment.
Darwin + Dart surface: fix Darwin init double-reply, implement forceInit semantics consistently, and translate AlreadyInitialized to a unified Dart exception type.
Windows/Web: implement forceInit tracking, and on Windows throw on real credential-store failures instead of returning “not found” sentinels.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`android/src/main/kotlin/design/codeux/biometric_storage/BiometricStoragePlugin.kt`	Fixes `NotInitialized` error code and properly clears/updates `attachedActivity` across config changes.
`darwin/biometric_storage/Sources/biometric_storage/BiometricStorageImpl.swift`	Fixes `init` double-reply, implements `forceInit` semantics, and improves `canAuthenticate()` mapping for unknown/lockout LAErrors.
`lib/src/biometric_storage.dart`	Translates `AlreadyInitialized` `PlatformException` into `BiometricStorageException` for consistent cross-platform catching.
`lib/src/biometric_storage_web.dart`	Implements per-runtime `forceInit` semantics by tracking initialized store names.
`lib/src/biometric_storage_win32.dart`	Implements per-runtime `forceInit` semantics and throws on credential-store failures (non-not-found) for `read`/`delete`.
`test/biometric_storage_win32_test.dart`	Adds a Windows runtime test for the `forceInit` behavior.
`CHANGELOG.md`	Documents the fixes and breaking behavior changes for `6.0.0-dev.2`.
`pubspec.yaml`	Bumps package version to `6.0.0-dev.2`.
`example/pubspec.lock`	Updates the example’s locked path dependency version to `6.0.0-dev.2`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    expect(
+      () => plugin.getStorage(name, forceInit: true),
+      throwsA(isA<BiometricStorageException>()),
+    );


+          e,
+          stackTrace,
+        );
+        throw BiometricStorageException(e.message ?? e.code);


Being attached to an activity is not the same as being able to show a dialog. androidx's BiometricPrompt refuses to start after onSaveInstanceState and returns *without invoking any callback*, so the pending Flutter result never completes — not an error the caller can catch, a permanent silent hang. The emulator A/B for the configuration-change fix caught it in the act: E/BiometricPromptCompat: Unable to start authentication. Called after onSaveInstanceState(). logged at the exact moment the read stopped, and absent from the passing run. That guard is reachable whenever the app is backgrounded, with a perfectly valid activity reference and no configuration change involved — so the lifecycle fix earlier on this branch does not cover it, and neither does the existing `attachedActivity ?: return` guard, since the reference is non-null. isFinishing, isDestroyed and isStateSaved are checked before the prompt is built, and the caller gets an AuthException it can act on. Also from review: The changelog told upgraders to "close the old store first" when they wanted different options. There is no close — nothing in the Dart API disposes a store, though the darwin side has a handler waiting for one. Replaced with advice that can actually be followed. The AlreadyInitialized translation now uses Error.throwWithStackTrace, so the frames showing which call re-initialized the store survive instead of the trace restarting at getStorage. Verified: analyze, format and tests clean; Android build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Nothing has ever executed the Linux implementation — not here, not in CI. Its forceInit bookkeeping, the GHashTable's lifetime and six rewritten response sites all shipped in 6.0.0-dev.3 verified by compiling and reading alone. That was the weakest evidence in the release, and this closes it. example/integration_test/ is new: the example had no integration_test dependency and no scaffold at all. Six tests exercising the real backend through the method channel — round-trip with non-ASCII, overwrite, the empty value that used to throw on win32, absent-read, and canAuthenticate mapping to a known enum member rather than the StateError that was #148. The fifth test earns its place separately: it calls getStorage twice, with and without forceInit. A round-trip suite alone never reaches handleInit's new branch, because it never initialises the same name twice — so the logic this release added to four platforms would have stayed unexecuted even with integration tests present. Every store uses authenticationRequired: false. An authenticated one needs a real biometric or credential gesture, which no runner can give; the emulator work in #151 shows what that costs. CI gains gnome-keyring, dbus-x11 and xvfb, and runs the suite under a session bus with an unlocked keyring. libsecret needs the bus, the desktop runner needs a display. Verified by running the suite against the real Android keystore on an emulator: all six pass. The Linux leg is what CI will prove. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* run the plugin against a real backend, and wire Linux into CI Nothing has ever executed the Linux implementation — not here, not in CI. Its forceInit bookkeeping, the GHashTable's lifetime and six rewritten response sites all shipped in 6.0.0-dev.3 verified by compiling and reading alone. That was the weakest evidence in the release, and this closes it. example/integration_test/ is new: the example had no integration_test dependency and no scaffold at all. Six tests exercising the real backend through the method channel — round-trip with non-ASCII, overwrite, the empty value that used to throw on win32, absent-read, and canAuthenticate mapping to a known enum member rather than the StateError that was #148. The fifth test earns its place separately: it calls getStorage twice, with and without forceInit. A round-trip suite alone never reaches handleInit's new branch, because it never initialises the same name twice — so the logic this release added to four platforms would have stayed unexecuted even with integration tests present. Every store uses authenticationRequired: false. An authenticated one needs a real biometric or credential gesture, which no runner can give; the emulator work in #151 shows what that costs. CI gains gnome-keyring, dbus-x11 and xvfb, and runs the suite under a session bus with an unlocked keyring. libsecret needs the bus, the desktop runner needs a display. Verified by running the suite against the real Android keystore on an emulator: all six pass. The Linux leg is what CI will prove. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: create a keyring the linux integration tests can write to The first run failed exactly where the fragility was predicted. The daemon started, but an empty unlock password creates no keyring, and the graphical prompter it falls back to cannot open a display — so every write failed with "Object does not exist at /org/freedesktop/secrets/collection/login" while the reads, the absent-read, forceInit and canAuthenticate all passed. That split is the evidence: the channel and the plugin were fine, there was nowhere to store. A non-empty password and an existing ~/.local/share/keyrings are what make the login keyring appear. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * tests: say what the integration assertions actually check Three points from review, all about assertions that pass for the wrong reason. The teardown wrapped `delete()` in a bare `catch (_)`, justified as "a test that failed mid-way may have left nothing". That case does not throw: Android guards on `exists()`, libsecret reports `removed = false` with no error set, the keychain maps `errSecItemNotFound` to `true`, and win32 maps `ERROR_NOT_FOUND` to `false`. So the only exception the catch could ever swallow was a store that genuinely failed — the class of bug this suite exists to surface — sitting in the one file whose whole purpose is to be loud. The win32 unit suite already uses the bare form. `canAuthenticate` asserted `isA<CanAuthenticateResponse>()`, which the return type makes a tautology. The real test was always the await: an unmapped native string throws a StateError inside `canAuthenticate`, which is how #148 surfaced. `completes` is the same test, and says so, so a later reader does not "fix" the tautology by deleting it. Linux hard-codes "ErrorHwUnavailable" in the C, so on that one platform the native-string-to-enum mapping itself can be pinned. The forceInit test asserted only the exception type. That is already discriminating on the method-channel platforms — nothing but the native repeat-plus-forceInit branch emits `AlreadyInitialized`, and any other native response stays a `PlatformException` — but naming the code matches the unit suite and guards the day a second code is translated. Verified: `flutter analyze --fatal-infos` clean in both packages, `flutter test` green, and the Linux job runs the suite against real libsecret. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

hpoul and others added 5 commits August 25, 2026 16:55

release 6.0.0-dev.2

7a1a0d0

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

hpoul requested a lite review from Copilot August 25, 2026 15:23

Copilot started reviewing on behalf of hpoul August 25, 2026 15:23 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

hpoul merged commit ed6b11c into main Aug 25, 2026
15 checks passed

hpoul deleted the pre-existing-bug-sweep branch August 25, 2026 16:06

hpoul mentioned this pull request Aug 25, 2026

run the plugin against a real backend, and wire Linux into CI #155

Merged

Navigation Menu

Uh oh!

Uh oh!

Six pre-existing bugs found reviewing 6.0.0-dev.1 - #151

Six pre-existing bugs found reviewing 6.0.0-dev.1#151
hpoul merged 6 commits into
mainfrom
pre-existing-bug-sweep

hpoul commented Aug 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

hpoul commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Android used a destroyed Activity after a configuration change

Reproduced and cured, on an emulator

1b. Android hung forever when the activity could not host a dialog

2. Android delivered an error message as the error code

3. Darwin init replied twice

4. Darwin reported a recoverable lockout as "plugin unsupported" — breaking

5. forceInit was implemented on Android only — breaking

6. win32 read() hid credential-store failures — breaking

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hpoul commented Aug 25, 2026 •

edited

Loading

1. Android used a destroyed `Activity` after a configuration change

3. Darwin `init` replied twice

5. `forceInit` was implemented on Android only — breaking

6. win32 `read()` hid credential-store failures — breaking