feat: native SQLCipher database readers and native key store (NATIVE-1276)#7398
feat: native SQLCipher database readers and native key store (NATIVE-1276)#7398diegolmello wants to merge 5 commits into
Conversation
…y store (NATIVE-1276) - ios/Libraries/DatabaseKeyStore.swift + .m: new RCTBridgeModule backed by kSecClassGenericPassword with kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, kSecAttrSynchronizable=false, and the full team-prefixed access group S6UPZG7ZR3.chat.rocket.reactnative; exposes getItem/setItem/removeItem to JS and a static read() helper callable from Database.swift in the NSE - ios/Shared/RocketChat/Database.swift: rewrites plain sqlite3_open to open SQLCipher-encrypted DBs; derives clean filenames via deriveServerDbName() (mirrors JS connection.ts); reads keys from DatabaseKeyStore using the JS KEY_PREFIX "db_key_v1:<dbName>"; applies PRAGMA key (raw x'...' form) then busy_timeout=500 then open-verify; ARC hazard fixed (handle private, never exposed to callers) - ios/Podfile: adds pod 'SQLCipher' ~> 4.7.0 to abstract_target defaults, adds SQLCipher to $static_framework list, and adds the two post_install xcconfig fixups from the spike (strip SDK sqlite3 defines that break Swift module maps, re-inject LIBRARY_SEARCH_PATHS suppressed by static_framework override); Dir.glob rooted with __dir__ for worktree/CI safety - android/app/src/.../storage/DatabaseKeyStore.java: new native module with AES-GCM AndroidKeyStore wrapping; SharedPreferences file "RCDatabaseKeyStore"; static getItemInternal/setItemInternal callable from Encryption.java without ReactApplicationContext - android/app/src/.../storage/DatabaseKeyStorePackage.java: ReactPackage wrapper - android/app/src/.../MainApplication.kt: registers DatabaseKeyStorePackage - android/app/src/.../notification/Encryption.java: replaces WMDatabase.getInstance with net.zetetic:sqlcipher-android SQLiteDatabase; uses raw-key string form "x'<64 hex>'" (never byte[] overload); derives clean .db filename via deriveDbName() matching JS convention; reads key from DatabaseKeyStore - android/app/build.gradle: adds net.zetetic:sqlcipher-android:4.7.0@aar - app/lib/database/driver/keyStore.ts: thin JS shim wrapping NativeModules.DatabaseKeyStore into an IKeychainShim; exports installNativeKeychainShim() to wire into keyService - app/lib/database/driver/__tests__/keyStore.test.ts: Jest tests for the shim delegation and missing-module error path
- ios/RocketChatRN.xcodeproj: register DatabaseKeyStore.swift in both the Rocket.Chat app and NotificationService Sources phases (Database.swift in the NSE references it) and the .m bridge in the app target; without this the files never compile - ios bridging header: import <React/RCTBridgeModule.h> explicitly rather than relying on RNCallKeep's transitive include - Encryption.java: add PRAGMA busy_timeout = 500 after open to match the JS driver and iOS reader (avoids SQLITE_BUSY when the notification path reads while the app holds a WAL lock); drop two unused SQLCipher imports
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details🧰 Additional context used🪛 ast-grep (0.43.0)android/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStoreModule.java[warning] 101-101: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information. (use-of-aes-ecb-java) [warning] 116-116: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information. (use-of-aes-ecb-java) [warning] 101-101: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES. (desede-is-deprecated-java) [warning] 116-116: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES. (desede-is-deprecated-java) 🔇 Additional comments (6)
WalkthroughAdds a cross-platform ChangesDatabaseKeyStore and SQLCipher Key-Based Database Access
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over JS,DatabaseKeyStoreModule: Key registration (app startup)
JS->>installNativeKeychainShim: installNativeKeychainShim()
installNativeKeychainShim->>keyService: installKeychainShim(nativeShim)
end
rect rgba(144, 238, 144, 0.5)
Note over Encryption,SQLCipher: Android FCM notification path
Encryption->>Encryption: deriveDbName(serverUrl)
Encryption->>DatabaseKeyStoreModule: getItemInternal(context, "db_key_v1:<dbName>")
DatabaseKeyStoreModule->>AndroidKeyStore: load AES-GCM key
DatabaseKeyStoreModule->>SharedPreferences: read IV+ciphertext
DatabaseKeyStoreModule-->>Encryption: hex key string
Encryption->>SQLCipher: openDatabase(path, rawKey)
Encryption->>SQLCipher: PRAGMA busy_timeout
Encryption->>SQLCipher: SELECT e2e_key, encrypted FROM subscriptions
SQLCipher-->>Encryption: Room row
end
rect rgba(255, 218, 185, 0.5)
Note over NotificationService,SQLite: iOS notification path
NotificationService->>Database: init(serverUrl)
Database->>Database: deriveServerDbName(serverUrl)
Database->>SQLite: sqlite3_open_v2(dbPath)
Database->>DatabaseKeyStore: read("db_key_v1:<dbName>")
DatabaseKeyStore->>Keychain: SecItemCopyMatching
Keychain-->>DatabaseKeyStore: stored key
DatabaseKeyStore-->>Database: hex key string
Database->>SQLite: PRAGMA key = x'...'
Database->>SQLite: PRAGMA busy_timeout = 500
Database->>SQLite: SELECT count(*) FROM sqlite_master
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java`:
- Line 205: The Android and iOS implementations have a column name mismatch in
their subscription queries. In
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java
at line 205, the rawQuery method uses WHERE id == ? but should use WHERE rid = ?
to match the actual database schema column name. The iOS implementation at
ios/Shared/RocketChat/Database.swift at lines 159-163 already correctly uses rid
as the column name, confirming this is the correct schema column. Update the
WHERE clause in the Android query to use rid instead of id to ensure consistency
with the actual database schema and match the iOS implementation.
- Around line 205-226: The cursor obtained from db.rawQuery() may leak if an
exception occurs during the column access operations (such as getColumnIndex or
getString calls). Wrap the entire cursor usage block in a try-finally statement,
ensuring that cursor.close() is called in the finally block so it executes
regardless of whether an exception is thrown. Move the cursor creation and all
subsequent operations (getCount, moveToFirst, getColumnIndex, getString, getInt)
into the try block, and place the cursor.close() call in the finally block to
guarantee resource cleanup.
In `@ios/Podfile`:
- Around line 106-110: The exact literal string matching for
LIBRARY_SEARCH_PATHS in the gsub call on line 108 is too rigid and fails
silently when xcconfigs have spacing variations or already contain additional
paths. Replace the exact string 'LIBRARY_SEARCH_PATHS = $(inherited)' with a
regex pattern that matches LIBRARY_SEARCH_PATHS with flexible whitespace around
the equals sign and captures any existing content after $(inherited), then
append the sqlcipher_lib_dir to the matched pattern. This ensures the SQLCipher
library path is correctly added regardless of spacing variations or pre-existing
paths in the xcconfig file.
In `@ios/Shared/RocketChat/Database.swift`:
- Around line 104-112: The verification query in the database open method only
calls sqlite3_prepare_v2 which validates SQL syntax but does not read encrypted
data, so a wrong encryption key won't be detected. After preparing the statement
with sqlite3_prepare_v2, add a call to sqlite3_step on the prepared statement to
actually execute the query and read from sqlite_master. The sqlite3_step call
should return SQLITE_ROW to indicate successful execution of the SELECT query,
and only then can you determine if the key is correct. Keep the sqlite3_finalize
call to properly clean up the statement pointer after the step is executed.
Update the verification logic to check the result of sqlite3_step rather than
just sqlite3_prepare_v2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1b44220-d55d-4afa-9b9f-42d6c66aa09b
📒 Files selected for processing (13)
android/app/build.gradleandroid/app/src/main/java/chat/rocket/reactnative/MainApplication.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/Encryption.javaandroid/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStore.javaandroid/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStorePackage.javaapp/lib/database/driver/__tests__/keyStore.test.tsapp/lib/database/driver/keyStore.tsios/Libraries/DatabaseKeyStore.mios/Libraries/DatabaseKeyStore.swiftios/Podfileios/RocketChatRN-Bridging-Header.hios/RocketChatRN.xcodeproj/project.pbxprojios/Shared/RocketChat/Database.swift
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/database/driver/__tests__/keyStore.test.tsapp/lib/database/driver/keyStore.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/database/driver/__tests__/keyStore.test.tsapp/lib/database/driver/keyStore.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/lib/database/driver/__tests__/keyStore.test.tsapp/lib/database/driver/keyStore.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/database/driver/__tests__/keyStore.test.tsapp/lib/database/driver/keyStore.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/database/driver/__tests__/keyStore.test.tsapp/lib/database/driver/keyStore.ts
🪛 ast-grep (0.43.0)
android/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStore.java
[warning] 106-106: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyStore.getInstance(KEYSTORE_PROVIDER)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(desede-is-deprecated-java)
[warning] 120-120: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(TRANSFORMATION)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(desede-is-deprecated-java)
[warning] 132-132: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyStore.getInstance(KEYSTORE_PROVIDER)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(desede-is-deprecated-java)
[warning] 137-137: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, KEYSTORE_PROVIDER)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(desede-is-deprecated-java)
[warning] 150-150: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(TRANSFORMATION)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(desede-is-deprecated-java)
[warning] 106-106: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: KeyStore.getInstance(KEYSTORE_PROVIDER)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(use-of-aes-ecb-java)
[warning] 120-120: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(TRANSFORMATION)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(use-of-aes-ecb-java)
[warning] 132-132: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: KeyStore.getInstance(KEYSTORE_PROVIDER)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(use-of-aes-ecb-java)
[warning] 137-137: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, KEYSTORE_PROVIDER)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(use-of-aes-ecb-java)
[warning] 150-150: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(TRANSFORMATION)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(use-of-aes-ecb-java)
🔇 Additional comments (13)
app/lib/database/driver/keyStore.ts (1)
15-45: LGTM!app/lib/database/driver/__tests__/keyStore.test.ts (1)
41-71: LGTM!android/app/build.gradle (1)
144-147: LGTM!android/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStorePackage.java (1)
12-23: LGTM!android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
20-53: LGTM!ios/RocketChatRN.xcodeproj/project.pbxproj (1)
22-24: LGTM!Also applies to: 299-300, 954-955, 1684-1684, 1735-1736
android/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStore.java (1)
1-171: LGTM!ios/Libraries/DatabaseKeyStore.swift (1)
1-121: LGTM!ios/Libraries/DatabaseKeyStore.m (1)
1-20: LGTM!ios/RocketChatRN-Bridging-Header.h (1)
6-6: LGTM!android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
239-252: LGTM!ios/Shared/RocketChat/Database.swift (2)
119-170: LGTM!
66-73: Remove this verification request — the path difference between platforms is intentional and consistent.The iOS path construction (
groupRoot + "/" + dbName) and Android path construction (getFilesDir() + "/SQLite/" + dbName) are platform-specific by design. On Android, expo-sqlite automatically stores databases in the/SQLite/subdirectory within the app's internal files directory. The JavaScript layer on Android passes the filename to expo-sqlite without an explicit path, allowing the library to manage the subdirectory. On iOS, the app group container directory is used instead of the default internal storage, so no subdirectory is needed. Both platforms correctly align their native and JavaScript implementations.> Likely an incorrect or invalid review comment.
The app runs the New Architecture (bridgeless), so the key store must be a
TurboModule with a codegen-style spec, not the legacy RCT_EXTERN_MODULE /
NativeModules path it used before.
iOS: replace the .m with a .mm module class (DatabaseKeyStoreModule) that
implements the spec and returns its JSI instance from getTurboModule. The
Swift Keychain helper stays a separate class (DatabaseKeyStore) so the JS
registration name does not collide with an existing ObjC class — required by
the bridgeless NSClassFromString fallback. getItem resolves explicit JS null
on a miss (nil bridges to undefined and breaks the Promise<string|null>
contract and the !== null check in getOrCreateDatabaseKey).
Android: add an abstract spec (NativeDatabaseKeyStoreSpec) plus a concrete
module and a TurboReactPackage that registers it with isTurboModule=true.
The AndroidKeyStore AES-GCM crypto is unchanged; only the registration name
and class layout move. Encryption.java (NSE reader) now calls the static
helpers on the concrete module class.
JS resolves the module via TurboModuleRegistry.get('DatabaseKeyStoreModule').
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/database/driver/keyStore.ts (1)
14-19: ⚡ Quick winAdd an explicit return type for
getNativeModule.Line 14 omits a return type annotation, which violates the TS guideline used in this repo.
Suggested change
-function getNativeModule() { +function getNativeModule(): NonNullable<typeof NativeDatabaseKeyStore> { if (!NativeDatabaseKeyStore) { throw new Error('DatabaseKeyStore native module not found — ensure the module is linked and the app is rebuilt'); } return NativeDatabaseKeyStore; }As per coding guidelines,
**/*.{ts,tsx}requires explicit type annotations for function return types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/database/driver/keyStore.ts` around lines 14 - 19, Add an explicit return type annotation to the getNativeModule function. The function currently has no return type specified on line 14. Add the appropriate return type annotation after the function signature (between the closing parenthesis and opening brace). The return type should match the type of NativeDatabaseKeyStore, which is what the function returns. This ensures compliance with the repository's TypeScript guidelines requiring explicit return type annotations for all functions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStoreModule.java`:
- Around line 53-59: The getItem method in DatabaseKeyStoreModule.java (lines
53-59, the getItem function) currently resolves the promise with null on any
exception during key retrieval, which causes downstream logic to interpret the
failure as a missing key rather than an actual error. Reject the promise instead
of resolving it when an unexpected exception occurs in the try block—use
promise.reject() to propagate the error. Correspondingly, in the iOS
implementation at ios/Libraries/DatabaseKeyStore.swift (lines 47-50),
distinguish between true not-found errors and other failures: only return nil
when the key genuinely does not exist, and for unexpected failures, propagate or
log them distinctly rather than also returning nil.
---
Nitpick comments:
In `@app/lib/database/driver/keyStore.ts`:
- Around line 14-19: Add an explicit return type annotation to the
getNativeModule function. The function currently has no return type specified on
line 14. Add the appropriate return type annotation after the function signature
(between the closing parenthesis and opening brace). The return type should
match the type of NativeDatabaseKeyStore, which is what the function returns.
This ensures compliance with the repository's TypeScript guidelines requiring
explicit return type annotations for all functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e95918b-e410-45db-a4e6-30a2f1f17d1a
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/Encryption.javaandroid/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStoreModule.javaandroid/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStoreTurboPackage.javaandroid/app/src/main/java/chat/rocket/reactnative/storage/NativeDatabaseKeyStoreSpec.javaapp/lib/database/driver/__tests__/keyStore.test.tsapp/lib/database/driver/keyStore.tsapp/lib/native/NativeDatabaseKeyStore.tsios/Libraries/DatabaseKeyStore.mmios/Libraries/DatabaseKeyStore.swiftios/RocketChatRN.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt
- android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java
- ios/RocketChatRN.xcodeproj/project.pbxproj
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/native/NativeDatabaseKeyStore.tsapp/lib/database/driver/keyStore.tsapp/lib/database/driver/__tests__/keyStore.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/native/NativeDatabaseKeyStore.tsapp/lib/database/driver/keyStore.tsapp/lib/database/driver/__tests__/keyStore.test.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/lib/native/NativeDatabaseKeyStore.tsapp/lib/database/driver/keyStore.tsapp/lib/database/driver/__tests__/keyStore.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/native/NativeDatabaseKeyStore.tsapp/lib/database/driver/keyStore.tsapp/lib/database/driver/__tests__/keyStore.test.ts
🧠 Learnings (3)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/native/NativeDatabaseKeyStore.tsapp/lib/database/driver/keyStore.tsapp/lib/database/driver/__tests__/keyStore.test.ts
📚 Learning: 2026-03-05T06:06:12.277Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:12.277Z
Learning: Do not re-activate or reset the WCSession singleton in iOS Objective-C/Swift bridge modules. Ensure WCSession is activated and its delegate is set in a single, central place (e.g., ios/RocketChat Watch App/Loaders/WatchSession.swift) and avoid duplicating activation or delegate assignment in other iOS bridge files like ios/RCTWatchModule.mm. If WCSession is already activated via the central loader, relying on WCSession.defaultSession is sufficient and maintains a single session lifecycle.
Applied to files:
ios/Libraries/DatabaseKeyStore.mm
📚 Learning: 2026-05-12T21:58:10.053Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6991
File: ios/Libraries/Challenge.mm:29-37
Timestamp: 2026-05-12T21:58:10.053Z
Learning: For iOS code using react-native-mmkv, when calling `[MMKV initializeMMKV:nil groupDir:groupDir logLevel:...]` (and when passing `rootPath` to MMKVBridge), pass the raw App Group container URL path only (e.g., `[[NSFileManager defaultManager] containerURLForSecurityApplicationGroupIdentifier:appGroup] path]`). Do NOT append `/mmkv` yourself. The MMKV SDK already appends `/mmkv` internally (`...stringByAppendingPathComponent:@"mmkv"`), so passing `<AppGroup>/mmkv` will cause double-nesting (`<AppGroup>/mmkv/mmkv`) and can break shared storage/SSL cert lookups.
Applied to files:
ios/Libraries/DatabaseKeyStore.mm
🔇 Additional comments (5)
app/lib/native/NativeDatabaseKeyStore.ts (1)
4-10: LGTM!app/lib/database/driver/__tests__/keyStore.test.ts (1)
17-23: LGTM!Also applies to: 46-49, 63-71
android/app/src/main/java/chat/rocket/reactnative/storage/NativeDatabaseKeyStoreSpec.java (1)
9-30: LGTM!android/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStoreTurboPackage.java (1)
16-43: LGTM!ios/Libraries/DatabaseKeyStore.mm (1)
25-33: LGTM!Also applies to: 35-45, 56-59
The iOS open-verify only prepared the probe statement; sqlite3_prepare_v2 parses SQL without decrypting a page, so a wrong key surfaced later at first real use instead of at open. Step the statement (SELECT count(*) FROM sqlite_master) so SQLCipher decrypts and the wrong-key case is caught at open time. On Android, the NSE subscription-read cursor leaked if getColumnIndex or getString threw between open and close. Wrap the cursor body in try/finally so it always closes.
getOrCreateDatabaseKey treats a null getItem result as "no key exists" and generates a replacement. Both platforms previously collapsed every read/decrypt failure into that same null, so a Keychain access error (iOS) or an invalidated Keystore master key / corrupt blob (Android) would silently mint a new key over an existing encrypted database and orphan its data permanently. Distinguish a genuine not-found from a read failure on the JS-facing path: getItem now resolves null only for a true miss and rejects on any other failure, so the caller aborts the open instead of regenerating. iOS: read gains an NSError out-param (errSecItemNotFound -> nil/no error; any other status -> nil + error). The .mm rejects when the error is set; the NSE reader in Database.swift fails the open on error without regenerating. Android: getItemInternal returns null only when the SharedPreferences blob is absent and throws otherwise (missing alias for an existing blob, decrypt failure). getItem rejects on those throws; the NSE reader in Encryption.java catches and skips the room rather than regenerating.
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Review: native SQLCipher readers + native key store
The security design is largely right: SQLCipher raw-key string form (x'…'), a dedicated Keychain service namespace, ThisDeviceOnly + non-synchronizable attributes, and AES-GCM on Android. Module naming is consistent across the iOS RCT_EXPORT_MODULE, the Android spec NAME, and the TS TurboModuleRegistry.get (all DatabaseKeyStoreModule) — I verified this against the PR branch, ignoring the stale DatabaseKeyStoreModule.mm / mismatched spec left uncommitted in the working tree.
The removeItemInternal alias leak and the hasConstants=true flag are the two worth prioritising.
Open questions worth confirming outside the diff: (1) the NSE .entitlements must list the keychain-access-group for the extension to read the key; (2) KeyGenParameterSpec does not set setIsStrongBoxBacked(true), so the claim of hardware-backing falls back to the TEE rather than StrongBox where present — fine if deliberate, worth a comment.
Reviewed by an automated pass; treat inline comments as suggestions, not blockers.
| } | ||
|
|
||
| /** Removes the SharedPreferences entry. Does not delete the AndroidKeyStore key. */ | ||
| public static void removeItemInternal(Context context, String key) { |
There was a problem hiding this comment.
🔴 [high] removeItemInternal leaks the AndroidKeyStore AES alias
Only the SharedPreferences blob is removed; ks.deleteEntry(key) is never called. After removal, a subsequent setItem for the same key finds containsAlias(key) == true and silently reuses the old AES key material instead of generating a fresh one. This breaks key rotation.
Fix:
public static void removeItemInternal(Context context, String key) {
SharedPreferences prefs = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE);
prefs.edit().remove(key).apply();
try {
KeyStore ks = KeyStore.getInstance("AndroidKeyStore");
ks.load(null);
if (ks.containsAlias(key)) ks.deleteEntry(key);
} catch (Exception e) {
Log.e(TAG, "removeItemInternal: failed to delete KeyStore alias for " + key, e);
}
}|
|
||
| // ------------------------------------------------------------------------- | ||
| // Native-side helpers — callable from Encryption.java (same process, plain Context) | ||
| // ------------------------------------------------------------------------- |
There was a problem hiding this comment.
🟡 [medium] removeItem catch resolves instead of rejects
The catch block on line 86 calls promise.resolve(null), so the JS caller receives a successful result even when the delete failed. The TS contract (Promise<void>) implies rejection on error; callers cannot distinguish success from failure and will proceed believing the key was removed.
Fix: replace promise.resolve(null) in the catch with promise.reject:
} catch (Exception e) {
Log.e(TAG, "removeItem failed for key: " + key, e);
promise.reject("KEYSTORE_REMOVE_ERROR", e);
}| NativeDatabaseKeyStoreSpec.NAME, | ||
| false, // canOverrideExistingModule | ||
| false, // needsEagerInit | ||
| true, // hasConstants |
There was a problem hiding this comment.
🔴 [high] hasConstants=true with no getConstants() triggers unnecessary eager sync call
When hasConstants is true, the RN TurboModule infrastructure makes a synchronous JS→native call at startup to fetch constants. DatabaseKeyStoreModule implements no getConstants(), so this call returns nothing and adds startup latency with zero benefit. On older devices this is a measurable main-thread stall.
Fix:
new ReactModuleInfo(
NativeDatabaseKeyStoreSpec.NAME,
NativeDatabaseKeyStoreSpec.NAME,
false, // canOverrideExistingModule
false, // needsEagerInit
false, // hasConstants — module exposes no constants
false, // isCxxModule
true // isTurboModule
)|
|
||
| Cipher cipher = Cipher.getInstance(TRANSFORMATION); | ||
| cipher.init(Cipher.DECRYPT_MODE, secretKey, new GCMParameterSpec(GCM_TAG_LENGTH, iv)); | ||
| byte[] plain = cipher.doFinal(encrypted); |
There was a problem hiding this comment.
🟢 [low] Missing length guard before new byte[combined.length - GCM_IV_LENGTH]
If combined is shorter than GCM_IV_LENGTH bytes (e.g. a truncated SharedPreferences blob), combined.length - GCM_IV_LENGTH is negative, throwing NegativeArraySizeException with no indication of what went wrong.
Fix: add an explicit guard before the array allocation:
if (combined.length <= GCM_IV_LENGTH) {
throw new IllegalStateException(
"stored blob for key '" + key + "' is too short to contain a GCM IV (" + combined.length + " bytes)");
}| } | ||
| if addStatus == errSecDuplicateItem { | ||
| attrs.removeValue(forKey: kSecValueData as String) | ||
| let updateStatus = SecItemUpdate(attrs as CFDictionary, [kSecValueData as String: data] as CFDictionary) |
There was a problem hiding this comment.
🟢 [low] SecItemUpdate dict missing kSecAttrAccessible
When SecItemAdd returns errSecDuplicateItem, the update dict passed to SecItemUpdate contains only kSecValueData. An item previously stored with a weaker accessibility (e.g. kSecAttrAccessibleAlways) will not be tightened to kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly.
Fix:
let updateStatus = SecItemUpdate(
attrs as CFDictionary,
[
kSecValueData as String: data,
kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly as String
] as CFDictionary
)
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Structural review (NATIVE-1276). Verified clean: SQLCipher raw-key x'<hex>' form on both platforms (not the byte[] overload), iOS Keychain attributes (AfterFirstUnlockThisDeviceOnly, non-synchronizable, NSE access group), TurboModule wiring (getTurboModule: on iOS, TurboReactPackage on Android — New Architecture path), busy_timeout PRAGMA, cursor/DB close in finally, and no key material in logs. Findings inline.
Note on URL→filename derivation: Android (Encryption.java replaceFirst("^(\\w+:)?//")) and iOS (Database.swift range(of: "://")) are two separate copies that agree on https:// today but diverge on edge cases (scheme-less //, trailing slashes), and neither is tested against the canonical JS deriveServerDbName. They decide which DB file gets opened — worth a shared fixture per platform.
| NativeDatabaseKeyStoreSpec.NAME, | ||
| false, // canOverrideExistingModule | ||
| false, // needsEagerInit | ||
| true, // hasConstants |
There was a problem hiding this comment.
bug: med — hasConstants = true but the module has no getConstants(). DatabaseKeyStoreModule never overrides getConstants(), so the base returns null; declaring hasConstants true asks the bridge to serialize a null constants map. Other modules here use false when they have no constants (PushNotification, VideoConf) and implement getConstants() when they set true (SSLPinning). Set this to false.
| promise.resolve(null); | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "removeItem failed for key: " + key, e); | ||
| promise.resolve(null); |
There was a problem hiding this comment.
bug: med — removeItem swallows failures. The catch logs and then calls promise.resolve(null), so any exception from removeItemInternal surfaces to JS as a successful void return. Reject with a distinct error code instead.
| // busy timeout that contention surfaces as an immediate SQLITE_BUSY. | ||
| db.execSQL("PRAGMA busy_timeout = 500;"); | ||
|
|
||
| Cursor cursor = db.rawQuery("SELECT * FROM subscriptions WHERE id == ? LIMIT 1", new String[]{ejson.rid}); |
There was a problem hiding this comment.
bug: low — wrong lookup column. SELECT * FROM subscriptions WHERE id == ? is queried with ejson.rid (a room id). The iOS reader correctly uses WHERE rid = ?. This worked under WMDB only because subscription.id == rid there; in the new Drizzle schema id may be a generated value, so this returns no rows and decryption silently fails. Query WHERE rid = ?.
| byte[] combined = Base64.decode(encoded, Base64.DEFAULT); | ||
|
|
||
| byte[] iv = new byte[GCM_IV_LENGTH]; | ||
| byte[] encrypted = new byte[combined.length - GCM_IV_LENGTH]; |
There was a problem hiding this comment.
bug: low — no length check before splitting the GCM blob. A corrupt blob that Base64-decodes to fewer than GCM_IV_LENGTH bytes makes combined.length - GCM_IV_LENGTH negative → NegativeArraySizeException. Guard with an explicit "ciphertext too short" check that throws a clear error.
| @@ -0,0 +1,66 @@ | |||
| #import <React/RCTBridgeModule.h> | |||
|
|
|||
| #import <SSLPinning/SSLPinning.h> | |||
There was a problem hiding this comment.
slop: heavy — spurious #import <SSLPinning/SSLPinning.h>. Copy-paste from another module; this file has no SSL-pinning dependency. Remove.
| String dbPath = context.getFilesDir().getAbsolutePath() + "/SQLite/" + dbName; | ||
|
|
||
| // Load the SQLCipher native library. Safe to call multiple times per process. | ||
| System.loadLibrary("sqlcipher"); |
There was a problem hiding this comment.
slop: heavy — System.loadLibrary("sqlcipher") runs inside readRoom(). Called on every incoming push. Move it to a static { } initializer so it loads once.
| } | ||
|
|
||
| /** Removes the SharedPreferences entry. Does not delete the AndroidKeyStore key. */ | ||
| public static void removeItemInternal(Context context, String key) { |
There was a problem hiding this comment.
slop: heavy — AndroidKeyStore alias never deleted on remove. removeItemInternal clears the SharedPreferences blob but leaves the AES-GCM wrapping key in the Keystore, so every server logout orphans one alias permanently. Call ks.deleteEntry(key) after removing the prefs entry.
| static let service = "chat.rocket.reactnative.dbkeys" | ||
|
|
||
| // Full team-prefixed access group — bare suffix fails with errSecMissingEntitlement | ||
| private static let accessGroup = "S6UPZG7ZR3.chat.rocket.reactnative" |
There was a problem hiding this comment.
slop: heavy — hardcoded team-prefixed Keychain access group. "S6UPZG7ZR3.chat.rocket.reactnative" is baked in, while the entitlements use $(AppIdentifierPrefix)chat.rocket.reactnative resolved at build time. If the signing team changes (enterprise build, fork, profile rotation) main-app writes succeed but NSE reads fail with errSecItemNotFound and notifications silently stop decrypting. Derive it from the bundle entitlement / a build-time constant.
| * Must be called once before any database is opened. | ||
| * Safe to call multiple times (subsequent calls are no-ops in keyService). | ||
| */ | ||
| export function installNativeKeychainShim(): void { |
There was a problem hiding this comment.
slop: moderate — installNativeKeychainShim() has no call site in this PR. The TurboModule key store is unreachable until this is invoked once at startup (the doc comment says "before any database is opened"). Wire it in the same PR.
| return NAME; | ||
| } | ||
|
|
||
| @ReactMethod |
There was a problem hiding this comment.
slop: nit — @ReactMethod on the TurboModule spec methods. TurboModule registration is via JSI/codegen, not @ReactMethod reflection. Harmless but bridge-era noise (mirrors the existing SSLPinning spec); drop it.
1 participant
Proposed changes
Rewrites the native (non-JS) database readers so they can open the SQLCipher-encrypted databases created by the new expo-sqlite/Drizzle driver, and adds a native key store that holds the per-database encryption keys. This is the native counterpart to the JS driver adapter — the iOS Notification Service Extension and the Android FCM notification path both read the local database directly (outside JS) to resolve E2E room keys, so they must be taught to open the encrypted files with the same key material and open sequence the JS side uses.
The key store is a TurboModule: the app runs the New Architecture (bridgeless), so native modules must expose a codegen-style spec rather than the legacy
RCT_EXTERN_MODULE/NativeModulespath.iOS
ios/Libraries/DatabaseKeyStore.swift: Keychain helper class (kSecClassGenericPassword,kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly,kSecAttrSynchronizable = false, access groupS6UPZG7ZR3.chat.rocket.reactnative). Staticread/write/deletethe NotificationService extension calls directly.ios/Libraries/DatabaseKeyStore.mm: the TurboModule (DatabaseKeyStoreModule) implementing the spec and returning its JSI instance fromgetTurboModule. The JS registration name is deliberately distinct from the Swift helper class so it does not collide with an existing ObjC class — required by the bridgelessNSClassFromStringfallback for app-target modules.getItemresolves explicit JSnullon a miss (nilbridges toundefined, which breaks thePromise<string | null>contract and the!== nullcheck ingetOrCreateDatabaseKey).ios/Shared/RocketChat/Database.swift: opens SQLCipher-encrypted databases (raw-keyPRAGMA key = "x'…'"→busy_timeout→ open-verify), derives the clean.dbfilename the same way the JS driver does, and reads the key from the Keychain helper. Room-key/encrypted query semantics are unchanged.ios/Podfile: addspod 'SQLCipher', '~> 4.7.0'(locked to the version expo-sqlite vendors) plus the static-framework andxcconfigfix-ups required to link SQLCipher alongside the SDK's built-in SQLite.ios/RocketChatRN.xcodeproj+ bridging header: register the new files in the app and extension targets.Android
…/storage/NativeDatabaseKeyStoreSpec.java: abstract TurboModule spec (getName()→DatabaseKeyStoreModule, abstractgetItem/setItem/removeItem).…/storage/DatabaseKeyStoreModule.java: concrete module. Each key is wrapped with an AndroidKeyStore AES-GCM key; the wrapped blob lives in its ownSharedPreferencesfile. Exposes the JS methods plus static helpers the notification path calls without a React context.…/storage/DatabaseKeyStoreTurboPackage.java(+MainApplication.ktregistration):TurboReactPackagethat maps the module name to aReactModuleInfowithisTurboModule = trueand returns the concrete instance.…/notification/Encryption.java: opens the database withnet.zeteticSQLCipher using the raw-key string form (never thebyte[]overload, which silently PBKDF2-derives and fails), derives the clean filename to match the JS convention, reads the key from the key store, and applies the samebusy_timeoutinvariant.android/app/build.gradle: addsnet.zetetic:sqlcipher-android:4.7.0.JS
app/lib/native/NativeDatabaseKeyStore.ts: TurboModule spec resolved viaTurboModuleRegistry.get('DatabaseKeyStoreModule').app/lib/database/driver/keyStore.ts(+ test): thin shim wrapping the native module as the driver'sIKeychainShim, withinstallNativeKeychainShim()to wire it into the key service at startup.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1276
How to test or reproduce
This PR is stacked on #7396 (driver adapter), which is stacked on #7395 (schema). Review/merge those first. The native open path is exercised end-to-end once the data layer (wipe-and-restore) lands; for this PR:
pnpm pod-installthen build the app and the NotificationService extension — both targets must compile with the new SQLCipher pod and the registered TurboModule. Runtime: a Keychain round-trip throughDatabaseKeyStoreModulereturnsnullon a miss, the stored value aftersetItem, andnullagain afterremoveItem.net.zetetic:sqlcipher-androidresolves andEncryption.javacompiles against the concrete module's static helpers.TZ=UTC pnpm test --testPathPattern='driver/__tests__/keyStore'andpnpm lint.Types of changes
Checklist
Further comments
Native modules use the New Architecture TurboModule path because the app is bridgeless; the legacy
RCT_EXTERN_MODULE/NativeModulesregistration does not vend modules under bridgeless. The crypto (iOS Keychain, Android AndroidKeyStore AES-GCM) is unchanged from the prior revision — only the module registration and class layout moved to the TurboModule shape.Summary by CodeRabbit