fix(contacts): do not let one broken contact blank the whole app#5564
Open
MiMoHo wants to merge 1 commit into
Open
fix(contacts): do not let one broken contact blank the whole app#5564MiMoHo wants to merge 1 commit into
MiMoHo wants to merge 1 commit into
Conversation
A single contact with a malformed property (e.g. a compact REV:20230911 as written by Thunderbird CardBook or DAVx5, or a value-typed UID) could take down the entire web UI: the throwing ical.js getter crashed the sortContacts mutation for all contacts, and the surrounding catch in getContactsFromAddressBook then removed every address book from the store, leaving the user with 'no address books' and no way to recover. - read the raw jCal value in the uid getter so uid/key access never throws for value-typed UID properties - skip contacts whose property getters throw while sorting instead of failing the whole contacts list - only remove an address book from the store when the DAV fetch itself fails, not when processing the fetched contacts fails Resolves nextcloud#5149 Helps with nextcloud#5250 (making the last-modified sort order work with malformed REV values is handled separately in nextcloud#5265) Assisted-by: Claude:claude-fable-5 Signed-off-by: MiMoHo <37556964+MiMoHo@users.noreply.github.com>
MiMoHo
requested review from
GVodyanov,
SebastianKrupinski and
hamza221
as code owners
July 12, 2026 17:27
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Summary
A single contact with one malformed property could take down the entire web UI, leaving the user with "no address books" and no way to recover from within the app. This PR fortifies the contact loading pipeline so that one broken contact can, at worst, only affect itself.
Failure mechanism (as diagnosed in #5149 and #5250)
Contactcan throw for malformed/exotic property values. Two confirmed classes:UIDproperties (e.g.UID;VALUE=DATE-TIME:…, diagnosed by @seblu in Parse errors in a contact make the entire app UI fail and show no contacts at all #5149):getFirstPropertyValue()returns a parsed object instead of a string, and its stringification inside thekeygetter can throw,REV:20230911as written by Thunderbird CardBook or DAVx5 (Contacts web app unusable, possible after adding a contact with a not expected date field — TypeError: can't access property "getAllSubcomponents", this.component is null in sortContacts #5250): with ical.js 2.2.1, evengetFirstPropertyValue('rev')itself throwsError: invalid date-time value: "2023-09-11T::"(reproducible with a plain node script against the bundled ical.js).sortContactsmutation, which iterates all contacts of all address books..catch()ofgetContactsFromAddressBook, which was written for DAV fetch errors and responds by deleting the address book from the store.sortContactsis global, the same broken contact makes the commit fail for every address book being loaded → all address books get removed from the store → the app shows no address books and no contacts at all, and even the settings for deleting/disabling the offending address book become unreachable.Changes
Contact.uidnow reads the raw jCal value (always a plain string) instead of the parsed value object, souid/keyaccess never throws. Identity access must be infallible —keyis used as store index everywhere.sortContactsmutation skips (and logs) contacts whose property getters throw, instead of failing the whole list.getContactsFromAddressBookonly removes the address book from the store when the DAV fetch itself fails. If processing the successfully fetched contacts fails, the address book stays visible and an error toast is shown — the data on the server is fine.Relationship to other work
Complementary to #5265, which makes the last modified sort order actually work with malformed
REVvalues (numeric sort keys read from raw jCal). This PR is about containment: no single contact may blank the app, whatever the sort order. Together they fully cover #5250.Checklist
uid/keyrobustness for the exact vCard diagnosed in Parse errors in a contact make the entire app UI fail and show no contacts at all #5149sortContactswith a realREV:20230911contact (Contacts web app unusable, possible after adding a contact with a not expected date field — TypeError: can't access property "getAllSubcomponents", this.component is null in sortContacts #5250 scenario): list survives, broken contact skipped; with a sort key that does not touch the broken property, the contact stays listedgetContactsFromAddressBook: address book removed on fetch failure, kept on processing failure, unparseable vCards skipped with toastnpm run lintclean for the touched files (0 errors), production build succeedsResolve #5149
Helps with #5250 (together with #5265)
🤖 Generated with Claude Code