Stop full-row site writes from clobbering app-password credentials

jkmassel · jkmassel · commit bc35787cf716 · 2026-06-10T15:55:18.000-06:00
The same full-row insertOrUpdateSite UPDATE that clobbered wpApiRestUrl also zeroed the application-password credential columns: a credential-less inbound SiteModel (e.g. a /me/sites sync) overwrote the encrypted username/password and their IVs with empty values. Exclude API_REST_USERNAME, API_REST_PASSWORD and their IV columns from the UpdateAllExceptId mapper (as a set — the IVs are required to decrypt), and route the credential writers through targeted single-column writers. Extends the #22905 fix to the credential columns.
diff --git a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/SiteSqlUtils.kt b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/SiteSqlUtils.kt
@@ -221,12 +221,20 @@ class SiteSqlUtils
             AppLog.d(DB, "Updating site: " + finalSiteModel.url)
             val oldId = siteResult[0].id
             try {
-                // WP_API_REST_URL is healed/discovered locally (see updateWpApiRestUrl) and must not be
-                // overwritten by stale full-row writes, so it is excluded from the generic update mapper.
+                // WP_API_REST_URL and the application-password credential columns are written out of band by
+                // dedicated single-column writers (updateWpApiRestUrl / updateApplicationPasswordCredentials),
+                // so they're excluded from the generic mapper to keep stale full-row writes from clobbering them.
                 WellSql.update(SiteModel::class.java).whereId(oldId)
                         .put(
                                 finalSiteModel,
-                                UpdateAllExceptId(SiteModel::class.java, SiteModelTable.WP_API_REST_URL)
+                                UpdateAllExceptId(
+                                        SiteModel::class.java,
+                                        SiteModelTable.WP_API_REST_URL,
+                                        SiteModelTable.API_REST_USERNAME,
+                                        SiteModelTable.API_REST_PASSWORD,
+                                        SiteModelTable.API_REST_USERNAME_IV,
+                                        SiteModelTable.API_REST_PASSWORD_IV
+                                )
                         ).execute()
             } catch (e: SQLiteConstraintException) {
                 AppLog.e(
@@ -288,21 +296,76 @@ class SiteSqlUtils
     fun clearWpApiRestUrl(localId: Int): Int = updateWpApiRestUrl(localId, "")
 
     /**
-     * Updates [SiteModel.wpApiRestUrl] for an application-password (ORIGIN_WPAPI) site identified by its URL.
-     * Such sites are fetched as fresh models with no local id and no remote site id
-     * (see SiteWPAPIRestClient.fetchWPAPISite), so the local-id-keyed [updateWpApiRestUrl] can't target them.
-     * Scoped to ORIGIN_WPAPI so it can't touch a WP.com/Jetpack row that happens to share the same URL.
+     * Targeted writer for the application-password credential columns: the encrypted username and password
+     * and their IVs. The four are written as a set — the IVs are required to decrypt the ciphertext, so
+     * persisting the values without their IVs would break reads. This is the sole writer of these columns on
+     * an existing row: the generic full-row update path excludes them so a credential-less inbound SiteModel
+     * (e.g. a /me/sites sync model) can't zero them out.
+     */
+    fun updateApplicationPasswordCredentials(localId: Int, usernamePlain: String, passwordPlain: String): Int {
+        val username = encryptionUtils.encrypt(usernamePlain)
+        val password = encryptionUtils.encrypt(passwordPlain)
+        return WellSql.update(SiteModel::class.java)
+                .whereId(localId)
+                .put(localId, { _ ->
+                    val cv = ContentValues()
+                    cv.put(SiteModelTable.API_REST_USERNAME, username.first)
+                    cv.put(SiteModelTable.API_REST_USERNAME_IV, username.second)
+                    cv.put(SiteModelTable.API_REST_PASSWORD, password.first)
+                    cv.put(SiteModelTable.API_REST_PASSWORD_IV, password.second)
+                    cv
+                }).execute()
+    }
+
+    /**
+     * Clears the application-password credential columns (encrypted username/password and their IVs) for the
+     * given local id. Companion to [updateApplicationPasswordCredentials] for the sign-out / revoke paths,
+     * since the generic update path no longer touches these columns.
+     */
+    fun clearApplicationPasswordCredentials(localId: Int): Int {
+        return WellSql.update(SiteModel::class.java)
+                .whereId(localId)
+                .put(localId, { _ ->
+                    val cv = ContentValues()
+                    cv.put(SiteModelTable.API_REST_USERNAME, "")
+                    cv.put(SiteModelTable.API_REST_USERNAME_IV, "")
+                    cv.put(SiteModelTable.API_REST_PASSWORD, "")
+                    cv.put(SiteModelTable.API_REST_PASSWORD_IV, "")
+                    cv
+                }).execute()
+    }
+
+    /**
+     * URL-keyed variant of [updateWpApiRestUrl] for application-password (ORIGIN_WPAPI) sites, which are
+     * fetched as fresh models with no local id (see SiteWPAPIRestClient.fetchWPAPISite). Scoped to
+     * ORIGIN_WPAPI so it can't touch a WP.com/Jetpack row that happens to share the same URL.
      */
     fun updateWpApiRestUrlForWPAPISite(siteUrl: String, wpApiRestUrl: String): Int {
-        val site = WellSql.select(SiteModel::class.java)
+        val localId = wpApiSiteLocalIdByUrl(siteUrl) ?: return 0
+        return updateWpApiRestUrl(localId, wpApiRestUrl)
+    }
+
+    /**
+     * URL-keyed variant of [updateApplicationPasswordCredentials] for ORIGIN_WPAPI sites fetched without a
+     * local id. See [updateWpApiRestUrlForWPAPISite].
+     */
+    fun updateApplicationPasswordCredentialsForWPAPISite(
+        siteUrl: String,
+        usernamePlain: String,
+        passwordPlain: String
+    ): Int {
+        val localId = wpApiSiteLocalIdByUrl(siteUrl) ?: return 0
+        return updateApplicationPasswordCredentials(localId, usernamePlain, passwordPlain)
+    }
+
+    private fun wpApiSiteLocalIdByUrl(siteUrl: String): Int? =
+        WellSql.select(SiteModel::class.java)
                 .where().beginGroup()
                 .equals(SiteModelTable.URL, siteUrl)
                 .equals(SiteModelTable.ORIGIN, SiteModel.ORIGIN_WPAPI)
                 .endGroup().endWhere()
                 .asModel
-                .firstOrNull() ?: return 0
-        return updateWpApiRestUrl(site.id, wpApiRestUrl)
-    }
+                .firstOrNull()?.id
 
     val wPComSites: SelectQuery<SiteModel>
         get() = WellSql.select(SiteModel::class.java)
diff --git a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/SiteStore.kt b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/SiteStore.kt
@@ -1616,10 +1616,19 @@ open class SiteStore @Inject constructor(
                     siteModel.wpApiRestUrl = payload.apiRootUrl
                 }
                 val result = updateSite(siteModel)
-                // updateSite's full-row write skips WP_API_REST_URL, and this fresh site has no local id
-                // (it's matched by URL), so persist the discovered URL via the URL-keyed writer. See SiteSqlUtils.
-                if (!siteModel.isError && payload.apiRootUrl.isNotEmpty()) {
-                    siteSqlUtils.updateWpApiRestUrlForWPAPISite(siteModel.url, payload.apiRootUrl)
+                // updateSite's full-row write skips WP_API_REST_URL and the credential columns, and this fresh
+                // site has no local id (it's matched by URL), so persist them via the URL-keyed writers.
+                if (!siteModel.isError) {
+                    val username = siteModel.apiRestUsernamePlain
+                    val password = siteModel.apiRestPasswordPlain
+                    if (!username.isNullOrEmpty() && !password.isNullOrEmpty()) {
+                        siteSqlUtils.updateApplicationPasswordCredentialsForWPAPISite(
+                            siteModel.url, username, password
+                        )
+                    }
+                    if (payload.apiRootUrl.isNotEmpty()) {
+                        siteSqlUtils.updateWpApiRestUrlForWPAPISite(siteModel.url, payload.apiRootUrl)
+                    }
                 }
                 result
             } catch (e: Exception) {
@@ -1702,9 +1711,14 @@ open class SiteStore @Inject constructor(
                 siteFromDB
             }
             val rowsAffected = siteSqlUtils.insertOrUpdateSite(siteToStore)
-            // The generic update path no longer writes WP_API_REST_URL (see SiteSqlUtils), so when the
-            // application-password flow discovered a REST URL for an existing site, persist it explicitly.
+            // The credential columns and WP_API_REST_URL are excluded from the generic update path (see
+            // SiteSqlUtils), so persist them through their targeted writers for an existing site.
             if (siteFromDB != null) {
+                val username = siteModel.apiRestUsernamePlain
+                val password = siteModel.apiRestPasswordPlain
+                if (!username.isNullOrEmpty() && !password.isNullOrEmpty()) {
+                    siteSqlUtils.updateApplicationPasswordCredentials(siteFromDB.id, username, password)
+                }
                 siteModel.wpApiRestUrl?.takeIf { it.isNotEmpty() }?.let {
                     siteSqlUtils.updateWpApiRestUrl(siteFromDB.id, it)
                 }
@@ -1741,8 +1755,9 @@ open class SiteStore @Inject constructor(
                     wpApiRestUrl = ""
                 }
                 val rowsAffected = siteSqlUtils.insertOrUpdateSite(siteFromDB)
-                // The generic update path no longer writes WP_API_REST_URL (see SiteSqlUtils), so clear the
-                // stored REST URL explicitly now that the application password backing it is gone.
+                // The credential columns and WP_API_REST_URL are excluded from the generic update path (see
+                // SiteSqlUtils), so clear them explicitly now that the application password is gone.
+                siteSqlUtils.clearApplicationPasswordCredentials(siteFromDB.id)
                 siteSqlUtils.clearWpApiRestUrl(siteFromDB.id)
                 OnSiteChanged(rowsAffected)
             }
@@ -2489,7 +2504,11 @@ open class SiteStore @Inject constructor(
                 apiRestUsernameIV = ""
                 apiRestPasswordIV = ""
             }
-            OnSiteChanged(siteSqlUtils.insertOrUpdateSite(siteFromDB))
+            val rowsAffected = siteSqlUtils.insertOrUpdateSite(siteFromDB)
+            // The credential columns are excluded from the generic update path (see SiteSqlUtils); clear them
+            // explicitly. wpApiRestUrl is intentionally preserved here (see KDoc) — rotation reuses it.
+            siteSqlUtils.clearApplicationPasswordCredentials(siteFromDB.id)
+            OnSiteChanged(rowsAffected)
         } catch (e: DuplicateSiteException) {
             OnSiteChanged(SiteError(DUPLICATE_SITE))
         }
diff --git a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/persistence/SiteSqlUtilsTest.kt b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/persistence/SiteSqlUtilsTest.kt
@@ -140,4 +140,58 @@ class SiteSqlUtilsTest {
         assertThat(rows).isEqualTo(0)
         assertThat(siteSqlUtils.getSites().single().wpApiRestUrl).isNull()
     }
+
+    @Test
+    fun `insertOrUpdateSite update does not clobber credential columns from a stale model`() {
+        WellSql.insert(SiteModel().apply {
+            siteId = 42
+            url = "https://example.test"
+            apiRestUsernameEncrypted = "enc_user"
+            apiRestUsernameIV = "iv_user"
+            apiRestPasswordEncrypted = "enc_pass"
+            apiRestPasswordIV = "iv_pass"
+        }).execute()
+        val localId = storedSite().id
+
+        // A credential-less inbound model (e.g. a /me/sites sync) must not zero the credential columns.
+        val stale = SiteModel().apply {
+            id = localId
+            siteId = 42
+            url = "https://example.test"
+            name = "Updated name"
+        }
+        siteSqlUtils.insertOrUpdateSite(stale)
+
+        val stored = storedSite()
+        assertThat(stored.apiRestUsernameEncrypted).isEqualTo("enc_user")
+        assertThat(stored.apiRestUsernameIV).isEqualTo("iv_user")
+        assertThat(stored.apiRestPasswordEncrypted).isEqualTo("enc_pass")
+        assertThat(stored.apiRestPasswordIV).isEqualTo("iv_pass")
+        assertThat(stored.name).isEqualTo("Updated name") // other columns still update
+    }
+
+    @Test
+    fun `clearApplicationPasswordCredentials empties the credential columns`() {
+        WellSql.insert(SiteModel().apply {
+            url = "https://example.test"
+            apiRestUsernameEncrypted = "enc_user"
+            apiRestUsernameIV = "iv_user"
+            apiRestPasswordEncrypted = "enc_pass"
+            apiRestPasswordIV = "iv_pass"
+        }).execute()
+        val localId = storedSite().id
+
+        val rows = siteSqlUtils.clearApplicationPasswordCredentials(localId)
+
+        assertThat(rows).isEqualTo(1)
+        val stored = storedSite()
+        assertThat(stored.apiRestUsernameEncrypted).isEmpty()
+        assertThat(stored.apiRestUsernameIV).isEmpty()
+        assertThat(stored.apiRestPasswordEncrypted).isEmpty()
+        assertThat(stored.apiRestPasswordIV).isEmpty()
+    }
+
+    // Raw read that bypasses SiteSqlUtils' decryptAPIRestCredentials, so tests can assert on the stored
+    // ciphertext columns directly without invoking the AndroidKeyStore-backed EncryptionUtils.
+    private fun storedSite(): SiteModel = WellSql.select(SiteModel::class.java).asModel.single()
 }
