Fixes for autofill service (multi-process) #8087
Open
CrisBarreiro wants to merge 4 commits intodevelopfrom
Open
Fixes for autofill service (multi-process) #8087CrisBarreiro wants to merge 4 commits intodevelopfrom
CrisBarreiro wants to merge 4 commits intodevelopfrom
Conversation
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
35 tasks
CrisBarreiro
force-pushed
the
feature/cris/add-multi-process-guard
branch
from
aca3b4b to
d500c69
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates secure storage key handling to better support the Autofill Service running in multi-process mode, ensuring keys are read/written from a multi-process-safe store and handling cross-process write races more gracefully.
Changes:
- Treat “autofill service enabled” as implicitly enabling Harmony + read-from-Harmony, and skip legacy EncryptedSharedPreferences reads/writes in multi-process mode.
- Introduce
SecureStorageException.KeyAlreadyExistsExceptionfor the write-once key overwrite guard and update call sites/tests accordingly. - Make
SecureStorageKeyProviderresilient to cross-process key creation races by retrying reads after a guarded write fails.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt |
Adds multi-process-aware Harmony flag snapshot and bypasses legacy store in multi-process mode; adjusts overwrite-guard exception type. |
autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageKeyProvider.kt |
Handles concurrent key/password/salt/L2-key initialization across processes by catching “already exists” and reading the stored values. |
autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/di/SecureStorageModule.kt |
Wires AutofillServiceFeature into RealSecureStorageKeyStore construction. |
autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageException.kt |
Adds KeyAlreadyExistsException to represent guarded “write-once” overwrite attempts. |
autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/store/keys/RealSecureStorageKeyStoreTest.kt |
Updates expected exception type and adds multi-process mode coverage (Harmony-only reads/writes, null Harmony prefs behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt
Outdated
Show resolved
Hide resolved
...ll-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageKeyProvider.kt
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Non-atomic L2 key writes cause mismatched recovery data
- Added detection in encryptAndStoreL2Key to throw InternalSecureStorageException when IV write fails but L2 key was already written by this process, preventing the caller's recovery path from reading mismatched L2 key/IV pairs.
Or push these changes by commenting:
@cursor push 3182242796
Preview (3182242796)
diff --git a/PixelDefinitions/pixels/definitions/autofill.json5 b/PixelDefinitions/pixels/definitions/autofill.json5
--- a/PixelDefinitions/pixels/definitions/autofill.json5
+++ b/PixelDefinitions/pixels/definitions/autofill.json5
@@ -215,6 +215,72 @@
}
]
},
+ "autofill_preferences_get_key_decode_failed": {
+ "description": "Legacy preferences returned a non-null value for a key but it could not be base64-decoded (daily unique)",
+ "owners": ["CrisBarreiro"],
+ "triggers": ["exception"],
+ "suffixes": ["form_factor"],
+ "parameters": [
+ "appVersion",
+ {
+ "key": "key",
+ "description": "The name of the key whose value failed to decode from legacy",
+ "type": "string",
+ "enum": ["KEY_GENERATED_PASSWORD", "KEY_L1KEY", "KEY_PASSWORD_SALT", "KEY_ENCRYPTED_L2KEY", "KEY_ENCRYPTED_L2KEY_IV"]
+ },
+ {
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
+ "type": "string",
+ "enum": ["true", "false"]
+ },
+ {
+ "key": "initialHarmonyValue",
+ "description": "The value of useHarmony flag when harmony preferences were first initialized",
+ "type": "string",
+ "enum": ["true", "false", "null"]
+ },
+ {
+ "key": "readFromHarmony",
+ "description": "Whether the readFromHarmony feature flag is enabled",
+ "type": "string",
+ "enum": ["true", "false"]
+ }
+ ]
+ },
+ "autofill_harmony_preferences_get_key_decode_failed": {
+ "description": "Harmony preferences returned a non-null value for a key but it could not be base64-decoded (daily unique)",
+ "owners": ["CrisBarreiro"],
+ "triggers": ["exception"],
+ "suffixes": ["form_factor"],
+ "parameters": [
+ "appVersion",
+ {
+ "key": "key",
+ "description": "The name of the key whose value failed to decode from Harmony",
+ "type": "string",
+ "enum": ["KEY_GENERATED_PASSWORD", "KEY_L1KEY", "KEY_PASSWORD_SALT", "KEY_ENCRYPTED_L2KEY", "KEY_ENCRYPTED_L2KEY_IV"]
+ },
+ {
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
+ "type": "string",
+ "enum": ["true", "false"]
+ },
+ {
+ "key": "initialHarmonyValue",
+ "description": "The value of useHarmony flag when harmony preferences were first initialized",
+ "type": "string",
+ "enum": ["true", "false", "null"]
+ },
+ {
+ "key": "readFromHarmony",
+ "description": "Whether the readFromHarmony feature flag is enabled",
+ "type": "string",
+ "enum": ["true", "false"]
+ }
+ ]
+ },
"autofill_harmony_key_mismatch": {
"description": "Harmony and legacy preferences both have non-null values for the same key but they differ (daily unique)",
"owners": ["CrisBarreiro"],
diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/pixel/AutofillPixelNames.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/pixel/AutofillPixelNames.kt
--- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/pixel/AutofillPixelNames.kt
+++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/pixel/AutofillPixelNames.kt
@@ -241,6 +241,8 @@
AUTOFILL_PREFERENCES_GET_KEY_NULL_FILE("autofill_preferences_get_key_null_file"),
AUTOFILL_HARMONY_PREFERENCES_GET_KEY_NULL_FILE("autofill_harmony_preferences_get_key_null_file"),
AUTOFILL_HARMONY_UPDATE_KEY_ROLLBACK_FAILED("autofill_harmony_update_key_rollback_failed"),
+ AUTOFILL_PREFERENCES_GET_KEY_DECODE_FAILED("autofill_preferences_get_key_decode_failed"),
+ AUTOFILL_HARMONY_PREFERENCES_GET_KEY_DECODE_FAILED("autofill_harmony_preferences_get_key_decode_failed"),
}
object AutofillPixelParameters {
diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageException.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageException.kt
--- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageException.kt
+++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageException.kt
@@ -28,4 +28,5 @@
override val message: String,
override val cause: Throwable? = null,
) : SecureStorageException()
+ data class KeyAlreadyExistsException(override val message: String) : SecureStorageException()
}
diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageKeyProvider.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageKeyProvider.kt
--- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageKeyProvider.kt
+++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageKeyProvider.kt
@@ -61,8 +61,13 @@
l1KeyMutex.withLock {
// If no key exists in the keystore, we generate a new one and store it
return if (secureStorageKeyRepository.getL1Key() == null) {
- randomBytesGenerator.generateBytes(L1_PASSPHRASE_SIZE).also {
- secureStorageKeyRepository.setL1Key(it)
+ val newKey = randomBytesGenerator.generateBytes(L1_PASSPHRASE_SIZE)
+ try {
+ secureStorageKeyRepository.setL1Key(newKey)
+ newKey
+ } catch (e: SecureStorageException.KeyAlreadyExistsException) {
+ // Another process wrote the key between our read and write — use theirs
+ secureStorageKeyRepository.getL1Key() ?: throw e
}
} else {
secureStorageKeyRepository.getL1Key()!!
@@ -73,8 +78,13 @@
override suspend fun getl2Key(): Key {
l2KeyMutex.withLock {
val userPassword = if (secureStorageKeyRepository.getPassword() == null) {
- randomBytesGenerator.generateBytes(PASSWORD_SIZE).also {
- secureStorageKeyRepository.setPassword(it)
+ val newPassword = randomBytesGenerator.generateBytes(PASSWORD_SIZE)
+ try {
+ secureStorageKeyRepository.setPassword(newPassword)
+ newPassword
+ } catch (e: SecureStorageException.KeyAlreadyExistsException) {
+ // Another process wrote the password between our read and write — use theirs
+ secureStorageKeyRepository.getPassword() ?: throw e
}
} else {
secureStorageKeyRepository.getPassword()
@@ -86,8 +96,19 @@
private suspend fun getl2Key(password: String): Key {
val keyMaterial = if (secureStorageKeyRepository.getEncryptedL2Key() == null) {
- secureStorageKeyGenerator.generateKey().encoded.also {
- encryptAndStoreL2Key(it, password)
+ val keyBytes = secureStorageKeyGenerator.generateKey().encoded
+ try {
+ encryptAndStoreL2Key(keyBytes, password)
+ keyBytes
+ } catch (e: SecureStorageException.KeyAlreadyExistsException) {
+ // Another process wrote the L2 key between our read and write — decrypt and use theirs
+ encryptionHelper.decrypt(
+ EncryptedBytes(
+ secureStorageKeyRepository.getEncryptedL2Key() ?: throw e,
+ secureStorageKeyRepository.getEncryptedL2KeyIV() ?: throw e,
+ ),
+ deriveKeyFromPassword(password),
+ )
}
} else {
encryptionHelper.decrypt(
@@ -104,21 +125,39 @@
private suspend fun encryptAndStoreL2Key(
keyBytes: ByteArray,
password: String,
- ): ByteArray =
- encryptionHelper.encrypt(
+ ): ByteArray {
+ val encrypted = encryptionHelper.encrypt(
keyBytes,
deriveKeyFromPassword(password),
- ).also {
- secureStorageKeyRepository.setEncryptedL2Key(it.data)
- secureStorageKeyRepository.setEncryptedL2KeyIV(it.iv)
- }.data
+ )
+ secureStorageKeyRepository.setEncryptedL2Key(encrypted.data)
+ try {
+ secureStorageKeyRepository.setEncryptedL2KeyIV(encrypted.iv)
+ } catch (e: SecureStorageException.KeyAlreadyExistsException) {
+ val storedL2Key = secureStorageKeyRepository.getEncryptedL2Key()
+ if (storedL2Key != null && storedL2Key.contentEquals(encrypted.data)) {
+ throw SecureStorageException.InternalSecureStorageException(
+ "L2 key written but IV already exists from another process — inconsistent state",
+ e,
+ )
+ }
+ throw e
+ }
+ return encrypted.data
+ }
- private suspend fun getPasswordSalt() = if (secureStorageKeyRepository.getPasswordSalt() == null) {
- randomBytesGenerator.generateBytes(PASSWORD_KEY_SALT_SIZE).also {
- secureStorageKeyRepository.setPasswordSalt(it)
+ private suspend fun getPasswordSalt(): ByteArray {
+ if (secureStorageKeyRepository.getPasswordSalt() != null) {
+ return secureStorageKeyRepository.getPasswordSalt()!!
}
- } else {
- secureStorageKeyRepository.getPasswordSalt()!!
+ val newSalt = randomBytesGenerator.generateBytes(PASSWORD_KEY_SALT_SIZE)
+ return try {
+ secureStorageKeyRepository.setPasswordSalt(newSalt)
+ newSalt
+ } catch (e: SecureStorageException.KeyAlreadyExistsException) {
+ // Another process wrote the salt between our read and write — use theirs
+ secureStorageKeyRepository.getPasswordSalt() ?: throw e
+ }
}
private suspend fun deriveKeyFromPassword(password: String) =
diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/di/SecureStorageModule.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/di/SecureStorageModule.kt
--- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/di/SecureStorageModule.kt
+++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/di/SecureStorageModule.kt
@@ -21,6 +21,7 @@
import com.duckduckgo.autofill.api.AutofillFeature
import com.duckduckgo.autofill.impl.securestorage.DerivedKeySecretFactory
import com.duckduckgo.autofill.impl.securestorage.RealDerivedKeySecretFactory
+import com.duckduckgo.autofill.impl.service.AutofillServiceFeature
import com.duckduckgo.autofill.store.RealSecureStorageKeyRepository
import com.duckduckgo.autofill.store.SecureStorageKeyRepository
import com.duckduckgo.autofill.store.keys.EncryptedPreferencesFactory
@@ -45,6 +46,7 @@
@AppCoroutineScope coroutineScope: CoroutineScope,
dispatcherProvider: DispatcherProvider,
autofillFeature: AutofillFeature,
+ autofillServiceFeature: AutofillServiceFeature,
sharedPreferencesProvider: SharedPreferencesProvider,
pixel: Pixel,
encryptedPreferencesFactory: EncryptedPreferencesFactory,
@@ -54,6 +56,7 @@
coroutineScope,
dispatcherProvider,
autofillFeature,
+ autofillServiceFeature,
sharedPreferencesProvider,
pixel,
encryptedPreferencesFactory,
diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt
--- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt
+++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt
@@ -16,6 +16,7 @@
package com.duckduckgo.autofill.store.keys
+import android.annotation.SuppressLint
import android.content.SharedPreferences
import androidx.core.content.edit
import com.duckduckgo.app.statistics.pixels.Pixel
@@ -24,11 +25,13 @@
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_HARMONY_KEY_MISMATCH
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_HARMONY_KEY_MISSING
+import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_HARMONY_PREFERENCES_GET_KEY_DECODE_FAILED
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_HARMONY_PREFERENCES_GET_KEY_FAILED
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_HARMONY_PREFERENCES_GET_KEY_NULL_FILE
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_HARMONY_PREFERENCES_RETRIEVAL_FAILED
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_HARMONY_PREFERENCES_UPDATE_KEY_FAILED
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_HARMONY_PREFERENCES_UPDATE_KEY_NULL_FILE
+import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_PREFERENCES_GET_KEY_DECODE_FAILED
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_PREFERENCES_GET_KEY_FAILED
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_PREFERENCES_GET_KEY_NULL_FILE
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_PREFERENCES_KEY_MISSING
@@ -36,6 +39,7 @@
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_PREFERENCES_UPDATE_KEY_NULL_FILE
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_STORE_KEY_ALREADY_EXISTS
import com.duckduckgo.autofill.impl.securestorage.SecureStorageException
+import com.duckduckgo.autofill.impl.service.AutofillServiceFeature
import com.duckduckgo.common.utils.DispatcherProvider
import com.duckduckgo.common.utils.sanitizeStackTrace
import com.duckduckgo.data.store.api.SharedPreferencesProvider
@@ -71,10 +75,12 @@
suspend fun canUseEncryption(): Boolean
}
+private const val TAG = "RealSecureStorageKeyStore"
class RealSecureStorageKeyStore(
private val coroutineScope: CoroutineScope,
private val dispatcherProvider: DispatcherProvider,
private val autofillFeature: AutofillFeature,
+ private val autofillServiceFeature: AutofillServiceFeature,
private val sharedPreferencesProvider: SharedPreferencesProvider,
private val pixel: Pixel,
private val encryptedPreferencesFactory: EncryptedPreferencesFactory,
@@ -85,12 +91,32 @@
private var initialUseHarmonyValue: Boolean? = null
- private fun useHarmony(): Boolean = autofillFeature.useHarmony().isEnabled()
+ /**
+ * Evaluates all three harmony-related flags in a single consistent snapshot.
+ * [multiProcess] (autofill service enabled) implies both [useHarmony] and [readFromHarmony]:
+ * Harmony is the only multi-process-safe store, so it is treated as implicitly active regardless
+ * of the remote-config flag state. All three values are derived from one [isMultiProcessMode]
+ * call, guaranteeing they can never be mutually inconsistent.
+ */
+ private data class HarmonyFlags(
+ val useHarmony: Boolean,
+ val readFromHarmony: Boolean,
+ val multiProcess: Boolean,
+ )
- private fun readFromHarmony(): Boolean = autofillFeature.useHarmony().isEnabled() && autofillFeature.readFromHarmony().isEnabled()
+ private fun harmonyFlags(): HarmonyFlags {
+ val autofillServiceFlag = autofillServiceFeature.self().isEnabled()
+ val useHarmonyFlag = autofillFeature.useHarmony().isEnabled()
+ return HarmonyFlags(
+ useHarmony = useHarmonyFlag || autofillServiceFlag,
+ readFromHarmony = (useHarmonyFlag && autofillFeature.readFromHarmony().isEnabled()) || autofillServiceFlag,
+ multiProcess = autofillServiceFlag,
+ )
+ }
private val encryptedPreferencesDeferred: Deferred<SharedPreferences?> by lazy {
coroutineScope.async(dispatcherProvider.io()) {
+ val harmonyFlags = harmonyFlags()
try {
mutex.withLock {
encryptedPreferencesFactory.create(FILENAME)
@@ -99,7 +125,7 @@
coroutineContext.ensureActive()
pixel.fire(
AutofillPixelNames.AUTOFILL_PREFERENCES_RETRIEVAL_FAILED,
- getPixelParams(throwable = e),
+ getPixelParams(throwable = e, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
type = Daily(),
)
null
@@ -109,22 +135,24 @@
private val harmonyPreferencesDeferred: Deferred<SharedPreferences?> by lazy {
coroutineScope.async(dispatcherProvider.io()) {
+ val harmonyFlags = harmonyFlags()
try {
harmonyMutex.withLock {
- useHarmony().let { useHarmony ->
- initialUseHarmonyValue = useHarmony
- if (useHarmony) {
- getEncryptedPreferences()?.let { legacyPreferences ->
- sharedPreferencesProvider.getMigratedEncryptedSharedPreferences(legacyPreferences, FILENAME_V2).also {
- if (it == null) {
- logcat { "autofill harmony preferences retrieval returned null" }
- }
+ initialUseHarmonyValue = harmonyFlags.useHarmony
+ if (!harmonyFlags.useHarmony) {
+ null
+ } else {
+ // Migration reads from legacy (safe in multi-process) and writes to harmony.
+ // If both processes race to migrate, they read identical data and write
+ // identical keys — idempotent and safe under Harmony's multi-process locking.
+ getEncryptedPreferences()?.let { legacyPreferences ->
+ sharedPreferencesProvider.getMigratedEncryptedSharedPreferences(legacyPreferences, FILENAME_V3).also {
+ if (it == null) {
+ logcat { "autofill harmony preferences retrieval returned null" }
}
- } ?: run {
- logcat { "autofill legacy preferences retrieval returned null. Harmony will also return null" }
- null
}
- } else {
+ } ?: run {
+ logcat { "autofill legacy preferences retrieval returned null. Harmony will also return null" }
null
}
}
@@ -133,7 +161,7 @@
coroutineContext.ensureActive()
pixel.fire(
AUTOFILL_HARMONY_PREFERENCES_RETRIEVAL_FAILED,
- getPixelParams(throwable = e),
+ getPixelParams(throwable = e, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
type = Daily(),
)
logcat { "autofill harmony preferences retrieval failed: $e" }
@@ -150,30 +178,85 @@
return harmonyPreferencesDeferred.await()
}
+ @SuppressLint("UseKtx")
override suspend fun updateKey(
keyName: String,
keyValue: ByteArray?,
) {
withContext(dispatcherProvider.io()) {
- val legacyPrefs = getEncryptedPreferences().also {
- if (it == null) {
- pixel.fire(
- AUTOFILL_PREFERENCES_UPDATE_KEY_NULL_FILE,
- getPixelParams(keyName = keyName),
- type = Daily(),
- )
- throw SecureStorageException.InternalSecureStorageException("Legacy Preferences file is null on write")
+ val harmonyFlags = harmonyFlags()
+
+ // In multi-process mode, skip legacy entirely — EncryptedSharedPreferences writes do not
+ // propagate between processes, so writing from two processes would produce divergent state.
+ val legacyPrefs: SharedPreferences? = if (harmonyFlags.multiProcess) {
+ null
+ } else {
+ getEncryptedPreferences().also {
+ if (it == null) {
+ pixel.fire(
+ AUTOFILL_PREFERENCES_UPDATE_KEY_NULL_FILE,
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
+ type = Daily(),
+ )
+ throw SecureStorageException.InternalSecureStorageException("Legacy Preferences file is null on write")
+ }
}
}
- val harmonyPrefs = if (!useHarmony()) {
+ fun onLegacyWriteFailure(message: String, cause: Throwable? = null): Nothing {
+ pixel.fire(
+ AUTOFILL_PREFERENCES_UPDATE_KEY_FAILED,
+ getPixelParams(
+ keyName = keyName,
+ throwable = cause,
+ useHarmony = harmonyFlags.useHarmony,
+ readFromHarmony = harmonyFlags.readFromHarmony,
+ ),
+ type = Daily(),
+ )
+ throw SecureStorageException.InternalSecureStorageException(message, cause)
+ }
+
+ fun onHarmonyWriteFailure(message: String, cause: Throwable? = null): Nothing {
+ pixel.fire(
+ AUTOFILL_HARMONY_PREFERENCES_UPDATE_KEY_FAILED,
+ getPixelParams(
+ keyName = keyName,
+ throwable = cause,
+ useHarmony = harmonyFlags.useHarmony,
+ readFromHarmony = harmonyFlags.readFromHarmony,
+ ),
+ type = Daily(),
+ )
+ // Rollback legacy write so we don't cause a corrupted state with out of sync files.
+ // In multi-process mode legacy was never written to, so no rollback is needed.
+ if (!harmonyFlags.multiProcess && keyValue != null) {
+ runCatching {
+ legacyPrefs?.edit(commit = true) { remove(keyName) }
+ }.onFailure { rollbackError ->
+ pixel.fire(
+ AutofillPixelNames.AUTOFILL_HARMONY_UPDATE_KEY_ROLLBACK_FAILED,
+ getPixelParams(
+ keyName = keyName,
+ throwable = rollbackError,
+ useHarmony = harmonyFlags.useHarmony,
+ readFromHarmony = harmonyFlags.readFromHarmony,
+ ),
+ type = Daily(),
+ )
+ }
+ }
+ throw SecureStorageException.InternalSecureStorageException(message, cause)
+ }
+
+ val harmonyPrefs = if (!harmonyFlags.useHarmony) {
null
} else {
getHarmonyEncryptedPreferences().also {
if (it == null) {
pixel.fire(
AUTOFILL_HARMONY_PREFERENCES_UPDATE_KEY_NULL_FILE,
- getPixelParams(keyName = keyName),
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
type = Daily(),
)
throw SecureStorageException.InternalSecureStorageException("Harmony Preferences file is null on write")
@@ -185,63 +268,50 @@
// for a key that already exists in either store, something upstream read null
// incorrectly and is about to overwrite a valid key — block the write to prevent
// irreversible corruption.
- if (keyValue != null && keyAlreadyExists(legacyPrefs, harmonyPrefs, keyName)) {
+ if (keyValue != null && keyAlreadyExists(legacyPrefs, harmonyPrefs, keyName, harmonyFlags.useHarmony)) {
pixel.fire(
AUTOFILL_STORE_KEY_ALREADY_EXISTS,
- getPixelParams(keyName = keyName),
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
type = Daily(),
)
- throw SecureStorageException.InternalSecureStorageException("Trying to overwrite already existing key")
+ throw SecureStorageException.KeyAlreadyExistsException("Trying to overwrite already existing key")
}
- runCatching {
- legacyPrefs?.edit(commit = true) {
+ if (!harmonyFlags.multiProcess) {
+ // Use the editor directly (not the KTX edit(commit=true) extension) so we can capture commit()'s boolean return value
+ val legacyCommitted = runCatching {
+ val editor = legacyPrefs!!.edit()
if (keyValue == null) {
- remove(keyName)
+ editor.remove(keyName)
} else {
- putString(keyName, keyValue.toByteString().base64())
+ editor.putString(keyName, keyValue.toByteString().base64())
}
+ editor.commit()
+ }.getOrElse {
+ ensureActive()
+ onLegacyWriteFailure("Error writing to legacy preferences", it)
}
- }.getOrElse {
- ensureActive()
- pixel.fire(
- AUTOFILL_PREFERENCES_UPDATE_KEY_FAILED,
- getPixelParams(keyName = keyName, throwable = it),
- type = Daily(),
- )
- throw SecureStorageException.InternalSecureStorageException("Error writing to legacy preferences", it)
+ if (!legacyCommitted) {
+ onLegacyWriteFailure("Legacy commit() returned false — write not persisted to disk")
+ }
}
- if (useHarmony()) {
- runCatching {
- harmonyPrefs?.edit(commit = true) {
- if (keyValue == null) {
- remove(keyName)
- } else {
- putString(keyName, keyValue.toByteString().base64())
- }
+ if (harmonyPrefs != null && harmonyFlags.useHarmony) {
+ val harmonyCommitted = runCatching {
+ val editor = harmonyPrefs.edit()
+ if (keyValue == null) {
+ editor.remove(keyName)
+ } else {
+ editor.putString(keyName, keyValue.toByteString().base64())
}
+ editor.commit()
}.getOrElse {
ensureActive()
- // Rollback legacy write so we don't cause a corrupted state with out of sync files
- pixel.fire(
- AUTOFILL_HARMONY_PREFERENCES_UPDATE_KEY_FAILED,
- getPixelParams(keyName = keyName, throwable = it),
- type = Daily(),
- )
- if (keyValue != null) {
- runCatching {
- legacyPrefs?.edit(commit = true) { remove(keyName) }
- }.onFailure { rollbackError ->
- pixel.fire(
- AutofillPixelNames.AUTOFILL_HARMONY_UPDATE_KEY_ROLLBACK_FAILED,
- getPixelParams(keyName = keyName, throwable = rollbackError),
- type = Daily(),
- )
- }
- }
- throw SecureStorageException.InternalSecureStorageException("Error writing to harmony preferences", it)
+ onHarmonyWriteFailure("Error writing to harmony preferences", it)
}
+ if (!harmonyCommitted) {
+ onHarmonyWriteFailure("Harmony commit() returned false — write not persisted to disk")
+ }
}
}
}
@@ -250,7 +320,12 @@
* Checks if the key already exists in either legacy or Harmony preferences.
* Used as a write guard to prevent overwriting write-once encryption keys.
*/
- private suspend fun keyAlreadyExists(legacyPrefs: SharedPreferences?, harmonyPrefs: SharedPreferences?, keyName: String): Boolean {
+ private suspend fun keyAlreadyExists(
+ legacyPrefs: SharedPreferences?,
+ harmonyPrefs: SharedPreferences?,
+ keyName: String,
+ useHarmony: Boolean,
+ ): Boolean {
val legacyExists = try {
legacyPrefs?.getString(keyName, null) != null
} catch (e: Exception) {
@@ -259,12 +334,14 @@
}
if (legacyExists) return true
- if (useHarmony()) {
+ if (useHarmony) {
val harmonyExists = try {
harmonyPrefs?.getString(keyName, null) != null
} catch (e: Exception) {
currentCoroutineContext().ensureActive()
- false
+ // Cannot confirm the key is absent — treat as exists to prevent a potentially destructive overwrite.
+ // Genuine Keystore faults reach this path as thrown exceptions; block the write conservatively.
+ true
}
if (harmonyExists) return true
}
@@ -274,125 +351,178 @@
override suspend fun getKey(keyName: String): ByteArray? {
return withContext(dispatcherProvider.io()) {
- // Always read from legacy — source of truth
+ val harmonyFlags = harmonyFlags()
- val legacyPrefs = getEncryptedPreferences().also {
- if (it == null) {
- pixel.fire(
- AUTOFILL_PREFERENCES_GET_KEY_NULL_FILE,
- getPixelParams(keyName = keyName),
- type = Daily(),
- )
- if (readFromHarmony()) {
- throw SecureStorageException.InternalSecureStorageException("Legacy Preferences file is null on read")
+ // In multi-process mode, skip legacy reads — EncryptedSharedPreferences writes do not
+ // propagate between processes, so legacy may be stale. readFromHarmony() returns true
+ // in this mode, so the existing readFromHarmony path returns the harmony value below.
+ val legacyPrefs: SharedPreferences? = if (!harmonyFlags.multiProcess) {
+ getEncryptedPreferences().also {
+ if (it == null) {
+ pixel.fire(
+ AUTOFILL_PREFERENCES_GET_KEY_NULL_FILE,
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
+ type = Daily(),
+ )
+ if (harmonyFlags.readFromHarmony) {
+ throw SecureStorageException.InternalSecureStorageException("Legacy Preferences file is null on read")
+ }
}
}
+ } else {
+ null
}
- val harmonyPrefs = if (!useHarmony()) {
+ val harmonyPrefs = if (!harmonyFlags.useHarmony) {
null
} else {
getHarmonyEncryptedPreferences().also {
if (it == null) {
pixel.fire(
AUTOFILL_HARMONY_PREFERENCES_GET_KEY_NULL_FILE,
- getPixelParams(keyName = keyName),
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
type = Daily(),
)
- if (readFromHarmony()) {
+ if (harmonyFlags.readFromHarmony) {
throw SecureStorageException.InternalSecureStorageException("Harmony Preferences file is null on read")
}
}
}
}
- val legacyValue: ByteArray? = runCatching {
- legacyPrefs?.getString(keyName, null)?.run {
- this.decodeBase64()?.toByteArray()
+ val legacyValue: ByteArray? = if (legacyPrefs != null) {
+ val legacyEncoded = runCatching {
+ legacyPrefs.getString(keyName, null)
+ }.getOrElse {
+ ensureActive()
+ pixel.fire(
+ AUTOFILL_PREFERENCES_GET_KEY_FAILED,
+ getPixelParams(
+ keyName = keyName,
+ throwable = it,
+ useHarmony = harmonyFlags.useHarmony,
+ readFromHarmony = harmonyFlags.readFromHarmony,
+ ),
+ type = Daily(),
+ )
+ throw it
}
- }.getOrElse {
- ensureActive()
- pixel.fire(
- AUTOFILL_PREFERENCES_GET_KEY_FAILED,
- getPixelParams(keyName = keyName, throwable = it),
- type = Daily(),
- )
- throw it
+ if (legacyEncoded != null) {
+ val decoded = legacyEncoded.decodeBase64()?.toByteArray()
+ if (decoded == null) {
+ pixel.fire(
+ AUTOFILL_PREFERENCES_GET_KEY_DECODE_FAILED,
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
+ type = Daily(),
+ )
+ throw SecureStorageException.InternalSecureStorageException("Legacy preferences key value is present but cannot be decoded")
+ }
+ decoded
+ } else {
+ null
+ }
+ } else {
+ null
}
// When useHarmony is ON, read Harmony and compare for diagnostic pixels
- if (useHarmony()) {
- val harmonyValue = runCatching {
- harmonyPrefs?.getString(keyName, null)?.run {
- this.decodeBase64()?.toByteArray()
- }
+ if (harmonyFlags.useHarmony) {
+ val harmonyEncoded = runCatching {
+ harmonyPrefs?.getString(keyName, null)
}.getOrElse {
ensureActive()
pixel.fire(
AUTOFILL_HARMONY_PREFERENCES_GET_KEY_FAILED,
- getPixelParams(keyName = keyName, throwable = it),
+ getPixelParams(
+ keyName = keyName,
+ throwable = it,
+ useHarmony = harmonyFlags.useHarmony,
+ readFromHarmony = harmonyFlags.readFromHarmony,
+ ),
type = Daily(),
)
- if (readFromHarmony()) {
+ if (harmonyFlags.readFromHarmony) {
throw SecureStorageException.InternalSecureStorageException("Harmony preferences getKey failed")
}
null
}
+ val harmonyValue: ByteArray? = if (harmonyEncoded != null) {
+ val decoded = harmonyEncoded.decodeBase64()?.toByteArray()
+ if (decoded == null) {
+ pixel.fire(
+ AUTOFILL_HARMONY_PREFERENCES_GET_KEY_DECODE_FAILED,
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
+ type = Daily(),
+ )
+ if (harmonyFlags.readFromHarmony) {
+ throw SecureStorageException.InternalSecureStorageException(
+ "Harmony preferences key value is present but cannot be decoded",
+ )
+ }
+ null
+ } else {
+ decoded
+ }
+ } else {
+ null
+ }
when {
harmonyPrefs != null && harmonyValue == null && legacyValue != null -> {
pixel.fire(
AUTOFILL_HARMONY_KEY_MISSING,
- getPixelParams(keyName = keyName),
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
type = Daily(),
)
- if (readFromHarmony()) {
+ if (harmonyFlags.readFromHarmony) {
throw SecureStorageException.InternalSecureStorageException("Harmony key missing")
}
}
legacyPrefs != null && harmonyValue != null && legacyValue == null -> {
pixel.fire(
AUTOFILL_PREFERENCES_KEY_MISSING,
- getPixelParams(keyName = keyName),
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
type = Daily(),
)
- if (readFromHarmony()) {
+ if (harmonyFlags.readFromHarmony) {
throw SecureStorageException.InternalSecureStorageException("Legacy key missing")
}
}
harmonyValue != null && legacyValue != null && !harmonyValue.contentEquals(legacyValue) -> {
pixel.fire(
AUTOFILL_HARMONY_KEY_MISMATCH,
- getPixelParams(keyName = keyName),
+ getPixelParams(keyName = keyName, useHarmony = harmonyFlags.useHarmony, readFromHarmony = harmonyFlags.readFromHarmony),
type = Daily(),
)
- if (readFromHarmony()) {
+ if (harmonyFlags.readFromHarmony) {
throw SecureStorageException.InternalSecureStorageException("Harmony key mismatch")
}
}
- readFromHarmony() -> {
+ harmonyFlags.readFromHarmony -> {
+ logcat(TAG) { "Returning $keyName from harmony" }
return@withContext harmonyValue
}
}
}
-
+ logcat(TAG) { "Returning $keyName from legacy" }
return@withContext legacyValue
}
}
override suspend fun canUseEncryption(): Boolean = withContext(dispatcherProvider.io()) {
- if (useHarmony()) {
- getEncryptedPreferences() != null && getHarmonyEncryptedPreferences() != null
- } else {
- getEncryptedPreferences() != null
+ val harmonyFlags = harmonyFlags()
+ when {
... diff truncated: showing 800 of 1268 lines
...ll-impl/src/main/java/com/duckduckgo/autofill/impl/securestorage/SecureStorageKeyProvider.kt
Show resolved
Hide resolved
CrisBarreiro
force-pushed
the
feature/cris/address-update-key-existing-issues
branch
from
6520cfc to
229646a
Compare
CrisBarreiro
force-pushed
the
feature/cris/add-multi-process-guard
branch
from
d500c69 to
9d5ab40
Compare
CrisBarreiro
commented
Mar 26, 2026
| } else { | ||
| val keyBytes = secureStorageKeyGenerator.generateKey().encoded | ||
| try { | ||
| encryptAndStoreL2Key(keyBytes, password) |
Collaborator
Author
There was a problem hiding this comment.
This operation isn't atomic, but it probably should. Otherwise, in some edge cases, it can lead to key already existing failures if we managed to write L2 but not L2 IV
CrisBarreiro
force-pushed
the
feature/cris/add-multi-process-guard
branch
from
9d5ab40 to
0821aac
Compare
CrisBarreiro
force-pushed
the
feature/cris/address-update-key-existing-issues
branch
from
3ec48e1 to
865e52e
Compare
CrisBarreiro
force-pushed
the
feature/cris/add-multi-process-guard
branch
2 times, most recently
from
7af9106 to
0932d18
Compare
CrisBarreiro
changed the base branch from
feature/cris/address-update-key-existing-issues
to
graphite-base/8087
CrisBarreiro
force-pushed
the
graphite-base/8087
branch
from
134586c to
7ef4849
Compare
CrisBarreiro
force-pushed
the
feature/cris/add-multi-process-guard
branch
2 times, most recently
from
ca961d9 to
a5fc461
Compare
CrisBarreiro
force-pushed
the
graphite-base/8087
branch
from
7ef4849 to
6f5220b
Compare
CrisBarreiro
force-pushed
the
feature/cris/add-multi-process-guard
branch
from
a5fc461 to
5de719e
Compare
CrisBarreiro
changed the base branch from
graphite-base/8087
to
feature/cris/address-update-key-existing-issues
CrisBarreiro
changed the base branch from
feature/cris/address-update-key-existing-issues
to
graphite-base/8087
CrisBarreiro
force-pushed
the
graphite-base/8087
branch
from
3f0cc17 to
a111f2b
Compare
CrisBarreiro
force-pushed
the
feature/cris/add-multi-process-guard
branch
from
c2cb4fa to
ab9f44b
Compare
CrisBarreiro
force-pushed
the
feature/cris/add-multi-process-guard
branch
from
ab9f44b to
c684a17
Compare
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
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.
Task/Issue URL: https://app.asana.com/1/137249556945/project/1212227266948491/task/1213745082298068
Description
Steps to test this PR
Feature 1
Returning KEY_L1KEY from harmonyandWriting KEY_L1KEY to harmonyFeature 2
Returning KEY_L1KEY from legacyand andWriting KEY_L1KEY to legacyPatch
UI changes
Note
Medium Risk
Touches secure key generation/storage and changes read/write behavior between legacy and Harmony stores, which could impact encryption key consistency across processes. Risk is mitigated by explicit multi-process gating, write-once guards, and expanded test coverage.
Overview
Improves secure storage behavior when the Autofill service runs in multi-process mode by treating Harmony as the single source of truth: legacy
EncryptedSharedPreferencesreads/writes are skipped, andcanUseEncryption()now reflects this mode.Hardens key initialization against cross-process races by introducing
SecureStorageException.KeyAlreadyExistsExceptionand updatingSecureStorageKeyProviderto gracefully fall back to the concurrently-written value for L1 key, password, L2 key material, and salt.Updates DI to pass
AutofillServiceFeatureintoRealSecureStorageKeyStore, relaxes theautofillService.self()toggle to be remotely controllable, and adds/updates unit tests to cover multi-process read/write paths and the new exception type.Written by Cursor Bugbot for commit c684a17. This will update automatically on new commits. Configure here.