feat(security): integrate cross-platform core-base security module#3139
feat(security): integrate cross-platform core-base security module#3139Divyateja2709 wants to merge 2 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive, multiplatform security module ( ChangesCore Security Module with Multiplatform Implementations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (19)
core-base/security/src/commonMain/kotlin/template/core/base/security/SensitiveString.kt-20-21 (1)
20-21: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument the security implications of
value()and consider usage guidance.The
value()method creates an immutableStringthat will persist in heap memory and cannot be zeroed, which partially undermines the security benefit of usingCharArray. Each call tovalue()creates a newStringcopy that may linger until garbage collection.Consider adding KDoc guidance:
- The returned
Stringcannot be cleaned and will persist in memory- Callers should minimize calls to
value()and use the returnedStringimmediately- Prefer using this class in short-lived scopes with
.use { }blocks📝 Proposed documentation enhancement
- /** Read the value. Call [close] when done. */ + /** + * Read the value as a [String]. + * + * **Security note**: The returned [String] is immutable and will persist + * in heap memory until garbage collected. Minimize calls to this method + * and use the returned value immediately. Prefer wrapping usage in a + * `.use { }` block to ensure [close] is called promptly. + * + * `@return` the sensitive value as a String (cannot be zeroed after use) + */ fun value(): String = chars.concatToString()🤖 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 KDoc for SensitiveString.value() to document the security implications: state that value() returns an immutable String created via chars.concatToString() that cannot be zeroed and may persist in heap memory, warn callers to minimize calls and to use the returned String immediately, recommend preferring short‑lived scopes and using the SensitiveString with .use { } or similar disposal patterns, and call out that repeated calls create additional lingering copies; attach this guidance directly above the value() function to make it obvious to consumers.core-base/security/src/commonMain/kotlin/template/core/base/security/SensitiveString.kt-31-35 (1)
31-35:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftImplement constant-time comparison to prevent timing attacks.
The current implementation uses
contentEquals, which is not constant-time and will short-circuit on the first differing character. This creates a timing side-channel that can leak information about the sensitive data, enabling attackers to guess passwords, tokens, or keys bit-by-bit through repeated comparisons.For security-critical credentials, you should implement a constant-time comparison that always examines every character regardless of mismatches.
🔒 Proposed constant-time comparison
override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is SensitiveString) return false - return chars.contentEquals(other.chars) + // Constant-time comparison to prevent timing attacks + if (chars.size != other.chars.size) return false + var result = 0 + for (i in chars.indices) { + result = result or (chars[i].code xor other.chars[i].code) + } + return result == 0 }🤖 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 31 - 35, The equals implementation in SensitiveString currently uses chars.contentEquals which may short-circuit and leak timing info; replace it with a constant-time comparison: after keeping the identity check (this === other) and the type check (other !is SensitiveString), compare lengths and then iterate every index of chars, accumulating a difference (e.g., via XOR of character codes) without early returns, and finally return true only if the accumulated difference indicates equality; operate on the SensitiveString.chars field and ensure you do not expose or allocate additional sensitive copies during the check.core-base/security/src/commonMain/kotlin/template/core/base/security/SensitiveString.kt-40-41 (1)
40-41: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument that the input
Stringparameter cannot be cleaned.The
fromStringmethod accepts aStringparameter that will remain in heap memory and cannot be zeroed. Callers creatingSensitiveStringfrom existingStringliterals or variables should be aware that the originalStringnegates some of the security benefits.📝 Proposed documentation
companion object { + /** + * Create a [SensitiveString] from an existing [String]. + * + * **Security limitation**: The input [value] parameter is an immutable + * [String] that cannot be zeroed and will persist in memory. When possible, + * construct [SensitiveString] directly from user input (e.g., char arrays + * from password fields) to avoid intermediate String allocation. + */ fun fromString(value: String): SensitiveString = SensitiveString(value.toCharArray()) }🤖 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 40 - 41, Update the KDoc for the factory method SensitiveString.fromString(value: String) to explicitly state that the String parameter cannot be zeroed/cleaned (it stays in JVM heap and retains sensitive data), warn callers that creating a SensitiveString from an existing String literal or variable weakens security, and recommend using the SensitiveString(CharArray) constructor or passing freshly-constructed char arrays to avoid leaving secrets in immutable Strings; include the method name SensitiveString.fromString and the alternative SensitiveString(CharArray) in the doc for discoverability.core-base/security/src/androidMain/kotlin/template/core/base/security/BuildInfo.kt-15-22 (1)
15-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Debug.isDebuggerConnected()is not a reliable build-type signal.Line 22 can misclassify both debug and release runtime states, which can incorrectly relax or tighten security policy defaults. Prefer compile-time build-type flags for this function.
However, the proposed fix requires an additional gradle configuration step. BuildConfig.DEBUG will only be available after adding
buildFeatures { buildConfig = true }to theandroidblock inbuild.gradle.kts:🔧 Required changes
Update
core-base/security/build.gradle.kts:android { namespace = "template.core.base.security" + buildFeatures { + buildConfig = true + } defaultConfig { consumerProguardFiles("consumer-rules.pro") } }Then update
BuildInfo.kt:-import android.os.Debug +import template.core.base.security.BuildConfig -actual fun isReleaseBuild(): Boolean = !Debug.isDebuggerConnected() +actual fun isReleaseBuild(): Boolean = !BuildConfig.DEBUG🤖 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` around lines 15 - 22, isReleaseBuild() currently uses Debug.isDebuggerConnected(), which is unreliable; change it to use the compile-time BuildConfig.DEBUG flag by enabling buildConfig generation in the module's android block (add buildFeatures { buildConfig = true } to the module's build.gradle.kts), update BuildInfo.kt to return !BuildConfig.DEBUG and add the appropriate BuildConfig import, and ensure the module is rebuilt so BuildConfig is generated for the isReleaseBuild() implementation.core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureRandom.kt-23-25 (1)
23-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd guards for zero-length requests in native SecureRandom implementation.
Kotlin/Native's
addressOf(0)is invalid for zero-lengthByteArraydue to bounds checking inaddressOfElement(), and will throw an exception ifnextBytes(0)is called. While current call sites use constant sizes (16, 32), the function should defensively guard against this edge case to prevent unexpected failures.🔧 Proposed fix
actual class SecureRandom { actual fun nextBytes(size: Int): ByteArray { + require(size >= 0) { "size must be >= 0" } + if (size == 0) return ByteArray(0) val bytes = ByteArray(size) bytes.usePinned { pinned -> val status = SecRandomCopyBytes(kSecRandomDefault, size.toULong(), pinned.addressOf(0)) if (status != errSecSuccess) { error("SecRandomCopyBytes failed: $status") } } 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/nativeMain/kotlin/template/core/base/security/SecureRandom.kt` around lines 23 - 25, The native SecureRandom implementation should guard against zero-length requests: in the function that fills the ByteArray (e.g., nextBytes / the method that allocates `bytes` and calls `bytes.usePinned { ... }` and SecRandomCopyBytes), add an early-return when size == 0 to avoid calling `pinned.addressOf(0)` (which is invalid for empty arrays) and skip the SecRandomCopyBytes call; keep the existing error check for non-zero sizes and ensure the function returns successfully for zero-length input.core-base/security/src/androidMain/kotlin/template/core/base/security/SecureWiper.kt-17-22 (1)
17-22:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
wipeSecureStorage()is a silent no-op despite its security contract.The
expectKDoc states this should "Wipe all secure preferences and encryption keys", andFailedAttemptTrackerinvokes it once the wipe threshold is exceeded. The Androidactualonly emits a warning log and leaves all secure state intact, so on Android the wipe-on-failure / logout-wipe defense is non-functional. Callers receive no signal that the wipe did not happen.A no-op default is risky here — consider one of:
- Make the wipe pluggable via DI (e.g., inject a
WipeStrategy/SecureStorageCleanerinterface that consumer apps must provide) and haveSecureWiperdelegate to it, defaulting to throwingNotImplementedErroror refusing to construct without a strategy registered.- Or, at minimum, expose a clearly-named
Noop/Stubvariant and require Koin to bind it explicitly so the no-op is opt-in rather than the silent default.🛡️ Sketch: pluggable wipe delegate
-actual class SecureWiper actual constructor() { - - 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. - } +interface SecureStorageCleaner { fun clear() } + +actual class SecureWiper( + private val cleaner: SecureStorageCleaner, +) { + + actual fun wipeSecureStorage() { + Logger.w("SecureWiper") { "Secure storage wipe triggered" } + cleaner.clear() + }(Equivalent expect-class signature change — or use a property setter/Koin override — would be 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/androidMain/kotlin/template/core/base/security/SecureWiper.kt` around lines 17 - 22, The Android actual SecureWiper.wipeSecureStorage implementation is a silent no-op; replace it with a pluggable strategy: define an interface (e.g., SecureStorageCleaner) and have SecureWiper delegate to a provided implementation (injection via constructor/Koin or a setter) so consumer apps must supply the real cleaner; make the default behavior explicit—either throw NotImplementedError if no implementation is bound or require an explicit NoopSecureStorageCleaner binding so the no-op is opt-in; update usages such as FailedAttemptTracker to rely on SecureWiper.wipeSecureStorage as before but ensure DI bind-time failure or explicit Noop makes the behavior obvious.core-base/security/src/desktopMain/kotlin/template/core/base/security/SecureKeyProvider.kt-26-43 (1)
26-43:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMaster AES key persisted in plaintext with default file permissions.
field_key.binis the raw 32-byte AES key used byFieldEncryptor, written verbatim to~/.mifos-secure/with whatever permissions the user's umask happens to grant (on most Linux setups:0644/0664; on macOS:0644). Any process running as the same user — backup agents, cloud-sync (Dropbox/iCloud Desktop), other apps, malware — can read it and decrypt every "encrypted" field on disk. This effectively negatesFieldEncryptoron Desktop.The KDoc acknowledges Phase 4 (T18) will move this to OS credential APIs — that's the correct end state. Until then, please at minimum:
- Create the file with owner-only permissions on POSIX (
PosixFilePermissions.asFileAttribute(setOf(OWNER_READ, OWNER_WRITE))) and the parent dir as0700.- Document the Windows gap (DPAPI is the right answer; without it, the file is readable by any process in the user session).
- Consider wrapping the on-disk bytes with a passphrase-derived KEK (Argon2id/PBKDF2 → AES-GCM-wrap) so a stolen file alone isn't enough.
🔒 Minimum hardening: restricted POSIX permissions
-import java.io.File -import java.security.SecureRandom as JSecureRandom -import javax.crypto.Cipher -import javax.crypto.spec.GCMParameterSpec -import javax.crypto.spec.SecretKeySpec +import java.io.File +import java.nio.file.Files +import java.nio.file.attribute.PosixFilePermission +import java.nio.file.attribute.PosixFilePermissions +import java.security.SecureRandom as JSecureRandom @@ - actual fun generateKey(): ByteArray { - val key = ByteArray(32) - JSecureRandom().nextBytes(key) - keyFile.parentFile?.mkdirs() - keyFile.writeBytes(key) - return key - } + actual fun generateKey(): ByteArray { + val key = ByteArray(32) + JSecureRandom().nextBytes(key) + val parent = keyFile.parentFile + parent?.mkdirs() + // Best-effort permission hardening on POSIX; no-op on Windows. + runCatching { + Files.setPosixFilePermissions( + parent!!.toPath(), + setOf(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_EXECUTE), + ) + } + keyFile.writeBytes(key) + runCatching { + Files.setPosixFilePermissions( + keyFile.toPath(), + setOf(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE), + ) + } + 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 26 - 43, The AES master key is being written plaintext with default permissions; update SecureKeyProvider so the parent directory created for keyFile (the File constructed as File(System.getProperty("user.home"), ".mifos-secure")) is created with POSIX 0700 and the key file itself is created with POSIX owner-only read/write (use PosixFilePermissions.asFileAttribute with OWNER_READ and OWNER_WRITE) when generateKey() writes the bytes, and ensure getKey() will still read the file; also add a short KDoc comment in the class noting the Windows gap (DPAPI should be used on Windows) and consider a follow-up to wrap the on-disk key with a passphrase-derived KEK (Argon2id/PBKDF2 → AES-GCM-wrap) as an enhancement.core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityPolicy.kt-16-25 (1)
16-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
SecurityPolicyinvariants at construction.Line 18–21 accept zero/negative thresholds and no relation check between lock and wipe attempts. This can produce unsafe or inconsistent runtime behavior.
Suggested guardrails
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 > 0" } + require(wipeAfterFailedAttempts >= lockAfterFailedAttempts) { + "wipeAfterFailedAttempts must be >= lockAfterFailedAttempts" + } + require(sessionTimeoutMinutes > 0) { "sessionTimeoutMinutes must be > 0" } + require(clipboardWipeSeconds > 0) { "clipboardWipeSeconds must be > 0" } + } + 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 - 25, Add runtime validation of SecurityPolicy invariants in the data class by adding an init block that checks each threshold and relation: ensure lockAfterFailedAttempts > 0, wipeAfterFailedAttempts > lockAfterFailedAttempts, sessionTimeoutMinutes > 0, and clipboardWipeSeconds >= 0 (and any other domain-specific bounds); if any check fails, throw an IllegalArgumentException with a clear message. Update the companion object/default factory (SecurityPolicy.default) if needed to return a valid instance and keep validation centralized in SecurityPolicy's init so all construction sites (including default()) are guarded.core-base/security/src/jsCommonMain/kotlin/template/core/base/security/FieldEncryptor.kt-38-57 (1)
38-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on JS/Wasm instead of silently treating plaintext as “encrypted.”
Line 49 and Line 55 return unencrypted data. Because this class satisfies the shared
FieldEncryptorcontract, callers can mistakenly assume confidentiality is enforced.Proposed minimal mitigation
actual fun encrypt(data: ByteArray): ByteArray { - // NO-OP: returns unmodified copy. SubtleCrypto is async-only on web. - return data.copyOf() + throw UnsupportedOperationException( + "FieldEncryptor is not implemented for JS/WasmJS yet (NO-OP disabled)." + ) } actual fun decrypt(data: ByteArray): ByteArray { - // NO-OP: returns unmodified copy. - return data.copyOf() + throw UnsupportedOperationException( + "FieldEncryptor is not implemented for JS/WasmJS yet (NO-OP disabled)." + ) }🤖 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/FieldEncryptor.kt` around lines 38 - 57, The JS/Wasm implementation of FieldEncryptor currently returns plaintext unchanged in encrypt(data: ByteArray)/decrypt(data: ByteArray) (and the String overloads encrypt(plaintext: String)/decrypt(ciphertext: String)), which silently fails closed; change these implementations to fail fast by throwing a clear exception (e.g., UnsupportedOperationException or IllegalStateException with a message like "Field encryption not supported on JS/Wasm") from encrypt(data: ByteArray) and decrypt(data: ByteArray) and ensure the String overloads (encrypt(plaintext: String), decrypt(ciphertext: String)) propagate that exception rather than returning unmodified data so callers cannot assume confidentiality is enforced.core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityConfig.kt-24-29 (1)
24-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd constructor validation for security thresholds.
Line 26–28 currently allow invalid values (e.g.,
0or negative), which can unintentionally disable controls.Suggested fix
data class SecurityConfig( val isReleaseBuild: Boolean = isReleaseBuild(), val maxFailedAttempts: Int = 10, val sessionTimeoutMinutes: Int = 30, val clipboardWipeSeconds: Int = 60, -) +) { + init { + require(maxFailedAttempts > 0) { "maxFailedAttempts must be > 0" } + require(sessionTimeoutMinutes > 0) { "sessionTimeoutMinutes must be > 0" } + require(clipboardWipeSeconds > 0) { "clipboardWipeSeconds must be > 0" } + } +}🤖 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/SecurityConfig.kt` around lines 24 - 29, SecurityConfig allows zero or negative thresholds which can disable protections; add validation in the SecurityConfig data class by adding an init block that uses Kotlin require(...) checks on maxFailedAttempts, sessionTimeoutMinutes, and clipboardWipeSeconds to enforce positive values (e.g., require(maxFailedAttempts > 0) { "maxFailedAttempts must be > 0" } and similarly for sessionTimeoutMinutes and clipboardWipeSeconds) so invalid constructor arguments throw IllegalArgumentException with clear messages.core-base/security/src/commonMain/kotlin/template/core/base/security/SessionManager.kt-36-37 (1)
36-37:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift@volatile provides insufficient concurrency guarantees for multiplatform.
The
@kotlin.concurrent.Volatileannotation doesn't provide atomicity or memory ordering guarantees needed for safe concurrent access across all Kotlin Multiplatform targets (especially JS/WasmJS). This creates race conditions whentouch()andcheckTimeout()accesslastActivityTimeconcurrently.Use
kotlinx.atomicfu.atomicfor cross-platform atomic operations, or document thatSessionManagerrequires external synchronization.🔒 Recommended fix using atomicfu
+import kotlinx.atomicfu.atomic + class SessionManager( private val policy: SecurityPolicy, private val onSessionExpired: () -> Unit = {}, private val clock: () -> Long = { currentTimeMillis() }, ) { private val _isSessionActive = MutableStateFlow(false) val isSessionActive: StateFlow<Boolean> = _isSessionActive.asStateFlow() - `@kotlin.concurrent.Volatile` - private var lastActivityTime: Long = clock() + private val lastActivityTime = atomic(clock()) fun startSession() { - lastActivityTime = clock() + lastActivityTime.value = clock() _isSessionActive.value = true Logger.d("SessionManager") { "Session started" } } fun touch() { if (_isSessionActive.value) { - lastActivityTime = clock() + lastActivityTime.value = clock() } } fun checkTimeout(): Boolean { if (!_isSessionActive.value) return false - val elapsed = clock() - lastActivityTime + val elapsed = clock() - lastActivityTime.value val timeoutMs = policy.sessionTimeoutMinutes * 60 * 1_000L return if (elapsed >= timeoutMs) {🤖 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/SessionManager.kt` around lines 36 - 37, The field lastActivityTime in SessionManager is annotated with `@kotlin.concurrent.Volatile` which is insufficient for multiplatform atomicity; replace the volatile Long with a kotlinx.atomicfu.atomic Long and update touch() and checkTimeout() to read/update lastActivityTime via atomicfu operations (get/set or compareAndSet as needed) so reads/writes are atomic across targets, or alternatively document/require external synchronization if you choose not to use atomicfu; ensure you import and initialize the atomic using atomic(clock()) and use its value/getValue/setValue APIs in touch() and checkTimeout().core-base/security/src/commonTest/kotlin/template/core/base/security/SessionManagerTest.kt-16-53 (1)
16-53: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTest coverage misses critical timeout and callback behavior.
The test suite validates basic session state transitions but omits:
- Actual timeout behavior — no test verifies that
checkTimeout()returnstruewhen time advances past the timeout thresholdtouch()behavior — no test confirms that touching resets the inactivity timeronSessionExpiredcallback — no test verifies the callback is invoked when timeout occursThese gaps leave core functionality untested.
🧪 Suggested additional tests
+ `@Test` + fun checkTimeoutReturnsTrueWhenExpired() { + var callbackInvoked = false + var currentTime = 0L + val manager = SessionManager( + policy = SecurityPolicy(sessionTimeoutMinutes = 30), + onSessionExpired = { callbackInvoked = true }, + clock = { currentTime } + ) + manager.startSession() + currentTime += 31 * 60 * 1_000L // Advance 31 minutes + assertTrue(manager.checkTimeout()) + assertTrue(callbackInvoked) + assertFalse(manager.isSessionActive.value) + } + + `@Test` + fun touchResetsInactivityTimer() { + var currentTime = 0L + val manager = SessionManager( + policy = SecurityPolicy(sessionTimeoutMinutes = 30), + clock = { currentTime } + ) + manager.startSession() + currentTime += 20 * 60 * 1_000L // Advance 20 minutes + manager.touch() // Reset timer + currentTime += 20 * 60 * 1_000L // Advance another 20 minutes (40 total, 20 since touch) + assertFalse(manager.checkTimeout()) // Should not timeout + }🤖 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/commonTest/kotlin/template/core/base/security/SessionManagerTest.kt` around lines 16 - 53, Add tests in SessionManagerTest to cover timeout and callback behavior: create a SessionManager(policy) with a small SecurityPolicy.sessionTimeoutMinutes, startSession(), then simulate advancing time past the timeout (or manipulate the manager's last-activity/clock) and assert checkTimeout() returns true and triggers the onSessionExpired callback; also add a test that calls touch() after startSession(), advances time less than the timeout and asserts checkTimeout() stays false (proving touch resets inactivity), and add a test that registers a mock/spy on onSessionExpired and asserts it was invoked when timeout elapses (and not invoked when endSession() is called before timeout). Ensure tests reference SessionManager, checkTimeout(), startSession(), touch(), endSession(), and the onSessionExpired callback so they fail/compile if APIs change.core-base/security/src/jsCommonMain/kotlin/template/core/base/security/SecureWiper.kt-16-19 (1)
16-19: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd TODO tracking for incomplete JS storage wipe implementation.
The JS
wipeSecureStorage()implementation is incomplete—it only logs a warning and comments about clearing localStorage/sessionStorage without executing any actual clearing. While this pattern is consistent across multiple platforms (Android and Native also have stub implementations with comments), there is no TODO comment or issue tracking to ensure completion. Add a TODO to track this work.Platforms with incomplete wipeSecureStorage implementations
- jsCommonMain: Only logs and comments (no implementation)
- androidMain: Only logs and comments (no implementation)
- nativeMain: Only logs and comments (no implementation)
- desktopMain: Has actual implementation (clears
.mifos-securedirectory)🤖 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 JS implementation of SecureWiper.wipeSecureStorage is currently a no-op that only logs; add a clear TODO comment (including an issue/track id placeholder or "TODO: implement secure wipe (create issue)") above the Logger.w line indicating this is incomplete and must clear browser storages (localStorage/sessionStorage) and link to work to be done; apply the same TODO pattern to the other stub implementations (androidMain/nativeMain) so each function (wipeSecureStorage in SecureWiper) has a visible TODO with tracking for future implementation.core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureWiper.kt-21-25 (1)
21-25:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
wipeSecureStorage()currently doesn't wipe anything.This method sits on the security wipe path, but the native implementation only logs. If
FailedAttemptTrackerreaches its wipe threshold, Keychain/UserDefaults secrets still survive the event. Either delete the module-owned entries here or make the contract explicit so callers cannot assume a wipe already happened.🤖 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 native implementation of wipeSecureStorage() only logs and does not actually remove stored secrets; update SecureWiper.wipeSecureStorage to delete module-owned Keychain entries (service "org.mifos.secure") and clear UserDefaults (or platform equivalents) so a wipe actually occurs when FailedAttemptTracker reaches its threshold, or alternatively change the function contract and name to make callers responsible for deletion; locate the function SecureWiper.wipeSecureStorage and implement platform-specific deletion of Keychain items and UserDefaults keys associated with this module.core-base/security/src/commonMain/kotlin/template/core/base/security/TamperDetector.kt-20-29 (1)
20-29: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftModel unsupported tamper checks explicitly instead of forcing booleans.
The current contract gives platform
actuals no way to say "not implemented", so several targets are forced to return safe-looking placeholders likefalseortrue. That will make future policy enforcement trust unsupported platforms by default, especially forisSignatureValid().🧭 Example shape for the shared API
sealed interface TamperCheckResult { data object Detected : TamperCheckResult data object NotDetected : TamperCheckResult data object Unsupported : TamperCheckResult }Then expose
TamperCheckResultfrom each check instead ofBoolean.🤖 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/TamperDetector.kt` around lines 20 - 29, The current expect class TamperDetector exposes Boolean-returning methods (isDeviceCompromised, isDebuggerAttached, isSignatureValid) which forces platform implementations to return safe placeholders; change the shared contract to return an explicit tri-state sealed type (e.g., TamperCheckResult with Detected, NotDetected, Unsupported) instead of Boolean so actuals can report Unsupported; update the expect declaration for TamperDetector to use TamperCheckResult for each check and add the sealed interface/class TamperCheckResult (or equivalent) to the commonMain API so platform implementations can return Detected/NotDetected/Unsupported.core-base/security/src/androidMain/kotlin/template/core/base/security/TamperDetector.kt-26-29 (1)
26-29:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
isSignatureValid()is a no-op that always returnstrue.This silently asserts the app is unmodified on every platform call site (notably
SecurityGate's startup tamper check), which is a security regression compared to having no signature check at all — UI/policy code consuming this signal will get a false sense of integrity. Either:
- gate this behind
SecurityConfigso it's an explicit opt-in (returningfalse/throwing until configured), or- accept an expected signature hash from
SecurityConfigand compare againstPackageManager's reported signing certificates (PackageManager.GET_SIGNING_CERTIFICATESon API 28+).Returning a hard-coded
truedefeats the purpose of having the API in the contract.🤖 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 isSignatureValid() implementation currently always returns true; change it to require explicit configuration via SecurityConfig or perform a real signature check: update TamperDetector.isSignatureValid() to read an expected signature hash (or a configured opt-in flag) from SecurityConfig and if absent return false (or throw) so callers like SecurityGate don't get a false-positive; when configured, fetch the app signing certs via Android PackageManager (use PackageManager.GET_SIGNING_CERTIFICATES on API 28+) and compare the runtime signing certificate hash against the expected hash(s) before returning true.core-base/security/src/androidMain/kotlin/template/core/base/security/TamperDetector.kt-43-48 (1)
43-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProcess leak in
canExecSuand weak detection signal.
Runtime.getRuntime().exec("su")returns aProcessthat owns native pipes/file descriptors; on devices wheresuexists (the case you actually care about), the process is started and never destroyed, leaking FDs and possibly leaving a hung child. Additionally, exec succeeding only proves the binary is invocable from PATH — it does not prove the device is rooted (the binary could deny the request). Combined with the file-path checks above, this is largely redundant.🔒 Suggested fix: destroy the process and rely on path/tag indicators
- val canExecSu = try { - Runtime.getRuntime().exec("su") - true - } catch (_: Exception) { - false - } - return hasRootFiles || hasTestKeys || canExecSu + val canExecSu = try { + val process = Runtime.getRuntime().exec(arrayOf("which", "su")) + try { + process.waitFor() + process.exitValue() == 0 + } finally { + process.destroy() + } + } catch (_: Exception) { + false + } + return hasRootFiles || hasTestKeys || canExecSu🤖 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 43 - 48, The canExecSu block in TamperDetector currently calls Runtime.getRuntime().exec("su") without destroying or closing the returned Process (leaking FDs) and gives a weak/root signal; replace this by either removing the exec-based check entirely and relying on the existing file-path/tag detections, or if you keep it, immediately capture the Process from Runtime.getRuntime().exec("su"), close its Input/Output/Error streams, call process.destroy() and waitFor with a short timeout, and use the process exit value only as a very weak indicator; update the canExecSu usage in the TamperDetector logic so it no longer leaks resources and does not overweigh this check.core-base/security/src/commonMain/kotlin/template/core/base/security/SecurityGate.kt-105-113 (1)
105-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSession timeout must respond to keyboard, navigation, and accessibility interactions on non-touch platforms.
The session is refreshed only on
awaitFirstDown(...)in the pointer input handler. On desktop, web, and native targets that this PR adds support for, keyboard-driven navigation, screen reader actions, and accessibility shortcuts never callsessionManager.touch(), causing active sessions to expire while the user is still interacting.Feed session activity from key, focus, and navigation events, or move the timeout refresh to the lifecycle manager that already monitors app resume.
🤖 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 105 - 113, The pointer-only session refresh in Box using modifier.pointerInput with awaitEachGesture/awaitFirstDown only catches touch; update the session refresh so non-touch interactions also call sessionManager.checkTimeout() and sessionManager.touch(): either add handlers to feed activity from keyboard, focus and navigation events (e.g., onKeyEvent/onFocusChanged/Navigator callbacks) or move the touch/timeout refresh into the existing lifecycle manager that already monitors app resume; change references around Box modifier.pointerInput/awaitEachGesture/awaitFirstDown so those only handle pointer input and ensure the lifecycle manager or input handlers invoke sessionManager.touch() and sessionManager.checkTimeout() for non-pointer interactions.core-base/security/src/commonMain/kotlin/template/core/base/security/FailedAttemptTracker.kt-27-45 (1)
27-45:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGuard
failedCountmutations with atomic operations or a mutex.
recordFailure()performs a read-modify-write onfailedCountwithfailedCount++.@Volatileensures visibility but does not guarantee atomicity for composite operations. If failures are recorded concurrently, increments can be lost and thresholds can be missed, weakening the lockout and wipe policy enforcement. Additionally, the threshold evaluation is not atomic relative to the increment, allowing race conditions in action selection. Replace withAtomicIntegeror guard all mutations and threshold checks with aMutexor synchronized block.🤖 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/FailedAttemptTracker.kt` around lines 27 - 45, The failedCount field is mutated with non-atomic read-modify-write (failedCount++) in recordFailure(), causing race conditions; replace failedCount with an AtomicInteger or protect all accesses (reads and writes used by currentFailedCount, isLockedOut and recordFailure()) with a Mutex/synchronized block so increments and threshold checks against policy.lockAfterFailedAttempts and policy.wipeAfterFailedAttempts are atomic; ensure recordFailure() performs the increment and then atomically evaluates thresholds before calling secureWiper.wipeSecureStorage() and resetting the counter, and make currentFailedCount/isLockedOut read atomically from the same AtomicInteger or under the same lock.
🟡 Minor comments (4)
core-base/security/src/desktopMain/kotlin/template/core/base/security/SecureKeyProvider.kt-14-16 (1)
14-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDrop unused crypto imports.
Cipher,GCMParameterSpec, andSecretKeySpecaren't referenced anywhere in this file (the desktop provider only generates and reads raw key bytes — encryption lives inFieldEncryptor.kt). They look like leftovers from an earlier draft.♻️ Proposed fix
import java.io.File import java.security.SecureRandom as JSecureRandom -import javax.crypto.Cipher -import javax.crypto.spec.GCMParameterSpec -import javax.crypto.spec.SecretKeySpec🤖 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 14 - 16, Remove the three unused crypto imports from SecureKeyProvider.kt: drop javax.crypto.Cipher, javax.crypto.spec.GCMParameterSpec, and javax.crypto.spec.SecretKeySpec since the desktop provider only handles raw key bytes and encryption is in FieldEncryptor.kt; update the import block for the SecureKeyProvider class to no longer reference these symbols to eliminate dead imports and linter warnings.core-base/security/src/desktopMain/kotlin/template/core/base/security/SecureWiper.kt-17-29 (1)
17-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten
wipeSecureStorage(): integer narrowing, non-recursive walk, and overstated guarantees.A few related concerns in this loop:
length.toInt()narrowing (Line 25).File.length()isLong; for any file ≥ 2 GiB this silently wraps to a negativeInt, andByteArray(negative)throwsNegativeArraySizeException— aborting the rest of the wipe. Realistically secure-state files are tiny, but a one-line guard (length.coerceAtMost(Int.MAX_VALUE.toLong()).toInt()) or chunked overwrite is cheap insurance.- Non-recursive (Line 22).
listFiles().forEachskips anything inside subdirectories under~/.mifos-secure. If the layout ever grows a nested folder (cache, per-account scope) it'll be missed.deleteRecursively()after overwriting all leaves is safer.- Overwrite-then-delete is largely theatrical on modern storage. Without
FileChannel.force(true)/fsync, journaling and copy-on-write filesystems may never physically overwrite the original sectors, and on SSDs wear-leveling makes in-place overwrite meaningless regardless. Either drop the overwrite step (keep just the delete and let the FS / TRIM handle it) or document the limitation in theexpectKDoc so callers don't infer a stronger guarantee than this provides.♻️ Sketch
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() - } - } + if (!secureDir.exists()) return + secureDir.walkBottomUp().filter { it.isFile }.forEach { file -> + val length = file.length().coerceAtMost(Int.MAX_VALUE.toLong()).toInt() + runCatching { file.writeBytes(ByteArray(length)) } + file.delete() + } + secureDir.deleteRecursively() }🤖 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 17 - 29, The wipeSecureStorage() implementation must (1) guard against Integer narrowing when converting File.length() to Int by using length.coerceAtMost(Int.MAX_VALUE.toLong()).toInt() or implement chunked overwrites that operate on Long-sized lengths, (2) recurse into subdirectories so all files under the secureDir are processed (e.g., walkTopDown()/forEach { if (it.isFile) ... } and then call deleteRecursively() on secureDir), and (3) either remove the overwriting step entirely and just delete files or add a clear KDoc to the expect declaration for wipeSecureStorage() explaining that this routine does not guarantee physical sector erasure on modern filesystems/SSDs (mentioning lack of fsync/FileChannel.force and COW/wear-leveling), so callers are not misled; locate changes in the wipeSecureStorage function and references to secureDir/File.length() to implement these fixes.core-base/security/src/commonMain/kotlin/template/core/base/security/SessionManager.kt-60-61 (1)
60-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRace condition in timeout calculation.
Lines 60-61 read
lastActivityTimeand callclock()separately, creating a time-of-check to time-of-use (TOCTOU) race. Iftouch()updateslastActivityTimebetween these two reads, the elapsed time calculation becomes inconsistent.Even with atomic variables, you need to snapshot both values atomically or accept minor imprecision. Document this behavior or use synchronized access.
🤖 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/SessionManager.kt` around lines 60 - 61, The elapsed/time-out calculation in SessionManager (the lines computing elapsed = clock() - lastActivityTime and timeoutMs = policy.sessionTimeoutMinutes * 60 * 1_000L) can produce a TOCTOU race if touch() updates lastActivityTime between reads; fix by taking an atomic snapshot of both values (e.g. acquire the same lock/Mutex used by touch() or read clock() into a local val now and then read lastActivityTime within the same synchronized block) so elapsed is computed from consistent values, or alternatively document the accepted minor imprecision in the SessionManager API if you choose not to synchronize; reference lastActivityTime, clock(), touch(), elapsed and timeoutMs when making the change.core-base/security/src/desktopMain/kotlin/template/core/base/security/FieldEncryptor.kt-36-38 (1)
36-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject malformed ciphertext before slicing the IV.
decrypt(String)accepts arbitrary Base64, butdecrypt(ByteArray)assumes the payload already contains a full IV and tag. Short or truncated input will fail incopyOfRange(...)instead of surfacing a controlled decryption error. Please add an upfront length check here (at leastGCM_IV_LENGTH + 16for GCM) and throw a clear security/validation exception.Also applies to: 50-54
🤖 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/FieldEncryptor.kt` around lines 36 - 38, The decrypt(String) path must validate the decoded payload length before slicing the IV/tag: in decrypt(ciphertext: String) decode to bytes then assert the byte array length is at least GCM_IV_LENGTH + 16 (IV + minimum tag) and throw a clear validation/security exception (e.g., IllegalArgumentException or a dedicated SecurityException) if too short; likewise add the same length check at the start of decrypt(payload: ByteArray) so copyOfRange(...) cannot throw for truncated input and the caller receives a descriptive error instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 362e5e1f-0867-436f-b055-cae1b3d14b7b
📒 Files selected for processing (64)
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.ktgradle/libs.versions.tomlsettings.gradle.kts
| actual fun encrypt(data: ByteArray): ByteArray { | ||
| val key = keyProvider.getKey() ?: keyProvider.generateKey() | ||
| val cipher = Cipher.getInstance(AES_GCM) | ||
| cipher.init(Cipher.ENCRYPT_MODE, SecretKeySpec(key, "AES")) | ||
| val iv = cipher.iv | ||
| val encrypted = cipher.doFinal(data) | ||
| return iv + encrypted | ||
| } | ||
|
|
||
| actual fun decrypt(data: ByteArray): ByteArray { | ||
| val key = keyProvider.getKey() | ||
| ?: throw SecurityException("Encryption key not found — data unrecoverable") | ||
| val iv = data.copyOfRange(0, GCM_IV_LENGTH) | ||
| val ciphertext = data.copyOfRange(GCM_IV_LENGTH, data.size) | ||
| val cipher = Cipher.getInstance(AES_GCM) | ||
| cipher.init( | ||
| Cipher.DECRYPT_MODE, | ||
| SecretKeySpec(key, "AES"), | ||
| GCMParameterSpec(GCM_TAG_LENGTH, iv), | ||
| ) | ||
| return cipher.doFinal(ciphertext) | ||
| } |
There was a problem hiding this comment.
Downstream of the SecureKeyProvider extraction issue.
The keyProvider.getKey() / keyProvider.generateKey() calls here will hit null/SecurityException paths on real devices because AndroidKeystore symmetric keys are non-extractable (see the comment on SecureKeyProvider.kt). Once SecureKeyProvider is restructured to expose a SecretKey (or a Cipher factory) rather than raw bytes, this class should consume that API directly — Cipher.init(ENCRYPT_MODE, keyStoreSecretKey) works without ever needing the raw material, and the IV-prefixing logic here can stay as-is.
🤖 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 33 - 54, FieldEncryptor.encrypt/decrypt currently pull raw key
bytes via keyProvider.getKey()/generateKey() which fails for non-extractable
AndroidKeystore keys; change the KeyProvider API usage so it returns a
javax.crypto.SecretKey (or a Cipher factory) and update encrypt and decrypt to
call cipher.init(Cipher.ENCRYPT_MODE, secretKey) /
cipher.init(Cipher.DECRYPT_MODE, secretKey, GCMParameterSpec(...)) instead of
constructing SecretKeySpec from bytes; keep the IV prefixing logic (iv +
encrypted) and the GCM constants (AES_GCM, GCM_IV_LENGTH, GCM_TAG_LENGTH) intact
and throw the same SecurityException when no SecretKey is available.
| actual fun getKey(): ByteArray? { | ||
| val keyStore = KeyStore.getInstance(ANDROID_KEYSTORE).apply { load(null) } | ||
| val entry = keyStore.getEntry(KEYSTORE_ALIAS, null) as? KeyStore.SecretKeyEntry | ||
| ?: return null | ||
| return entry.secretKey.encoded | ||
| } | ||
|
|
||
| actual fun generateKey(): ByteArray { | ||
| val keyGenerator = KeyGenerator.getInstance( | ||
| KeyProperties.KEY_ALGORITHM_AES, | ||
| ANDROID_KEYSTORE, | ||
| ) | ||
| keyGenerator.init( | ||
| KeyGenParameterSpec.Builder( | ||
| KEYSTORE_ALIAS, | ||
| KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT, | ||
| ) | ||
| .setBlockModes(KeyProperties.BLOCK_MODE_GCM) | ||
| .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) | ||
| .setKeySize(256) | ||
| .build(), | ||
| ) | ||
| val secretKey: SecretKey = keyGenerator.generateKey() | ||
| return secretKey.encoded ?: getKey() | ||
| ?: throw SecurityException("Failed to generate encryption key") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Android Keystore SecretKey getEncoded null AES KeyGenParameterSpec
💡 Result:
In Android Keystore, calling getEncoded on a keystore-backed AES SecretKey commonly returns null (i.e., the key bytes are not exportable). This is “by design” for keys that are meant to stay inside secure hardware. What the docs say - SecretKey/Key#getEncoded returns “the key in its primary encoding format, or null if this key does not support encoding.” [1][2] - Android Keystore is designed so key material may not be exportable; when a key is backed by Android Keystore, libraries that assume exportable key bytes can break because getEncoded returns null. [3][4] Why you see null with KeyGenParameterSpec + AES - When you generate an AES key using KeyGenerator with the “AndroidKeyStore” provider and a KeyGenParameterSpec, you later retrieve it via KeyStore#getKey. The key is usable for Cipher operations, but it is not necessarily exportable. (The generation/retrieval pattern is shown in the official KeyGenParameterSpec docs.) [5] - For keys that cannot be exported, getEncoded returns null, and downstream code may crash if it blindly constructs parameters from the missing bytes. This exact failure mode is described in an Android issue report noting that “AndroidKeyStoreKey.getEncoded, which returns null” and then a library tries to use that null array. [3] What to do instead - Don’t call getEncoded on keystore-backed SecretKey if your goal is to persist/copy the raw AES key bytes; instead, keep using the key reference for crypto operations (Cipher.init with the SecretKey from KeyStore). [5] - If you truly need the raw AES key bytes (e.g., to interoperate with an API that requires byte[]), you must use a different key-management approach: generate the key outside Android Keystore (e.g., SecretKeySpec / in-memory key generation) or use a scheme where you export by wrapping (e.g., wrap/import encrypted keys). Android documents secure importing/export mechanisms for encrypted keys so the plaintext key material doesn’t have to appear in host memory. [6] Bottom line - “Android Keystore secret key getEncoded null” is expected for keystore-backed AES SecretKeys; it returns null when the key does not support encoding/export. [1][2] - AES key generation via KeyGenParameterSpec does not guarantee key material exportability; you should use the returned SecretKey for Cipher, not its encoded bytes. [5]
Citations:
- 1: https://developer.android.com/reference/java/security/Key
- 2: https://developer.android.com/reference/kotlin/javax/crypto/SecretKey
- 3: https://issuetracker.google.com/issues/229764028
- 4: https://stackoverflow.com/questions/42348944/calling-getencoded-on-secretkey-returns-null
- 5: https://developer.android.com/reference/android/security/keystore/KeyGenParameterSpec
- 6: https://developer.android.com/privacy-and-security/keystore
🏁 Script executed:
# First, let's locate the SecureKeyProvider file and inspect it
fd -t f "SecureKeyProvider.kt" --exec wc -l {} \;Repository: openMF/mifos-mobile
Length of output: 543
🏁 Script executed:
# Find FieldEncryptor to understand how it uses the ByteArray
fd -t f "FieldEncryptor"Repository: openMF/mifos-mobile
Length of output: 502
🏁 Script executed:
# Look for the expect declaration in common code
fd -t f "SecureKeyProvider" | grep -E "(commonMain|common)"Repository: openMF/mifos-mobile
Length of output: 153
🏁 Script executed:
cat -n core-base/security/src/commonMain/kotlin/template/core/base/security/SecureKeyProvider.ktRepository: openMF/mifos-mobile
Length of output: 1265
🏁 Script executed:
cat -n core-base/security/src/androidMain/kotlin/template/core/base/security/SecureKeyProvider.ktRepository: openMF/mifos-mobile
Length of output: 2445
🏁 Script executed:
cat -n core-base/security/src/androidMain/kotlin/template/core/base/security/FieldEncryptor.ktRepository: openMF/mifos-mobile
Length of output: 2402
🏁 Script executed:
cat -n core-base/security/src/commonMain/kotlin/template/core/base/security/FieldEncryptor.ktRepository: openMF/mifos-mobile
Length of output: 1447
🏁 Script executed:
# Check other platform implementations to see if they return ByteArray
cat -n core-base/security/src/desktopMain/kotlin/template/core/base/security/SecureKeyProvider.ktRepository: openMF/mifos-mobile
Length of output: 1777
🏁 Script executed:
cat -n core-base/security/src/nativeMain/kotlin/template/core/base/security/SecureKeyProvider.ktRepository: openMF/mifos-mobile
Length of output: 4408
Critical: AndroidKeystore AES keys are non-extractable — secretKey.encoded returns null.
The Android Keystore is explicitly designed to keep symmetric key material outside the application process. SecretKey.getEncoded() returns null for keys generated/stored under AndroidKeyStore. This causes immediate failures:
getKey()(line 27) will returnnullfor any key generated by this class.generateKey()(line 46) succeeds in producing a key, then immediately falls through togetKey() ?: throw SecurityException(...), so it throws"Failed to generate encryption key"on the very first call.- Downstream,
FieldEncryptor.encrypt()at line 34 callskeyProvider.getKey() ?: keyProvider.generateKey(), expecting a non-nullByteArray. When this fails, theSecretKeySpecconstructor at lines 36 and 50 crashes.
The fix requires an API-shape change across the common expect declaration and all platform actuals. Instead of returning ByteArray, expose a non-extractable SecretKey or a pre-configured Cipher, and have FieldEncryptor initialize Cipher with that handle directly. Desktop and iOS implementations (which manage extractable keys) will need similar refactoring. This is high-effort but necessary for the module to function on Android.
// Sketch of revised API (needs application across all expect/actual pairs and FieldEncryptor):
// commonMain expect
expect class SecureKeyProvider {
fun getOrCreateEncryptionCipher(): Cipher // ENCRYPT_MODE, IV auto-generated
fun getDecryptionCipher(iv: ByteArray): Cipher // DECRYPT_MODE with caller-supplied IV
fun deleteKey()
}
// androidMain actual (sketch)
override fun getOrCreateEncryptionCipher(): Cipher {
val ks = KeyStore.getInstance(ANDROID_KEYSTORE).apply { load(null) }
val key = (ks.getEntry(KEYSTORE_ALIAS, null) as? KeyStore.SecretKeyEntry)?.secretKey
?: run {
KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, ANDROID_KEYSTORE).apply {
init(
KeyGenParameterSpec.Builder(
KEYSTORE_ALIAS,
KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT,
)
.setBlockModes(KeyProperties.BLOCK_MODE_GCM)
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE)
.setKeySize(256)
.build(),
)
}.generateKey()
}
return Cipher.getInstance("AES/GCM/NoPadding").apply { init(Cipher.ENCRYPT_MODE, 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/androidMain/kotlin/template/core/base/security/SecureKeyProvider.kt`
around lines 23 - 48, The Android implementation uses SecretKey.getEncoded()
which returns null for AndroidKeyStore keys (see SecureKeyProvider.getKey() and
generateKey()), causing generateKey() to fail and FieldEncryptor.encrypt() to
crash; change the API shape: modify the common expect SecureKeyProvider and all
platform actuals to expose non-extractable cryptographic handles (e.g., return a
configured Cipher or a SecretKey handle) instead of ByteArray, and update
FieldEncryptor.encrypt() to call getOrCreateEncryptionCipher() (or equivalent)
and use the returned Cipher for init/ENCRYPT_MODE and getDecryptionCipher(iv)
for DECRYPT_MODE; update SecureKeyProvider.generateKey()/getKey()
implementations to create or load the AndroidKeyStore SecretKey and return a
Cipher/SecretKey handle rather than calling SecretKey.getEncoded().
| val hostValid = if (schemeValid && allowedHosts.isNotEmpty()) { | ||
| val hostPart = uri.substringAfter("://", "").substringBefore("/").substringBefore("?") | ||
| val host = hostPart.substringBefore(":").lowercase() | ||
| val allowed = host in allowedHosts | ||
| if (!allowed) Logger.w("DeepLinkValidator") { "Rejected host: $host" } | ||
| allowed |
There was a problem hiding this comment.
Host allowlist can be bypassed via userinfo syntax.
Line 39–41 can be bypassed by URIs like https://api.mifos.org:443@evil.com/path: current parsing treats api.mifos.org as host, but the real host is evil.com.
Suggested hardening
- val hostPart = uri.substringAfter("://", "").substringBefore("/").substringBefore("?")
- val host = hostPart.substringBefore(":").lowercase()
+ val authority = uri.substringAfter("://", "").substringBefore("/").substringBefore("?")
+ if (authority.isBlank() || authority.contains("@")) {
+ Logger.w("DeepLinkValidator") { "Rejected malformed authority" }
+ return false
+ }
+ val host = authority.substringBefore(":").lowercase()
+ if (host.isBlank()) {
+ Logger.w("DeepLinkValidator") { "Rejected empty host" }
+ return false
+ }
val allowed = host in allowedHosts🤖 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 - 43, The host extraction in DeepLinkValidator (hostPart → host)
is vulnerable to userinfo (e.g. user:pass@host) because it treats the portion
before '@' as the host; fix by stripping any userinfo before deriving host: when
computing hostPart in the hostValid block, first call hostPart =
hostPart.substringAfterLast("@") to remove any userinfo, then proceed with
substringBefore(":").lowercase() and the existing allowed-host check (refer to
hostPart, host, hostValid and DeepLinkValidator).
| expect class FieldEncryptor { | ||
| /** | ||
| * Encrypts plaintext to a Base64-encoded ciphertext string. | ||
| * The returned value is prefixed with "ENC:" for format detection. | ||
| */ | ||
| fun encrypt(plaintext: String): String | ||
|
|
||
| /** Decrypts a Base64-encoded ciphertext string back to plaintext. */ | ||
| fun decrypt(ciphertext: String): String | ||
|
|
||
| /** Encrypts raw bytes. */ | ||
| fun encrypt(data: ByteArray): ByteArray | ||
|
|
||
| /** Decrypts raw bytes. */ | ||
| fun decrypt(data: ByteArray): ByteArray | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at each FieldEncryptor actual and check for the "ENC:" literal
fd -i 'FieldEncryptor.kt' core-base/security/src --exec sh -c 'echo "=== {} ==="; cat {}'Repository: openMF/mifos-mobile
Length of output: 12149
Add "ENC:" prefix to all four actual implementations of FieldEncryptor.encrypt(String).
The expect class documents that encrypt(String) returns Base64 ciphertext prefixed with "ENC:" for format detection. However, none of the four actual implementations (jsCommon, desktop, android, native) currently prepend this prefix or validate/strip it on decrypt. This breaks the documented contract and silently prevents format detection and cross-platform portability. Update all four encrypt implementations to prepend "ENC:" to the Base64 output, and update all four decrypt implementations to strip and validate the prefix before decoding.
🤖 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/FieldEncryptor.kt`
around lines 19 - 34, The expect API promises that
FieldEncryptor.encrypt(plaintext: String) returns a Base64 ciphertext prefixed
with "ENC:" and decrypt(ciphertext: String) accepts that form; update each
actual implementation for jsCommon, desktop, android, and native so that
encrypt(String) prepends "ENC:" to the Base64-encoded result and decrypt(String)
validates and strips the "ENC:" prefix (throwing/handling an error if the prefix
is missing) before Base64-decoding; keep the existing ByteArray encrypt/decrypt
behavior unchanged and reuse the same encode/decode helpers used by the concrete
classes where possible.
| @Suppress("MagicNumber") | ||
| actual class SecureRandom { | ||
| actual fun nextBytes(size: Int): ByteArray { | ||
| return kotlin.random.Random.nextBytes(size) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n core-base/security/src/jsCommonMain/kotlin/template/core/base/security/SecureRandom.ktRepository: openMF/mifos-mobile
Length of output: 1157
🏁 Script executed:
find . -name "SecureKeyProvider.kt" -type fRepository: openMF/mifos-mobile
Length of output: 527
🏁 Script executed:
find . -name "SecureRandom.kt" -type fRepository: openMF/mifos-mobile
Length of output: 502
🏁 Script executed:
rg "class SecureKeyProvider" -A 10Repository: openMF/mifos-mobile
Length of output: 6771
🏁 Script executed:
rg "SecureRandom()" -B 2 -A 2Repository: openMF/mifos-mobile
Length of output: 12804
Remove non-CSPRNG key and IV generation on JS/Wasm by throwing until WebCrypto is available.
SecureKeyProvider.generateKey() and FieldEncryptor.encrypt() both call SecureRandom().nextBytes() on JS/Wasm, which currently uses kotlin.random.Random—a PRNG, not a CSPRNG. This generates predictable encryption keys and IVs. The file's KDoc already documents this weakness. Replace with a fail-closed exception (as suggested below) until WebCrypto integration is complete.
Fail-closed fix
actual class SecureRandom {
actual fun nextBytes(size: Int): ByteArray {
- return kotlin.random.Random.nextBytes(size)
+ throw UnsupportedOperationException(
+ "SecureRandom is not available on JS/Wasm until WebCrypto getRandomValues is implemented."
+ )
}
}📝 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.
| @Suppress("MagicNumber") | |
| actual class SecureRandom { | |
| actual fun nextBytes(size: Int): ByteArray { | |
| return kotlin.random.Random.nextBytes(size) | |
| } | |
| `@Suppress`("MagicNumber") | |
| actual class SecureRandom { | |
| actual fun nextBytes(size: Int): ByteArray { | |
| throw UnsupportedOperationException( | |
| "SecureRandom is not available on JS/Wasm until WebCrypto getRandomValues is implemented." | |
| ) | |
| } |
🤖 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/SecureRandom.kt`
around lines 21 - 25, The current SecureRandom.nextBytes(size) on JS/Wasm
returns kotlin.random.Random bytes (non-CSPRNG); replace this behavior with a
fail-closed implementation that throws a clear exception until a proper
WebCrypto-based CSPRNG is implemented: modify the actual class SecureRandom and
its nextBytes method to throw an IllegalStateException (or platform-appropriate
exception) indicating "secure random not available on JS/Wasm; WebCrypto
required", so callers like SecureKeyProvider.generateKey and
FieldEncryptor.encrypt will fail fast rather than produce insecure keys/IVs.
| actual fun encrypt(data: ByteArray): ByteArray { | ||
| val key = keyProvider.getKey() ?: keyProvider.generateKey() | ||
| val iv = SecureRandom().nextBytes(CBC_IV_LENGTH) | ||
| val encrypted = ccCrypt(kCCEncrypt, key, iv, data) | ||
| return iv + encrypted | ||
| } | ||
|
|
||
| actual fun decrypt(data: ByteArray): ByteArray { | ||
| val key = keyProvider.getKey() | ||
| ?: error("Encryption key not found — data unrecoverable") | ||
| val iv = data.copyOfRange(0, CBC_IV_LENGTH) | ||
| val ciphertext = data.copyOfRange(CBC_IV_LENGTH, data.size) | ||
| return ccCrypt(kCCDecrypt, key, iv, ciphertext) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all FieldEncryptor implementations
find . -name "FieldEncryptor.kt" -type fRepository: openMF/mifos-mobile
Length of output: 512
🏁 Script executed:
# Search for the desktop implementation to compare cipher modes
rg -l "FieldEncryptor" --type kotlinRepository: openMF/mifos-mobile
Length of output: 976
🏁 Script executed:
# Locate and examine the ccCrypt function definition
rg "fun ccCrypt" -A 10 --type kotlinRepository: openMF/mifos-mobile
Length of output: 1387
🏁 Script executed:
# Find CBC_IV_LENGTH and related constants
rg "CBC_IV_LENGTH" -B 2 -A 2 --type kotlinRepository: openMF/mifos-mobile
Length of output: 2200
🏁 Script executed:
# Search for AES-GCM usage in the codebase
rg "GCM|AES" --type kotlinRepository: openMF/mifos-mobile
Length of output: 4336
🏁 Script executed:
# Get full ccCrypt implementation in native
sed -n '77,125p' ./core-base/security/src/nativeMain/kotlin/template/core/base/security/FieldEncryptor.ktRepository: openMF/mifos-mobile
Length of output: 1544
🏁 Script executed:
# Also check the common interface to see what it promises
cat ./core-base/security/src/commonMain/kotlin/template/core/base/security/FieldEncryptor.ktRepository: openMF/mifos-mobile
Length of output: 1209
Use authenticated AES-GCM encryption across all platforms.
The native implementation uses non-authenticated AES-CBC with PKCS7 padding, while the desktop and Android implementations use authenticated AES-GCM. This violates the common interface contract (which specifies "AES-256-GCM field encryption") and creates two critical problems:
- Incompatible payloads: Native uses a 16-byte IV (CBC block size) while desktop/Android use 12-byte IVs (GCM standard), preventing cross-platform data interchange.
- Loss of integrity protection: CBC mode has no authentication tag, so the native implementation cannot detect tampered ciphertext—a capability that desktop and Android provide via AEAD.
Migrate the native cipher to AES-GCM to match the other platforms and restore cryptographic integrity guarantees.
🤖 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/FieldEncryptor.kt`
around lines 63 - 75, The native FieldEncryptor implementation currently uses
CBC; update encrypt(data: ByteArray) and decrypt(data: ByteArray) to AES-256-GCM
to match the interface: obtain a 32-byte key from
keyProvider.getKey()/generateKey(), generate a 12-byte IV (GCM standard) via
SecureRandom, perform AES-GCM encryption to produce ciphertext and a 16-byte
auth tag, and return a single payload formatted as IV (12) || ciphertext || tag
(16). In decrypt, parse the incoming data into iv = first 12 bytes, tag = last
16 bytes, ciphertext = middle bytes, then run AES-GCM decryption/verification
using those values and the same key; replace usages of CBC_IV_LENGTH and ccCrypt
with GCM-specific routines and constants (e.g., GCM_IV_LENGTH = 12,
GCM_TAG_LENGTH = 16) and ensure you surface an error when authentication fails.
| androidxSecurityCrypto = "1.1.0-alpha06" | ||
| bouncycastle = "1.78.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest stable version of androidx.security:security-crypto, and is 1.1.0-alpha06 recommended for production use?
💡 Result:
Latest stable version of androidx.security:security-crypto: 1.1.0. [1] Is 1.1.0-alpha06 recommended for production use? No. 1.1.0-alpha06 is an alpha (pre-release) version, while the production (stable) release is 1.1.0; Android’s Security release page lists 1.1.0 as the stable release and only 1.1.0-alpha06 under Alpha Release. [1] Additionally, Google’s “Cryptography | Security” page states that all APIs in the security-crypto Jetpack library were deprecated in the stable release of version 1.1.0, and that there won’t be any subsequent releases of this library—so you should prefer other recommended alternatives rather than relying on an alpha. [2]
Citations:
- 1: https://developer.android.com/jetpack/androidx/releases/security
- 2: https://developer.android.com/privacy-and-security/cryptography
Remove deprecated androidx.security:security-crypto dependency; Google has deprecated all APIs in 1.1.0 and recommends alternatives.
Lines 21 and 138 pin androidx.security:security-crypto to 1.1.0-alpha06. Using an alpha version in a security module is problematic, but the core issue is more severe: Google's official documentation states that all APIs in this library were deprecated in the stable 1.1.0 release, with no further releases planned. Using a deprecated security library on a security-critical path introduces maintenance and behavior risk. Migrate to Google's recommended alternatives for your use case.
🤖 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` around lines 21 - 22, The pinned dependency
androidxSecurityCrypto = "1.1.0-alpha06" is deprecated and must be removed;
delete the androidxSecurityCrypto entry from libs.versions.toml (and any
corresponding alias usage) and migrate code paths that depend on security-crypto
to one of Google’s recommended alternatives (e.g., Google Tink or direct Android
Keystore/EncryptedSharedPreferences/EncryptedFile patterns) — search for usages
of the androidxSecurityCrypto alias, update build.gradle/Kotlin code to use the
chosen replacement library, and adjust any initialization/APIs to the
replacement's APIs.
… request/response payloads
1 participant
Hi @therajanmaurya sir, ive made some improvements to the repo based on the requirements for security remediation project
Summary
This PR introduces the foundational security module needed for MM-573 Cipher remediation work in
mifos-mobile.What was added
core-base/securitySecureKeyProvider,FieldEncryptor,SecureRandom,SecureWiperTamperDetector,SessionManager,SecureAuthManagerSecurityConfig,SecurityPolicy,SecurityGate,SecurityStateDeepLinkValidator,SecureNavHandler,CertificatePinConfigSecurityModule.android.ktSecurityModule.desktop.ktSecurityModule.native.ktSecurityModuleforjsCommonMaincore-base/security/src/commonTestIntegration updates
settings.gradle.kts:include(":core-base:security")gradle/libs.versions.toml:androidx-security-cryptobouncycastlecmp-navigation/build.gradle.ktscmp-navigation/.../KoinModules.kt(includesSecurityModule)Why this change
This creates a reusable and consistent security baseline across platforms so we can implement and verify MM-573 findings in a structured way, instead of scattered ad-hoc fixes.
Scope note
This PR is infrastructure/foundation only.
It does not close all Cipher findings yet. Follow-up PRs will map and remediate each official finding (CRITICAL -> HIGH/MEDIUM -> LOW), including production pinning and policy-level enforcement.
Test plan
:core-base:securitycore-base/securitySecurityModuleSummary by CodeRabbit