fix(security): copy account tables by shared columns, not SELECT *

Device upgrade testing surfaced a crash: on a cache last written before v13, account_settings has 4 columns (accountId, signature, signatureEnabled, notificationsEnabled) but the destination table has 6 (retentionCount/retentionMonths were added at v13). The migrator ran `INSERT OR IGNORE INTO account_settings SELECT * FROM cache...`, which supplied 4 values for 6 columns and threw SQLiteException — and because the done-flag is only set after a successful copy, every launch re-ran and re-crashed (crash loop). AccountDataMigrator now copies each table by the column names present in BOTH the freshly-created destination and the (possibly older) source, so columns the source lacks take the destination's defaults instead of overflowing the value list. Verified on-device: the upgrade migrates a pre-v13 install cleanly and the account stays signed in (sync/backfill workers run). Regression test seeds a v12 cache and asserts the copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-02 08:47:05 -05:00
parent 37e6115b45
commit e53a553398
2 changed files with 65 additions and 3 deletions
@@ -187,6 +187,40 @@ class AccountDataMigratorTest {
        }
    }

+    @Test
+    fun copiesFromACacheOlderThanTheCurrentSchema() = runBlocking<Unit> {
+        // A cache last written at v12 — before account_settings gained retentionCount/retentionMonths
+        // (v13). The copy must not choke on the columns the destination has but the source lacks
+        // (a device upgrade from an old install crashed the migrator here).
+        helper.createDatabase(cacheName, 12).apply {
+            execSQL(
+                "INSERT INTO accounts (id, email, displayName, authType, imap_host, imap_port, imap_security, " +
+                    "smtp_host, smtp_port, smtp_security) VALUES ('acct', 'ada@example.org', 'Ada', " +
+                    "'PASSWORD_IMAP', 'imap.example.org', 993, 'SSL_TLS', 'smtp.example.org', 465, 'SSL_TLS')",
+            )
+            execSQL("INSERT INTO credentials (accountId, encryptedSecret) VALUES ('acct', 'sealed-secret')")
+            execSQL(
+                "INSERT INTO account_settings (accountId, signature, signatureEnabled, notificationsEnabled) " +
+                    "VALUES ('acct', 'Sig', 0, 1)",
+            )
+            close()
+        }
+
+        AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile)
+
+        openAccountsDb().apply {
+            assertEquals("ada@example.org", accountDao().getById("acct")?.email)
+            assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret)
+            val settings = accountSettingsDao().get("acct")
+            assertEquals(false, settings?.signatureEnabled)
+            assertEquals(true, settings?.notificationsEnabled)
+            // Columns the v12 source lacked come across as the destination's defaults (null).
+            assertNull("retentionCount absent from a v12 cache must default to null", settings?.retentionCount)
+            assertNull(settings?.retentionMonths)
+            close()
+        }
+    }
+
    @Test
    fun migratorDdlMatchesExportedAccountDatabaseSchema() {
        val schema = JSONObject(
@@ -158,10 +158,15 @@ class AccountDataMigrator @Inject constructor(
                    if (present.isEmpty()) return // fresh cache or already dropped: nothing to move
                    TABLES.forEach { db.rawExecSQL(CREATE_TABLE_SQL.getValue(it)) }
                    db.rawExecSQL(SIGNATURES_INDEX_SQL)
-                    // Parent first so an enforced foreign key (Room enables them; this raw connection
-                    // does not) would still be satisfied. INSERT OR IGNORE makes each copy idempotent.
+                    // Copy by explicit shared column names, never SELECT *: the on-disk cache may predate
+                    // columns the current schema added (e.g. account_settings gained retentionCount /
+                    // retentionMonths at v13), and a bare SELECT * would then supply fewer values than the
+                    // destination has columns and fail the whole migration. Listing the columns the source
+                    // actually has lets the destination's newer columns take their defaults (NULL). Parent
+                    // first so an enforced foreign key would still be satisfied; INSERT OR IGNORE is idempotent.
                    TABLES.filter { it in present }.forEach { table ->
-                        db.rawExecSQL("INSERT OR IGNORE INTO `$table` SELECT * FROM cache.`$table`")
+                        val cols = sharedColumns(db, table)
+                        db.rawExecSQL("INSERT OR IGNORE INTO `$table` ($cols) SELECT $cols FROM cache.`$table`")
                    }
                    Log.d(TAG, "moved account tables into the account database: $present")
                } finally {
@@ -189,5 +194,28 @@ class AccountDataMigrator @Inject constructor(
            }
            return present
        }
+
+        /**
+         * Column names present in BOTH the freshly-created destination `$table` (always the current
+         * schema) and the source `cache.$table` (possibly an older on-disk schema), backtick-quoted and
+         * comma-joined for an INSERT/SELECT column list. Destination-only columns are omitted so they
+         * take their defaults instead of overflowing the value list.
+         */
+        private fun sharedColumns(db: SQLiteDatabase, table: String): String {
+            val source = tableColumns(db, "cache", table)
+            return tableColumns(db, "main", table)
+                .filter { it in source }
+                .joinToString(", ") { "`$it`" }
+        }
+
+        /** The column names of `$schema.$table`, in declared order, via `PRAGMA table_info`. */
+        private fun tableColumns(db: SQLiteDatabase, schema: String, table: String): List<String> {
+            val columns = mutableListOf<String>()
+            db.rawQuery("PRAGMA $schema.table_info(`$table`)", null).use { cursor ->
+                val nameIndex = cursor.getColumnIndexOrThrow("name")
+                while (cursor.moveToNext()) columns += cursor.getString(nameIndex)
+            }
+            return columns
+        }
    }
 }