feat: invalidate biometric keys when biometric is changed on ios#79
Merged
Conversation
📝 WalkthroughWalkthroughAdds per-key biometric domain-state persistence and change detection on iOS; integrates these checks into key creation, deletion, integrity validation, and signature verification; and updates access-control creation to optionally use Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant RNBiometrics as RNBiometrics (iOS)
participant LA as LocalAuthentication (LAContext)
participant Keychain
participant Storage as UserDefaults
App->>RNBiometrics: createKeys(alias, biometricStrength, allowDeviceCredentials)
RNBiometrics->>LA: evaluatePolicy / get evaluatedPolicyDomainState
LA-->>RNBiometrics: domainState (base64)
RNBiometrics->>RNBiometrics: compute storage key for alias
RNBiometrics->>Keychain: create SecKey with SecAccessControl(useBiometryCurrentSet?)
Keychain-->>RNBiometrics: key created
alt allowDeviceCredentials == true
RNBiometrics->>Storage: clear stored domain state for alias
else
RNBiometrics->>Storage: persist domainState for alias
end
App->>RNBiometrics: verifyKeySignature(alias, data)
RNBiometrics->>Storage: read stored domainState for alias
RNBiometrics->>LA: obtain current domainState
RNBiometrics->>RNBiometrics: compare stored vs current
alt domain state changed
RNBiometrics-->>App: { success: false, error: BIOMETRY_CURRENT_SET_CHANGED }
else
RNBiometrics->>Keychain: locate SecKey and attempt signing/verification
Keychain-->>RNBiometrics: sign/verify result
RNBiometrics-->>App: { success: true/false, details }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/ReactNativeBiometrics.swift`:
- Around line 61-78: hasBiometricDomainStateChanged currently returns false when
LAContext.canEvaluatePolicy fails (biometrics unavailable/unenrolled), which
hides the change; update hasBiometricDomainStateChanged (and its use of
LAContext.canEvaluatePolicy and evaluatedPolicyDomainState) to treat a failed
canEvaluatePolicy as a domain-state change (return true) or propagate/translate
it into the biometryCurrentSetChanged condition so callers will re-enroll;
specifically, after creating LAContext() and calling
context.canEvaluatePolicy(.deviceOwnerAuthenticationWithBiometrics, error:
&error), if it returns false or context.evaluatedPolicyDomainState is nil,
return true (or surface the equivalent error) instead of false, keeping
biometricDomainStateStorageKey lookup and storedDomainState comparison intact.
In `@ios/Utils.swift`:
- Around line 165-172: Changing the SecAccessControlCreateFlags from
.biometryAny to .biometryCurrentSet is a breaking change for existing keys;
update the code and release to handle migration by either (1) attempting to
access existing keys by trying both policies when reading keys (check code paths
that use authConstraint and the allowDeviceCredentialsFallback decision), (2)
providing a migration path to recreate/re-enroll keys created with .biometryAny,
or (3) gating the new behavior behind a flag and documenting it; also add clear
release notes/migration guidance stating that keys created prior to this change
remain bound to .biometryAny and will be invalidated by biometric enrollment
changes when using .biometryCurrentSet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8063956a-6c18-41cc-a8ee-74adf256f39a
📒 Files selected for processing (3)
ios/ReactNativeBiometrics.swiftios/ReactNativeBiometricsError.swiftios/Utils.swift
Owner
|
@omar0404 can you please resolve coderabbit threads ? |
Author
done , waiting for the second review |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/ReactNativeBiometrics.swift (1)
565-577:⚠️ Potential issue | 🟡 MinorClear the cached domain state only after deletion is confirmed.
The
errSecSuccessbranch removes the cached state before the follow-up lookup verifies that the key is actually gone. If that verification fails, the key remains but future enrollment-change checks for that alias are silently disabled.Suggested fix
case errSecSuccess: - clearStoredBiometricDomainState(for: keyTag) ReactNativeBiometricDebug.debugLog("Key with tag '\(keyTag)' deleted successfully") // Verify deletion let verifyStatus = SecItemCopyMatching(query as CFDictionary, nil) if verifyStatus == errSecItemNotFound { + clearStoredBiometricDomainState(for: keyTag) ReactNativeBiometricDebug.debugLog("Keys deleted and verified successfully") resolve(["success": true]) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/ReactNativeBiometrics.swift` around lines 565 - 577, The code currently calls clearStoredBiometricDomainState(for: keyTag) immediately when deleteStatus == errSecSuccess before verifying the key removal; change the flow in the switch branch so that you only call clearStoredBiometricDomainState(for: keyTag) after the follow-up verification (the SecItemCopyMatching call stored in verifyStatus) confirms the key is gone (verifyStatus == errSecItemNotFound) and you resolve(["success": true]); if verification fails, do not clear the cached state and instead run the existing error path (handleError(.keyDeletionFailed, reject: reject)) so enrollment-change checks remain intact for that alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/ReactNativeBiometrics.swift`:
- Around line 441-443: The branch currently computes biometricStrengthValue and
useBiometryCurrentSet but still stores domain state for all non-fallback keys;
change the domain-state storage logic so it only records domain state when
useBiometryCurrentSet is true (i.e., only for .biometryCurrentSet keys) to avoid
blocking .biometryAny/default/weak keys after enrollment changes; apply the same
conditional fix in the other occurrence around the biometric domain-state
storage lines (the block referenced at 523–527) and ensure
hasBiometricDomainStateChanged checks remain compatible with this restricted
storage behavior.
- Around line 69-75: The guard conflates all canEvaluatePolicy failures with
enrollment changes; modify the logic around LAContext.canEvaluatePolicy and
evaluatedPolicyDomainState so you inspect the NSError (from canEvaluatePolicy)
and only treat LAError.biometryNotEnrolled as an enrollment change (return false
or trigger re-enroll), while treating LAError.biometryLockout as a temporary
state (return true / do not trigger re-enroll). Concretely, call
context.canEvaluatePolicy(.deviceOwnerAuthenticationWithBiometrics, error:
&error), then if it returns false check (error as? LAError)?.code ==
.biometryNotEnrolled -> handle as enrollment-changed, else if .biometryLockout
-> treat as temporary and return success, and if evaluatedPolicyDomainState is
nil then treat that as an enrollment change.
---
Outside diff comments:
In `@ios/ReactNativeBiometrics.swift`:
- Around line 565-577: The code currently calls
clearStoredBiometricDomainState(for: keyTag) immediately when deleteStatus ==
errSecSuccess before verifying the key removal; change the flow in the switch
branch so that you only call clearStoredBiometricDomainState(for: keyTag) after
the follow-up verification (the SecItemCopyMatching call stored in verifyStatus)
confirms the key is gone (verifyStatus == errSecItemNotFound) and you
resolve(["success": true]); if verification fails, do not clear the cached state
and instead run the existing error path (handleError(.keyDeletionFailed, reject:
reject)) so enrollment-change checks remain intact for that alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0942dd8-1499-44e2-b27f-fa221aa0532b
📒 Files selected for processing (4)
README.mddocs/CRYPTOGRAPHIC_KEYS.mdios/ReactNativeBiometrics.swiftios/Utils.swift
✅ Files skipped from review due to trivial changes (2)
- docs/CRYPTOGRAPHIC_KEYS.md
- README.md
Owner
|
Thank, and sorry for the delay it was Holidays for us |
Owner
|
can you please use rebase (thats what we use in the branch) 
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 830-838: Remove the duplicated `failIfExists` documentation line
so the parameter is documented only once; keep the iOS-specific clarifications
for `biometricStrength` and `allowDeviceCredentials` intact and ensure the
single `failIfExists` entry (using `failIfExists` and `KEY_ALREADY_EXISTS`)
remains in the section, deleting the redundant duplicate line to avoid
repetition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b43a44b-9040-4b3d-9b0b-6bd20714001a
📒 Files selected for processing (5)
README.mddocs/CRYPTOGRAPHIC_KEYS.mdios/ReactNativeBiometrics.swiftios/ReactNativeBiometricsError.swiftios/Utils.swift
✅ Files skipped from review due to trivial changes (2)
- docs/CRYPTOGRAPHIC_KEYS.md
- ios/ReactNativeBiometrics.swift
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
3 participants
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.
#77
Summary by CodeRabbit
New Features
Improvements
Documentation