From 8cb2bbead3ff20abeb0e67e6d5d8683a8fa2336b Mon Sep 17 00:00:00 2001 From: MiMoHo <37556964+MiMoHo@users.noreply.github.com> Date: Sun, 12 Jul 2026 19:21:25 +0200 Subject: [PATCH] fix(contacts): do not let one broken contact blank the whole app 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 #5149 Helps with #5250 (making the last-modified sort order work with malformed REV values is handled separately in #5265) Assisted-by: Claude:claude-fable-5 Signed-off-by: MiMoHo <37556964+MiMoHo@users.noreply.github.com> --- src/models/contact.js | 10 +- src/store/addressbooks.js | 104 ++++++++++-------- src/store/contacts.js | 24 +++- tests/javascript/models/contact.test.js | 33 ++++++ .../store/addressbooksActions.test.js | 89 +++++++++++++++ tests/javascript/store/contacts.test.js | 70 ++++++++++++ 6 files changed, 277 insertions(+), 53 deletions(-) create mode 100644 tests/javascript/store/addressbooksActions.test.js create mode 100644 tests/javascript/store/contacts.test.js diff --git a/src/models/contact.js b/src/models/contact.js index 325b641c8f..44806b1616 100644 --- a/src/models/contact.js +++ b/src/models/contact.js @@ -171,7 +171,15 @@ export default class Contact { * @memberof Contact */ get uid() { - return this.vCard.getFirstPropertyValue('uid') + const uid = this.vCard.getFirstProperty('uid') + if (!uid) { + return null + } + // Read the raw jCal value, which is always a plain string. + // getFirstPropertyValue() returns a parsed object for value-typed + // properties (e.g. UID;VALUE=DATE-TIME) whose stringification can + // throw and break every consumer of uid and key (see issue #5149) + return String(uid.jCal[3]) } /** diff --git a/src/store/addressbooks.js b/src/store/addressbooks.js index f9600ca083..9e55e44f94 100644 --- a/src/store/addressbooks.js +++ b/src/store/addressbooks.js @@ -375,54 +375,66 @@ const actions = { * @return {Promise} */ async getContactsFromAddressBook(context, { addressbook }) { - return addressbook.dav - .findAllAndFilterBySimpleProperties(MinimalContactProperties) - .then((response) => { - // We don't want to lose the url information - // so we need to parse one by one - let failed = 0 - const contacts = response - .reduce((contacts, item) => { - try { - const contact = new Contact(item.data, addressbook) - contact.dav = item - contacts.push(contact) - } catch (error) { - // PARSING FAILED - console.error('Error reading contact', item.url, item.data) - console.error(error) - failed++ - } - return contacts - }, []) - - if (failed > 0) { - showError(n( - 'contacts', - '{failed} contact failed to be read', - '{failed} contacts failed to be read', - failed, - { failed }, - )) - } + let response + try { + response = await addressbook.dav + .findAllAndFilterBySimpleProperties(MinimalContactProperties) + } catch (error) { + // unrecoverable error, if no contacts were loaded, + // remove the addressbook + // TODO: create a failed addressbook state and show that there was an issue? + context.commit('deleteAddressbook', addressbook) + console.error(error) + return + } - context.commit('appendContactsToAddressbook', { addressbook, contacts }) - context.commit('extractGroupsFromContacts', contacts) + try { + // We don't want to lose the url information + // so we need to parse one by one + let failed = 0 + const contacts = response + .reduce((contacts, item) => { + try { + const contact = new Contact(item.data, addressbook) + contact.dav = item + contacts.push(contact) + } catch (error) { + // PARSING FAILED + console.error('Error reading contact', item.url, item.data) + console.error(error) + failed++ + } + return contacts + }, []) + + if (failed > 0) { + showError(n( + 'contacts', + '{failed} contact failed to be read', + '{failed} contacts failed to be read', + failed, + { failed }, + )) + } - // don't add contacts from disabled address book to contacts store - if (addressbook.enabled) { - context.commit('appendContacts', contacts) - context.commit('sortContacts') - } - return contacts - }) - .catch((error) => { - // unrecoverable error, if no contacts were loaded, - // remove the addressbook - // TODO: create a failed addressbook state and show that there was an issue? - context.commit('deleteAddressbook', addressbook) - console.error(error) - }) + context.commit('appendContactsToAddressbook', { addressbook, contacts }) + context.commit('extractGroupsFromContacts', contacts) + + // don't add contacts from disabled address book to contacts store + if (addressbook.enabled) { + context.commit('appendContacts', contacts) + context.commit('sortContacts') + } + return contacts + } catch (error) { + // The contacts were fetched, so the addressbook itself is fine: + // keep it in the store instead of hiding it from the user + // (see issues #5149, #5250) + console.error('Error processing the contacts of the following addressbook', addressbook.id, error) + showError(t('contacts', 'Errors occurred while processing the contacts of {addressbook}', { + addressbook: addressbook.displayName, + })) + } }, /** diff --git a/src/store/contacts.js b/src/store/contacts.js index 26fc8597a2..93b905d386 100644 --- a/src/store/contacts.js +++ b/src/store/contacts.js @@ -283,12 +283,24 @@ const mutations = { */ sortContacts(state) { state.sortedContacts = Object.values(state.contacts) - .filter((contact) => contact.kind !== 'group') - .map((contact) => ({ - key: contact.key, - value: contact[state.orderKey], - favorite: contact.favorite || false, - })) + .reduce((sortedContacts, contact) => { + // ical.js getters can throw on malformed property values, + // e.g. a compact `REV:20230911` when sorting by last modified. + // Skip the broken contact instead of blanking the whole list + // (see issues #5149, #5250) + try { + if (contact.kind !== 'group') { + sortedContacts.push({ + key: contact.key, + value: contact[state.orderKey], + favorite: contact.favorite || false, + }) + } + } catch (error) { + console.error('Could not sort the following contact, skipping', contact, error) + } + return sortedContacts + }, []) .sort(sortByFavoriteAndName) }, diff --git a/tests/javascript/models/contact.test.js b/tests/javascript/models/contact.test.js index c4c429e46a..3abf7cd3a2 100644 --- a/tests/javascript/models/contact.test.js +++ b/tests/javascript/models/contact.test.js @@ -9,6 +9,39 @@ const getPropertyLines = (property, vcard) => { return vcard.match(new RegExp(`^${property}[;:].*`, 'gmi')) } +describe('Test uid getter robustness', () => { + + test('uid is returned as a plain string for a regular UID', () => { + const contact = new Contact(` + BEGIN:VCARD + VERSION:3.0 + UID:123456789-123465-123456-123456789 + FN:Test contact + END:VCARD`.replace(/\t/gmi, ''), + { id: 'addressbook1' }) + + expect(contact.uid).toStrictEqual('123456789-123465-123456-123456789') + expect(typeof contact.key).toStrictEqual('string') + }) + + test('uid does not throw for a value-typed UID (issue #5149)', () => { + // vCard diagnosed in https://github.com/nextcloud/contacts/issues/5149 + const contact = new Contact(` + BEGIN:VCARD + VERSION:3.0 + FN:Foo + N:Bar;Baz;;; + UID;VALUE=DATE-TIME:20260203T091857Z + REV:20260203T091742Z + END:VCARD`.replace(/\t/gmi, ''), + { id: 'addressbook1' }) + + expect(typeof contact.uid).toStrictEqual('string') + expect(typeof contact.key).toStrictEqual('string') + }) + +}) + describe('Test stripping quotes from TYPE', () => { let contact diff --git a/tests/javascript/store/addressbooksActions.test.js b/tests/javascript/store/addressbooksActions.test.js new file mode 100644 index 0000000000..445865d860 --- /dev/null +++ b/tests/javascript/store/addressbooksActions.test.js @@ -0,0 +1,89 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { showError } from '@nextcloud/dialogs' +import addressbooksStore from '../../../src/store/addressbooks.js' + +jest.mock('@nextcloud/dialogs', () => ({ + showError: jest.fn(), +})) + +// break the circular import chain +// addressbooks.js -> models/contact.js -> store/index.js -> addressbooks.js +jest.mock('../../../src/store/index.js', () => ({ getters: {} })) + +const { actions } = addressbooksStore + +const VALID_VCARD = 'BEGIN:VCARD\r\nVERSION:4.0\r\nUID:valid-contact\r\nFN:Valid contact\r\nEND:VCARD' + +const buildAddressbook = (response) => ({ + id: 'addressbook1', + displayName: 'Addressbook 1', + enabled: true, + dav: { + findAllAndFilterBySimpleProperties: jest.fn() + .mockImplementation(() => response instanceof Error + ? Promise.reject(response) + : Promise.resolve(response)), + }, +}) + +describe('getContactsFromAddressBook action', () => { + + let context + + beforeEach(() => { + jest.clearAllMocks() + context = { commit: jest.fn() } + }) + + test('parses the fetched contacts and commits them to the store', async () => { + const addressbook = buildAddressbook([{ url: '/valid', data: VALID_VCARD }]) + + const contacts = await actions.getContactsFromAddressBook(context, { addressbook }) + + expect(contacts).toHaveLength(1) + expect(context.commit).toHaveBeenCalledWith('appendContactsToAddressbook', { addressbook, contacts }) + expect(context.commit).toHaveBeenCalledWith('sortContacts') + expect(context.commit).not.toHaveBeenCalledWith('deleteAddressbook', addressbook) + }) + + test('removes the addressbook from the store when the DAV fetch fails', async () => { + const addressbook = buildAddressbook(new Error('network error')) + + await actions.getContactsFromAddressBook(context, { addressbook }) + + expect(context.commit).toHaveBeenCalledWith('deleteAddressbook', addressbook) + }) + + test('keeps the addressbook when processing the contacts fails (issues #5149, #5250)', async () => { + const addressbook = buildAddressbook([{ url: '/valid', data: VALID_VCARD }]) + context.commit.mockImplementation((mutation) => { + if (mutation === 'sortContacts') { + throw new Error('a broken contact crashed a store mutation') + } + }) + + await expect(actions.getContactsFromAddressBook(context, { addressbook })) + .resolves.toBeUndefined() + + expect(context.commit).not.toHaveBeenCalledWith('deleteAddressbook', addressbook) + expect(showError).toHaveBeenCalled() + }) + + test('skips unparseable contacts and shows an error toast', async () => { + const addressbook = buildAddressbook([ + { url: '/valid', data: VALID_VCARD }, + { url: '/invalid', data: 'BEGIN:VCALENDAR\r\nEND:VCALENDAR' }, + ]) + + const contacts = await actions.getContactsFromAddressBook(context, { addressbook }) + + expect(contacts).toHaveLength(1) + expect(showError).toHaveBeenCalled() + expect(context.commit).not.toHaveBeenCalledWith('deleteAddressbook', addressbook) + }) + +}) diff --git a/tests/javascript/store/contacts.test.js b/tests/javascript/store/contacts.test.js new file mode 100644 index 0000000000..f69b0a2500 --- /dev/null +++ b/tests/javascript/store/contacts.test.js @@ -0,0 +1,70 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import Contact from '../../../src/models/contact.js' +import contactsStore from '../../../src/store/contacts.js' + +const { mutations } = contactsStore + +const buildContact = (vcard) => new Contact(vcard.replace(/\t/gmi, ''), { id: 'addressbook1' }) + +describe('sortContacts mutation', () => { + + let goodContact + let brokenRevContact + + beforeEach(() => { + goodContact = buildContact(` + BEGIN:VCARD + VERSION:4.0 + UID:good-contact + FN:Good contact + REV:20230911T123456Z + END:VCARD`) + + // compact REV as written by e.g. Thunderbird CardBook or DAVx5: + // ical.js throws on any parsed-value access of this property + brokenRevContact = buildContact(` + BEGIN:VCARD + VERSION:4.0 + UID:broken-rev-contact + FN:Broken rev contact + REV:20230911 + END:VCARD`) + }) + + test('one broken contact does not blank the whole list (issue #5250)', () => { + const state = { + contacts: { + [goodContact.key]: goodContact, + [brokenRevContact.key]: brokenRevContact, + }, + sortedContacts: [], + orderKey: 'rev', + } + + expect(() => mutations.sortContacts(state)).not.toThrow() + + expect(state.sortedContacts.map((contact) => contact.key)) + .toStrictEqual([goodContact.key]) + }) + + test('a broken property not used for sorting keeps the contact listed', () => { + const state = { + contacts: { + [goodContact.key]: goodContact, + [brokenRevContact.key]: brokenRevContact, + }, + sortedContacts: [], + orderKey: 'displayName', + } + + mutations.sortContacts(state) + + expect(state.sortedContacts.map((contact) => contact.key).sort()) + .toStrictEqual([brokenRevContact.key, goodContact.key].sort()) + }) + +})