From 73d971dbf96582c953e2c4a1b7d3eddfccc896be Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:37:16 +0200 Subject: [PATCH] fix: save contacts with calendar links and cleared fields correctly Stalwart rejects a whole ContactCard over one property it does not know, and several reached it: - the form sent `calendarUri`, `schedulingUri` and `freeBusyUri`, so any contact with a calendar, scheduling or free/busy link failed to save ("Invalid property.") - vCard import sent `source`, and Pro mode left `localAccountId` on the card - vCard import wrote addresses as flat `street`/`locality` fields instead of RFC 9553 `components` The card now passes through `lib/jmap/contact-wire.ts` on its way to the server: client-only fields are dropped, the links are written as `calendars` / `schedulingAddresses` (and SOURCE as a `directories` entry), flat addresses become components, and reads map the links back for the form and detail view. Clearing a field (nickname, notes, a link) never reached the server: the form set it to `undefined`, which JSON drops, and an omitted property keeps its value. Updates now send `null` for cleared keys. Deleting an address book that still held contacts always failed with `addressBookHasContents`. The delete now passes `onDestroyRemoveContents`, which the confirm dialog already promised. --- lib/__tests__/contact-wire.test.ts | 77 ++++++++++++++++++++ lib/jmap/client-interface.ts | 2 +- lib/jmap/client.ts | 31 ++++++--- lib/jmap/contact-wire.ts | 108 +++++++++++++++++++++++++++++ lib/jmap/types.ts | 5 ++ stores/contact-store.ts | 3 +- 6 files changed, 213 insertions(+), 13 deletions(-) create mode 100644 lib/__tests__/contact-wire.test.ts create mode 100644 lib/jmap/contact-wire.ts diff --git a/lib/__tests__/contact-wire.test.ts b/lib/__tests__/contact-wire.test.ts new file mode 100644 index 000000000..adf0da69d --- /dev/null +++ b/lib/__tests__/contact-wire.test.ts @@ -0,0 +1,77 @@ +import { describe, it, expect } from 'vitest'; +import { contactFromWire, contactToWire } from '../jmap/contact-wire'; +import type { ContactCard } from '../jmap/types'; + +describe('contactToWire', () => { + it('drops client-only fields, which ContactCard/set rejects', () => { + const wire = contactToWire({ + name: { full: 'A' }, + originalId: 'x', accountId: 'a', accountName: 'n', isShared: true, localAccountId: 'l', + }, 'create'); + expect(wire).toEqual({ name: { full: 'A' } }); + }); + + it('writes the calendar URIs as RFC 9553 calendars and schedulingAddresses', () => { + const wire = contactToWire({ + calendarUri: 'https://x.test/cal', + freeBusyUri: 'https://x.test/fb', + schedulingUri: 'mailto:a@x.test', + }, 'create'); + expect(wire).toEqual({ + calendars: { + cal: { '@type': 'Calendar', kind: 'calendar', uri: 'https://x.test/cal' }, + fb: { '@type': 'Calendar', kind: 'freeBusy', uri: 'https://x.test/fb' }, + }, + schedulingAddresses: { sched: { '@type': 'SchedulingAddress', uri: 'mailto:a@x.test' } }, + }); + }); + + it('sends null for cleared fields on update, but omits them on create', () => { + const cleared: Partial = { nicknames: undefined, calendarUri: undefined, schedulingUri: undefined }; + expect(contactToWire(cleared, 'update')).toEqual({ nicknames: null, calendars: null, schedulingAddresses: null }); + expect(contactToWire(cleared, 'create')).toEqual({}); + }); + + it('leaves calendar links alone when the update does not mention them', () => { + expect(contactToWire({ keywords: { vip: true } }, 'update')).toEqual({ keywords: { vip: true } }); + }); + + it('converts flat vCard addresses to components and SOURCE to a directory entry', () => { + const wire = contactToWire({ + addresses: { a0: { street: 'Main St 1', locality: 'Town', country: 'DE', fullAddress: 'Main St 1, Town', contexts: { work: true } } }, + source: 'https://x.test/card.vcf', + }, 'create'); + expect(wire.addresses).toEqual({ + a0: { + components: [ + { kind: 'name', value: 'Main St 1' }, + { kind: 'locality', value: 'Town' }, + { kind: 'country', value: 'DE' }, + ], + isOrdered: true, + defaultSeparator: ', ', + full: 'Main St 1, Town', + contexts: { work: true }, + }, + }); + expect(wire.directories).toEqual({ source: { '@type': 'Directory', kind: 'entry', uri: 'https://x.test/card.vcf' } }); + }); +}); + +describe('contactFromWire', () => { + it('fills the flat URI fields from the server card', () => { + const card = contactFromWire({ + id: 'c1', + addressBookIds: {}, + calendars: { k: { kind: 'calendar', uri: 'https://x.test/cal' }, f: { kind: 'freeBusy', uri: 'https://x.test/fb' } }, + schedulingAddresses: { s: { uri: 'mailto:a@x.test' } }, + directories: { d: { kind: 'entry', uri: 'https://x.test/card.vcf' } }, + }); + expect(card).toMatchObject({ + calendarUri: 'https://x.test/cal', + freeBusyUri: 'https://x.test/fb', + schedulingUri: 'mailto:a@x.test', + source: 'https://x.test/card.vcf', + }); + }); +}); diff --git a/lib/jmap/client-interface.ts b/lib/jmap/client-interface.ts index 2e009d8d9..1966d122f 100644 --- a/lib/jmap/client-interface.ts +++ b/lib/jmap/client-interface.ts @@ -391,7 +391,7 @@ export interface IJMAPClient { createAddressBook(name: string): Promise; updateAddressBook(addressBookId: string, updates: Partial, targetAccountId?: string): Promise; setDefaultAddressBook(addressBookId: string, targetAccountId?: string): Promise; - deleteAddressBook(addressBookId: string, targetAccountId?: string): Promise; + deleteAddressBook(addressBookId: string, targetAccountId?: string, options?: { removeContents?: boolean }): Promise; getContacts(addressBookId?: string, options?: { throwOnError?: boolean }): Promise; getAllContacts(): Promise; getContact(contactId: string, accountId?: string): Promise; diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index b23b2ef1e..4c8d13724 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -14,6 +14,7 @@ import { findTasksOnlyCalendarIds, isTaskLikeObject, type ScannedCalendarObject import { DEFAULT_CALENDAR_COMPONENTS, mkCalendarCollection, newCalendarCollectionName } from "@/lib/webdav/calendar-collection"; import { sanitizeDisplayName, splitMailbox } from "@/lib/rfc5322-mailbox"; import { decodeFileNodeName } from "./filenode-name"; +import { contactFromWire, contactToWire } from "./contact-wire"; import { getEffectiveTimeZone } from "@/lib/timezone"; import { buildEmailSort, compareEmails, hasKeywordLevels, type KeywordSortPolarity, type SortLevel } from "@/lib/message-list-order"; @@ -5310,10 +5311,20 @@ export class JMAPClient implements IJMAPClient { } } - async deleteAddressBook(addressBookId: string, targetAccountId?: string): Promise { + async deleteAddressBook( + addressBookId: string, + targetAccountId?: string, + options?: { removeContents?: boolean }, + ): Promise { const accountId = targetAccountId || this.getContactsAccountId(); + // Without onDestroyRemoveContents a book that still holds cards is + // refused with `addressBookHasContents` (RFC 9610 §2.3). const response = await this.request([ - ["AddressBook/set", { accountId, destroy: [addressBookId] }, "0"], + ["AddressBook/set", { + accountId, + destroy: [addressBookId], + ...(options?.removeContents ? { onDestroyRemoveContents: true } : {}), + }, "0"], ], this.contactUsing()); const result = response.methodResponses?.[0]?.[1]; @@ -5675,7 +5686,7 @@ export class JMAPClient implements IJMAPClient { if (response.methodResponses?.[0]?.[0] === "ContactCard/get") { const list = (response.methodResponses[0][1].list || []) as ContactCard[]; - allContacts.push(...list); + allContacts.push(...list.map(contactFromWire)); } } @@ -5741,8 +5752,8 @@ export class JMAPClient implements IJMAPClient { ], this.contactUsing()); if (response.methodResponses?.[0]?.[0] === "ContactCard/get") { - const list = response.methodResponses[0][1].list || []; - return list[0] || null; + const list = (response.methodResponses[0][1].list || []) as ContactCard[]; + return list[0] ? contactFromWire(list[0]) : null; } return null; } catch (error) { @@ -5762,8 +5773,7 @@ export class JMAPClient implements IJMAPClient { } } - // Strip shared-only fields before sending to JMAP - const { originalId: _oid, accountId: _aid, accountName: _an, isShared: _is, ...contactData } = contact as ContactCard; + const contactData = contactToWire(contact, 'create'); const response = await this.request([ ["ContactCard/set", { @@ -5772,7 +5782,7 @@ export class JMAPClient implements IJMAPClient { "new-contact": { ...contactData, // Stalwart stores the card without one if omitted (#644) - uid: contactData.uid || `urn:uuid:${generateUUID()}`, + uid: contact.uid || `urn:uuid:${generateUUID()}`, addressBookIds, } } @@ -5800,8 +5810,7 @@ export class JMAPClient implements IJMAPClient { async updateContact(contactId: string, updates: Partial, targetAccountId?: string): Promise { const accountId = targetAccountId || this.getContactsAccountId(); - // Strip shared-only fields before sending to JMAP - const { originalId: _oid, accountId: _aid, accountName: _an, isShared: _is, ...cleanUpdates } = updates as ContactCard; + const cleanUpdates = contactToWire(updates, 'update'); const response = await this.request([ ["ContactCard/set", { @@ -5873,7 +5882,7 @@ export class JMAPClient implements IJMAPClient { if (response.methodResponses?.[1]?.[0] === "ContactCard/get") { const rawContacts = (response.methodResponses[1][1].list || []) as ContactCard[]; - const contacts = rawContacts.map((contact) => ({ + const contacts = rawContacts.map(contactFromWire).map((contact) => ({ ...contact, id: isPrimary ? contact.id : `${accountId}:${contact.id}`, originalId: contact.id, diff --git a/lib/jmap/contact-wire.ts b/lib/jmap/contact-wire.ts new file mode 100644 index 000000000..578e013a7 --- /dev/null +++ b/lib/jmap/contact-wire.ts @@ -0,0 +1,108 @@ +import type { AddressComponent, ContactAddress, ContactCard } from './types'; + +/** + * Translation between the ContactCard shape the UI works with and the RFC 9553 + * card the server stores. + * + * ContactCard/set rejects a whole card over a single property it does not + * know ("Invalid property."), so client-side fields must never reach it, and + * the UI's flat conveniences are written as their JSContact equivalents. + */ + +/** Fields the client adds to cards it lists; not part of RFC 9553. */ +const CLIENT_ONLY_KEYS = [ + 'originalId', 'accountId', 'accountName', 'isShared', 'localAccountId', +] as const; + +/** vCard CALURI / FBURL / CALADRURI, kept flat in the UI. */ +const URI_KEYS = ['calendarUri', 'freeBusyUri', 'schedulingUri', 'source'] as const; + +const LEGACY_ADDRESS_FIELDS: Array<[keyof ContactAddress, AddressComponent['kind']]> = [ + ['street', 'name'], + ['locality', 'locality'], + ['region', 'region'], + ['postcode', 'postcode'], + ['country', 'country'], +]; + +/** vCard imports store flat fields; RFC 9553 wants `components`. */ +function addressToWire(address: ContactAddress): Record { + const { street: _s, locality: _l, region: _r, postcode: _p, country: _c, fullAddress, ...rest } = address; + const out: Record = { ...rest }; + if (!address.components?.length) { + const components = LEGACY_ADDRESS_FIELDS + .map(([field, kind]) => ({ kind, value: (address[field] as string | undefined)?.trim() ?? '' })) + .filter(c => c.value); + if (components.length) { + out.components = components; + out.isOrdered = true; + out.defaultSeparator = ', '; + } + } + if (fullAddress && !address.full) out.full = fullAddress; + return out; +} + +type Mode = 'create' | 'update'; + +/** + * The card as ContactCard/set expects it. On `update`, a key that is present + * with the value `undefined` means "the user cleared it" and is sent as + * `null`: JSON drops undefined, and an omitted property keeps its old value. + */ +export function contactToWire(card: Partial, mode: Mode): Record { + const out: Record = {}; + for (const [key, value] of Object.entries(card)) { + if ((CLIENT_ONLY_KEYS as readonly string[]).includes(key)) continue; + if ((URI_KEYS as readonly string[]).includes(key)) continue; + if (value === undefined) { + if (mode === 'update') out[key] = null; + continue; + } + out[key] = value; + } + + if (card.addresses) { + out.addresses = Object.fromEntries( + Object.entries(card.addresses).map(([id, a]) => [id, addressToWire(a)]), + ); + } + + // Rebuild the calendar links only when the caller manages them, so an + // update that does not mention them leaves the server's entries alone. + if ('calendarUri' in card || 'freeBusyUri' in card) { + const calendars: Record = {}; + if (card.calendarUri) calendars.cal = { '@type': 'Calendar', kind: 'calendar', uri: card.calendarUri }; + if (card.freeBusyUri) calendars.fb = { '@type': 'Calendar', kind: 'freeBusy', uri: card.freeBusyUri }; + if (Object.keys(calendars).length) out.calendars = calendars; + else if (mode === 'update') out.calendars = null; + } + if ('schedulingUri' in card) { + if (card.schedulingUri) out.schedulingAddresses = { sched: { '@type': 'SchedulingAddress', uri: card.schedulingUri } }; + else if (mode === 'update') out.schedulingAddresses = null; + } + if (card.source) { + // vCard SOURCE is a JSContact directory entry (RFC 9553 §2.6.2). + const directories = { ...(card.directories ?? {}) } as Record; + directories.source = { '@type': 'Directory', kind: 'entry', uri: card.source }; + out.directories = directories; + } + return out; +} + +/** Fill the UI's flat URI fields from the RFC 9553 properties. */ +export function contactFromWire(card: T): T { + const calendars = Object.values(card.calendars ?? {}); + const calendarUri = card.calendarUri ?? calendars.find(c => c?.kind === 'calendar')?.uri; + const freeBusyUri = card.freeBusyUri ?? calendars.find(c => c?.kind === 'freeBusy')?.uri; + const schedulingUri = card.schedulingUri ?? Object.values(card.schedulingAddresses ?? {})[0]?.uri; + const source = card.source ?? Object.values(card.directories ?? {}).find(d => d?.kind === 'entry')?.uri; + if (!calendarUri && !freeBusyUri && !schedulingUri && !source) return card; + return { + ...card, + ...(calendarUri ? { calendarUri } : {}), + ...(freeBusyUri ? { freeBusyUri } : {}), + ...(schedulingUri ? { schedulingUri } : {}), + ...(source ? { source } : {}), + }; +} diff --git a/lib/jmap/types.ts b/lib/jmap/types.ts index 711382c98..8e76bb6d5 100644 --- a/lib/jmap/types.ts +++ b/lib/jmap/types.ts @@ -312,6 +312,11 @@ export interface ContactCard { grammaticalGender?: string; pronouns?: Record }>; }; + calendars?: Record }>; + schedulingAddresses?: Record; pref?: number }>; + // Client-side flat views of `calendars`, `schedulingAddresses` and the + // `directories` entry (vCard CALURI / FBURL / CALADRURI / SOURCE). They are + // not RFC 9553 properties; lib/jmap/contact-wire.ts maps them. calendarUri?: string; schedulingUri?: string; freeBusyUri?: string; diff --git a/stores/contact-store.ts b/stores/contact-store.ts index 3ea3e98a3..55b9b5ef9 100644 --- a/stores/contact-store.ts +++ b/stores/contact-store.ts @@ -1098,7 +1098,8 @@ export const useContactStore = create()( const originalId = addressBook.originalId || stripLocalAccountPrefix(addressBook.id, addressBook.localAccountId); const accountId = addressBook.isShared ? addressBook.accountId : undefined; client = resolveAccountClient(client, addressBook.localAccountId); - await client.deleteAddressBook(originalId, accountId); + // The confirm dialog tells the user the contacts go with the book. + await client.deleteAddressBook(originalId, accountId, { removeContents: true }); set((state) => ({ addressBooks: state.addressBooks.filter(b => b.id !== addressBook.id), contacts: state.contacts.filter(c => !c.addressBookIds?.[addressBook.id]),