Fix "Device" contacts vulnerable to deletion via unrelated account changes (#477) - #495
Fix "Device" contacts vulnerable to deletion via unrelated account changes (#477)#495Adam-Winwood wants to merge 2 commits into
Conversation
DeviceContactsRepository created local contacts and groups with account_type="com.android.contacts", account_name="DEVICE" - AccountType.androidDefault. This string pair has never corresponded to a real registered AccountManager account. Android's ContactsProvider2 reconciles raw_contacts/data/groups rows against the current real account list on every account-list change (any account, any type, added or removed anywhere on the device) and deletes rows whose account no longer exists. Since "DEVICE" never existed, every "Device" contact ConnectYou has ever created was permanently exposed to this cleanup. Confirmed on emulator (repeatable, both add and remove of an unrelated throwaway account) and on my actual phone. Fix: create local contacts/groups with account_type=null, account_name=null instead - Android's (AOSP anyway) actual documented convention for local/unsynced contacts (RawContacts.getLocalAccountType()/ getLocalAccountName() resolve to null on most devices). Verified via the real createContact() path that a null-account contact survives the same account add/remove cycles that reliably wiped the old fake-account ones, checked over a full 2-minute window each direction.
The previous commit stopped new "Device" contacts/groups from being created under the fake account_type=AccountType.androidDefault.type, account_name=AccountType.androidDefault.name pair. Contacts already created that way on existing installs were still exposed to the bug and needed moving to the real null/null local-contact convention too. Handles the case where the pair might legitimately be a real account on some device rather than our fake one: checks AccountManager for a real account matching that exact identity first, and skips migration entirely if one exists, since individual raw contacts can't be distinguished by creating app via public API. This fails safe either way - if the pair is genuinely fake (the normal case), migrating is a strict improvement; if a real account exists, nothing happens at all, so there's no action for a backup to protect against. Runs once from App.onCreate(), gated by a SharedPreferences flag that's only set once a migration attempt actually completes (not on a missing-permission or ambiguous-collision skip), so either condition naturally retries on a later launch once resolved. Verified all three paths on an emulator: normal migration (fake contacts get nulled, flag set), collision present (contacts untouched, flag stays unset), and recovery after the collision resolves (previously-protected contacts correctly migrated on the next launch).
There was a problem hiding this comment.
Thanks for the pull request!
I couldn't reproduce the contact deletion bug on my end by changing accounts, but makes sense that Android cleans up all accounts that don't exist. I've looked at the Fossify Contacts app and it also just stores a null value for account name and type.
Please clarify if and how you used AI to work on this pull request. Fyi, AI contributions are not allowed on this repository.
| // makes ContactsProvider2 treat these raw contacts as belonging to an account | ||
| // that doesn't exist, so any unrelated account being added/removed anywhere on | ||
| // the device wipes them. null/null is Android's actual local-contact convention. | ||
| val isLocalAccount = resolvedAccountType == AccountType.androidDefault.type |
There was a problem hiding this comment.
Using AccountType.androidDefault here is not really the cleanest solution, instead we should remove AccountType.androidDefault and just always pass null around as the account name and type. So the idea would be to modify setLastChosenAccount and getLastChosenAccount to also handle null values. Then we don't have to do this case distinction here and can just run getCreateAction(resolvedAccountType, resolvedAccountName).
| return@withContext | ||
| } | ||
|
|
||
| val sentinelType = AccountType.androidDefault.type |
There was a problem hiding this comment.
Why do you call this sentinel, it's not really a sentinel?
| } | ||
| if (realAccountExists) return@withContext | ||
|
|
||
| runCatching { |
There was a problem hiding this comment.
Why the runCatching? In theory this should never fail/raise an exception, or in which case should it?
AI clarificationMany apologies, yes I have used AI in the writing of this. If I had known, I would not have used it on this repo. Specifically, I have used AI for first drafts of commit messages (lazy yes I know), and the prototyping of code. Perhaps 40% of the writing, and 80% of the code could be attributed to AI. Whilst I am happy to have a stab at redoing it all fresh and hand writing the code etc. (likely as a new PR), I understand if you would prefer I'm not involved in fixing this particular bug at this point. Reproduction of the issueI am surprised to hear that you haven't managed to reproduce the bug on your end - did you add a contact under DEVICE specifically, and add/remove an account? On my actual device I just deleted a couple of old unused accounts from Settings -> "Accounts and backup" ->"Manage accounts". Whilst on the emulator I added a bit of debug code that I could poke with Reproducing on my actual device is sad as it vaporises all my contacts and I have to re-import them from backups I make whenever I add new contacts, hence my focus on reproducing this on the emulator. SummaryAnyway, let me know what you want me to do (or not do) now. |
I just removed an account I no longer used. But possibly this just varies depending on the ROM / Android version, so it's not an issue that I can't reproduce it.
Yes, I think that creating a new pull request from the ground up would be cool. We should probably fully remove |
2 participants
Root cause: "Device" contacts were created with a fake account_type/account_name pair that never corresponds to a real registered account, making them vulnerable to Android's ContactsProvider2 wiping them whenever any account is added or removed anywhere on the device - unrelated to ConnectYou or even to contacts syncing.
Fix: create local contacts/groups with null/null instead — Android's actual documented local-contact convention.
Second commit: one-time migration for contacts already affected on existing installs, with a safe-fail check against the (extremely unlikely) case where that account pair happens to be a real registered account on some device.
Fixes #477
Fixes #275