diff --git a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/SiteStoreTest.kt b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/SiteStoreTest.kt
@@ -11,6 +11,7 @@ import org.mockito.junit.MockitoJUnitRunner
 import org.mockito.kotlin.any
 import org.mockito.kotlin.argWhere
 import org.mockito.kotlin.inOrder
+import org.mockito.kotlin.mock
 import org.mockito.kotlin.never
 import org.mockito.kotlin.verify
 import org.mockito.kotlin.verifyNoInteractions
@@ -25,6 +26,7 @@ import org.wordpress.android.fluxc.model.jetpacksocial.JetpackSocialMapper
 import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError
 import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType.NETWORK_ERROR
 import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType.PARSE_ERROR
+import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordsManager
 import org.wordpress.android.fluxc.network.rest.wpapi.site.SiteWPAPIRestClient
 import org.wordpress.android.fluxc.network.rest.wpcom.WPComGsonRequest.WPComGsonNetworkError
 import org.wordpress.android.fluxc.network.rest.wpcom.WPComGsonRequestBuilder.Response
@@ -62,6 +64,7 @@ import org.wordpress.android.fluxc.store.SiteStore.SiteFilter.WPCOM
 import org.wordpress.android.fluxc.store.SiteStore.SiteVisibility.PUBLIC
 import org.wordpress.android.fluxc.test
 import org.wordpress.android.fluxc.tools.initCoroutineEngine
