Fix cross-platform encryption security issues and harden keystore handling - #3142
Open
Divyateja2709 wants to merge 8 commits into
Open
Fix cross-platform encryption security issues and harden keystore handling#3142Divyateja2709 wants to merge 8 commits into
Divyateja2709 wants to merge 8 commits into
Conversation
… request/response payloads
Contributor
📝 WalkthroughWalkthroughThis PR introduces a comprehensive multiplatform security module ( ChangesSecurity Module Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 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. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (15)
core-base/security/consumer-rules.pro (1)
4-4: ⚡ Quick winConsider more targeted ProGuard rules.
The blanket
-keep class template.core.base.security.** { *; }rule keeps all classes and members, which may increase APK size. While this is a common pattern for expect/actual classes to prevent R8/ProGuard issues, consider whether more selective rules targeting only the classes that truly need protection (e.g., classes used via reflection, or specific public API entry points) would suffice.🤖 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 `@core-base/security/consumer-rules.pro` at line 4, The current ProGuard rule `-keep class template.core.base.security.** { *; }` is too broad; narrow it to only classes/members that require retention (e.g., reflection entry points, expect/actual bridging classes, or public API types). Replace the blanket keep with targeted rules that reference specific class names or packages (for example keep individual classes like template.core.base.security.SecurityManager, template.core.base.security.TokenProvider) or use more specific keepmembers/keepclasseswithmembers patterns or annotations to preserve only what’s necessary; update the rule(s) that reference `template.core.base.security.**` accordingly to reduce APK size while retaining required runtime behavior.core/network/src/desktopMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.desktop.kt (1)
30-39: ⚡ Quick winRemove dead logger configuration when LogLevel is NONE.
Setting
level = LogLevel.NONEprevents Ktor from generating any log messages, making the custom logger at lines 34-38 and theLogger.DEFAULTassignment at line 31 unreachable. Consider removing the logger configuration entirely for clarity, or if you intend to enable logging in the future, extract the level to a build configuration constant.🧹 Proposed cleanup
install(Logging) { - logger = Logger.DEFAULT // Avoid leaking request/response payloads and auth data in logs. level = LogLevel.NONE - logger = object : Logger { - override fun log(message: String) { - co.touchlab.kermit.Logger.d(tag = "KtorClient", messageString = message) - } - } }🤖 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 `@core/network/src/desktopMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.desktop.kt` around lines 30 - 39, The Logging install block in KtorHttpClient.desktop.kt sets level = LogLevel.NONE, which disables all logs and renders the subsequent logger assignments (Logger.DEFAULT and the anonymous Logger implementation) dead code; remove the entire custom logger configuration (the logger = Logger.DEFAULT line and the anonymous object block inside install(Logging)) to avoid confusion, or alternatively make the log level configurable (replace LogLevel.NONE with a build/config constant and only set the custom logger when that constant enables logging) so the custom logger and level are consistent.core/network/src/androidMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.android.kt (1)
32-41: ⚡ Quick winRemove dead logger configuration when LogLevel is NONE.
Setting
level = LogLevel.NONEprevents Ktor from generating any log messages, making the custom logger at lines 36-40 and theLogger.DEFAULTassignment at line 33 unreachable. Consider removing the logger configuration entirely for clarity, or if you intend to enable logging in the future, extract the level to a build configuration constant.🧹 Proposed cleanup
install(Logging) { - logger = Logger.DEFAULT // Avoid leaking request/response payloads and auth data in logs. level = LogLevel.NONE - logger = object : Logger { - override fun log(message: String) { - KermitLogger.d(tag = "KtorClient", messageString = message) - } - } }🤖 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 `@core/network/src/androidMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.android.kt` around lines 32 - 41, The Logging installation currently sets level = LogLevel.NONE which disables all logs, making the custom logger and the Logger.DEFAULT assignment dead code; remove the entire install(Logging) block (or at minimum remove the unreachable logger = Logger.DEFAULT and the anonymous Logger that calls KermitLogger.d) from KtorHttpClient.android.kt, or alternatively replace the hardcoded LogLevel.NONE with a build-time/config constant (e.g., LOG_LEVEL) and keep the custom logger (the anonymous Logger that calls KermitLogger.d with tag "KtorClient") so logging can be enabled via configuration.core/network/src/wasmJsMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.wasmJs.kt (1)
30-39: ⚡ Quick winRemove redundant logger configuration when logging is disabled.
When
LogLevel.NONEis set, the Ktor Logging plugin doesn't produce any log messages, so the custom logger implementation (lines 34-38) will never be invoked. Additionally, line 31'slogger = Logger.DEFAULTassignment is immediately overridden by the custom logger, making it dead code. This configuration is confusing and serves no purpose when logging is disabled.♻️ Simplified configuration
install(Logging) { - logger = Logger.DEFAULT // Avoid leaking request/response payloads and auth data in logs. level = LogLevel.NONE - logger = object : Logger { - override fun log(message: String) { - co.touchlab.kermit.Logger.d(tag = "KtorClient", messageString = message) - } - } }🤖 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 `@core/network/src/wasmJsMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.wasmJs.kt` around lines 30 - 39, Remove the redundant logger setup in the Logging plugin configuration: since level = LogLevel.NONE produces no logs, delete the custom Logger implementation and the initial logger = Logger.DEFAULT assignment inside the install(Logging) block in KtorHttpClient.wasmJs.kt; instead keep a minimal install(Logging) with level = LogLevel.NONE (or remove the entire install if not needed) so there is no dead/never-invoked Logger object.core/network/src/jsMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.js.kt (1)
30-39: ⚡ Quick winRemove redundant logger configuration when logging is disabled.
When
LogLevel.NONEis set, the Ktor Logging plugin doesn't produce any log messages, so the custom logger implementation (lines 34-38) will never be invoked. Additionally, line 31'slogger = Logger.DEFAULTassignment is immediately overridden by the custom logger, making it dead code. This configuration is confusing and serves no purpose when logging is disabled.♻️ Simplified configuration
install(Logging) { - logger = Logger.DEFAULT // Avoid leaking request/response payloads and auth data in logs. level = LogLevel.NONE - logger = object : Logger { - override fun log(message: String) { - co.touchlab.kermit.Logger.d(tag = "KtorClient", messageString = message) - } - } }🤖 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 `@core/network/src/jsMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.js.kt` around lines 30 - 39, The Logging plugin is configured with LogLevel.NONE so no logs will be emitted, making the custom logger implementation and the initial logger = Logger.DEFAULT assignment redundant; update the install(Logging) block in KtorHttpClient.js.kt to either remove both logger = Logger.DEFAULT and the object : Logger { override fun log(...) } when level is NONE, or conditionally assign the custom logger only when level != LogLevel.NONE so the custom implementation (the object Logger and its override) isn’t dead code.core/network/src/nativeMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.native.kt (1)
30-39: ⚡ Quick winRemove redundant logger configuration when logging is disabled.
When
LogLevel.NONEis set, the Ktor Logging plugin doesn't produce any log messages, so the custom logger implementation (lines 34-38) will never be invoked. Additionally, line 31'slogger = Logger.DEFAULTassignment is immediately overridden by the custom logger, making it dead code. This configuration is confusing and serves no purpose when logging is disabled.♻️ Simplified configuration
install(Logging) { - logger = Logger.DEFAULT // Avoid leaking request/response payloads and auth data in logs. level = LogLevel.NONE - logger = object : Logger { - override fun log(message: String) { - co.touchlab.kermit.Logger.d(tag = "KtorClient", messageString = message) - } - } }🤖 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 `@core/network/src/nativeMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.native.kt` around lines 30 - 39, The install(Logging) block currently sets logger = Logger.DEFAULT and then immediately overrides it with a custom Logger object while also setting level = LogLevel.NONE, which makes the custom logger and the DEFAULT assignment redundant; remove the unnecessary assignments and custom Logger when LogLevel.NONE is used. Edit the install(Logging) { ... } block in KtorHttpClient.native.kt to either remove the entire Logging installation when LogLevel.NONE is configured or keep only the level assignment (LogLevel.NONE) without assigning Logger.DEFAULT or the anonymous Logger object, ensuring no dead/unused logger code remains. Make changes around the install(Logging) block and the anonymous object implementing Logger to eliminate the redundant logger configuration.core-base/security/src/commonMain/kotlin/template/core/base/security/SensitiveString.kt (1)
20-21: 💤 Low valueClarify that Strings returned by value() cannot be zeroed.
Line 20 advises "Call [close] when done," but calling
close()only zeros the internalCharArray—it cannot affect anyStringinstances created by previousvalue()calls. Those strings remain in memory until garbage collection. Consider adding a note thatvalue()should be called sparingly and the returned string should not be stored long-term to maintain the security benefit ofSensitiveString.🤖 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 `@core-base/security/src/commonMain/kotlin/template/core/base/security/SensitiveString.kt` around lines 20 - 21, Update the documentation for SensitiveString.value() to explicitly state that the returned String cannot be zeroed by close() (close() only zeros the internal CharArray 'chars'), so callers must call value() sparingly, avoid storing or caching the returned String long-term, and discard it immediately after use to preserve the security guarantees of SensitiveString; reference the value() method, the close() behavior, and the internal 'chars' array in the note.core-base/security/src/commonMain/kotlin/template/core/base/security/DeepLinkValidator.kt (1)
38-56: ⚡ Quick winConsider handling malformed IPv6 URIs more explicitly.
The host extraction logic (lines 44-47) handles bracketed IPv6 addresses correctly, but an IPv6 address without brackets (e.g.,
https://::1/path) would result in an empty host string aftersubstringBefore(":")on line 46. While RFC 3986 requires brackets for IPv6 literals in URIs, the validator could be more defensive:
- If
allowedHostscontains an empty string, malformed URIs with unbracketed IPv6 would incorrectly pass validation- If
allowedHostsis empty (default), the check is skipped, so no security impact🛡️ Proposed fix to reject empty host explicitly
val hostPort = authority.substringAfterLast("@") val host = when { hostPort.startsWith("[") -> hostPort.substringAfter("[").substringBefore("]").lowercase() else -> hostPort.substringBefore(":").lowercase() } + if (host.isEmpty()) { + Logger.w("DeepLinkValidator") { "Rejected empty or malformed host in: $uri" } + return false + } val allowed = host in allowedHosts if (!allowed) Logger.w("DeepLinkValidator") { "Rejected host: $host" } allowed🤖 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 `@core-base/security/src/commonMain/kotlin/template/core/base/security/DeepLinkValidator.kt` around lines 38 - 56, DeepLinkValidator's host extraction can yield an empty host for malformed unbracketed IPv6 (host variable), which may wrongly pass when allowedHosts contains "". Modify the host validation in the hostValid block: after computing hostPort and host, explicitly check if host.isBlank() and treat that as disallowed (return false for hostValid), log a warning (e.g., Logger.w("DeepLinkValidator") { "Empty or malformed host from URI: $uri" }), and only then check membership in allowedHosts; ensure this change affects the hostValid calculation used by the return of validate method.core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityPolicy.kt (1)
16-26: ⚡ Quick winAdd validation to SecurityPolicy construction.
The
SecurityPolicydata class accepts numeric thresholds without validation. Invalid configurations could cause unexpected behavior:
lockAfterFailedAttempts >= wipeAfterFailedAttemptswould allow wipe without lockout- Negative or zero values could disable thresholds unintentionally
- Extremely large
sessionTimeoutMinutescould cause overflow downstream✅ Proposed validation logic
data class SecurityPolicy( val requireBiometricForSensitiveOps: Boolean = true, val lockAfterFailedAttempts: Int = 5, val wipeAfterFailedAttempts: Int = 10, val sessionTimeoutMinutes: Int = 30, val clipboardWipeSeconds: Int = 60, ) { + init { + require(lockAfterFailedAttempts > 0) { "lockAfterFailedAttempts must be positive" } + require(wipeAfterFailedAttempts > lockAfterFailedAttempts) { + "wipeAfterFailedAttempts ($wipeAfterFailedAttempts) must be greater than lockAfterFailedAttempts ($lockAfterFailedAttempts)" + } + require(sessionTimeoutMinutes > 0) { "sessionTimeoutMinutes must be positive" } + require(clipboardWipeSeconds > 0) { "clipboardWipeSeconds must be positive" } + } + companion object { fun default(): SecurityPolicy = SecurityPolicy() } }🤖 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 `@core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityPolicy.kt` around lines 16 - 26, SecurityPolicy's constructor accepts numeric thresholds without validation; add validation in the SecurityPolicy data class (using an init block or a validated factory in the companion object) to enforce: lockAfterFailedAttempts > 0, wipeAfterFailedAttempts > 0, sessionTimeoutMinutes > 0, clipboardWipeSeconds > 0, and lockAfterFailedAttempts < wipeAfterFailedAttempts, plus sensible upper bounds for sessionTimeoutMinutes and clipboardWipeSeconds (e.g., cap to a reasonable max to avoid overflow). When a value is invalid throw an IllegalArgumentException with a clear message referencing the offending parameter (e.g., lockAfterFailedAttempts, wipeAfterFailedAttempts, sessionTimeoutMinutes, clipboardWipeSeconds) so callers get immediate feedback; update the companion object default() only if you change construction approach to ensure defaults remain valid.core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityGate.kt (1)
65-67: ⚡ Quick winMove state synchronization into a side effect.
The direct assignment
securityState.isSessionActive = isSessionActivein the composable body (line 67) runs on every recomposition. This pattern can cause unnecessary recompositions and should be performed in aLaunchedEffectinstead.♻️ Refactor to use LaunchedEffect
- // Collect session state from SessionManager - val isSessionActive by sessionManager.isSessionActive.collectAsState() - securityState.isSessionActive = isSessionActive + // Sync session state from SessionManager to SecurityState + LaunchedEffect(Unit) { + sessionManager.isSessionActive.collect { isActive -> + securityState.isSessionActive = isActive + } + } + val isSessionActive by sessionManager.isSessionActive.collectAsState()🤖 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 `@core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityGate.kt` around lines 65 - 67, Direct assignment of securityState.isSessionActive inside the composable body causes updates on every recomposition; replace the assignment with a side-effect by removing the direct line and wrapping the update in a LaunchedEffect keyed on the collected state (e.g., after val isSessionActive by sessionManager.isSessionActive.collectAsState(), add LaunchedEffect(isSessionActive) { securityState.isSessionActive = isSessionActive }), referencing SecurityGate, securityState and sessionManager.isSessionActive.collectAsState to locate where to change.core-base/security/src/jsCommonMain/kotlin/template/core/base/security/SecureWiper.kt (1)
16-19: ⚡ Quick winIncomplete storage wipe implementation.
The comment on Line 18 suggests clearing
localStorageandsessionStorage, but the method only logs. While this is acceptable for a stub, implementing the actual clearing would be straightforward and improve the implementation's completeness.🧹 Proposed implementation
actual fun wipeSecureStorage() { Logger.w("SecureWiper") { "Secure storage wipe triggered" } - // Clear localStorage/sessionStorage in browser context. + try { + js("localStorage.clear()") + js("sessionStorage.clear()") + } catch (e: dynamic) { + Logger.e("SecureWiper", e) { "Failed to clear browser storage" } + } }Would you like me to open an issue to track implementing browser storage clearing for the JS/Wasm platform?
🤖 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 `@core-base/security/src/jsCommonMain/kotlin/template/core/base/security/SecureWiper.kt` around lines 16 - 19, The current actual implementation of SecureWiper.wipeSecureStorage only logs and does not clear browser storages; update the function (SecureWiper.wipeSecureStorage) to clear both window.localStorage and window.sessionStorage when running in a JS/WASM browser context, wrapping access in a try/catch to avoid exceptions in non-browser environments and logging any errors via Logger.w (or Logger.e) for diagnostics; ensure the method checks for the presence of the global window and the storage objects before calling .clear() so it remains safe as a multiplatform stub.core-base/security/src/nativeMain/kotlin/template/core/base/security/BiometricAuthenticator.kt (1)
22-28: 💤 Low valueConsider capturing error information for diagnostics.
The
canEvaluatePolicycall passesnullfor the error parameter, which discards any failure reason. While the boolean result is sufficient for availability checking, capturing the error could aid debugging when biometrics are unavailable.🤖 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 `@core-base/security/src/nativeMain/kotlin/template/core/base/security/BiometricAuthenticator.kt` around lines 22 - 28, In isAvailable() capture the NSError returned by LAContext.canEvaluatePolicy instead of passing null so you can record why evaluation failed; update the call in BiometricAuthenticator.isAvailable() to create an NSError pointer/variable, pass it into LAContext.canEvaluatePolicy(LAPolicyDeviceOwnerAuthenticationWithBiometrics, error = ...), and then log or surface the error details (via your existing logger or a diagnostic print) when the boolean is false to aid debugging.core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureWiper.kt (1)
21-25: 💤 Low valueDocument consumer responsibility for Keychain cleanup.
The
wipeSecureStorage()method logs a warning but doesn't actually delete Keychain items. The comment on Lines 23-24 notes that consumer apps should handle this, but this contract should be more prominently documented (e.g., in KDoc or the common expect declaration) to prevent misuse.🤖 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 `@core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureWiper.kt` around lines 21 - 25, The platform implementation SecureWiper.wipeSecureStorage only logs and relies on consumers to remove Keychain items; add explicit, prominent documentation: update the common expect declaration for wipeSecureStorage (and the SecureWiper type) with KDoc that states the method does not perform Keychain/UserDefaults deletion on Apple platforms and that consumer apps MUST delete items for service "org.mifos.secure" and clear any UserDefaults, and mirror the same KDoc on the nativeMain actual implementation to make the contract unambiguous to callers (referencing SecureWiper and wipeSecureStorage by name).core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureKeyProvider.kt (1)
61-67: ⚖️ Poor tradeoffVerify memcpy safety and consider alternatives.
The
platform.posix.memcpycall on Line 64 copies raw bytes fromNSDatainto the pinnedByteArray. While this works, it relies on low-level C interop. Consider whether there's a safer Kotlin/Native idiomatic approach or if additional bounds checking is needed.🤖 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 `@core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureKeyProvider.kt` around lines 61 - 67, The memcpy call in SecureKeyProvider.kt copies raw bytes from NSData into a pinned ByteArray using platform.posix.memcpy, which is unsafe without checks; update the code in the result handling (the block that references nsData, nsData.length and nsData.bytes) to first validate nsData.bytes is non-null and nsData.length fits in Int (e.g. nsData.length <= Int.MAX_VALUE), then perform the copy via a safer Cocoa API or Kotlin/Native helper: either call nsData.getBytes(pinned.addressOf(0), nsData.length) if available, or keep the pinned ByteArray but replace direct memcpy with a bounded copy using the NSData.getBytes API or kotlinx.cinterop utilities; ensure you reference the existing symbols nsData, length, bytes, and the pinned ByteArray in your change and add the null/length checks before copying.core-base/security/src/nativeMain/kotlin/template/core/base/security/TamperDetector.kt (1)
24-28: Debugger detection is stubbed.The
isDebuggerAttached()method always returnsfalse. The comment indicates this is intentional pending platform-specific implementation, but callers should be aware this check is not currently functional.🤖 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 `@core-base/security/src/nativeMain/kotlin/template/core/base/security/TamperDetector.kt` around lines 24 - 28, The isDebuggerAttached() implementation is a stub that always returns false; replace the hardcoded return with a real platform-specific detection (or fail fast) so callers don't assume debugging is absent. Update the actual fun isDebuggerAttached() to perform OS checks: on Apple targets use sysctl/KERN_PROC to check P_TRACED; on Linux read /proc/self/status (TracerPid) or use ptrace; on Windows call IsDebuggerPresent via native interop; if you cannot implement all targets now, throw NotImplementedError in isDebuggerAttached() so callers surface that it's unimplemented rather than silently returning false. Ensure you modify the isDebuggerAttached() function and add any small helper functions or native imports needed for each target.
🤖 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
`@core-base/security/src/androidMain/kotlin/template/core/base/security/BuildInfo.kt`:
- Line 22: Replace the runtime-debugger check in isReleaseBuild() (currently
using Debug.isDebuggerConnected()) with the Android build flag: return
!BuildConfig.DEBUG so build-type detection uses the generated BuildConfig.DEBUG
value; update the import/qualifier as needed for your app's BuildConfig and
ensure SecurityConfig will use isReleaseBuild() for applying release-level
hardening.
In
`@core-base/security/src/androidMain/kotlin/template/core/base/security/FieldEncryptor.kt`:
- Around line 52-56: In FieldEncryptor's decrypt path replace the incorrect
SecretKeySpec usage: change the cipher.init call in the decrypt method (the
block that currently calls Cipher.DECRYPT_MODE with SecretKeySpec(key, "AES"))
to pass the existing SecretKey instance directly (same pattern as the encrypt
method) along with the GCMParameterSpec(iv) so you don't attempt to construct
raw-key bytes; remove any unused SecretKeySpec import if present.
In
`@core-base/security/src/androidMain/kotlin/template/core/base/security/SecureRandom.kt`:
- Around line 15-17: The nextBytes function in SecureRandom should validate the
size parameter before allocating the ByteArray to avoid a
NegativeArraySizeException; add an explicit precondition in nextBytes (e.g., in
the SecureRandom.actual fun nextBytes(size: Int)) that checks size is
non‑negative and throws a clear IllegalArgumentException (or uses require(size
>= 0) with a message) if not, then proceed to allocate bytes and call
random.nextBytes(bytes).
In
`@core-base/security/src/androidMain/kotlin/template/core/base/security/SecureWiper.kt`:
- Around line 17-22: wipeSecureStorage() currently only logs and must actively
remove cryptographic material; update the SecureWiper.actual fun
wipeSecureStorage implementation to (1) open the AndroidKeyStore
(KeyStore.getInstance("AndroidKeyStore"), load(null)) and delete relevant key
aliases via deleteEntry(alias) (or iterate and delete all app-specific aliases),
(2) clear any EncryptedSharedPreferences used by the app by obtaining the
EncryptedSharedPreferences instance and calling edit().clear().commit()/apply(),
and (3) clear the app's Room databases (invoke the Room database clear/DAO
cleanup method). Focus changes inside SecureWiper.wipeSecureStorage so key
deletion, EncryptedSharedPreferences clearing, and Room DB wipe are performed
synchronously or with proper coroutine handling as appropriate.
In
`@core-base/security/src/androidMain/kotlin/template/core/base/security/TamperDetector.kt`:
- Around line 26-29: The current TamperDetector.actual function
isSignatureValid() unconditionally returns true, effectively disabling signature
checks; change it to fail closed and rely on a real verifier via dependency
injection: make isSignatureValid() return false (or throw) by default, introduce
or use an injectable SignatureVerifier (or IVerifier) interface that
TamperDetector depends on, and have the platform-specific implementation call
that verifier; ensure consumer apps provide a ReleaseSignatureVerifier when
wiring TamperDetector so the production check uses the real signature hash
instead of the unconditional true.
In
`@core-base/security/src/commonMain/kotlin/template/core/base/security/FailedAttemptTracker.kt`:
- Around line 27-54: Replace the volatile Int counter with an atomic integer and
make recordFailure() use atomic operations so increments and resets are
thread-safe: change failedCount to an AtomicInteger and in recordFailure() call
incrementAndGet() to obtain the new count, log using that value, then check
thresholds using that new value; for the wipe threshold use
compareAndSet(newValue, 0) so only the thread that successfully resets the
counter performs secureWiper.wipeSecureStorage() and returns
FailureAction.DATA_WIPED, otherwise if newValue >=
policy.lockAfterFailedAttempts call onLockout() and return
FailureAction.LOCKED_OUT, else return FailureAction.ATTEMPT_RECORDED. Ensure any
reset to 0 after wipe uses the atomic CAS to avoid double-wipe or lost
increments.
In
`@core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityConfig.kt`:
- Around line 16-17: The KDoc for SecurityConfig is outdated: it claims
instances are "created per-platform and passed to [securityModule] as a function
parameter", but the actual module uses a direct instantiation (single {
SecurityConfig() }) in SecurityModule; update the documentation in
SecurityConfig.kt to reflect the real usage by stating that SecurityConfig is
provided by the DI module (securityModule) via single { SecurityConfig() }
rather than being passed in as a function parameter, and mention the
SecurityModule and SecurityConfig symbols so readers can find the
implementation.
In
`@core-base/security/src/commonMain/kotlin/template/core/base/security/SessionManager.kt`:
- Around line 36-71: Replace the volatile Long lastActivityTime with an
AtomicLong and update all accesses to use atomic operations: in startSession()
set the atomic to clock(), in touch() call set(clock()) only when
_isSessionActive.value is true (no read-modify race), and in checkTimeout() read
lastActivityTime via atomic.get() to compute elapsed and decide expiration; keep
endSession() and onSessionExpired() calls as-is but ensure any place that
previously read or wrote lastActivityTime now uses the AtomicLong API (get/set
or compareAndSet if needed) to eliminate the race between touch() and
checkTimeout().
In
`@core-base/security/src/commonTest/kotlin/template/core/base/security/DeepLinkValidatorTest.kt`:
- Around line 16-65: Add two regression tests to DeepLinkValidatorTest to cover
userinfo bypasses: implement tests named urlWithUserinfoIsParsedCorrectly and
urlWithUserinfoAndPasswordIsParsedCorrectly that instantiate
DeepLinkValidator(allowedHosts = setOf("api.mifos.org")) and assert that
"https://api.mifos.org@evil.com/steal" is rejected, that
"https://user:pass@api.mifos.org/path" is accepted, and that
"https://user:pass@evil.com/path" is rejected; these tests should call
DeepLinkValidator.isValid(...) so we verify host parsing ignores userinfo and
prevents the bypass.
In
`@core-base/security/src/commonTest/kotlin/template/core/base/security/SessionManagerTest.kt`:
- Around line 16-53: Add tests that verify SessionManager's timeout behavior:
create a SecurityPolicy with a very short timeout (e.g., milliseconds) or add a
test-only constructor/clock so you can control time, then start a session with
SessionManager, wait past the timeout and assert checkTimeout() returns true,
and also assert that calling recordActivity() before the timeout resets the
timer so checkTimeout() remains false after the same wait; reference
SessionManager, SecurityPolicy(sessionTimeoutMinutes), startSession(),
checkTimeout(), recordActivity(), and endSession() when adding these tests.
In
`@core-base/security/src/desktopMain/kotlin/template/core/base/security/FieldEncryptor.kt`:
- Around line 44-51: In FieldEncryptor.encrypt, the raw key ByteArray remains
live after creating the SecretKeySpec; change the code to create the
SecretKeySpec first, then immediately overwrite/zero the original key array
(e.g., key.fill(0)) and ensure zeroing happens even on exceptions by placing the
zeroing in a finally block around the cipher.init / doFinal operations; continue
to use the created SecretKeySpec (not the original key) when calling Cipher.init
so the key material is scrubbed from the source ByteArray while preserving the
SecretKeySpec for encryption.
- Around line 53-66: The decrypt function leaves the raw key bytes in memory
after building the SecretKeySpec; update actual fun decrypt(...) to zero-out the
key ByteArray returned by keyProvider.getExistingKey() after use (both on
successful decryption and on exceptions). Specifically, capture the key into the
local variable key (as already done), perform cipher.init and cipher.doFinal
using SecretKeySpec(key, "AES"), and then overwrite key.fill(0) in a finally
block (or equivalent cleanup) so the sensitive bytes are scrubbed regardless of
return or thrown exceptions; ensure you do not log or expose the key and that
SecretKeySpec is only constructed from the local key variable.
In
`@core-base/security/src/desktopMain/kotlin/template/core/base/security/SecureKeyProvider.kt`:
- Around line 23-27: The .mifos-secure directory created in the
SecureKeyProvider keyFile lazy initializer is created with default permissions;
update the keyFile initialization to immediately harden the directory
permissions after dir.mkdirs() — for POSIX systems use
java.nio.file.Files.setPosixFilePermissions on dir.toPath() to set owner-only
(rwx for owner) permissions and for Windows fallback to File#setReadable(false,
false)/setReadable(true, true) and equivalent setWritable/setExecutable calls to
restrict access to the current user; ensure any security exceptions are handled
(log or rethrow) so creation fails safely if permissions cannot be applied.
- Around line 34-43: The key file is created with default permissions in
getOrCreateKey (SecureKeyProvider.kt); update getOrCreateKey to apply
restrictive owner-only permissions (rw-------) after creating the file: use
java.nio.file.Files.setPosixFilePermissions on keyFile.toPath() when POSIX is
supported, and fall back to keyFile.setReadable(true, true) / setWritable(true,
true) (and setExecutable(false, false)) for non-POSIX platforms so group/world
cannot read/write the key; ensure this permission-setting happens immediately
after writeBytes(key) (or create) and before returning the key.
In
`@core-base/security/src/desktopMain/kotlin/template/core/base/security/SecureWiper.kt`:
- Around line 18-29: The wipeSecureStorage implementation in
SecureWiper.wipeSecureStorage currently calls file.length().toInt() which will
overflow for files > Int.MAX_VALUE; instead validate the Long size first and
avoid direct toInt conversion: if file.length() <= 0 handle as empty, if
file.length() > Int.MAX_VALUE then overwrite the file in a loop using a
fixed-size ByteArray buffer (e.g., 8KB/64KB) and write successive buffers until
the full length is overwritten, otherwise you may safely allocate a ByteArray of
size file.length().toInt(); after streaming the overwrite ensure you flush and
close the stream before calling file.delete().
In
`@core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureKeyProvider.kt`:
- Around line 70-86: In getOrCreateKey(), if SecItemAdd (call in
SecItemAdd(cfQuery, null)) fails you must securely wipe the freshly generated
key ByteArray before throwing; after creating key with
SecureRandom().nextBytes(32) and before calling error("Failed to store key in
Keychain: $status"), overwrite the key contents (e.g. key.fill(0) or equivalent
secure zeroing) to avoid leaving key material in memory, and ensure any
pinned/native buffers are unpinned/cleared as needed; keep the rest of logic
(getExistingKey(), deleteKey()) unchanged.
In `@gradle/libs.versions.toml`:
- Line 21: Update the BouncyCastle version string for the dependency key
bouncycastle from "1.78.1" to "1.84" (or later) in gradle/libs.versions.toml and
any other occurrences of the same bouncycastle entry (the duplicate referenced
around line 137); search for the symbol "bouncycastle" and replace its version
value so the toml entry reads the updated version to remediate the CVE issues.
---
Nitpick comments:
In `@core-base/security/consumer-rules.pro`:
- Line 4: The current ProGuard rule `-keep class template.core.base.security.**
{ *; }` is too broad; narrow it to only classes/members that require retention
(e.g., reflection entry points, expect/actual bridging classes, or public API
types). Replace the blanket keep with targeted rules that reference specific
class names or packages (for example keep individual classes like
template.core.base.security.SecurityManager,
template.core.base.security.TokenProvider) or use more specific
keepmembers/keepclasseswithmembers patterns or annotations to preserve only
what’s necessary; update the rule(s) that reference
`template.core.base.security.**` accordingly to reduce APK size while retaining
required runtime behavior.
In
`@core-base/security/src/commonMain/kotlin/template/core/base/security/DeepLinkValidator.kt`:
- Around line 38-56: DeepLinkValidator's host extraction can yield an empty host
for malformed unbracketed IPv6 (host variable), which may wrongly pass when
allowedHosts contains "". Modify the host validation in the hostValid block:
after computing hostPort and host, explicitly check if host.isBlank() and treat
that as disallowed (return false for hostValid), log a warning (e.g.,
Logger.w("DeepLinkValidator") { "Empty or malformed host from URI: $uri" }), and
only then check membership in allowedHosts; ensure this change affects the
hostValid calculation used by the return of validate method.
In
`@core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityGate.kt`:
- Around line 65-67: Direct assignment of securityState.isSessionActive inside
the composable body causes updates on every recomposition; replace the
assignment with a side-effect by removing the direct line and wrapping the
update in a LaunchedEffect keyed on the collected state (e.g., after val
isSessionActive by sessionManager.isSessionActive.collectAsState(), add
LaunchedEffect(isSessionActive) { securityState.isSessionActive =
isSessionActive }), referencing SecurityGate, securityState and
sessionManager.isSessionActive.collectAsState to locate where to change.
In
`@core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityPolicy.kt`:
- Around line 16-26: SecurityPolicy's constructor accepts numeric thresholds
without validation; add validation in the SecurityPolicy data class (using an
init block or a validated factory in the companion object) to enforce:
lockAfterFailedAttempts > 0, wipeAfterFailedAttempts > 0, sessionTimeoutMinutes
> 0, clipboardWipeSeconds > 0, and lockAfterFailedAttempts <
wipeAfterFailedAttempts, plus sensible upper bounds for sessionTimeoutMinutes
and clipboardWipeSeconds (e.g., cap to a reasonable max to avoid overflow). When
a value is invalid throw an IllegalArgumentException with a clear message
referencing the offending parameter (e.g., lockAfterFailedAttempts,
wipeAfterFailedAttempts, sessionTimeoutMinutes, clipboardWipeSeconds) so callers
get immediate feedback; update the companion object default() only if you change
construction approach to ensure defaults remain valid.
In
`@core-base/security/src/commonMain/kotlin/template/core/base/security/SensitiveString.kt`:
- Around line 20-21: Update the documentation for SensitiveString.value() to
explicitly state that the returned String cannot be zeroed by close() (close()
only zeros the internal CharArray 'chars'), so callers must call value()
sparingly, avoid storing or caching the returned String long-term, and discard
it immediately after use to preserve the security guarantees of SensitiveString;
reference the value() method, the close() behavior, and the internal 'chars'
array in the note.
In
`@core-base/security/src/jsCommonMain/kotlin/template/core/base/security/SecureWiper.kt`:
- Around line 16-19: The current actual implementation of
SecureWiper.wipeSecureStorage only logs and does not clear browser storages;
update the function (SecureWiper.wipeSecureStorage) to clear both
window.localStorage and window.sessionStorage when running in a JS/WASM browser
context, wrapping access in a try/catch to avoid exceptions in non-browser
environments and logging any errors via Logger.w (or Logger.e) for diagnostics;
ensure the method checks for the presence of the global window and the storage
objects before calling .clear() so it remains safe as a multiplatform stub.
In
`@core-base/security/src/nativeMain/kotlin/template/core/base/security/BiometricAuthenticator.kt`:
- Around line 22-28: In isAvailable() capture the NSError returned by
LAContext.canEvaluatePolicy instead of passing null so you can record why
evaluation failed; update the call in BiometricAuthenticator.isAvailable() to
create an NSError pointer/variable, pass it into
LAContext.canEvaluatePolicy(LAPolicyDeviceOwnerAuthenticationWithBiometrics,
error = ...), and then log or surface the error details (via your existing
logger or a diagnostic print) when the boolean is false to aid debugging.
In
`@core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureKeyProvider.kt`:
- Around line 61-67: The memcpy call in SecureKeyProvider.kt copies raw bytes
from NSData into a pinned ByteArray using platform.posix.memcpy, which is unsafe
without checks; update the code in the result handling (the block that
references nsData, nsData.length and nsData.bytes) to first validate
nsData.bytes is non-null and nsData.length fits in Int (e.g. nsData.length <=
Int.MAX_VALUE), then perform the copy via a safer Cocoa API or Kotlin/Native
helper: either call nsData.getBytes(pinned.addressOf(0), nsData.length) if
available, or keep the pinned ByteArray but replace direct memcpy with a bounded
copy using the NSData.getBytes API or kotlinx.cinterop utilities; ensure you
reference the existing symbols nsData, length, bytes, and the pinned ByteArray
in your change and add the null/length checks before copying.
In
`@core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureWiper.kt`:
- Around line 21-25: The platform implementation SecureWiper.wipeSecureStorage
only logs and relies on consumers to remove Keychain items; add explicit,
prominent documentation: update the common expect declaration for
wipeSecureStorage (and the SecureWiper type) with KDoc that states the method
does not perform Keychain/UserDefaults deletion on Apple platforms and that
consumer apps MUST delete items for service "org.mifos.secure" and clear any
UserDefaults, and mirror the same KDoc on the nativeMain actual implementation
to make the contract unambiguous to callers (referencing SecureWiper and
wipeSecureStorage by name).
In
`@core-base/security/src/nativeMain/kotlin/template/core/base/security/TamperDetector.kt`:
- Around line 24-28: The isDebuggerAttached() implementation is a stub that
always returns false; replace the hardcoded return with a real platform-specific
detection (or fail fast) so callers don't assume debugging is absent. Update the
actual fun isDebuggerAttached() to perform OS checks: on Apple targets use
sysctl/KERN_PROC to check P_TRACED; on Linux read /proc/self/status (TracerPid)
or use ptrace; on Windows call IsDebuggerPresent via native interop; if you
cannot implement all targets now, throw NotImplementedError in
isDebuggerAttached() so callers surface that it's unimplemented rather than
silently returning false. Ensure you modify the isDebuggerAttached() function
and add any small helper functions or native imports needed for each target.
In
`@core/network/src/androidMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.android.kt`:
- Around line 32-41: The Logging installation currently sets level =
LogLevel.NONE which disables all logs, making the custom logger and the
Logger.DEFAULT assignment dead code; remove the entire install(Logging) block
(or at minimum remove the unreachable logger = Logger.DEFAULT and the anonymous
Logger that calls KermitLogger.d) from KtorHttpClient.android.kt, or
alternatively replace the hardcoded LogLevel.NONE with a build-time/config
constant (e.g., LOG_LEVEL) and keep the custom logger (the anonymous Logger that
calls KermitLogger.d with tag "KtorClient") so logging can be enabled via
configuration.
In
`@core/network/src/desktopMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.desktop.kt`:
- Around line 30-39: The Logging install block in KtorHttpClient.desktop.kt sets
level = LogLevel.NONE, which disables all logs and renders the subsequent logger
assignments (Logger.DEFAULT and the anonymous Logger implementation) dead code;
remove the entire custom logger configuration (the logger = Logger.DEFAULT line
and the anonymous object block inside install(Logging)) to avoid confusion, or
alternatively make the log level configurable (replace LogLevel.NONE with a
build/config constant and only set the custom logger when that constant enables
logging) so the custom logger and level are consistent.
In
`@core/network/src/jsMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.js.kt`:
- Around line 30-39: The Logging plugin is configured with LogLevel.NONE so no
logs will be emitted, making the custom logger implementation and the initial
logger = Logger.DEFAULT assignment redundant; update the install(Logging) block
in KtorHttpClient.js.kt to either remove both logger = Logger.DEFAULT and the
object : Logger { override fun log(...) } when level is NONE, or conditionally
assign the custom logger only when level != LogLevel.NONE so the custom
implementation (the object Logger and its override) isn’t dead code.
In
`@core/network/src/nativeMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.native.kt`:
- Around line 30-39: The install(Logging) block currently sets logger =
Logger.DEFAULT and then immediately overrides it with a custom Logger object
while also setting level = LogLevel.NONE, which makes the custom logger and the
DEFAULT assignment redundant; remove the unnecessary assignments and custom
Logger when LogLevel.NONE is used. Edit the install(Logging) { ... } block in
KtorHttpClient.native.kt to either remove the entire Logging installation when
LogLevel.NONE is configured or keep only the level assignment (LogLevel.NONE)
without assigning Logger.DEFAULT or the anonymous Logger object, ensuring no
dead/unused logger code remains. Make changes around the install(Logging) block
and the anonymous object implementing Logger to eliminate the redundant logger
configuration.
In
`@core/network/src/wasmJsMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.wasmJs.kt`:
- Around line 30-39: Remove the redundant logger setup in the Logging plugin
configuration: since level = LogLevel.NONE produces no logs, delete the custom
Logger implementation and the initial logger = Logger.DEFAULT assignment inside
the install(Logging) block in KtorHttpClient.wasmJs.kt; instead keep a minimal
install(Logging) with level = LogLevel.NONE (or remove the entire install if not
needed) so there is no dead/never-invoked Logger object.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86c80e65-7a74-483f-9b93-99fe40bfc255
📒 Files selected for processing (69)
cmp-navigation/build.gradle.ktscmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.ktcore-base/security/build.gradle.ktscore-base/security/consumer-rules.procore-base/security/src/androidMain/kotlin/template/core/base/security/BiometricAuthenticator.ktcore-base/security/src/androidMain/kotlin/template/core/base/security/BuildInfo.ktcore-base/security/src/androidMain/kotlin/template/core/base/security/FieldEncryptor.ktcore-base/security/src/androidMain/kotlin/template/core/base/security/SecureKeyProvider.ktcore-base/security/src/androidMain/kotlin/template/core/base/security/SecureRandom.ktcore-base/security/src/androidMain/kotlin/template/core/base/security/SecureWiper.ktcore-base/security/src/androidMain/kotlin/template/core/base/security/TamperDetector.ktcore-base/security/src/androidMain/kotlin/template/core/base/security/di/SecurityModule.android.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/BiometricAuthenticator.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/BuildInfo.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/CertificatePinConfig.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/DeepLinkValidator.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/FailedAttemptTracker.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/FieldEncryptor.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SecureAuthManager.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SecureKeyProvider.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SecureNavHandler.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SecureRandom.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SecureWiper.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SecurityConfig.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SecurityGate.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SecurityPolicy.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SecurityState.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SensitiveString.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/SessionManager.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/TamperDetector.ktcore-base/security/src/commonMain/kotlin/template/core/base/security/di/SecurityModule.ktcore-base/security/src/commonTest/kotlin/template/core/base/security/DeepLinkValidatorTest.ktcore-base/security/src/commonTest/kotlin/template/core/base/security/FailedAttemptTrackerTest.ktcore-base/security/src/commonTest/kotlin/template/core/base/security/SecureAuthManagerTest.ktcore-base/security/src/commonTest/kotlin/template/core/base/security/SecureNavHandlerTest.ktcore-base/security/src/commonTest/kotlin/template/core/base/security/SecurityStateTest.ktcore-base/security/src/commonTest/kotlin/template/core/base/security/SensitiveStringTest.ktcore-base/security/src/commonTest/kotlin/template/core/base/security/SessionManagerTest.ktcore-base/security/src/desktopMain/kotlin/template/core/base/security/BiometricAuthenticator.ktcore-base/security/src/desktopMain/kotlin/template/core/base/security/BuildInfo.ktcore-base/security/src/desktopMain/kotlin/template/core/base/security/FieldEncryptor.ktcore-base/security/src/desktopMain/kotlin/template/core/base/security/SecureKeyProvider.ktcore-base/security/src/desktopMain/kotlin/template/core/base/security/SecureRandom.ktcore-base/security/src/desktopMain/kotlin/template/core/base/security/SecureWiper.ktcore-base/security/src/desktopMain/kotlin/template/core/base/security/TamperDetector.ktcore-base/security/src/desktopMain/kotlin/template/core/base/security/di/SecurityModule.desktop.ktcore-base/security/src/jsCommonMain/kotlin/template/core/base/security/BiometricAuthenticator.ktcore-base/security/src/jsCommonMain/kotlin/template/core/base/security/BuildInfo.ktcore-base/security/src/jsCommonMain/kotlin/template/core/base/security/FieldEncryptor.ktcore-base/security/src/jsCommonMain/kotlin/template/core/base/security/SecureKeyProvider.ktcore-base/security/src/jsCommonMain/kotlin/template/core/base/security/SecureRandom.ktcore-base/security/src/jsCommonMain/kotlin/template/core/base/security/SecureWiper.ktcore-base/security/src/jsCommonMain/kotlin/template/core/base/security/TamperDetector.ktcore-base/security/src/jsCommonMain/kotlin/template/core/base/security/di/SecurityModule.ktcore-base/security/src/nativeMain/kotlin/template/core/base/security/BiometricAuthenticator.ktcore-base/security/src/nativeMain/kotlin/template/core/base/security/BuildInfo.ktcore-base/security/src/nativeMain/kotlin/template/core/base/security/FieldEncryptor.ktcore-base/security/src/nativeMain/kotlin/template/core/base/security/SecureKeyProvider.ktcore-base/security/src/nativeMain/kotlin/template/core/base/security/SecureRandom.ktcore-base/security/src/nativeMain/kotlin/template/core/base/security/SecureWiper.ktcore-base/security/src/nativeMain/kotlin/template/core/base/security/TamperDetector.ktcore-base/security/src/nativeMain/kotlin/template/core/base/security/di/SecurityModule.native.ktcore/network/src/androidMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.android.ktcore/network/src/desktopMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.desktop.ktcore/network/src/jsMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.js.ktcore/network/src/nativeMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.native.ktcore/network/src/wasmJsMain/kotlin/org/mifos/mobile/core/network/KtorHttpClient.wasmJs.ktgradle/libs.versions.tomlsettings.gradle.kts
| * module loading. Debug builds typically have a debugger available; | ||
| * release builds do not. | ||
| */ | ||
| actual fun isReleaseBuild(): Boolean = !Debug.isDebuggerConnected() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On Android, does Debug.isDebuggerConnected() indicate build type (debug vs release), or only runtime debugger attachment?
💡 Result:
Debug.isDebuggerConnected indicates only runtime debugger attachment, not build type (debug vs release).[1][2][3] The official Android documentation states it "Determine[s] if a debugger is currently attached."[2] The method delegates to VMDebug.isDebuggerConnected, a native method that checks if a JDWP debugger is actively connected to the VM.[1][3][4] This returns true only when a debugger (e.g., via Android Studio's debug button or ADB) is attached at runtime, even on release builds if debuggable.[5][6] It does not detect build type. Debug builds have debuggable=true (check via BuildConfig.DEBUG or ApplicationInfo.FLAG_DEBUGGABLE), allowing debugger attachment, but isDebuggerConnected remains false without an active connection.[5][6][7][8] Release builds typically have debuggable=false, preventing attachment.[9]
Citations:
- 1: https://android.googlesource.com/platform/frameworks/base/+/HEAD/core/java/android/os/Debug.java
- 2: https://developer.android.com/reference/android/os/Debug
- 3: https://android.googlesource.com/platform/libcore/+/master/dalvik/src/main/java/dalvik/system/VMDebug.java
- 4: https://android.googlesource.com/platform/dalvik.git/+/android-4.2.2_r1/vm/Debugger.cpp
- 5: https://stackoverflow.com/questions/53560750/how-to-programmatically-tell-if-my-app-is-being-debugged
- 6: https://stackoverflow.com/questions/7022653/how-to-check-programmatically-whether-app-is-running-in-debug-mode-or-not
- 7: https://stackoverflow.com/questions/23844667/how-do-i-detect-if-i-am-in-release-or-debug-mode
- 8: https://stackoverflow.com/questions/63243128/why-is-buildconfig-debug-still-true-if-app-isnt-launched-using-the-debug-button
- 9: https://stackoverflow.com/questions/38864358/difference-between-debug-and-release-apks
🏁 Script executed:
# Find BuildConfig files in the project
fd -type f -name "BuildConfig.kt" -o -name "BuildConfig.java"Repository: openMF/mifos-mobile
Length of output: 234
🏁 Script executed:
# Look for the expect declaration of isReleaseBuild
rg "expect fun isReleaseBuild" --type kotlinRepository: openMF/mifos-mobile
Length of output: 182
🏁 Script executed:
# Find all usages of isReleaseBuild
rg "isReleaseBuild" --type kotlin -B 2 -A 2Repository: openMF/mifos-mobile
Length of output: 4186
🏁 Script executed:
# Check the file structure around the Android security module
ls -la core-base/security/src/Repository: openMF/mifos-mobile
Length of output: 530
🏁 Script executed:
# Get the full context of the expect declaration
cat -n core-base/security/src/commonMain/kotlin/template/core/base/security/BuildInfo.ktRepository: openMF/mifos-mobile
Length of output: 850
🏁 Script executed:
# Get the full context of the Android implementation
cat -n core-base/security/src/androidMain/kotlin/template/core/base/security/BuildInfo.ktRepository: openMF/mifos-mobile
Length of output: 984
🏁 Script executed:
# Check how isReleaseBuild is used in SecurityConfig
cat -n core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityConfig.ktRepository: openMF/mifos-mobile
Length of output: 1400
🏁 Script executed:
# Search for any references to BuildConfig in Android module
rg "BuildConfig" core-base/security/src/androidMain/ -B 3 -A 3Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
# Check if there's any defensive logic checking debugger state elsewhere
rg "isDebuggerConnected\|BuildConfig" core-base/security/src/ -B 2 -A 2Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
# Check build.gradle to understand the build type configuration
fd -name "build.gradle*" -o -name "*.gradle.kts" | head -5 | xargs grep -l "debug\|release" 2>/dev/nullRepository: openMF/mifos-mobile
Length of output: 296
Build-type detection relies on runtime debugger state instead of build configuration.
!Debug.isDebuggerConnected() returns true only when no debugger is actively attached—not based on actual build type. A debug build run without a debugger will be misclassified as release, causing SecurityConfig to apply release-level hardening (data wipe thresholds, session timeout, clipboard auto-wipe) in a debug scenario. Use BuildConfig.DEBUG as the primary signal for build-type detection on Android.
🤖 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
`@core-base/security/src/androidMain/kotlin/template/core/base/security/BuildInfo.kt`
at line 22, Replace the runtime-debugger check in isReleaseBuild() (currently
using Debug.isDebuggerConnected()) with the Android build flag: return
!BuildConfig.DEBUG so build-type detection uses the generated BuildConfig.DEBUG
value; update the import/qualifier as needed for your app's BuildConfig and
ensure SecurityConfig will use isReleaseBuild() for applying release-level
hardening.
Comment on lines
+52
to
+56
| cipher.init( | ||
| Cipher.DECRYPT_MODE, | ||
| SecretKeySpec(key, "AES"), | ||
| GCMParameterSpec(GCM_TAG_LENGTH, iv), | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no remaining SecretKeySpec reconstruction in Android security crypto paths.
rg -n --type=kt 'SecretKeySpec\s*\(' core-base/security/src/androidMain/kotlin/template/core/base/securityRepository: openMF/mifos-mobile
Length of output: 1964
🏁 Script executed:
#!/bin/bash
# First, locate the file to understand its full path
find . -name "FieldEncryptor.kt" -type f 2>/dev/null | head -20Repository: openMF/mifos-mobile
Length of output: 512
🏁 Script executed:
#!/bin/bash
# Read the Android implementation file to verify the code at lines 52-56
cat -n ./core-base/security/src/androidMain/kotlin/template/core/base/security/FieldEncryptor.ktRepository: openMF/mifos-mobile
Length of output: 2706
🏁 Script executed:
#!/bin/bash
# Check for any SecretKeySpec usage in the Android security directory
rg -n "SecretKeySpec" ./core-base/security/src/androidMain/kotlin/Repository: openMF/mifos-mobile
Length of output: 195
🏁 Script executed:
#!/bin/bash
# Check the other implementations (commonMain, desktopMain) to see how they handle decrypt
echo "=== Common Main Implementation ==="
sed -n '45,59p' ./core-base/security/src/commonMain/kotlin/template/core/base/security/FieldEncryptor.kt
echo -e "\n=== Desktop Main Implementation ==="
sed -n '45,59p' ./core-base/security/src/desktopMain/kotlin/template/core/base/security/FieldEncryptor.ktRepository: openMF/mifos-mobile
Length of output: 937
🏁 Script executed:
#!/bin/bash
# Check the complete decrypt method in desktopMain to see the full pattern
sed -n '45,60p' ./core-base/security/src/desktopMain/kotlin/template/core/base/security/FieldEncryptor.ktRepository: openMF/mifos-mobile
Length of output: 886
🏁 Script executed:
#!/bin/bash
# Get the full desktopMain decrypt method cipher.init call
sed -n '50,65p' ./core-base/security/src/desktopMain/kotlin/template/core/base/security/FieldEncryptor.ktRepository: openMF/mifos-mobile
Length of output: 775
🏁 Script executed:
#!/bin/bash
# Verify the exact import statements in androidMain to confirm SecretKeySpec is missing
head -20 ./core-base/security/src/androidMain/kotlin/template/core/base/security/FieldEncryptor.kt | grep -E "^import|^package"Repository: openMF/mifos-mobile
Length of output: 225
Invalid key initialization in decrypt path — compilation failure and API mismatch.
This uses SecretKeySpec(key, "AES"), but key is a SecretKey object, not raw bytes. SecretKeySpec is not even imported, causing a compilation error. More critically, SecretKeySpec constructor expects raw bytes (byte[]), not a SecretKey. The encrypt method (line 39) correctly passes the SecretKey directly; the decrypt method should do the same. This breaks Android Keystore integration, which relies on non-extractable key objects that cannot be converted to raw bytes.
Suggested fix
cipher.init(
Cipher.DECRYPT_MODE,
- SecretKeySpec(key, "AES"),
+ key,
GCMParameterSpec(GCM_TAG_LENGTH, iv),
)🤖 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
`@core-base/security/src/androidMain/kotlin/template/core/base/security/FieldEncryptor.kt`
around lines 52 - 56, In FieldEncryptor's decrypt path replace the incorrect
SecretKeySpec usage: change the cipher.init call in the decrypt method (the
block that currently calls Cipher.DECRYPT_MODE with SecretKeySpec(key, "AES"))
to pass the existing SecretKey instance directly (same pattern as the encrypt
method) along with the GCMParameterSpec(iv) so you don't attempt to construct
raw-key bytes; remove any unused SecretKeySpec import if present.
Comment on lines
+15
to
+17
| actual fun nextBytes(size: Int): ByteArray { | ||
| val bytes = ByteArray(size) | ||
| random.nextBytes(bytes) |
Contributor
There was a problem hiding this comment.
Validate size before allocation.
A negative size currently fails with NegativeArraySizeException. Add an explicit precondition for clearer failure behavior.
Suggested fix
actual fun nextBytes(size: Int): ByteArray {
+ require(size >= 0) { "size must be non-negative" }
val bytes = ByteArray(size)
random.nextBytes(bytes)
return bytes
}📝 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.
Suggested change
| actual fun nextBytes(size: Int): ByteArray { | |
| val bytes = ByteArray(size) | |
| random.nextBytes(bytes) | |
| actual fun nextBytes(size: Int): ByteArray { | |
| require(size >= 0) { "size must be non-negative" } | |
| val bytes = ByteArray(size) | |
| random.nextBytes(bytes) | |
| return bytes | |
| } |
🤖 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
`@core-base/security/src/androidMain/kotlin/template/core/base/security/SecureRandom.kt`
around lines 15 - 17, The nextBytes function in SecureRandom should validate the
size parameter before allocating the ByteArray to avoid a
NegativeArraySizeException; add an explicit precondition in nextBytes (e.g., in
the SecureRandom.actual fun nextBytes(size: Int)) that checks size is
non‑negative and throws a clear IllegalArgumentException (or uses require(size
>= 0) with a message) if not, then proceed to allocate bytes and call
random.nextBytes(bytes).
Comment on lines
+17
to
+22
| actual fun wipeSecureStorage() { | ||
| Logger.w("SecureWiper") { "Secure storage wipe triggered" } | ||
| // Consumer apps should clear EncryptedSharedPreferences, | ||
| // delete encryption keys from Android Keystore, and | ||
| // clear Room database here. | ||
| } |
Contributor
There was a problem hiding this comment.
wipeSecureStorage() is effectively a no-op.
Only logging here leaves cryptographic material intact after a wipe trigger. This should perform at least keystore key deletion in this module implementation.
Suggested direction
actual class SecureWiper actual constructor() {
actual fun wipeSecureStorage() {
Logger.w("SecureWiper") { "Secure storage wipe triggered" }
+ SecureKeyProvider().deleteKey()
// Consumer apps should clear EncryptedSharedPreferences,
// delete encryption keys from Android Keystore, and
// clear Room database here.
}🤖 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
`@core-base/security/src/androidMain/kotlin/template/core/base/security/SecureWiper.kt`
around lines 17 - 22, wipeSecureStorage() currently only logs and must actively
remove cryptographic material; update the SecureWiper.actual fun
wipeSecureStorage implementation to (1) open the AndroidKeyStore
(KeyStore.getInstance("AndroidKeyStore"), load(null)) and delete relevant key
aliases via deleteEntry(alias) (or iterate and delete all app-specific aliases),
(2) clear any EncryptedSharedPreferences used by the app by obtaining the
EncryptedSharedPreferences instance and calling edit().clear().commit()/apply(),
and (3) clear the app's Room databases (invoke the Room database clear/DAO
cleanup method). Focus changes inside SecureWiper.wipeSecureStorage so key
deletion, EncryptedSharedPreferences clearing, and Room DB wipe are performed
synchronously or with proper coroutine handling as appropriate.
Comment on lines
+26
to
+29
| actual fun isSignatureValid(): Boolean { | ||
| // Consumer apps should override with their release signature hash | ||
| return true | ||
| } |
Contributor
There was a problem hiding this comment.
Signature integrity check is bypassed by default.
Returning true unconditionally disables this security signal. Please switch to a real verifier via DI, or fail closed until one is provided.
🤖 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
`@core-base/security/src/androidMain/kotlin/template/core/base/security/TamperDetector.kt`
around lines 26 - 29, The current TamperDetector.actual function
isSignatureValid() unconditionally returns true, effectively disabling signature
checks; change it to fail closed and rely on a real verifier via dependency
injection: make isSignatureValid() return false (or throw) by default, introduce
or use an injectable SignatureVerifier (or IVerifier) interface that
TamperDetector depends on, and have the platform-specific implementation call
that verifier; ensure consumer apps provide a ReleaseSignatureVerifier when
wiring TamperDetector so the production check uses the real signature hash
instead of the unconditional true.
Comment on lines
+23
to
+27
| private val keyFile: File by lazy { | ||
| val dir = File(System.getProperty("user.home"), ".mifos-secure") | ||
| dir.mkdirs() | ||
| File(dir, "field_key.bin") | ||
| } |
Contributor
There was a problem hiding this comment.
Set restrictive permissions on the key directory.
The .mifos-secure directory is created with default permissions, which may allow other users on the same system to access it. On multi-user desktop environments, this exposes the key file to local privilege escalation.
🔒 Proposed fix to harden directory permissions
private val keyFile: File by lazy {
val dir = File(System.getProperty("user.home"), ".mifos-secure")
- dir.mkdirs()
+ if (!dir.exists()) {
+ dir.mkdirs()
+ dir.setReadable(true, true) // Owner-only read
+ dir.setWritable(true, true) // Owner-only write
+ dir.setExecutable(true, true) // Owner-only execute (required for directory access)
+ }
File(dir, "field_key.bin")
}📝 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.
Suggested change
| private val keyFile: File by lazy { | |
| val dir = File(System.getProperty("user.home"), ".mifos-secure") | |
| dir.mkdirs() | |
| File(dir, "field_key.bin") | |
| } | |
| private val keyFile: File by lazy { | |
| val dir = File(System.getProperty("user.home"), ".mifos-secure") | |
| if (!dir.exists()) { | |
| dir.mkdirs() | |
| dir.setReadable(true, true) // Owner-only read | |
| dir.setWritable(true, true) // Owner-only write | |
| dir.setExecutable(true, true) // Owner-only execute (required for directory access) | |
| } | |
| File(dir, "field_key.bin") | |
| } |
🤖 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
`@core-base/security/src/desktopMain/kotlin/template/core/base/security/SecureKeyProvider.kt`
around lines 23 - 27, The .mifos-secure directory created in the
SecureKeyProvider keyFile lazy initializer is created with default permissions;
update the keyFile initialization to immediately harden the directory
permissions after dir.mkdirs() — for POSIX systems use
java.nio.file.Files.setPosixFilePermissions on dir.toPath() to set owner-only
(rwx for owner) permissions and for Windows fallback to File#setReadable(false,
false)/setReadable(true, true) and equivalent setWritable/setExecutable calls to
restrict access to the current user; ensure any security exceptions are handled
(log or rethrow) so creation fails safely if permissions cannot be applied.
Comment on lines
+34
to
+43
| actual fun getOrCreateKey(): Any { | ||
| val existing = getExistingKey() as? ByteArray | ||
| if (existing != null) return existing | ||
|
|
||
| val key = ByteArray(32) | ||
| JSecureRandom().nextBytes(key) | ||
| keyFile.parentFile?.mkdirs() | ||
| keyFile.writeBytes(key) | ||
| return key | ||
| } |
Contributor
There was a problem hiding this comment.
Set restrictive permissions on the key file.
The key file is created with default permissions. On Unix-like systems, this typically allows group/world read access depending on umask. For a plaintext encryption key, only the owner should have read/write access.
🔒 Proposed fix to harden file permissions
val key = ByteArray(32)
JSecureRandom().nextBytes(key)
keyFile.parentFile?.mkdirs()
keyFile.writeBytes(key)
+ keyFile.setReadable(true, true) // Owner-only read
+ keyFile.setWritable(true, true) // Owner-only write
+ keyFile.setExecutable(false, false) // No execute permission
return key🤖 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
`@core-base/security/src/desktopMain/kotlin/template/core/base/security/SecureKeyProvider.kt`
around lines 34 - 43, The key file is created with default permissions in
getOrCreateKey (SecureKeyProvider.kt); update getOrCreateKey to apply
restrictive owner-only permissions (rw-------) after creating the file: use
java.nio.file.Files.setPosixFilePermissions on keyFile.toPath() when POSIX is
supported, and fall back to keyFile.setReadable(true, true) / setWritable(true,
true) (and setExecutable(false, false)) for non-POSIX platforms so group/world
cannot read/write the key; ensure this permission-setting happens immediately
after writeBytes(key) (or create) and before returning the key.
Comment on lines
+18
to
+29
| actual fun wipeSecureStorage() { | ||
| Logger.w("SecureWiper") { "Secure storage wipe triggered" } | ||
| val secureDir = File(System.getProperty("user.home"), ".mifos-secure") | ||
| if (secureDir.exists()) { | ||
| secureDir.listFiles()?.forEach { file -> | ||
| // Overwrite before delete | ||
| val length = file.length() | ||
| file.writeBytes(ByteArray(length.toInt())) | ||
| file.delete() | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Validate file size before converting to Int.
Line 25 converts file.length() (Long) to Int without bounds checking. For files larger than ~2GB, this overflows and creates a small overwrite array, leaving most of the file data intact on disk.
🛡️ Proposed fix to add size validation
if (secureDir.exists()) {
secureDir.listFiles()?.forEach { file ->
// Overwrite before delete
val length = file.length()
+ if (length > Int.MAX_VALUE) {
+ Logger.w("SecureWiper") { "Skipping overwrite of large file: ${file.name} (${length} bytes)" }
+ file.delete()
+ return@forEach
+ }
file.writeBytes(ByteArray(length.toInt()))
file.delete()
}
}🤖 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
`@core-base/security/src/desktopMain/kotlin/template/core/base/security/SecureWiper.kt`
around lines 18 - 29, The wipeSecureStorage implementation in
SecureWiper.wipeSecureStorage currently calls file.length().toInt() which will
overflow for files > Int.MAX_VALUE; instead validate the Long size first and
avoid direct toInt conversion: if file.length() <= 0 handle as empty, if
file.length() > Int.MAX_VALUE then overwrite the file in a loop using a
fixed-size ByteArray buffer (e.g., 8KB/64KB) and write successive buffers until
the full length is overwritten, otherwise you may safely allocate a ByteArray of
size file.length().toInt(); after streaming the overwrite ensure you flush and
close the stream before calling file.delete().
Comment on lines
+70
to
+86
| actual fun getOrCreateKey(): Any { | ||
| val existing = getExistingKey() as? ByteArray | ||
| if (existing != null) return existing | ||
|
|
||
| deleteKey() | ||
| val key = SecureRandom().nextBytes(32) | ||
| val nsData = key.usePinned { pinned -> | ||
| NSData.create(bytes = pinned.addressOf(0), length = key.size.toULong()) | ||
| } | ||
| val query = buildKeychainQuery(kSecValueData to nsData) | ||
| val cfQuery = query as CFDictionaryRef | ||
| val status = SecItemAdd(cfQuery, null) | ||
| if (status != errSecSuccess) { | ||
| error("Failed to store key in Keychain: $status") | ||
| } | ||
| return key | ||
| } |
Contributor
There was a problem hiding this comment.
Potential key material leak on Keychain storage failure.
If SecItemAdd fails on Line 82, the function throws via error() on Line 83, but the freshly generated key ByteArray (Line 75) remains in memory without being wiped. Consider wiping the key material on the error path to prevent leakage.
🔒 Proposed fix to wipe key on error
val query = buildKeychainQuery(kSecValueData to nsData)
val cfQuery = query as CFDictionaryRef
val status = SecItemAdd(cfQuery, null)
if (status != errSecSuccess) {
+ key.fill(0) // Wipe key material before throwing
error("Failed to store key in Keychain: $status")
}
return key📝 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.
Suggested change
| actual fun getOrCreateKey(): Any { | |
| val existing = getExistingKey() as? ByteArray | |
| if (existing != null) return existing | |
| deleteKey() | |
| val key = SecureRandom().nextBytes(32) | |
| val nsData = key.usePinned { pinned -> | |
| NSData.create(bytes = pinned.addressOf(0), length = key.size.toULong()) | |
| } | |
| val query = buildKeychainQuery(kSecValueData to nsData) | |
| val cfQuery = query as CFDictionaryRef | |
| val status = SecItemAdd(cfQuery, null) | |
| if (status != errSecSuccess) { | |
| error("Failed to store key in Keychain: $status") | |
| } | |
| return key | |
| } | |
| actual fun getOrCreateKey(): Any { | |
| val existing = getExistingKey() as? ByteArray | |
| if (existing != null) return existing | |
| deleteKey() | |
| val key = SecureRandom().nextBytes(32) | |
| val nsData = key.usePinned { pinned -> | |
| NSData.create(bytes = pinned.addressOf(0), length = key.size.toULong()) | |
| } | |
| val query = buildKeychainQuery(kSecValueData to nsData) | |
| val cfQuery = query as CFDictionaryRef | |
| val status = SecItemAdd(cfQuery, null) | |
| if (status != errSecSuccess) { | |
| key.fill(0) // Wipe key material before throwing | |
| error("Failed to store key in Keychain: $status") | |
| } | |
| return key | |
| } |
🤖 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
`@core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureKeyProvider.kt`
around lines 70 - 86, In getOrCreateKey(), if SecItemAdd (call in
SecItemAdd(cfQuery, null)) fails you must securely wipe the freshly generated
key ByteArray before throwing; after creating key with
SecureRandom().nextBytes(32) and before calling error("Failed to store key in
Keychain: $status"), overwrite the key contents (e.g. key.fill(0) or equivalent
secure zeroing) to avoid leaving key material in memory, and ensure any
pinned/native buffers are unpinned/cleared as needed; keep the rest of logic
(getExistingKey(), deleteKey()) unchanged.
| androidxTracing = "1.3.0" | ||
| appcompatVersion = "1.7.1" | ||
| androidxBrowser = "1.8.0" | ||
| bouncycastle = "1.78.1" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of BouncyCastle bcprov-jdk18on and are there any security advisories for version 1.78.1?
💡 Result:
Latest version (bcprov-jdk18on): 1.84 as of 2026-05-13 [1][2]. Security advisories for version 1.78.1: - The Bouncy Castle Java release notes for 1.84 list security fixes including CVE-2026-5588 (PKIX draft CompositeVerifier accepts empty signature sequence as valid) and CVE-2026-5598 (non-constant time comparisons risk private key leakage in FrodoKEM). These fixes are included starting with 1.84 [2]. - Public CVE coverage indicates that CVE-2026-5588 affects BC-JAVA versions from 1.49 up to but not including 1.84, which includes 1.78.1 [3]. So yes—there are relevant security issues affecting the codebase used by bcprov-jdk18on 1.78.1; upgrading to at least 1.84 is the remediating step indicated by the 1.84 security fixes and the affected-version range for CVE-2026-5588 [2][3].
Citations:
- 1: https://repo1.maven.org/maven2/org/bouncycastle/bcprov-jdk18on/
- 2: https://www.bouncycastle.org/download/bouncy-castle-java/
- 3: https://vulert.com/vuln-db/--use-of-a-broken-or-risky-cryptographic-algorithm-vulnerability-in-legion-of-the-bouncy-castle-inc----
Upgrade BouncyCastle to version 1.84 to address known security vulnerabilities.
Version 1.78.1 is outdated (latest is 1.84 as of May 2026) and affected by CVE-2026-5588 (PKIX draft CompositeVerifier incorrectly accepts empty signature sequences) and CVE-2026-5598 (non-constant time comparisons in FrodoKEM). Upgrade to 1.84 or later to remediate.
Also applies to: line 137
🤖 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 `@gradle/libs.versions.toml` at line 21, Update the BouncyCastle version string
for the dependency key bouncycastle from "1.78.1" to "1.84" (or later) in
gradle/libs.versions.toml and any other occurrences of the same bouncycastle
entry (the duplicate referenced around line 137); search for the symbol
"bouncycastle" and replace its version value so the toml entry reads the updated
version to remediate the CVE issues.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
hi @therajanmaurya sir
This PR fixes multiple security and platform compatibility issues identified during CodeRabbit review of the pull request #3139 in the
core-base/securitymodule.The changes focus on secure key management, encryption consistency, deep-link validation hardening, and fail-safe behavior across unsupported platforms.
What was fixed
Secure Android Keystore Handling
Refactored the key API from raw-byte extraction to key-handle based access:
getExistingKeygetOrCreateKeyAndroid Keystore keys are non-extractable (
secretKey.encoded == null) on real devices, so the previous implementation was not reliable.FieldEncryptornow usesSecretKeyobjects directly instead of reconstructing keys usingSecretKeySpec(byte[]).Encryption Contract Consistency
Added and validated the
ENC:prefix consistently across:Prevents ambiguity between encrypted and plain-text values.
Deep Link Validation Hardening
Fixed authority parsing logic to prevent bypasses using crafted URLs such as:
userinfo@hostValidation now correctly resolves and verifies the actual host component.
JS/Wasm Security Hardening
kotlin.random.Random) from cryptographic flows.SecureRandomnow fails closed instead of silently using insecure randomness.FieldEncryptornow throws instead of behaving as a no-op.Native Encryption Safety
Dependency Cleanup
Removed deprecated:
androidx.security:security-cryptoCleaned related version catalog references.
Files Updated
SecureKeyProvider.ktFieldEncryptor.ktDeepLinkValidator.ktSecureRandom.ktbuild.gradle.ktslibs.versions.tomlSummary by CodeRabbit
New Features
Security Improvements