SELF-799: Hotfix/audit fixes #1193
Conversation
WalkthroughThe PR adds adaptive, biometric-aware keychain security utilities and integrates them across auth, passport, and splash flows (including a migration path). It updates Loading navigation to accept route params and derives those params from documents. Android manifests and network security configs are added to restrict cleartext by default and allow debug localhost exceptions. Settings persist migration state. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Screen as Screen/Provider
participant Auth as AuthProvider
participant Sec as keychainSecurity
participant KC as RN_Keychain
participant Bio as Biometrics
User->>Screen: Request secure data
Screen->>Auth: _getWithBiometrics(fn, formatter, {requireAuth:true})
Auth->>Sec: detectSecurityCapabilities()
Sec-->>Auth: capabilities
Auth->>Bio: prompt user (if requireAuth)
Bio-->>Auth: success / failure
alt biometrics success
Auth->>Sec: createKeychainOptions({requireAuth:true})
Sec-->>Auth: {getOptions,setOptions}
Auth->>KC: getGenericPassword(getOptions)
KC-->>Auth: secret / error
Auth-->>Screen: signed payload / throw
else fail
Auth-->>Screen: authentication error
end
sequenceDiagram
autonumber
participant App as SplashScreen
participant Auth as AuthProvider
participant Sec as keychainSecurity
participant KC as RN_Keychain
participant Store as settingStore
App->>Auth: migrateToSecureKeychain()
Auth->>Store: check hasCompletedKeychainMigration
alt not completed
Auth->>Sec: detectSecurityCapabilities()
Auth->>Sec: createKeychainOptions({requireAuth:false})
Auth->>KC: read existing mnemonic
KC-->>Auth: mnemonic / none
alt mnemonic exists
Auth->>KC: write mnemonic with secure setOptions
KC-->>Auth: ok / err
Auth->>Store: setKeychainMigrationCompleted()
else none
Auth-->>App: nothing to migrate
end
else already migrated
Auth-->>App: skip
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/providers/authProvider.tsx (1)
69-95: Biometric helper ignores security options
_getWithBiometricsdrops theoptionsargument entirely, sorequireAuth/prompt metadata never influence the Keychain call. Callers (e.g. passportDataProvider) expect this function to honorrequireAuth: true; today it just runsfn()with default Keychain settings, bypassing the secure access-control you introduced. Please mirror_getSecurelyby derivingkeychainOptionsfromcreateKeychainOptionsand passing them intofn, or change the signature so callers can’t provide an option you silently ignore.
🧹 Nitpick comments (1)
app/src/screens/system/SplashScreen.tsx (1)
77-82: Add automatic migration via Keychain.SECURITY_RULES.AUTOMATIC_UPGRADE
migrateToSecureKeychain(andcreateKeychainOptions) currently omitsecurityRules: include
securityRules: Keychain.SECURITY_RULES.AUTOMATIC_UPGRADEin yoursetOptionsfor transparent legacy-item upgrades.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/android/app/src/debug/AndroidManifest.xml(1 hunks)app/android/app/src/main/AndroidManifest.xml(1 hunks)app/src/navigation/system.tsx(2 hunks)app/src/providers/authProvider.tsx(12 hunks)app/src/providers/passportDataProvider.tsx(6 hunks)app/src/screens/prove/ConfirmBelongingScreen.tsx(2 hunks)app/src/screens/system/Loading.tsx(7 hunks)app/src/screens/system/SplashScreen.tsx(2 hunks)app/src/stores/settingStore.ts(2 hunks)app/src/utils/keychainSecurity.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/android/**/*
⚙️ 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/debug/AndroidManifest.xmlapp/android/app/src/main/AndroidManifest.xml
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: 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/screens/prove/ConfirmBelongingScreen.tsxapp/src/screens/system/SplashScreen.tsxapp/src/stores/settingStore.tsapp/src/navigation/system.tsxapp/src/screens/system/Loading.tsxapp/src/providers/authProvider.tsxapp/src/providers/passportDataProvider.tsxapp/src/utils/keychainSecurity.ts
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Type checking must pass before PRs (yarn types)
Files:
app/src/screens/prove/ConfirmBelongingScreen.tsxapp/src/screens/system/SplashScreen.tsxapp/src/stores/settingStore.tsapp/src/navigation/system.tsxapp/src/screens/system/Loading.tsxapp/src/providers/authProvider.tsxapp/src/providers/passportDataProvider.tsxapp/src/utils/keychainSecurity.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/screens/prove/ConfirmBelongingScreen.tsxapp/src/screens/system/SplashScreen.tsxapp/src/stores/settingStore.tsapp/src/navigation/system.tsxapp/src/screens/system/Loading.tsxapp/src/providers/authProvider.tsxapp/src/providers/passportDataProvider.tsxapp/src/utils/keychainSecurity.ts
🧠 Learnings (3)
📚 Learning: 2025-09-10T14:47:40.945Z
Learnt from: shazarre
PR: selfxyz/self#1041
File: app/src/providers/passportDataProvider.tsx:297-301
Timestamp: 2025-09-10T14:47:40.945Z
Learning: In app/src/providers/passportDataProvider.tsx: The deleteDocumentDirectlyFromKeychain function is a low-level utility used by the DocumentsAdapter and should not include error handling since callers like deleteDocument() already implement appropriate try/catch with logging for Keychain operations.
Applied to files:
app/src/providers/passportDataProvider.tsx
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Test isPassportDataValid() with realistic synthetic passport data (never real user data)
Applied to files:
app/src/providers/passportDataProvider.tsx
📚 Learning: 2025-08-26T14:42:45.297Z
Learnt from: aaronmgdr
PR: selfxyz/self#936
File: app/src/utils/proving/validateDocument.ts:53-54
Timestamp: 2025-08-26T14:42:45.297Z
Learning: In the Self app, native modules (including Keychain) are initialized early in the app startup sequence, before functions like checkAndUpdateRegistrationStates() are called, so additional native module readiness guards in document-related functions are not needed.
Applied to files:
app/src/providers/passportDataProvider.tsx
🧬 Code graph analysis (4)
app/src/screens/system/SplashScreen.tsx (1)
app/src/providers/authProvider.tsx (1)
migrateToSecureKeychain(332-379)
app/src/screens/system/Loading.tsx (1)
common/src/utils/types.ts (1)
DocumentCategory(37-37)
app/src/providers/authProvider.tsx (4)
app/src/utils/keychainSecurity.ts (3)
GetSecurelyOptions(22-25)detectSecurityCapabilities(115-133)createKeychainOptions(76-110)packages/mobile-sdk-alpha/src/constants/analytics.ts (1)
AuthEvents(52-65)app/src/stores/settingStore.ts (1)
useSettingStore(37-96)app/src/providers/authProvider.web.tsx (1)
unsafe_getPrivateKey(285-287)
app/src/providers/passportDataProvider.tsx (2)
app/src/providers/authProvider.tsx (1)
useAuth(407-409)app/src/utils/keychainSecurity.ts (1)
createKeychainOptions(76-110)
🔇 Additional comments (14)
app/android/app/src/main/AndroidManifest.xml (1)
24-24: Security improvement: cleartext traffic disabled.Disabling cleartext traffic in production is a security best practice that prevents unencrypted HTTP connections. The debug manifest properly allows override for development needs.
app/android/app/src/debug/AndroidManifest.xml (1)
15-15: LGTM!The debug manifest correctly allows overriding the cleartext traffic setting for development, complementing the production security hardening in the main manifest.
app/src/navigation/system.tsx (1)
29-33: LGTM!The route parameter extension properly types the cryptographic metadata fields and maintains backward compatibility with optional params. This aligns with the Loading screen's new param-driven behavior.
app/src/screens/prove/ConfirmBelongingScreen.tsx (2)
37-48: LGTM!The document metadata state and navigation parameter passing properly support dynamic parameterization of the proving workflow. Type casting for documentCategory is appropriate.
60-89: LGTM!The initialization logic correctly:
- Sets Aadhaar defaults (rsa/65537) for Aadhaar documents
- Extracts passport metadata from loaded document data
- Falls back to passport defaults on error
The error handling ensures the flow continues with sensible defaults.
app/src/providers/passportDataProvider.tsx (3)
150-158: Biometric-gated retrieval properly implemented.The getData method now correctly uses
_getWithBiometricswithrequireAuth: trueto enforce biometric authentication before accessing passport data. This aligns with the keychain security enhancements.
165-196: LGTM!All secure data retrieval methods now enforce authentication via
requireAuth: true:
- getSelectedData
- getPassportDataAndSecret
- getSelectedPassportDataAndSecret
This provides consistent biometric protection across the provider.
772-777: Require authentication for document storage writes? BothsaveDocumentCatalogDirectlyToKeychain(L740–747) andstoreDocumentDirectlyToKeychain(L768–777) usecreateKeychainOptions({ requireAuth: false }); consider settingrequireAuth: truefor sensitive passport/ID data writes or confirm that omitting write-time auth aligns with your security model.app/src/stores/settingStore.ts (2)
23-24: LGTM!The migration flag interface extension properly tracks keychain migration completion status.
74-76: LGTM!The migration flag is correctly initialized to
falseand persisted to AsyncStorage, preventing redundant migrations on subsequent app launches.app/src/screens/system/Loading.tsx (4)
29-35: LGTM!The param type definition properly extends the screen props contract with optional cryptographic metadata fields, maintaining backward compatibility.
64-68: LGTM!Safe param extraction with optional chaining and destructuring fallback prevents runtime errors if route or params are undefined.
103-110: LGTM!The param usage correctly:
- Provides sensible defaults (rsa/65537) when params are missing
- Uses provided params when available
- Maintains backward compatibility with existing flows
151-157: LGTM!Effect dependencies correctly include the new param values to ensure proper re-renders when navigation params change.
| const { setOptions } = await createKeychainOptions({ requireAuth: false }); | ||
| await Keychain.setGenericPassword('catalog', JSON.stringify(catalog), { | ||
| service: 'documentCatalog', | ||
| ...setOptions, | ||
| // securityLevel: Keychain.SECURITY_LEVEL.SECURE_HARDWARE, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify adaptive security configuration for catalog storage.
The catalog is now saved using createKeychainOptions, but with requireAuth: false. Ensure this is intentional and that the catalog doesn't contain sensitive data requiring authentication.
Review the catalog structure to confirm it only contains metadata (not sensitive document content):
🏁 Script executed:
#!/bin/bash
# Check DocumentCatalog type definition to verify it contains only metadata
ast-grep --pattern 'interface DocumentCatalog {
$$$
}'
ast-grep --pattern 'type DocumentCatalog = {
$$$
}'Length of output: 245
🏁 Script executed:
#!/bin/bash
# Check DocumentMetadata interface to verify it contains only metadata
ast-grep --pattern 'interface DocumentMetadata {
$$$
}'Length of output: 1239
Require authentication when storing the catalog
The data field in DocumentMetadata contains MRZ/PII; using requireAuth: false exposes sensitive data. Update to requireAuth: true in app/src/providers/passportDataProvider.tsx (lines 740–745).
🤖 Prompt for AI Agents
In app/src/providers/passportDataProvider.tsx around lines 740 to 745, the
Keychain write currently disables authentication which stores sensitive MRZ/PII
in plaintext; change the createKeychainOptions call to use requireAuth: true so
the returned setOptions enforces user authentication, and keep passing those
setOptions into Keychain.setGenericPassword (remove or comment any override that
disables auth) so the catalog is stored with device authentication protections.
| try { | ||
| const testService = `passcode-test-${Date.now()}`; | ||
| await Keychain.setGenericPassword('test', 'test', { | ||
| service: testService, | ||
| accessible: Keychain.ACCESSIBLE.WHEN_PASSCODE_SET_THIS_DEVICE_ONLY, | ||
| }); | ||
| // Clean up test entry | ||
| await Keychain.resetGenericPassword({ service: testService }); | ||
| return true; | ||
| } catch (error) { | ||
| console.log('Device passcode not available:', error); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Android passcode detection is unreliable
This probe uses accessible: WHEN_PASSCODE_SET_THIS_DEVICE_ONLY to infer a passcode. That flag is iOS-only; on Android it’s ignored, so the write succeeds even when no device credential exists. Downstream you treat hasPasscode as true and request ACCESS_CONTROL.DEVICE_PASSCODE, which then blows up for every Android user without a PIN/pattern. Please gate this logic to iOS (e.g. Platform.OS === 'ios') and derive Android passcode support from Keychain.getSecurityLevel()/SECURITY_LEVEL instead.
🤖 Prompt for AI Agents
In app/src/utils/keychainSecurity.ts around lines 58 to 70, the current probe
uses accessible: WHEN_PASSCODE_SET_THIS_DEVICE_ONLY (iOS-only) to infer a device
passcode, which is ignored on Android and yields false positives; update the
logic so it only uses that iOS-only check when Platform.OS === 'ios' and for
Android call Keychain.getSecurityLevel() (or compare to Keychain.SECURITY_LEVEL
constants) to determine whether device credentials exist before returning true
or requesting ACCESS_CONTROL.DEVICE_PASSCODE; i.e., gate the current try/catch
behind an iOS platform check and implement an Android branch that derives
hasPasscode from getSecurityLevel() (mapping the relevant SECURITY_LEVEL to a
boolean) and return false when the Android level indicates no device credential.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/screens/system/SplashScreen.tsx (1)
77-82: Delay biometric‐protected keychain migration until after onboarding
Invoking migrateToSecureKeychain() on the splash screen triggers a biometric prompt via Keychain accessControl before the user’s context is ready. Defer this call to a post‐login or home screen flow, or surface a brief UI explaining the upcoming biometric prompt.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/providers/authProvider.tsx(12 hunks)app/src/providers/authProvider.web.tsx(1 hunks)app/src/screens/prove/ConfirmBelongingScreen.tsx(2 hunks)app/src/screens/system/Loading.tsx(6 hunks)app/src/screens/system/SplashScreen.tsx(2 hunks)app/src/stores/settingStore.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/stores/settingStore.ts
- app/src/screens/prove/ConfirmBelongingScreen.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: 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/screens/system/SplashScreen.tsxapp/src/providers/authProvider.web.tsxapp/src/providers/authProvider.tsxapp/src/screens/system/Loading.tsx
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Type checking must pass before PRs (yarn types)
Files:
app/src/screens/system/SplashScreen.tsxapp/src/providers/authProvider.web.tsxapp/src/providers/authProvider.tsxapp/src/screens/system/Loading.tsx
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/screens/system/SplashScreen.tsxapp/src/providers/authProvider.web.tsxapp/src/providers/authProvider.tsxapp/src/screens/system/Loading.tsx
app/**/*.{ios,android,web}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Explain platform-specific code paths in the PR description (files with .ios, .android, or .web extensions)
Files:
app/src/providers/authProvider.web.tsx
🧠 Learnings (3)
📚 Learning: 2025-09-10T14:47:40.945Z
Learnt from: shazarre
PR: selfxyz/self#1041
File: app/src/providers/passportDataProvider.tsx:297-301
Timestamp: 2025-09-10T14:47:40.945Z
Learning: In app/src/providers/passportDataProvider.tsx: The deleteDocumentDirectlyFromKeychain function is a low-level utility used by the DocumentsAdapter and should not include error handling since callers like deleteDocument() already implement appropriate try/catch with logging for Keychain operations.
Applied to files:
app/src/providers/authProvider.web.tsx
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to **/*.{js,ts,tsx,jsx,sol,nr} : NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
Applied to files:
app/src/providers/authProvider.tsx
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to **/*.{js,ts,tsx,jsx,sol,nr} : ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g., `***-***-1234` for passport numbers, `J*** D***` for names).
Applied to files:
app/src/providers/authProvider.tsx
🧬 Code graph analysis (4)
app/src/screens/system/SplashScreen.tsx (1)
app/src/providers/authProvider.tsx (1)
migrateToSecureKeychain(329-375)
app/src/providers/authProvider.web.tsx (1)
app/src/providers/authProvider.tsx (1)
migrateToSecureKeychain(329-375)
app/src/providers/authProvider.tsx (2)
app/src/utils/keychainSecurity.ts (3)
GetSecurelyOptions(22-25)detectSecurityCapabilities(115-133)createKeychainOptions(76-110)app/src/stores/settingStore.ts (1)
useSettingStore(39-100)
app/src/screens/system/Loading.tsx (2)
common/src/utils/types.ts (1)
DocumentCategory(37-37)app/src/utils/haptic/index.ts (1)
loadingScreenProgress(115-133)
⏰ 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). (6)
- GitHub Check: build-deps
- GitHub Check: android-build-test
- GitHub Check: e2e-ios
- GitHub Check: workspace-lint
- GitHub Check: analyze-ios
- GitHub Check: analyze-android
🔇 Additional comments (14)
app/src/providers/authProvider.web.tsx (1)
274-277: LGTM! Web stub is appropriate.The web implementation correctly provides a no-op stub that matches the non-web signature. The warning message is clear, and returning
falseappropriately indicates no migration occurred.app/src/screens/system/SplashScreen.tsx (1)
18-18: LGTM! Import is correct.The import of
migrateToSecureKeychainis properly added and matches the exported function from the authProvider.app/src/screens/system/Loading.tsx (5)
31-37: LGTM! Type definitions are well-structured.The
LoadingScreenParamstype with optional fields and the use ofStaticScreenPropsfollows React Navigation best practices. The optional nature of the fields provides flexibility while maintaining type safety.
49-49: LGTM! Component signature correctly typed.The component properly receives and types the
routeprop using the definedLoadingScreenPropstype.
73-77: LGTM! Safe parameter extraction.The parameter extraction from
route.paramswith an empty object fallback is safe and properly handles undefined route params.
122-132: LGTM! Simplified useEffect is cleaner.Removing the isMounted-based logic and simplifying to focus-based notification setup is a good refactor. The cleanup function safely handles the unsubscribe.
143-150: LGTM! Fallback logic is sound.The fallback to RSA defaults (
'rsa'and'65537') when params are not provided is appropriate. The conditional check ensures both params are used together or both default.app/src/providers/authProvider.tsx (7)
15-15: LGTM! Imports and type definition are correct.The keychain security imports and the
KeychainOptionstype provide a solid foundation for the adaptive security implementation.Also applies to: 22-26, 33-36
37-67: Review error logging for sensitive data.The updated
_getSecurelyfunction properly integrates adaptive security. However,console.error('Error in _getSecurely:', error);at line 59 could log sensitive information if the error object contains keychain data or user information.Apply this diff to sanitize the error log:
} catch (error: unknown) { - console.error('Error in _getSecurely:', error); + console.error('Error in _getSecurely'); const message = error instanceof Error ? error.message : String(error); trackEvent(AuthEvents.BIOMETRIC_AUTH_FAILED, {This follows the coding guideline to never log sensitive data. Based on learnings.
69-105: Review error logging for sensitive data.The new
_getWithBiometricsfunction provides good biometric-gated access. However,console.error('Error in _getWithBiometrics:', error);at line 97 has the same issue as_getSecurely– it could log sensitive information.Apply this diff:
} catch (error: unknown) { - console.error('Error in _getWithBiometrics:', error); + console.error('Error in _getWithBiometrics'); const message = error instanceof Error ? error.message : String(error);Based on learnings.
123-150: LGTM! Options properly integrated.The
restoreFromMnemonicfunction correctly accepts and applies theKeychainOptionswhen writing to the keychain. The validation and error handling logic is preserved.
152-197: LGTM! Options integrated and logging issue resolved.The
loadOrCreateMnemonicfunction correctly usesgetOptionsfor reading andsetOptionsfor writing. The sensitive mnemonic logging issue from the previous review has been properly addressed.
210-210: LGTM! Context and provider properly updated.The
AuthContextandAuthProvidercorrectly integrate_getWithBiometricsand apply appropriaterequireAuthflags:falsefor get/create operations andtruefor restore operations, which aligns with security best practices.Also applies to: 222-222, 276-298, 300-318
387-401: LGTM! Optional KeychainOptions properly handled.The
unsafe_getPrivateKeyfunction correctly handles optionalKeychainOptionswith an appropriate fallback that usesrequireAuth: true, which is the right security posture for private key access.
| // Migrates existing mnemonic to use new security settings with accessControl. | ||
| export async function migrateToSecureKeychain(): Promise<boolean> { | ||
| try { | ||
| const { hasCompletedKeychainMigration, setKeychainMigrationCompleted } = | ||
| useSettingStore.getState(); | ||
|
|
||
| if (hasCompletedKeychainMigration) { | ||
| return false; | ||
| } | ||
|
|
||
| // we try to get with old settings (no accessControl) | ||
| const existingMnemonic = await Keychain.getGenericPassword({ | ||
| service: SERVICE_NAME, | ||
| }); | ||
|
|
||
| if (!existingMnemonic) { | ||
| setKeychainMigrationCompleted(); | ||
| return false; | ||
| } | ||
|
|
||
| const capabilities = await detectSecurityCapabilities(); | ||
| const { setOptions } = await createKeychainOptions( | ||
| { requireAuth: true }, | ||
| capabilities, | ||
| ); | ||
|
|
||
| await Keychain.setGenericPassword(SERVICE_NAME, existingMnemonic.password, { | ||
| ...setOptions, | ||
| service: SERVICE_NAME, | ||
| }); | ||
|
|
||
| console.log('Successfully migrated mnemonic to secure keychain'); | ||
| trackEvent(AuthEvents.MNEMONIC_CREATED, { migrated: true }); | ||
|
|
||
| setKeychainMigrationCompleted(); | ||
|
|
||
| return true; | ||
| } catch (error: unknown) { | ||
| console.error('Error during keychain migration:', error); | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| trackEvent(AuthEvents.MNEMONIC_RESTORE_FAILED, { | ||
| reason: 'migration_failed', | ||
| error: message, | ||
| }); | ||
|
|
||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Review redundant SERVICE_NAME and error logging.
The migration logic is sound, but there are two concerns:
-
Line 354:
Keychain.setGenericPassword(SERVICE_NAME, existingMnemonic.password, { ...setOptions, service: SERVICE_NAME })usesSERVICE_NAMEas both the username and in the options. Verify if the username should beSERVICE_NAMEor a more descriptive value like'secret'(matching line 184). -
Line 366:
console.error('Error during keychain migration:', error);could log sensitive information from the error object.
Apply this diff to sanitize the error log:
} catch (error: unknown) {
- console.error('Error during keychain migration:', error);
+ console.error('Error during keychain migration');
const message = error instanceof Error ? error.message : String(error);Run this script to verify the username parameter pattern:
Based on learnings.
🏁 Script executed:
#!/bin/bash
# Search for Keychain.setGenericPassword calls to compare username usage
rg -n 'Keychain\.setGenericPassword' --type ts --type tsx -A 2Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for all Keychain.setGenericPassword invocations in .ts and .tsx files
rg -nP 'Keychain\.setGenericPassword' -g '*.{ts,tsx}' -C2Length of output: 2946
Unify keychain username and sanitize migration error log
- authProvider.tsx@354: change
Keychain.setGenericPassword(SERVICE_NAME, existingMnemonic.password, …)
to
Keychain.setGenericPassword('secret', existingMnemonic.password, …)
(matches other storage calls) - authProvider.tsx catch block: remove the error object from the log
replace
console.error('Error during keychain migration:', error)
with
console.error('Error during keychain migration')
🤖 Prompt for AI Agents
In app/src/providers/authProvider.tsx around lines 328 to 375, update the call
to Keychain.setGenericPassword to use the unified username 'secret' instead of
SERVICE_NAME (i.e. pass 'secret' as the first argument while keeping the
existing password and options/service fields), and sanitize the catch logging by
removing the error object from console.error so it only logs a simple message
like 'Error during keychain migration' (retain the subsequent trackEvent that
records the error details).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/screens/prove/ConfirmBelongingScreen.tsx (2)
40-44: Avoidas anycast and improve type safety.The
documentCategoryfield is cast toanyat line 48, which defeats TypeScript's type checking. Consider defining a proper type fordocumentMetadatausing the SDK's types or an explicit union type that matches the navigation params contract.If the SDK exports a
DocumentMetadatatype or similar, use it:+import { loadSelectedDocument, useSelfClient, type DocumentMetadata } from '@selfxyz/mobile-sdk-alpha'; ... - const [documentMetadata, setDocumentMetadata] = useState<{ - documentCategory?: string; - signatureAlgorithm?: string; - curveOrExponent?: string; - }>({}); + const [documentMetadata, setDocumentMetadata] = useState<Partial<DocumentMetadata>>({});And remove the
as anycast:params: { - documentCategory: documentMetadata.documentCategory as any, + documentCategory: documentMetadata.documentCategory, signatureAlgorithm: documentMetadata.signatureAlgorithm, curveOrExponent: documentMetadata.curveOrExponent, },Also applies to: 48-48
90-90: Potential unnecessary re-initialization.The
useEffectdepends onselfClient, which may be stable but could theoretically change and trigger re-initialization, overwritingdocumentMetadata. IfselfClientis guaranteed stable from context (as noted in learnings), consider using an empty dependency array[]to ensure this runs only once on mount.- }, [selfClient]); + }, []);Verify that
selfClientis stable throughout the component's lifecycle. Based on learnings: "SelfClientProvider is wrapped in app/App.tsx, providing context for useSelfClient() hook usage throughout the React Native app navigation stacks."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/android/app/src/main/AndroidManifest.xml(1 hunks)app/android/app/src/main/res/xml/network_security_config.xml(1 hunks)app/src/screens/prove/ConfirmBelongingScreen.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/android/**/*
⚙️ 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.xmlapp/android/app/src/main/res/xml/network_security_config.xml
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: 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/screens/prove/ConfirmBelongingScreen.tsx
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Type checking must pass before PRs (yarn types)
Files:
app/src/screens/prove/ConfirmBelongingScreen.tsx
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/screens/prove/ConfirmBelongingScreen.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-26T14:49:11.190Z
Learnt from: shazarre
PR: selfxyz/self#936
File: app/src/screens/passport/PassportNFCScanScreen.tsx:28-31
Timestamp: 2025-08-26T14:49:11.190Z
Learning: SelfClientProvider is wrapped in app/App.tsx, providing context for useSelfClient() hook usage throughout the React Native app navigation stacks.
Applied to files:
app/src/screens/prove/ConfirmBelongingScreen.tsx
🧬 Code graph analysis (1)
app/src/screens/prove/ConfirmBelongingScreen.tsx (3)
app/src/stores/settingStore.ts (1)
useSettingStore(39-100)app/src/utils/haptic/index.ts (1)
notificationSuccess(137-137)packages/mobile-sdk-alpha/src/documents/utils.ts (1)
loadSelectedDocument(83-119)
⏰ 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). (6)
- GitHub Check: analyze-ios
- GitHub Check: build-deps
- GitHub Check: android-build-test
- GitHub Check: e2e-ios
- GitHub Check: analyze-android
- GitHub Check: lint
🔇 Additional comments (2)
app/src/screens/prove/ConfirmBelongingScreen.tsx (2)
79-79: Verify error logging doesn't expose sensitive data.The error is logged to console at line 79. Ensure that
loadSelectedDocumenterrors don't contain PII (document data, user identifiers) or credentials.As per coding guidelines: "NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers."
Review the SDK's error types or add explicit sanitization:
} catch (error) { - console.error('Error loading selected document:', error); + console.error('Error loading selected document:', error instanceof Error ? error.message : 'Unknown error');
81-85: Good fallback strategy on error.Setting passport defaults when document loading fails ensures the flow doesn't break and provides sensible defaults for the cryptographic params.
| const navigate = useHapticNavigation('Loading', { | ||
| params: {}, | ||
| params: { | ||
| documentCategory: documentMetadata.documentCategory as any, | ||
| signatureAlgorithm: documentMetadata.signatureAlgorithm, | ||
| curveOrExponent: documentMetadata.curveOrExponent, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Critical: Navigation params capture stale state.
The navigate function is created during render with params that reference documentMetadata, but it captures the initial empty object {}. When navigate() is called in onOkPress (line 108), it will pass undefined values for all three fields, regardless of what initializeProving set in state.
Move the navigation call inline or use a callback pattern:
- const navigate = useHapticNavigation('Loading', {
- params: {
- documentCategory: documentMetadata.documentCategory as any,
- signatureAlgorithm: documentMetadata.signatureAlgorithm,
- curveOrExponent: documentMetadata.curveOrExponent,
- },
- });
+ const navigateToLoading = useHapticNavigation('Loading');Then update onOkPress:
- navigate();
+ navigateToLoading({
+ params: {
+ documentCategory: documentMetadata.documentCategory as any,
+ signatureAlgorithm: documentMetadata.signatureAlgorithm,
+ curveOrExponent: documentMetadata.curveOrExponent,
+ },
+ });Or verify if useHapticNavigation supports a callback form that reads current state.
Also applies to: 108-108
app/src/providers/authProvider.tsx
Outdated
| import { | ||
| createKeychainOptions, | ||
| detectSecurityCapabilities, | ||
| GetSecurelyOptions, |
There was a problem hiding this comment.
nit: GetSecureOptions sounds better imo
app/src/providers/authProvider.tsx
Outdated
| service: SERVICE_NAME, | ||
| }); | ||
|
|
||
| console.log('Successfully migrated mnemonic to secure keychain'); |
There was a problem hiding this comment.
nit: do we need all these console logs?
| const { trackEvent } = selfClient; | ||
| const navigate = useHapticNavigation('Loading', { | ||
| params: {}, | ||
| params: { | ||
| documentCategory: documentMetadata.documentCategory as any, |
There was a problem hiding this comment.
nit: are we able to properly type this?
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/providers/authProvider.tsx (1)
69-105: Synchronize _getWithBiometrics signature, use options, and sanitize logs
- Remove or utilize the unused
options: GetSecureOptionsparameter; if you need it, updatefnto(opts: GetSecureOptions) => Promise<string | false>and callawait fn(options).- Mirror
_getSecurelyby passing storage options intofn.- Strip sensitive details from logs: replace
with a generic message.console.error('Error in _getWithBiometrics:', error)
♻️ Duplicate comments (3)
app/src/screens/document/ConfirmBelongingScreen.tsx (1)
47-53: Critical: Navigation params still capture stale state (unresolved).The
useHapticNavigationcall on line 47 capturesdocumentMetadataat render time when it's still{}. Line 109'snavigate()will passundefinedfor all three fields, regardless of whatinitializeProvingsets later.The previous review identified this exact issue. Move the navigation call inline or use a lazy callback:
- const navigate = useHapticNavigation('Loading', { - params: { - documentCategory: documentMetadata.documentCategory, - signatureAlgorithm: documentMetadata.signatureAlgorithm, - curveOrExponent: documentMetadata.curveOrExponent, - }, - }); + const navigateToLoading = useHapticNavigation('Loading');Then at line 109:
- navigate(); + navigateToLoading({ + params: { + documentCategory: documentMetadata.documentCategory, + signatureAlgorithm: documentMetadata.signatureAlgorithm, + curveOrExponent: documentMetadata.curveOrExponent, + }, + });app/src/providers/authProvider.tsx (1)
328-374: Unresolved issues from previous review.Two issues remain from the previous review:
Line 354:
Keychain.setGenericPassword(SERVICE_NAME, ...)usesSERVICE_NAMEas the username, but other calls use'secret'(e.g., line 137, 184). This inconsistency should be fixed for uniformity.Line 365:
console.error('Error during keychain migration:', error)may log sensitive keychain data or mnemonic information.Apply these diffs:
Fix #1 - Unify username:
- await Keychain.setGenericPassword(SERVICE_NAME, existingMnemonic.password, { + await Keychain.setGenericPassword('secret', existingMnemonic.password, { ...setOptions, service: SERVICE_NAME, });Fix #2 - Sanitize error log:
- console.error('Error during keychain migration:', error); + console.error('Error during keychain migration');Based on learnings.
app/src/utils/keychainSecurity.ts (1)
57-71: Critical: Android passcode detection is unreliable (unresolved).The previous review identified that
WHEN_PASSCODE_SET_THIS_DEVICE_ONLY(line 62) is iOS-only and ignored on Android, causing false positives. On Android, the write succeeds even without a device credential, making the function reporthasPasscode: trueincorrectly.This breaks downstream logic at line 170 (
ACCESS_CONTROL.DEVICE_PASSCODE), causing failures for Android users without a PIN/pattern.Gate the iOS-only check and add Android support:
+import { Platform } from 'react-native'; + export async function checkPasscodeAvailable(): Promise<boolean> { + if (Platform.OS === 'ios') { try { const testService = `passcode-test-${Date.now()}`; await Keychain.setGenericPassword('test', 'test', { service: testService, accessible: Keychain.ACCESSIBLE.WHEN_PASSCODE_SET_THIS_DEVICE_ONLY, }); // Clean up test entry await Keychain.resetGenericPassword({ service: testService }); return true; } catch (error) { console.log('Device passcode not available:', error); return false; } + } else { + // Android: derive from security level + try { + const level = await Keychain.getSecurityLevel(); + // SECURE_SOFTWARE or SECURE_HARDWARE indicate device credentials + return level !== null && level !== Keychain.SECURITY_LEVEL.ANY; + } catch (error) { + console.log('Device passcode not available:', error); + return false; + } + } }
🧹 Nitpick comments (1)
app/src/utils/keychainSecurity.ts (1)
87-87: Remove or gate debug logging.Lines 87, 106-107, and the entire
logSecurityConfigfunction (210-221) contain debug logs. These should be removed for production or gated behind__DEV__.Apply this diff:
- console.log('config', config); + if (__DEV__) console.log('config', config);- console.log('setOptions', setOptions); - console.log('getOptions', getOptions); + if (__DEV__) { + console.log('setOptions', setOptions); + console.log('getOptions', getOptions); + }And either remove
logSecurityConfigor gate its implementation:export function logSecurityConfig( capabilities: SecurityCapabilities, config: AdaptiveSecurityConfig, ): void { + if (!__DEV__) return; console.log('🔒 Device Security Capabilities:', {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/android/app/src/debug/res/xml/network_security_config.xml(1 hunks)app/android/app/src/main/res/xml/network_security_config.xml(1 hunks)app/src/providers/authProvider.tsx(12 hunks)app/src/screens/document/ConfirmBelongingScreen.tsx(2 hunks)app/src/utils/keychainSecurity.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/android/app/src/main/res/xml/network_security_config.xml
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: 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/providers/authProvider.tsxapp/src/utils/keychainSecurity.tsapp/src/screens/document/ConfirmBelongingScreen.tsx
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Type checking must pass before PRs (yarn types)
Files:
app/src/providers/authProvider.tsxapp/src/utils/keychainSecurity.tsapp/src/screens/document/ConfirmBelongingScreen.tsx
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/providers/authProvider.tsxapp/src/utils/keychainSecurity.tsapp/src/screens/document/ConfirmBelongingScreen.tsx
app/android/**/*
⚙️ 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/debug/res/xml/network_security_config.xml
🧠 Learnings (3)
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to **/*.{js,ts,tsx,jsx,sol,nr} : NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
Applied to files:
app/src/providers/authProvider.tsx
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to **/*.{js,ts,tsx,jsx,sol,nr} : ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g., `***-***-1234` for passport numbers, `J*** D***` for names).
Applied to files:
app/src/providers/authProvider.tsx
📚 Learning: 2025-08-26T14:49:11.190Z
Learnt from: shazarre
PR: selfxyz/self#936
File: app/src/screens/passport/PassportNFCScanScreen.tsx:28-31
Timestamp: 2025-08-26T14:49:11.190Z
Learning: SelfClientProvider is wrapped in app/App.tsx, providing context for useSelfClient() hook usage throughout the React Native app navigation stacks.
Applied to files:
app/src/screens/document/ConfirmBelongingScreen.tsx
🧬 Code graph analysis (2)
app/src/providers/authProvider.tsx (3)
app/src/utils/keychainSecurity.ts (3)
GetSecureOptions(22-25)detectSecurityCapabilities(115-133)createKeychainOptions(76-110)packages/mobile-sdk-alpha/src/constants/analytics.ts (1)
AuthEvents(46-59)app/src/stores/settingStore.ts (1)
useSettingStore(39-100)
app/src/screens/document/ConfirmBelongingScreen.tsx (2)
app/src/stores/settingStore.ts (1)
useSettingStore(39-100)app/src/utils/haptic/index.ts (1)
notificationSuccess(137-137)
🪛 GitHub Check: workspace-lint
app/src/providers/authProvider.tsx
[warning] 72-72:
'options' is defined but never used. Allowed unused args must match /^_/u
⏰ 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). (5)
- GitHub Check: android-build-test
- GitHub Check: build-deps
- GitHub Check: e2e-ios
- GitHub Check: analyze-android
- GitHub Check: analyze-ios
🔇 Additional comments (10)
app/src/screens/document/ConfirmBelongingScreen.tsx (1)
63-78: Document metadata derivation looks good.The conditional logic correctly handles Aadhaar documents with hardcoded RSA/65537 defaults and derives passport metadata from CSCA fields. This aligns with the PR's goal of deriving params from loaded data.
app/src/providers/authProvider.tsx (4)
123-150: restoreFromMnemonic refactor looks good.The function now correctly accepts and applies
KeychainOptionsfor secure mnemonic storage. The error handling and event tracking remain intact.
152-197: loadOrCreateMnemonic refactor looks good.The function correctly applies adaptive
keychainOptionsfor both reading and writing operations. The sensitive mnemonic logging issue from the previous review appears to be resolved.
386-400: unsafe_getPrivateKey refactor looks good.The function now supports optional
keychainOptionswith sensible defaults (requireAuth: true), enabling both legacy and secure biometric-authenticated flows.
276-318: Context and provider updates look good.The
requireAuthflags are appropriate:
getOrCreateMnemonic:falsefor initial setuprestoreAccountFromMnemonic:truefor security-sensitive restoreThe
_getWithBiometricsintegration into the context is correct.app/src/utils/keychainSecurity.ts (5)
138-184: Adaptive security logic looks sound.The configuration logic correctly adapts to device capabilities with appropriate fallbacks. However, it depends on
checkPasscodeAvailablereturning accurate results, which currently fails on Android (see previous comment).Once the passcode detection is fixed, this function will provide the correct security configuration across platforms.
Verify this function's behavior on Android devices after fixing
checkPasscodeAvailable.
40-52: Biometrics detection looks good.The dynamic import avoids circular dependencies, and the try-catch properly handles unavailability.
76-110: Option creation looks good.The function correctly constructs
setOptionsandgetOptionsfrom the adaptive config, including the authentication prompt whenaccessControlis set.
115-133: Capability detection looks good.Parallel capability detection using
Promise.allis efficient. ThehasSecureHardwarederivation is correct.
189-201: Security level detection looks good.The fallback to
SECURITY_LEVEL.ANYhandles errors gracefully.
| <base-config cleartextTrafficPermitted="true"> | ||
| <trust-anchors> | ||
| <!-- Trust system certificates --> | ||
| <certificates src="system" /> | ||
|
|
||
| <!-- Trust user-added certificates in debug build --> | ||
| <certificates src="user" /> | ||
| </trust-anchors> | ||
| </base-config> |
There was a problem hiding this comment.
Constrain cleartext traffic to the intended dev hosts
Setting cleartextTrafficPermitted="true" on <base-config> enables cleartext for every host in debug builds, not just the localhost/emulator domains called out below; that undercuts the “local development only” intent and invites accidental HTTP calls to real services. Flip the base config back to false (or drop it entirely) and rely on the targeted <domain-config> entries to keep remote traffic TLS-only.
- <base-config cleartextTrafficPermitted="true">
+ <base-config cleartextTrafficPermitted="false">
<trust-anchors>
<!-- Trust system certificates -->
<certificates src="system" />
<!-- Trust user-added certificates in debug build -->
<certificates src="user" />
</trust-anchors>
</base-config>📝 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.
| <base-config cleartextTrafficPermitted="true"> | |
| <trust-anchors> | |
| <!-- Trust system certificates --> | |
| <certificates src="system" /> | |
| <!-- Trust user-added certificates in debug build --> | |
| <certificates src="user" /> | |
| </trust-anchors> | |
| </base-config> | |
| <base-config cleartextTrafficPermitted="false"> | |
| <trust-anchors> | |
| <!-- Trust system certificates --> | |
| <certificates src="system" /> | |
| <!-- Trust user-added certificates in debug build --> | |
| <certificates src="user" /> | |
| </trust-anchors> | |
| </base-config> |
🤖 Prompt for AI Agents
In app/android/app/src/debug/res/xml/network_security_config.xml around lines 4
to 12, the base-config currently enables cleartextTrafficPermitted="true" for
all hosts in debug builds; change this to cleartextTrafficPermitted="false" (or
remove the base-config entirely) and instead add explicit <domain-config>
entries for the intended dev hosts (e.g., localhost, 10.0.2.2, emulator-specific
domains) with cleartextTrafficPermitted="true" so only those local/emulator
domains allow HTTP while all other hosts remain TLS-only.
| try { | ||
| const selectedDocument = await loadSelectedDocument(selfClient); | ||
| if (selectedDocument?.data?.documentCategory === 'aadhaar') { | ||
| setDocumentMetadata({ |
There was a problem hiding this comment.
nit: we could use the if/else to set the object and call setDocumentMetadata once
app/src/utils/keychainSecurity.ts
Outdated
| options.requireAuth, | ||
| capabilities, | ||
| ); | ||
| console.log('config', config); |
There was a problem hiding this comment.
can we remove this console log?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
app/src/screens/document/ConfirmBelongingScreen.tsx (1)
47-53: Critical: Navigation params still capture stale state.Despite past review feedback, the
navigatefunction is created during render with params that referencedocumentMetadata. SincedocumentMetadatastarts as{}, the closure captures the initial empty object. WheninitializeProvingupdates state (lines 60-96), the navigate closure already holds stale, empty params. Callingnavigate()(line 114) will passundefinedfor all three fields.Move the navigation call inline with current state:
- const navigate = useHapticNavigation('Loading', { - params: { - documentCategory: documentMetadata.documentCategory, - signatureAlgorithm: documentMetadata.signatureAlgorithm, - curveOrExponent: documentMetadata.curveOrExponent, - }, - }); + const navigateToLoading = useHapticNavigation('Loading');Then at line 114:
- navigate(); + navigateToLoading({ + params: { + documentCategory: documentMetadata.documentCategory, + signatureAlgorithm: documentMetadata.signatureAlgorithm, + curveOrExponent: documentMetadata.curveOrExponent, + }, + });app/src/utils/keychainSecurity.ts (1)
57-71: Critical: Android passcode detection is unreliable.
ACCESSIBLE.WHEN_PASSCODE_SET_THIS_DEVICE_ONLY(line 62) is iOS-only and ignored on Android. On Android,setGenericPasswordsucceeds even when no device credential exists, causingcheckPasscodeAvailableto returntrueincorrectly. Downstream code (line 166 ingetAdaptiveSecurityConfig) then requestsACCESS_CONTROL.DEVICE_PASSCODE, which fails for every Android user without a PIN/pattern/password.Gate the iOS-only check and use
getSecurityLevel()for Android:export async function checkPasscodeAvailable(): Promise<boolean> { + const { Platform } = await import('react-native'); + + if (Platform.OS === 'ios') { try { const testService = `passcode-test-${Date.now()}`; await Keychain.setGenericPassword('test', 'test', { service: testService, accessible: Keychain.ACCESSIBLE.WHEN_PASSCODE_SET_THIS_DEVICE_ONLY, }); // Clean up test entry await Keychain.resetGenericPassword({ service: testService }); return true; } catch (error) { console.log('Device passcode not available'); return false; } + } else { + // Android: derive from security level + try { + const level = await Keychain.getSecurityLevel(); + // SECURE_HARDWARE or SECURE_SOFTWARE implies device credential + return level === Keychain.SECURITY_LEVEL.SECURE_HARDWARE || + level === Keychain.SECURITY_LEVEL.SECURE_SOFTWARE; + } catch (error) { + console.log('Could not determine Android passcode availability'); + return false; + } + } }Based on learnings.
app/src/providers/authProvider.tsx (2)
363-363: Sanitize error logging to prevent sensitive data exposure.Line 363's
console.error('Error during keychain migration:', error)may log sensitive keychain data or mnemonic details from the error object. Per coding guidelines, redact sensitive fields.Apply this diff:
- console.error('Error during keychain migration:', error); + console.error('Error during keychain migration');Based on learnings.
352-352: Unify keychain username for consistency.Line 352 uses
SERVICE_NAMEas the username insetGenericPassword, but line 182 uses'secret'. This inconsistency could cause confusion. The pattern elsewhere (line 135, 182) is to use'secret'as the username.Apply this diff:
- await Keychain.setGenericPassword(SERVICE_NAME, existingMnemonic.password, { + await Keychain.setGenericPassword('secret', existingMnemonic.password, {
🧹 Nitpick comments (1)
app/src/utils/keychainSecurity.ts (1)
49-49: Optional: Consider reducing debug logs.Console logs at lines 49, 68, and 191 are non-sensitive and useful for debugging, but could be removed or gated behind a
__DEV__check for production builds to reduce noise.Also applies to: 68-68, 191-191
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/providers/authProvider.tsx(12 hunks)app/src/providers/authProvider.web.tsx(1 hunks)app/src/screens/document/ConfirmBelongingScreen.tsx(2 hunks)app/src/screens/system/Loading.tsx(7 hunks)app/src/utils/keychainSecurity.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: 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/screens/document/ConfirmBelongingScreen.tsxapp/src/utils/keychainSecurity.tsapp/src/providers/authProvider.tsxapp/src/providers/authProvider.web.tsxapp/src/screens/system/Loading.tsx
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Type checking must pass before PRs (yarn types)
Files:
app/src/screens/document/ConfirmBelongingScreen.tsxapp/src/utils/keychainSecurity.tsapp/src/providers/authProvider.tsxapp/src/providers/authProvider.web.tsxapp/src/screens/system/Loading.tsx
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/screens/document/ConfirmBelongingScreen.tsxapp/src/utils/keychainSecurity.tsapp/src/providers/authProvider.tsxapp/src/providers/authProvider.web.tsxapp/src/screens/system/Loading.tsx
app/**/*.{ios,android,web}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Explain platform-specific code paths in the PR description (files with .ios, .android, or .web extensions)
Files:
app/src/providers/authProvider.web.tsx
🧠 Learnings (4)
📚 Learning: 2025-08-26T14:49:11.190Z
Learnt from: shazarre
PR: selfxyz/self#936
File: app/src/screens/passport/PassportNFCScanScreen.tsx:28-31
Timestamp: 2025-08-26T14:49:11.190Z
Learning: SelfClientProvider is wrapped in app/App.tsx, providing context for useSelfClient() hook usage throughout the React Native app navigation stacks.
Applied to files:
app/src/screens/document/ConfirmBelongingScreen.tsx
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to **/*.{js,ts,tsx,jsx,sol,nr} : NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
Applied to files:
app/src/providers/authProvider.tsx
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to **/*.{js,ts,tsx,jsx,sol,nr} : ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g., `***-***-1234` for passport numbers, `J*** D***` for names).
Applied to files:
app/src/providers/authProvider.tsx
📚 Learning: 2025-09-10T14:47:40.945Z
Learnt from: shazarre
PR: selfxyz/self#1041
File: app/src/providers/passportDataProvider.tsx:297-301
Timestamp: 2025-09-10T14:47:40.945Z
Learning: In app/src/providers/passportDataProvider.tsx: The deleteDocumentDirectlyFromKeychain function is a low-level utility used by the DocumentsAdapter and should not include error handling since callers like deleteDocument() already implement appropriate try/catch with logging for Keychain operations.
Applied to files:
app/src/providers/authProvider.web.tsx
🧬 Code graph analysis (4)
app/src/screens/document/ConfirmBelongingScreen.tsx (2)
app/src/stores/settingStore.ts (1)
useSettingStore(39-100)app/src/utils/haptic/index.ts (1)
notificationSuccess(137-137)
app/src/providers/authProvider.tsx (4)
app/src/utils/keychainSecurity.ts (3)
GetSecureOptions(22-25)detectSecurityCapabilities(111-129)createKeychainOptions(76-106)packages/mobile-sdk-alpha/src/constants/analytics.ts (1)
AuthEvents(46-59)app/src/providers/authProvider.web.tsx (2)
migrateToSecureKeychain(273-276)unsafe_getPrivateKey(289-291)app/src/stores/settingStore.ts (1)
useSettingStore(39-100)
app/src/providers/authProvider.web.tsx (1)
app/src/providers/authProvider.tsx (1)
migrateToSecureKeychain(327-372)
app/src/screens/system/Loading.tsx (2)
common/src/utils/types.ts (1)
DocumentCategory(37-37)app/src/utils/haptic/index.ts (1)
loadingScreenProgress(115-133)
🪛 GitHub Check: workspace-lint
app/src/screens/document/ConfirmBelongingScreen.tsx
[warning] 82-82:
'error' is defined but never used
app/src/providers/authProvider.tsx
[warning] 71-71:
'options' is defined but never used. Allowed unused args must match /^_/u
⏰ 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). (5)
- GitHub Check: android-build-test
- GitHub Check: e2e-ios
- GitHub Check: build-deps
- GitHub Check: analyze-android
- GitHub Check: analyze-ios
🔇 Additional comments (7)
app/src/providers/authProvider.web.tsx (1)
273-276: LGTM: Web stub is appropriate.The
migrateToSecureKeychainstub correctly returnsfalseand logs a warning for web environments where keychain migration is unavailable. This aligns with the web-specific limitations.app/src/screens/document/ConfirmBelongingScreen.tsx (1)
85-92: Linter warning is a false positive.The unused
errorvariable at line 85 is intentional—errors are silently caught with fallback metadata set (lines 87-91). This is appropriate defensive coding for non-critical initialization.app/src/screens/system/Loading.tsx (2)
31-37: LGTM: Route-based params implementation is clean.The refactor to accept
LoadingScreenParamsvia route props is well-structured. Params are extracted with proper fallbacks (lines 73-77, 143-150), and the component signature is correctly typed asStaticScreenProps<LoadingScreenParams>.Also applies to: 49-49, 73-77
110-110: Good: Error logging sanitized.Removing the error object from the log at line 110 correctly prevents sensitive document data from leaking into logs.
Based on coding guidelines.
app/src/providers/authProvider.tsx (3)
71-71: Linter warning is a false positive.The
optionsparameter at line 71 in_getWithBiometricsis intentionally unused in the function body. It's kept for API consistency with_getSecurelyand to allow future enhancements. The linter rule doesn't recognize this design pattern.
37-66: LGTM: Adaptive keychain integration is well-structured.The refactor to use
KeychainOptionsand adaptive security configuration (viacreateKeychainOptionsanddetectSecurityCapabilities) is correctly implemented across_getSecurely,loadOrCreateMnemonic, andrestoreFromMnemonic. The pattern correctly detects capabilities and derives platform-appropriate options.Based on learnings.
Also applies to: 121-148, 150-195
326-372: Migration logic is sound.
migrateToSecureKeychaincorrectly:
- Checks migration state to avoid re-running
- Reads existing mnemonic with old (no accessControl) settings
- Detects capabilities and creates secure options
- Re-writes mnemonic with new security settings
- Tracks migration completion
The logic aligns with the adaptive security utilities introduced in
keychainSecurity.ts.
Summary by CodeRabbit
New Features
Chores