SELF-1497: add keychain patch #1607
Conversation
📝 WalkthroughWalkthroughAdded an Android "Use StrongBox" persisted setting with Dev UI toggle, wired it into keychain options via a new ExtendedSetOptions.useStrongBox flag, disabled Android auto-backup in the manifest, expanded keychain error matching, and bumped app version metadata for Android/iOS/package. Changes
Sequence DiagramsequenceDiagram
actor User as User
participant Dev as DevSettingsScreen
participant Store as SettingStore
participant Integration as KeychainIntegration
participant Native as NativeKeychain
participant StrongBox as AndroidStrongBox
User->>Dev: Toggle "Use StrongBox"
Dev->>User: Show confirmation Alert
User-->>Dev: Confirm
Dev->>Store: setUseStrongBox(boolean)
Store->>Store: Persist flag
Note over Integration,Store: On subsequent secure operations
Integration->>Store: read useStrongBox
Store-->>Integration: return boolean
Integration->>Native: setGenericPassword(ExtendedSetOptions)
Native->>StrongBox: create/keygen (useStrongBox flag)
StrongBox-->>Native: key result
Native-->>Integration: operation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@app/src/integrations/keychain/index.ts`:
- Around line 94-101: createKeychainOptions reads
useSettingStore.getState().useStrongBox synchronously which can return a default
value before AsyncStorage rehydration; guard this by checking store hydration
before falling back to getState (e.g., use useSettingStore.persist.hasHydrated()
and only read getState() if hydrated, or subscribe to onRehydrateStorage to
defer resolving useStrongBox), and update the logic around useStrongBox (the
options.useStrongBox ?? ...) so it prefers the explicit option, then the
persisted value only when persist.hasHydrated() is true, otherwise use a safe
fallback until rehydration completes.
🧹 Nitpick comments (1)
app/src/screens/dev/DevSettingsScreen.tsx (1)
759-785: LGTM! Platform-specific rendering correctly implemented.The
Platform.OS === 'android'guard ensures this section only renders on Android devices. The confirmation alert with clear messaging about the impact on new vs. existing keys is a nice UX touch.One minor observation:
TopicToggleButtonis semantically named for notification topics. Consider extracting a more generically-namedToggleButtoncomponent if this pattern is reused elsewhere.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/android/app/src/main/AndroidManifest.xmlapp/src/integrations/keychain/index.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/stores/settingStore.tsapp/src/types/react-native-keychain.d.tspatches/react-native-keychain+10.0.0.patch
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
app/src/types/react-native-keychain.d.tsapp/src/stores/settingStore.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/integrations/keychain/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx,js,jsx}: Use React Navigation withcreateStaticNavigationfor type-safe navigation in React Native applications.
Implement platform-specific handling withPlatform.OS === 'ios' ? 'iOS' : 'Android'checks before platform-specific code in React Native.
Initialize native modules withinitializeNativeModules()before any native operations in React Native.
Implement lazy loading for screens usingReact.lazy()in React Native applications.
Implement custom modal system withuseModalhook and callback registry in React Native.
Integrate haptic feedback usinguseHapticNavigationhook in React Native navigation.
Use platform-specific initial routes: web uses 'Home', mobile uses 'Splash' in React Navigation.
Use Zustand for global state management in React Native applications.
Use custom hooks for complex state (useModal,useHapticNavigation) instead of inline logic.
Use AsyncStorage for simple data, SQLite for complex data, and Keychain for sensitive data in React Native.
Use@/alias for src imports and@tests/alias for test imports in TypeScript/JavaScript files.
Use conditional rendering with Platform.OS for platform-specific code in React Native.
Use Tamagui for UI components in React Native applications.
Do not log sensitive data in production, including identity verification and passport information.
Use Keychain for secure storage of sensitive data in React Native.
Implement proper cleanup of sensitive data after use.
Implement certificate validation for passport data verification.
Always use try-catch for async operations in React Native and TypeScript code.
Implement graceful degradation when native modules fail in React Native.
Provide user-friendly error messages in UI and error handlers.
Lazy load screens and components to optimize bundle size in React Native.
Prevent memory leaks in native modules in React Native.
Files:
app/src/types/react-native-keychain.d.tsapp/src/stores/settingStore.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/integrations/keychain/index.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (.cursorrules)
Implement proper cleanup in useEffect and component unmount hooks in React.
Files:
app/src/types/react-native-keychain.d.tsapp/src/stores/settingStore.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/integrations/keychain/index.ts
**/{mobile,client,app,time,verification}/**/*.{ts,tsx,js,swift,kt}
📄 CodeRabbit inference engine (.cursor/rules/compliance-verification.mdc)
Use server-signed time tokens or chain block timestamps for trusted time in mobile clients, do not trust device wall-clock alone
Files:
app/src/types/react-native-keychain.d.tsapp/src/stores/settingStore.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/integrations/keychain/index.ts
**/{mobile,client,app,proof,zk}/**/*.{ts,tsx,js,swift,kt}
📄 CodeRabbit inference engine (.cursor/rules/compliance-verification.mdc)
**/{mobile,client,app,proof,zk}/**/*.{ts,tsx,js,swift,kt}: Include trusted time anchor in proof generation and verify time anchor authenticity before proof generation in mobile implementations
Achieve proof generation in <60 seconds on mid-tier mobile devices
Files:
app/src/types/react-native-keychain.d.tsapp/src/stores/settingStore.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/integrations/keychain/index.ts
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Ensure
yarn typespasses (TypeScript validation) before creating a PR
Files:
app/src/types/react-native-keychain.d.tsapp/src/stores/settingStore.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/integrations/keychain/index.ts
app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
app/**/*.{ts,tsx,js,jsx}: Ensure web build succeeds withyarn webbefore creating a PR
Do not include sensitive data in logs - avoid logging PII, credentials, and tokens
Usereact-native-dotenvfor environment configuration via@envimport
Confirm no sensitive data exposed before PR merge
Files:
app/src/types/react-native-keychain.d.tsapp/src/stores/settingStore.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/integrations/keychain/index.ts
app/src/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
app/src/**/*.{ts,tsx,js,jsx}: Review React Native TypeScript code for:
- Component architecture and reusability
- State management patterns
- Performance optimizations
- TypeScript type safety
- React hooks usage and dependencies
- Navigation patterns
Files:
app/src/types/react-native-keychain.d.tsapp/src/stores/settingStore.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/integrations/keychain/index.ts
app/android/**/*
📄 CodeRabbit inference engine (app/AGENTS.md)
Ensure Android build succeeds with
yarn androidbefore creating a PR
Files:
app/android/app/src/main/AndroidManifest.xml
⚙️ CodeRabbit configuration file
app/android/**/*: Review Android-specific code for:
- Platform-specific implementations
- Performance considerations
- Security best practices for mobile
Files:
app/android/app/src/main/AndroidManifest.xml
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursorrules)
Implement comprehensive error boundaries in React components.
Files:
app/src/screens/dev/DevSettingsScreen.tsx
🧠 Learnings (16)
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Keychain for secure storage of sensitive data in React Native.
Applied to files:
app/src/types/react-native-keychain.d.tsapp/src/screens/dev/DevSettingsScreen.tsxapp/src/integrations/keychain/index.ts
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Prevent memory leaks in native modules in React Native.
Applied to files:
app/src/types/react-native-keychain.d.ts
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Implement platform-specific handling with `Platform.OS === 'ios' ? 'iOS' : 'Android'` checks before platform-specific code in React Native.
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-12-25T19:19:35.354Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.{ts,tsx} : Verify cross-platform compatibility for both React Native and Web environments
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-12-25T19:19:35.354Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.{ts,tsx} : Ensure no breaking changes to public API or document them properly
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Tamagui for UI components in React Native applications.
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Implement custom modal system with `useModal` hook and callback registry in React Native.
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use React Navigation with `createStaticNavigation` for type-safe navigation in React Native applications.
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Integrate haptic feedback using `useHapticNavigation` hook in React Native navigation.
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Zustand for global state management in React Native applications.
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use custom hooks for complex state (`useModal`, `useHapticNavigation`) instead of inline logic.
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use conditional rendering with Platform.OS for platform-specific code in React Native.
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-08-26T14:41:41.821Z
Learnt from: shazarre
Repo: selfxyz/self PR: 936
File: app/src/screens/aesop/PassportOnboardingScreen.tsx:0-0
Timestamp: 2025-08-26T14:41:41.821Z
Learning: When verifying provider hierarchies in React Native apps, always check the main App.tsx file at the app root, not just navigation/index.tsx and layout files, as providers are often configured at the top-level App component.
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-12-25T19:19:04.954Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: app/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:04.954Z
Learning: Applies to app/**/*.test.{ts,tsx,js,jsx} : Avoid nested `require('react')` or `require('react-native')` calls in test files - use ES6 import statements instead
Applied to files:
app/src/screens/dev/DevSettingsScreen.tsx
📚 Learning: 2025-11-25T14:07:28.188Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursor/rules/compliance-verification.mdc:0-0
Timestamp: 2025-11-25T14:07:28.188Z
Learning: Applies to **/{compliance,crypto,key,security,auth}/**/*.{ts,tsx,js,py} : Implement proper key rotation and secure storage for compliance verification keys
Applied to files:
app/src/integrations/keychain/index.ts
📚 Learning: 2025-11-25T14:06:55.970Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use AsyncStorage for simple data, SQLite for complex data, and Keychain for sensitive data in React Native.
Applied to files:
app/src/integrations/keychain/index.ts
🧬 Code graph analysis (2)
app/src/screens/dev/DevSettingsScreen.tsx (2)
app/src/stores/settingStore.ts (1)
useSettingStore(60-174)packages/mobile-sdk-demo/tests/mocks/react-native.ts (2)
Platform(14-25)Alert(6-6)
app/src/integrations/keychain/index.ts (2)
app/src/types/react-native-keychain.d.ts (1)
ExtendedSetOptions(11-21)app/src/stores/settingStore.ts (1)
useSettingStore(60-174)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: android-build-test
🔇 Additional comments (7)
app/android/app/src/main/AndroidManifest.xml (1)
28-29: LGTM! Solid security hardening.Disabling
android:allowBackupprevents the Android backup system from exfiltrating sensitive keystore data to cloud storage. This is the right call for an app managing cryptographic secrets and passport data.app/src/stores/settingStore.ts (2)
41-47: LGTM! Clean addition to the persisted settings.The
useStrongBoxsetting follows the established store patterns with proper typing. Defaulting totrueis the right security-first choice.
152-155: LGTM! Implementation is consistent with other settings.The setter and initial value follow the existing conventions in this store.
app/src/integrations/keychain/index.ts (2)
65-69: Good defensive choice for the passcode check.Hardcoding
useStrongBox: falsefor the test entry ensures the passcode availability check won't fail on devices with StrongBox issues. The type assertion toExtendedSetOptionsis necessary here.
13-14: Imports are correctly structured.The store import and custom type import align with the codebase's
@/alias pattern.app/src/screens/dev/DevSettingsScreen.tsx (1)
402-403: Store bindings look good.The hook usage follows the Zustand selector pattern correctly.
app/src/types/react-native-keychain.d.ts (1)
1-21: Patch is properly implemented and integrated end-to-end.The
react-native-keychain+10.0.0.patchexists and is applied via patch-package during postinstall. The native Kotlin implementation correctly handlesuseStrongBoxwith proper fallback logic (if (useStrongBox && supportsSecureHardware)), and the type definition accurately reflects this. Integration is complete: the keychain integration layer, Zustand store, and dev UI all properly use the setting. No issues identified.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const useStrongBox = | ||
| options.useStrongBox ?? useSettingStore.getState().useStrongBox; | ||
|
|
||
| const setOptions: ExtendedSetOptions = { | ||
| accessible: config.accessible, | ||
| ...(config.securityLevel && { securityLevel: config.securityLevel }), | ||
| ...(config.accessControl && { accessControl: config.accessControl }), | ||
| useStrongBox, |
There was a problem hiding this comment.
Potential race condition with store rehydration.
useSettingStore.getState().useStrongBox accesses the persisted store synchronously. If createKeychainOptions is called early during app startup before AsyncStorage rehydration completes, it may read the default true instead of the user's persisted preference.
Consider checking useSettingStore.persist.hasHydrated() or subscribing to onRehydrateStorage if this function can be invoked during initial app boot.
🛠️ Potential safeguard
const useStrongBox =
- options.useStrongBox ?? useSettingStore.getState().useStrongBox;
+ options.useStrongBox ??
+ (useSettingStore.persist.hasHydrated()
+ ? useSettingStore.getState().useStrongBox
+ : true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const useStrongBox = | |
| options.useStrongBox ?? useSettingStore.getState().useStrongBox; | |
| const setOptions: ExtendedSetOptions = { | |
| accessible: config.accessible, | |
| ...(config.securityLevel && { securityLevel: config.securityLevel }), | |
| ...(config.accessControl && { accessControl: config.accessControl }), | |
| useStrongBox, | |
| const useStrongBox = | |
| options.useStrongBox ?? | |
| (useSettingStore.persist.hasHydrated() | |
| ? useSettingStore.getState().useStrongBox | |
| : true); | |
| const setOptions: ExtendedSetOptions = { | |
| accessible: config.accessible, | |
| ...(config.securityLevel && { securityLevel: config.securityLevel }), | |
| ...(config.accessControl && { accessControl: config.accessControl }), | |
| useStrongBox, |
🤖 Prompt for AI Agents
In `@app/src/integrations/keychain/index.ts` around lines 94 - 101,
createKeychainOptions reads useSettingStore.getState().useStrongBox
synchronously which can return a default value before AsyncStorage rehydration;
guard this by checking store hydration before falling back to getState (e.g.,
use useSettingStore.persist.hasHydrated() and only read getState() if hydrated,
or subscribe to onRehydrateStorage to defer resolving useStrongBox), and update
the logic around useStrongBox (the options.useStrongBox ?? ...) so it prefers
the explicit option, then the persisted value only when persist.hasHydrated() is
true, otherwise use a safe fallback until rehydration completes.
There was a problem hiding this comment.
That’s acceptable
There was a problem hiding this comment.
@seshanthS, understood. I've noted your decision. The implementation will proceed with the current approach.
🧠 Learnings used
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Keychain for secure storage of sensitive data in React Native.
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use AsyncStorage for simple data, SQLite for complex data, and Keychain for sensitive data in React Native.
Learnt from: transphorm
Repo: selfxyz/self PR: 1516
File: app/src/screens/account/recovery/RecoverWithPhraseScreen.tsx:51-52
Timestamp: 2025-12-17T15:27:42.298Z
Learning: In app/src/screens/account/recovery/RecoverWithPhraseScreen.tsx, ethers.Mnemonic.isValidMnemonic should not be used as a gate for pasting recovery phrases because it produces false negatives and rejects valid mnemonics. Allow users to paste any clipboard content; validation should only occur when the user attempts to restore the account.
seshanthS
changed the title
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on January 17. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
transphorm
changed the title
transphorm
changed the title
seshanthS
changed the title
Description
Disables strongbox by default. Can be enabled optionally in DevSettingScreen
Tested
How to QA
Generating regular security key for alias {...}adb logcat --pid=$(adb shell pidof -s com.proofofpassportapp)Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.