Add diagnostics pixels for harmony and prevent corrupted states#7846
Open
CrisBarreiro wants to merge 44 commits intodevelopfrom
Open
Add diagnostics pixels for harmony and prevent corrupted states#7846CrisBarreiro wants to merge 44 commits intodevelopfrom
CrisBarreiro wants to merge 44 commits intodevelopfrom
Conversation
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
CrisBarreiro
force-pushed
the
feature/cris/autofill-harmony-add-diagnostic-pixels
branch
from
4807e42 to
3b5604f
Compare
Contributor
|
Privacy Review task: https://app.asana.com/0/69071770703008/1213494582843183 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Harmony read failure falsely triggers key-missing diagnostic pixel
- Changed the catch block to return early instead of setting harmonyValue to null, preventing the when block from incorrectly firing AUTOFILL_HARMONY_KEY_MISSING after a read exception.
Or push these changes by commenting:
@cursor push b9cff212db
Preview (b9cff212db)
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
@@ -290,7 +290,7 @@
),
type = Daily(),
)
- null
+ return
}
when {
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt
Outdated
Show resolved
Hide resolved
CrisBarreiro
force-pushed
the
feature/cris/autofill-harmony-add-diagnostic-pixels
branch
from
dab31ad to
930a527
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing diagnostic pixel for legacy-null-harmony-present discrepancy
- Added AUTOFILL_LEGACY_KEY_MISSING pixel that fires in compareWithHarmony when harmony has the key but legacy doesn't, providing diagnostic information for the root cause of blocked writes.
Or push these changes by commenting:
@cursor push c6a771e0d4
Preview (c6a771e0d4)
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
@@ -236,6 +236,7 @@
AUTOFILL_HARMONY_PREFERENCES_UPDATE_KEY_FAILED("autofill_harmony_preferences_update_key_failed"),
AUTOFILL_HARMONY_KEY_MISMATCH("autofill_harmony_key_mismatch"),
AUTOFILL_HARMONY_KEY_MISSING("autofill_harmony_key_missing"),
+ AUTOFILL_LEGACY_KEY_MISSING("autofill_legacy_key_missing"),
AUTOFILL_STORE_KEY_ALREADY_EXISTS("autofill_store_key_already_exists"),
LIBRARY_LOAD_TIMEOUT_SQLCIPHER("library_load_timeout_sqlcipher"),
LIBRARY_LOAD_FAILURE_SQLCIPHER("library_load_failure_sqlcipher"),
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
@@ -30,6 +30,7 @@
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_HARMONY_PREFERENCES_GET_KEY_FAILED
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_LEGACY_KEY_MISSING
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_PREFERENCES_GET_KEY_FAILED
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_PREFERENCES_UPDATE_KEY_FAILED
import com.duckduckgo.autofill.impl.pixel.AutofillPixelNames.AUTOFILL_STORE_KEY_ALREADY_EXISTS
@@ -304,6 +305,16 @@
type = Daily(),
)
}
+ harmonyValue != null && legacyValue == null -> {
+ pixel.fire(
+ AUTOFILL_LEGACY_KEY_MISSING,
+ mapOf(
+ "key" to keyName,
+ "addHarmonyFixes" to autofillFeature.addHarmonyFixes().isEnabled().toString(),
+ ),
+ type = Daily(),
+ )
+ }
harmonyValue != null && legacyValue != null && !harmonyValue.contentEquals(legacyValue) -> {
pixel.fire(
AUTOFILL_HARMONY_KEY_MISMATCH,
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt
Outdated
Show resolved
Hide resolved
CrisBarreiro
force-pushed
the
feature/cris/autofill-harmony-add-diagnostic-pixels
branch
from
826fe31 to
1fad110
Compare
CrisBarreiro
commented
Mar 3, 2026
| ), | ||
| type = Daily(), | ||
| ) | ||
| return@withContext |
Collaborator
Author
There was a problem hiding this comment.
On second thought, I think this guard introduces unnecessary complexity, as well as some risks
- If we prevent writing, we're introducing a behavior change. Since the call executed correctly, the consumer sometimes force unwraps the result, which will cause a NPE
- If we prevent writing and throw an exception, we're also introducing a behavior change. If the consumer doesn't handle this scenario properly, the app will crash
Since we're always reading from legacy, I think at this point, it's safer not to introduce any additional changes
Member
There was a problem hiding this comment.
Exception sounds good (assuming still caught by something higher up)
Collaborator
Author
There was a problem hiding this comment.
Not at this point, that would require changes higher up. At this point the plan was to add diagnostics but not change behavior to keep risk as low as possible. I think we eventually need to address this, but since we're not using legacy as the source of truth, I think the risk of a catastrophical mismatch on write is minimal, so I'm kinda leaning towards handling this on the next step
CDRussell
approved these changes
Mar 3, 2026
CrisBarreiro
force-pushed
the
feature/cris/autofill-harmony-add-diagnostic-pixels
branch
from
1fad110 to
a75b11b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CrisBarreiro
force-pushed
the
feature/cris/autofill-harmony-add-diagnostic-pixels
branch
from
a75b11b to
a959d89
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CrisBarreiro
force-pushed
the
feature/cris/autofill-harmony-add-diagnostic-pixels
branch
from
f384371 to
be9d66b
Compare
CrisBarreiro
force-pushed
the
feature/cris/autofill-harmony-add-diagnostic-pixels
branch
from
be9d66b to
6d7ce02
Compare
CrisBarreiro
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Pixel parameter names mismatch between code and definitions
- Replaced addWriteGuard/addReadGuard with useHarmony/readFromHarmony in autofill.json5 to match the actual parameters sent by getPixelParams() and added missing pixel definitions for the four null_file pixels.
- ✅ Fixed: New diagnostic pixels missing from ATB removal interceptor
- Added all six missing pixels (AUTOFILL_PREFERENCES_KEY_MISSING, AUTOFILL_HARMONY_UPDATE_KEY_ROLLBACK_FAILED, and four null_file pixels) to PixelParamRemovalInterceptor with removeAtb().
Or push these changes by commenting:
@cursor push 945201de27
Preview (945201de27)
diff --git a/PixelDefinitions/pixels/autofill.json5 b/PixelDefinitions/pixels/autofill.json5
--- a/PixelDefinitions/pixels/autofill.json5
+++ b/PixelDefinitions/pixels/autofill.json5
@@ -12,14 +12,14 @@
"type": "string"
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard feature flag is enabled",
+ "key": "readFromHarmony",
+ "description": "Whether the readFromHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
@@ -44,16 +44,22 @@
"type": "string"
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard feature flag is enabled",
+ "key": "readFromHarmony",
+ "description": "Whether the readFromHarmony 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"]
}
]
},
@@ -76,16 +82,22 @@
"type": "string"
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard feature flag is enabled",
+ "key": "readFromHarmony",
+ "description": "Whether the readFromHarmony 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"]
}
]
},
@@ -108,18 +120,12 @@
"type": "string"
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard 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",
@@ -152,18 +158,12 @@
"type": "string"
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard 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",
@@ -196,15 +196,21 @@
"type": "string"
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard feature flag is enabled",
+ "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"]
}
]
@@ -223,18 +229,12 @@
"enum": ["KEY_GENERATED_PASSWORD", "KEY_L1KEY", "KEY_PASSWORD_SALT", "KEY_ENCRYPTED_L2KEY", "KEY_ENCRYPTED_L2KEY_IV"]
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard 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",
@@ -262,18 +262,12 @@
"enum": ["KEY_GENERATED_PASSWORD", "KEY_L1KEY", "KEY_PASSWORD_SALT", "KEY_ENCRYPTED_L2KEY", "KEY_ENCRYPTED_L2KEY_IV"]
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard 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",
@@ -301,8 +295,8 @@
"enum": ["KEY_GENERATED_PASSWORD", "KEY_L1KEY", "KEY_PASSWORD_SALT", "KEY_ENCRYPTED_L2KEY", "KEY_ENCRYPTED_L2KEY_IV"]
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
@@ -311,6 +305,12 @@
"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"]
}
]
},
@@ -328,18 +328,12 @@
"enum": ["KEY_GENERATED_PASSWORD", "KEY_L1KEY", "KEY_PASSWORD_SALT", "KEY_ENCRYPTED_L2KEY", "KEY_ENCRYPTED_L2KEY_IV"]
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard 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",
@@ -372,18 +366,45 @@
"type": "string"
},
{
- "key": "addWriteGuard",
- "description": "Whether the addWriteGuard feature flag is enabled",
+ "key": "useHarmony",
+ "description": "Whether the useHarmony feature flag is enabled",
"type": "string",
"enum": ["true", "false"]
},
{
- "key": "addReadGuard",
- "description": "Whether the addReadGuard feature flag is enabled",
+ "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_preferences_update_key_null_file": {
+ "description": "Legacy preferences file is null on write (daily unique)",
+ "owners": ["CrisBarreiro"],
+ "triggers": ["exception"],
+ "suffixes": ["form_factor"],
+ "parameters": [
+ "appVersion",
+ {
+ "key": "key",
+ "description": "The name of the key that failed to update",
+ "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",
@@ -397,6 +418,105 @@
}
]
},
+ "autofill_harmony_preferences_update_key_null_file": {
+ "description": "Harmony preferences file is null on write (daily unique)",
+ "owners": ["CrisBarreiro"],
+ "triggers": ["exception"],
+ "suffixes": ["form_factor"],
+ "parameters": [
+ "appVersion",
+ {
+ "key": "key",
+ "description": "The name of the key that failed to update in 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_preferences_get_key_null_file": {
+ "description": "Legacy preferences file is null on read (daily unique)",
+ "owners": ["CrisBarreiro"],
+ "triggers": ["exception"],
+ "suffixes": ["form_factor"],
+ "parameters": [
+ "appVersion",
+ {
+ "key": "key",
+ "description": "The name of the key that failed to read 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_null_file": {
+ "description": "Harmony preferences file is null on read (daily unique)",
+ "owners": ["CrisBarreiro"],
+ "triggers": ["exception"],
+ "suffixes": ["form_factor"],
+ "parameters": [
+ "appVersion",
+ {
+ "key": "key",
+ "description": "The name of the key that failed to read 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"]
+ }
+ ]
+ },
"library_load_timeout_sqlcipher": {
"description": "Fired when the sqlcipher native library takes longer than the timeout period to load asynchronously",
"owners": ["CDRussell"],
diff --git a/app/src/main/java/com/duckduckgo/app/global/api/PixelParamRemovalInterceptor.kt b/app/src/main/java/com/duckduckgo/app/global/api/PixelParamRemovalInterceptor.kt
--- a/app/src/main/java/com/duckduckgo/app/global/api/PixelParamRemovalInterceptor.kt
+++ b/app/src/main/java/com/duckduckgo/app/global/api/PixelParamRemovalInterceptor.kt
@@ -174,6 +174,12 @@
AutofillPixelNames.AUTOFILL_HARMONY_KEY_MISMATCH.pixelName to PixelParameter.removeAtb(),
AutofillPixelNames.AUTOFILL_HARMONY_KEY_MISSING.pixelName to PixelParameter.removeAtb(),
AutofillPixelNames.AUTOFILL_STORE_KEY_ALREADY_EXISTS.pixelName to PixelParameter.removeAtb(),
+ AutofillPixelNames.AUTOFILL_PREFERENCES_KEY_MISSING.pixelName to PixelParameter.removeAtb(),
+ AutofillPixelNames.AUTOFILL_HARMONY_UPDATE_KEY_ROLLBACK_FAILED.pixelName to PixelParameter.removeAtb(),
+ AutofillPixelNames.AUTOFILL_PREFERENCES_UPDATE_KEY_NULL_FILE.pixelName to PixelParameter.removeAtb(),
+ AutofillPixelNames.AUTOFILL_HARMONY_PREFERENCES_UPDATE_KEY_NULL_FILE.pixelName to PixelParameter.removeAtb(),
+ AutofillPixelNames.AUTOFILL_PREFERENCES_GET_KEY_NULL_FILE.pixelName to PixelParameter.removeAtb(),
+ AutofillPixelNames.AUTOFILL_HARMONY_PREFERENCES_GET_KEY_NULL_FILE.pixelName to PixelParameter.removeAtb(),
AppPixelName.APP_INSTALL_VERIFIED_INSTALL.pixelName to PixelParameter.removeAtb(),
AppPixelName.APP_UPDATE_VERIFIED_INSTALL.pixelName to PixelParameter.removeAtb(),
DataStorePixelNames.DATA_STORE_MIGRATE_ENCRYPTED_GET_PREFERENCES_ORIGIN_FAILED.pixelName to PixelParameter.removeAtb(),
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/global/api/PixelParamRemovalInterceptor.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Refactoring dropped ensureActive coroutine cancellation checks
- Added ensureActive() calls before returning null in all getOrElse blocks within migrateEncryptedToHarmonyIfNecessary methods to properly propagate CancellationException.
Or push these changes by commenting:
@cursor push 1d8f0f2d99
Preview (1d8f0f2d99)
diff --git a/data-store/data-store-impl/src/main/java/com/duckduckgo/data/store/impl/SharedPreferencesProviderImpl.kt b/data-store/data-store-impl/src/main/java/com/duckduckgo/data/store/impl/SharedPreferencesProviderImpl.kt
--- a/data-store/data-store-impl/src/main/java/com/duckduckgo/data/store/impl/SharedPreferencesProviderImpl.kt
+++ b/data-store/data-store-impl/src/main/java/com/duckduckgo/data/store/impl/SharedPreferencesProviderImpl.kt
@@ -224,13 +224,14 @@
private suspend fun migrateEncryptedToHarmonyIfNecessary(name: String): SharedPreferences? {
return withContext(dispatcherProvider.io()) {
- val destination = getEncryptedHarmonyDestination(name) ?: return@withContext null
+ val destination = getEncryptedHarmonyDestination(name) ?: run { ensureActive(); return@withContext null }
runCatching {
when (isAlreadyMigratedToHarmony(destination, name)) {
true -> return@withContext destination
false -> Unit
}
}.getOrElse {
+ ensureActive()
return@withContext null
}
@@ -257,6 +258,7 @@
runCatching {
migrateContentsToHarmony(origin, destination, name)
}.getOrElse {
+ ensureActive()
return@withContext null
}
@@ -266,19 +268,21 @@
private suspend fun migrateEncryptedToHarmonyIfNecessary(origin: SharedPreferences, name: String): SharedPreferences? {
return withContext(dispatcherProvider.io()) {
- val destination = getEncryptedHarmonyDestination(name) ?: return@withContext null
+ val destination = getEncryptedHarmonyDestination(name) ?: run { ensureActive(); return@withContext null }
runCatching {
when (isAlreadyMigratedToHarmony(destination, name)) {
true -> return@withContext destination
false -> Unit
}
}.getOrElse {
+ ensureActive()
return@withContext null
}
runCatching {
migrateContentsToHarmony(origin, destination, name)
}.getOrElse {
+ ensureActive()
return@withContext null
}
...ata-store-impl/src/main/java/com/duckduckgo/data/store/impl/SharedPreferencesProviderImpl.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Legacy write failure silently swallowed when harmony disabled
- Removed the conditional if (useHarmony()) check so the exception is always thrown when legacy preference write fails, restoring the original safe behavior.
Or push these changes by commenting:
@cursor push eb2a5c77a7
Preview (eb2a5c77a7)
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
@@ -213,9 +213,7 @@
getPixelParams(keyName = keyName, throwable = it),
type = Daily(),
)
- if (useHarmony()) {
- throw SecureStorageException.InternalSecureStorageException("Error writing to legacy preferences", it)
- }
+ throw SecureStorageException.InternalSecureStorageException("Error writing to legacy preferences", it)
}
if (useHarmony()) {
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/store/keys/SecureStorageKeyStore.kt
Outdated
Show resolved
Hide resolved
CrisBarreiro
force-pushed
the
feature/cris/autofill-harmony-add-diagnostic-pixels
branch
from
1124f12 to
fed9e3e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Write guard unconditionally throws when harmony is disabled
- Gated the throw statement in the write guard behind an if (useHarmony()) check so exceptions are only thrown when harmony is enabled.
- ✅ Fixed: Null legacy prefs unconditionally throws on write path
- Gated the throw statement behind an if (useHarmony()) check and added return@withContext to preserve pre-existing no-op behavior when harmony is disabled.
Or push these changes by commenting:
@cursor push 8baa52bb74
Preview (8baa52bb74)
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
@@ -162,7 +162,10 @@
getPixelParams(keyName = keyName),
type = Daily(),
)
- throw SecureStorageException.InternalSecureStorageException("Legacy Preferences file is null on write")
+ if (useHarmony()) {
+ throw SecureStorageException.InternalSecureStorageException("Legacy Preferences file is null on write")
+ }
+ return@withContext
}
}
@@ -191,7 +194,9 @@
getPixelParams(keyName = keyName),
type = Daily(),
)
- throw SecureStorageException.InternalSecureStorageException("Trying to overwrite already existing key")
+ if (useHarmony()) {
+ throw SecureStorageException.InternalSecureStorageException("Trying to overwrite already existing key")
+ }
}
runCatching {| type = Daily(), | ||
| ) | ||
| throw SecureStorageException.InternalSecureStorageException("Trying to overwrite already existing key") | ||
| } |
There was a problem hiding this comment.
Write guard unconditionally throws when harmony is disabled
High Severity
The write guard at keyAlreadyExists unconditionally throws SecureStorageException regardless of the useHarmony() state. The corresponding test whenHarmonyDisabledAndKeyAlreadyExistsThenPixelFiredButNoException expects no exception when harmony is disabled — only a diagnostic pixel. The getKey method correctly gates its exceptions behind readFromHarmony(), but updateKey does not. When harmony is off, this changes pre-existing behavior and will crash the app whenever an already-existing key write is attempted.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
| ) | ||
| throw SecureStorageException.InternalSecureStorageException("Legacy Preferences file is null on write") | ||
| } | ||
| } |
There was a problem hiding this comment.
Null legacy prefs unconditionally throws on write path
High Severity
When legacy encrypted preferences is null during updateKey, the code unconditionally throws SecureStorageException regardless of whether useHarmony() is enabled. The test whenLegacyPrefsThrowsOnWriteAndHarmonyDisabledThenPixelFiredButNoException expects only a pixel fire with no exception. The pre-existing behavior used a ?. safe call that silently skipped the write. This behavioral change will crash the app when legacy prefs retrieval fails and harmony is off.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
Collaborator
Author
There was a problem hiding this comment.
If we don't skip the write, getL1Key (et al) will do this
return if (secureStorageKeyRepository.getL1Key() == null) {
randomBytesGenerator.generateBytes(L1_PASSPHRASE_SIZE).also {
secureStorageKeyRepository.setL1Key(it)
}
} else {
secureStorageKeyRepository.getL1Key()!!
}
So if it was null, will do a fire-and-forget update, and return the value it tried to store. In practice, this might cause permanent corruption if we create the database with an L1 key we never got to properly store
CrisBarreiro
commented
Mar 6, 2026
| EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, | ||
| EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM, | ||
| ) | ||
| encryptedPreferencesFactory.create(FILENAME) |
Collaborator
Author
There was a problem hiding this comment.
extracted for testing purposes
| ) | ||
| throw SecureStorageException.InternalSecureStorageException("Legacy Preferences file is null on write") | ||
| } | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
If we don't skip the write, getL1Key (et al) will do this
return if (secureStorageKeyRepository.getL1Key() == null) {
randomBytesGenerator.generateBytes(L1_PASSPHRASE_SIZE).also {
secureStorageKeyRepository.setL1Key(it)
}
} else {
secureStorageKeyRepository.getL1Key()!!
}
So if it was null, will do a fire-and-forget update, and return the value it tried to store. In practice, this might cause permanent corruption if we create the database with an L1 key we never got to properly store
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
2 participants
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/1213193785919631?focus=true
Description
Can be rolled out in 2 phases
Write to both legacy and harmony
Steps to test this PR
Important
Apply required patches from here
Logcat filter:
autofill_harmony | autofill_preferences | data_store | InternalSecureStorageException | autofill_store | "autofill harmony" | "Secure storage database exists but is not readable" | "Autofill-DB-Init:"Existing corrupted setup doesn't propagate when enabling useHarmony
1. Corrupt previous fileTrying to write when harmony is null throws exception but doesn't corrupt data
Force null harmony on writeAutofill-DB-Init: Database created successfullyuseHarmonyon, restart the appAutofill-DB-Init: Database created successfullyautofill_harmony_preferences_update_key_null_fileis firedTrying to write when legacy is null throws exception but doesn't corrupt data
Force legacy to be null on writeAutofill-DB-Init: Cannot read l1Key - database creation abortedautofill_preferences_update_key_null_fileis fireduseHarmonyon, restart the appCannot read l1Key - database creation abortedautofill_preferences_update_key_null_fileAutofill-DB-Init: Database created successfullyTrying to write when a key already exists throws exception but doesn't corrupt data
Force key already exists on writeCannot read l1Key - database creation abortedautofill_store_key_already_existsfireduseHarmonyon, restart the appCannot read l1Key - database creation abortedautofill_store_key_already_existsskippedAutofill-DB-Init: Database created successfullyKey failed to store in legacy throws exception but doesn't corrupt data
Force updateKey to fail on legacyautofill_preferences_update_key_failedis sentuseHarmonyon, restart the appCannot read l1Key - database creation abortedandautofill_preferences_update_key_failedignoredKey failed to store in legacy throws exception but doesn't corrupt data
Force updateKey to fail on legacyCannot read l1Key - database creation abortedautofill_preferences_update_key_failedfireduseHarmonyon, restart the appCannot read l1Key - database creation abortedautofill_preferences_update_key_failedignoredAutofill-DB-Init: Database created successfullyKey failed to store in harmony throws exception but doesn't corrupt data
Force updateKey to fail on harmonyCannot read l1Key - database creation abortedautofill_harmony_preferences_update_key_faileduseHarmonyon, restart the appCannot read l1Key - database creation abortedautofill_harmony_preferences_update_key_failedignoredAutofill-DB-Init: Database created successfullyTrying to read when legacy is null and readFromHarmony false doesn't corrupt data
Force legacy to be null on readCannot read l1Key - database creation abortedautofill_preferences_update_key_null_fileandautofill_preferences_get_key_null_fileare fireduseHarmonyon, restart the appAutofill-DB-Init: Cannot read l1Key - database creation abortedautofill_preferences_update_key_null_fileandautofill_preferences_get_key_null_fileignoredAutofill-DB-Init: Database created successfullyTrying to read when legacy is null and readFromHarmony true doesn't corrupt data
Force legacy to be null on readAutofill-DB-Init: Cannot read l1Key - database creation abortedautofill_preferences_update_key_null_fileandautofill_preferences_get_key_null_filefireduseHarmonyandreadFromHarmonyon, restart the appAutofill-DB-Init: Cannot read l1Key - database creation abortedautofill_preferences_get_key_null_fileignored (noautofill_preferences_update_key_null_file)Autofill-DB-Init: Database created successfullyTrying to read when harmony is null and readFromHarmony false doesn't corrupt data
Force harmony to be null on readuseHarmonyon, restart the appAutofill-DB-Init: Cannot read l1Key - database creation abortedautofill_preferences_get_key_null_filefired (notautofill_preferences_update_key_null_file)Autofill-DB-Init: Database created successfullyTrying to read when harmony is null and readFromHarmony true doesn't corrupt data
Force harmony to be null on readuseHarmonyandreadFromHarmonyon, restart the appAutofill-DB-Init: Cannot read l1Key - database creation abortedautofill_harmony_preferences_get_key_null_filefired (noautofill_harmony_preferences_update_key_null_file)Autofill-DB-Init: Database created successfullyUI changes
n/a
Note
High Risk
Changes encrypted key storage read/write and migration paths for Autofill, including new failure modes (intentional exceptions) behind feature flags; issues here could impact password/credential access.
Overview
Adds a new
readFromHarmonytoggle and updates Autofill secure key storage to dual-write to Harmony while keeping legacy as the default read source, migrating into a new Harmony filename to avoid previously-corrupted data.Introduces corruption/consistency protections in
RealSecureStorageKeyStore: null-preferences detection, write-once key overwrite guard, rollback of legacy writes if Harmony write fails, and optional strict behavior (throwing) whenreadFromHarmonyis enabled; all paths emit new/expanded diagnostic pixels with feature-flag context.Extends the data-store API to migrate encrypted preferences from an explicit origin, refactors migration helpers, adds an
EncryptedPreferencesFactoryfor testable encrypted prefs creation, updates pixel param removal configuration, and adds comprehensive unit tests for the new secure key behavior.Written by Cursor Bugbot for commit 6b6174d. This will update automatically on new commits. Configure here.