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()) + }) + +})