fix(contacts): do not let one broken contact blank the whole app

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])
 	}
 
 	/**

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,
+			}))
+		}
 	},
 
 	/**

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)
 	},
 

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

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

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