+import javax.inject.Provider
 import kotlin.test.assertEquals
 
 @RunWith(MockitoJUnitRunner::class)
@@ -582,10 +585,15 @@ class SiteStoreTest {
                 "https://example.com",
                 "https://example.com/wp-json/"
             )
+            verify(siteSqlUtils).updateApplicationPasswordCredentialsForWPAPISite(
+                "https://example.com",
+                "appUser",
+                "appPass"
+            )
         }
 
     @Test
-    fun `updateApplicationPassword persists discovered wpApiRestUrl via targeted writer`() {
+    fun `updateApplicationPassword persists credentials and wpApiRestUrl via targeted writers`() {
         val existing = SiteModel().apply {
             id = 3
             url = "https://selfhosted.test"
@@ -601,11 +609,12 @@ class SiteStoreTest {
 
         siteStore.onAction(SiteActionBuilder.newUpdateApplicationPasswordAction(incoming))
 
+        verify(siteSqlUtils).updateApplicationPasswordCredentials(3, "user", "pass")
         verify(siteSqlUtils).updateWpApiRestUrl(3, "https://selfhosted.test/wp-json/")
     }
 
     @Test
-    fun `removeApplicationPassword clears wpApiRestUrl via targeted writer`() {
+    fun `removeApplicationPassword clears credentials and wpApiRestUrl via targeted writers`() {
         val existing = SiteModel().apply {
             id = 4
             url = "https://selfhosted.test"
@@ -618,9 +627,26 @@ class SiteStoreTest {
             SiteActionBuilder.newRemoveApplicationPasswordAction(SiteModel().apply { id = 4 })
         )
 
+        verify(siteSqlUtils).clearApplicationPasswordCredentials(4)
         verify(siteSqlUtils).clearWpApiRestUrl(4)
     }
 
+    @Test
+    fun `clearApplicationPasswordColumns clears credentials but preserves wpApiRestUrl`() {
+        val existing = SiteModel().apply {
+            id = 8
+            url = "https://selfhosted.test"
+        }
+        whenever(siteSqlUtils.getSitesWithLocalId(8)).thenReturn(listOf(existing))
+        whenever(siteSqlUtils.insertOrUpdateSite(any())).thenReturn(1)
+        siteStore.applicationPasswordsManagerProvider = Provider { mock<ApplicationPasswordsManager>() }
+
+        siteStore.deleteStoredApplicationPasswordCredentials(SiteModel().apply { id = 8 })
+
+        verify(siteSqlUtils).clearApplicationPasswordCredentials(8)
+        verify(siteSqlUtils, never()).clearWpApiRestUrl(8)
+    }
+
     @Test
     fun `fetchPostFormats returns empty list for WPAPI site without crashing`() =
         test {
