From 1aba571ac39c33504bb02062b3af39273d56f60f Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:05:48 +0100 Subject: [PATCH 1/9] fix: keep a signed or encrypted reply-to name with a comma as one address --- src/lib/crypto/__tests__/compose-secure.test.ts | 12 ++++++++++++ src/lib/crypto/compose-secure.ts | 4 ++-- src/lib/crypto/mime.ts | 3 ++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/lib/crypto/__tests__/compose-secure.test.ts b/src/lib/crypto/__tests__/compose-secure.test.ts index dac3936a..ac73ad25 100644 --- a/src/lib/crypto/__tests__/compose-secure.test.ts +++ b/src/lib/crypto/__tests__/compose-secure.test.ts @@ -102,6 +102,18 @@ describe('prepareSecureSend', () => { expect(new TextDecoder().decode(uploaded!)).not.toMatch(/bcc@example.org/i); }, 60_000); + it('quotes a Reply-To name with a comma so it stays one address', async () => { + await importPgpKey(me.privateKey, me.passphrase); + let uploaded: Uint8Array | null = null; + const msg = { ...outgoing(), replyTo: [{ name: 'Support, ACME', email: 'help@acme.com' }] }; + await prepareSecureSend(msg, { mode: 'pgp', sign: true, encrypt: false }, { + fetchBlob: async () => new Uint8Array([1]), + upload: async (bytes) => { uploaded = bytes; return 'x'; }, + }); + const text = new TextDecoder().decode(uploaded!); + expect(text).toMatch(/^Reply-To: "Support, ACME" $/m); + }, 60_000); + it('stops when a recipient has no key, or when the signing key is locked', async () => { await importPgpKey(me.privateKey, me.passphrase); const deps = { fetchBlob: async () => new Uint8Array(), upload: async () => 'x' }; diff --git a/src/lib/crypto/compose-secure.ts b/src/lib/crypto/compose-secure.ts index 053f7b30..5135b5e9 100644 --- a/src/lib/crypto/compose-secure.ts +++ b/src/lib/crypto/compose-secure.ts @@ -8,7 +8,7 @@ import type { OutgoingEmail } from '../../api/email'; import type { ContactCard } from '../../api/types'; import { useCryptoKeysStore, keysForEmail, isRecordCurrent, type CryptoKeyRecord, type PgpKeyRecord, type SmimeCertRecord } from '../../stores/crypto-keys-store'; import { messagePriorityHeaders } from '../message-priority'; -import { base64Decode, formatHeader, encodeHeaderWords, type MimeAttachment, type MimeMessageInput } from './mime'; +import { base64Decode, formatHeader, formatAddress, type MimeAttachment, type MimeMessageInput } from './mime'; import { buildSecureMessage, type SecureMode } from './secure-send'; import { ownKeyFor, unlockedPgpKey, unlockedSmimeKey } from './keyring'; @@ -100,7 +100,7 @@ export function secureReadiness(choice: SecureChoice, fromEmail: string, recipie function extraHeaders(outgoing: OutgoingEmail): string[] { const lines: string[] = []; if (outgoing.replyTo?.length) { - lines.push(formatHeader('Reply-To', outgoing.replyTo.map((a) => (a.name ? `${encodeHeaderWords(a.name)} <${a.email}>` : a.email)).join(', '))); + lines.push(formatHeader('Reply-To', outgoing.replyTo.map(formatAddress).join(', '))); } for (const [prop, value] of Object.entries(messagePriorityHeaders(outgoing.priority))) { const name = /^header:([^:]+):/.exec(prop)?.[1]; diff --git a/src/lib/crypto/mime.ts b/src/lib/crypto/mime.ts index e6df1a96..71f8da03 100644 --- a/src/lib/crypto/mime.ts +++ b/src/lib/crypto/mime.ts @@ -90,7 +90,8 @@ function unfold(value: string | undefined): string { return String(value ?? '').replace(/[\r\n\t]+/g, ' ').replace(/ {2,}/g, ' ').trim(); } -function formatAddress(addr: MimeAddress): string { +/** One address for a header: the name quoted (or RFC 2047-encoded) so commas in it stay inside. */ +export function formatAddress(addr: MimeAddress): string { if (addr.name) { const name = unfold(addr.name); // Raw 8-bit in a header is rejected by some MTAs; encode it. From 1194bf284f4e9f97d2b88b6da20270310cad068e Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:10:20 +0100 Subject: [PATCH 2/9] fix: keep ios push renewal from reporting an extension the server refused --- plugins/ios-push/Jmap.swift | 23 +++++++++++++++++ plugins/ios-push/PushRenewal.swift | 17 +++++++++--- src/lib/__tests__/ios-push-renewal.test.ts | 30 ++++++++++++++++++++++ 3 files changed, 66 insertions(+), 4 deletions(-) diff --git a/plugins/ios-push/Jmap.swift b/plugins/ios-push/Jmap.swift index 5cb6fa60..f9ca3bb1 100644 --- a/plugins/ios-push/Jmap.swift +++ b/plugins/ios-push/Jmap.swift @@ -21,6 +21,8 @@ enum JmapError: Error { case needsRefresh case http(Int) case badResponse + /// A /set call listed the record under notUpdated (or not under updated). + case notUpdated(String) } /// Credentials as the app stores them (src/api/jmap-client.ts StoredCredentials). @@ -166,9 +168,30 @@ enum Jmap { ["PushSubscription/get", ["ids": [id], "properties": pushSubscriptionProperties], "1"], ] let responses = try await call(apiUrl: apiUrl, authorization: authorization, using: ["urn:ietf:params:jmap:core"], methodCalls: methodCalls) + if let refused = notUpdated(responses, method: "PushSubscription/set", callId: "0", id: id) { + throw JmapError.notUpdated(refused) + } return listedPushSubscription(responses, callId: "1", id: id) } + /// Why the /set answer `callId` didn't update `id`, or nil when it did. + /// A method error or an answer without `id` under updated counts as refused. + static func notUpdated(_ responses: [[Any]], method: String, callId: String, id: String) -> String? { + for response in responses where response.count >= 3 && response[2] as? String == callId { + guard response[0] as? String == method, let args = response[1] as? [String: Any] else { + let args = response[1] as? [String: Any] + return (args?["type"] as? String) ?? "error" + } + if let refused = (args["notUpdated"] as? [String: Any])?[id] { + let error = refused as? [String: Any] + return (error?["description"] as? String) ?? (error?["type"] as? String) ?? "notUpdated" + } + if let updated = args["updated"] as? [String: Any], updated.keys.contains(id) { return nil } + return "not in updated" + } + return "no answer" + } + static func fetch(apiUrl: URL, accountId: String, emailIds: [String], authorization: String) async throws -> JmapFetchResult { var request = URLRequest(url: apiUrl) request.httpMethod = "POST" diff --git a/plugins/ios-push/PushRenewal.swift b/plugins/ios-push/PushRenewal.swift index 68ac8523..06e8ec25 100644 --- a/plugins/ios-push/PushRenewal.swift +++ b/plugins/ios-push/PushRenewal.swift @@ -83,23 +83,32 @@ enum PushRenewal { } // The recorded expiry was stale: the server's copy has time left. if let expires = current.expires, !needsExtension(expires) { - record(key: key, account: account, expires: expires) + record(key: key, subscriptionId: subscriptionId, expires: expires) return false } let wanted = formatDate(Date().addingTimeInterval(wantedSeconds)) let extended = try await Jmap.extendPushSubscription( apiUrl: apiUrl, id: subscriptionId, expires: wanted, authorization: authorization) - if let expires = extended?.expires { record(key: key, account: account, expires: expires) } + if let expires = extended?.expires { record(key: key, subscriptionId: subscriptionId, expires: expires) } pushLog.info("extended subscription \(subscriptionId, privacy: .public)") return true + } catch JmapError.notUpdated(let reason) { + // The server kept the old expiry: not extended, nothing to record. + pushLog.error("server did not extend subscription \(subscriptionId, privacy: .public): \(reason, privacy: .public)") + return false } catch { pushLog.error("renewal failed: \(String(describing: error), privacy: .public)") return false } } - private static func record(key: String, account: [String: Any], expires: String) { - var json = account + /// Writes the expiry into the account's entry as it is now, not as it was + /// when the renewal started: the app may have replaced the subscription (or + /// the credentials key) meanwhile. Only the expiry changes, and only while + /// the entry still names the subscription that was extended. + private static func record(key: String, subscriptionId: String, expires: String) { + guard var json = SharedKeychain.readJSON(key), + json["subscriptionId"] as? String == subscriptionId else { return } json["subscriptionExpires"] = expires SharedKeychain.update(key, json: json) } diff --git a/src/lib/__tests__/ios-push-renewal.test.ts b/src/lib/__tests__/ios-push-renewal.test.ts index 8b04a8b4..a742bff8 100644 --- a/src/lib/__tests__/ios-push-renewal.test.ts +++ b/src/lib/__tests__/ios-push-renewal.test.ts @@ -143,6 +143,36 @@ describe('the native renewal', () => { expect(set).toBeGreaterThan(own); }); + it('treats an update the server refused as not extended, and records nothing', () => { + const jmap = read('plugins/ios-push/Jmap.swift'); + const extend = jmap.slice(jmap.indexOf('static func extendPushSubscription(')); + const check = extend.indexOf('notUpdated(responses, method: "PushSubscription/set", callId: "0", id: id)'); + expect(check).toBeGreaterThan(-1); + expect(extend.indexOf('throw JmapError.notUpdated(')).toBeGreaterThan(check); + expect(check).toBeLessThan(extend.indexOf('return listedPushSubscription(responses, callId: "1", id: id)')); + // The renewal stops on it before logging "extended" or recording. + const body = renewal.slice(renewal.indexOf('static func renew(key:'), renewal.indexOf('private static func record(')); + const refused = body.match(/catch JmapError\.notUpdated[\s\S]*?\}/)?.[0] ?? ''; + expect(refused).toContain('return false'); + expect(refused).not.toContain('record('); + expect(body.indexOf('try await Jmap.extendPushSubscription(')).toBeLessThan(body.indexOf('pushLog.info("extended subscription')); + }); + + it('writes back only the expiry, into the entry as it is now, while it names the same subscription', () => { + const record = renewal.slice(renewal.indexOf('private static func record(')); + expect(record).toMatch(/^private static func record\(key: String, subscriptionId: String, expires: String\)/); + expect(record).toMatch(/guard var json = SharedKeychain\.readJSON\(key\),\s*json\["subscriptionId"\] as\? String == subscriptionId else \{ return \}/); + expect(record).toMatch(/json\["subscriptionExpires"\] = expires\s*SharedKeychain\.update\(key, json: json\)/); + expect(renewal).not.toMatch(/record\(key: key, account:/); + }); + + it('reads a refused update the way JMAP reports it', () => { + const jmap = read('plugins/ios-push/Jmap.swift'); + const helper = jmap.slice(jmap.indexOf('static func notUpdated(')); + expect(helper).toMatch(/args\["notUpdated"\] as\? \[String: Any\]\)\?\[id\]/); + expect(helper).toMatch(/updated\.keys\.contains\(id\) \{ return nil \}/); + }); + it('runs from the notification extension after the fetch for the pushed account', () => { const renew = nse.indexOf('await PushRenewal.renewIfDue(jmapAccountId: accountId)'); expect(renew).toBeGreaterThan(nse.indexOf('await NotificationService.enrich(')); From b1a0eaf8054175f81892678d49d4e5c54116128f Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:15:50 +0100 Subject: [PATCH 3/9] fix: turn sender favicons off on upgrade unless they were switched on by choice --- .../__tests__/settings-store-policy.test.ts | 48 ++++++++++++++++ src/stores/settings-store.ts | 55 ++++++++++++++++--- 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/src/stores/__tests__/settings-store-policy.test.ts b/src/stores/__tests__/settings-store-policy.test.ts index 750c4eef..476eac3d 100644 --- a/src/stores/__tests__/settings-store-policy.test.ts +++ b/src/stores/__tests__/settings-store-policy.test.ts @@ -44,6 +44,54 @@ describe('explicitSettings', () => { }); }); +describe('settings saved before a default changed', () => { + // Older versions saved the full snapshot, so senderFavicons: true (the old + // default) sits in it whether or not the user ever turned it on. + it('does not count an old default as chosen', () => { + expect(legacyExplicitSettings({ senderFavicons: true, showBirthdayCalendar: true, density: 'compact' })).toEqual(['density']); + }); + + it('moves an old default to the new one on upgrade, and lets a policy default apply', async () => { + await AsyncStorage.setItem('webmail:settings:v1', JSON.stringify({ + senderFavicons: true, + includeGroupInUnified: false, + autoSelectReplyIdentity: true, + showBirthdayCalendar: true, + density: 'compact', + })); + await useSettingsStore.getState().hydrate(); + expect(useSettingsStore.getState()).toMatchObject({ + senderFavicons: false, + includeGroupInUnified: true, + autoSelectReplyIdentity: false, + showBirthdayCalendar: false, + density: 'compact', + explicitSettings: ['density'], + }); + + resetPolicyRulesForTests(); + await AsyncStorage.setItem('webmail:settings:v1', JSON.stringify({ senderFavicons: true, density: 'compact' })); + useSettingsStore.setState({ hydrated: false }); + useSettingsStore.getState().applyPolicyRules(rules({ defaults: { senderFavicons: true, density: 'comfortable' } })); + await useSettingsStore.getState().hydrate(); + // The organisation's default reaches the setting nobody chose. + expect(useSettingsStore.getState()).toMatchObject({ senderFavicons: true, density: 'compact', explicitSettings: ['density'] }); + }); + + it('keeps settings saved with explicitSettings as they are', async () => { + await AsyncStorage.setItem('webmail:settings:v1', JSON.stringify({ senderFavicons: true, explicitSettings: ['senderFavicons'] })); + await useSettingsStore.getState().hydrate(); + expect(useSettingsStore.getState()).toMatchObject({ senderFavicons: true, explicitSettings: ['senderFavicons'] }); + }); + + it('treats an old default in an imported file the same way', () => { + useSettingsStore.setState({ hydrated: true }); + expect(useSettingsStore.getState().importSettings(JSON.stringify({ senderFavicons: true, dateFormat: 'full' }))).toBe(true); + expect(useSettingsStore.getState()).toMatchObject({ senderFavicons: false, dateFormat: 'full' }); + expect(useSettingsStore.getState().explicitSettings).not.toContain('senderFavicons'); + }); +}); + describe('applyPolicyRules', () => { it('does not write the defaults over stored settings it has not read yet', async () => { await AsyncStorage.setItem('webmail:settings:v1', JSON.stringify({ theme: 'dark', explicitSettings: ['theme'] })); diff --git a/src/stores/settings-store.ts b/src/stores/settings-store.ts index eaf87145..1eb90c51 100644 --- a/src/stores/settings-store.ts +++ b/src/stores/settings-store.ts @@ -858,17 +858,54 @@ function policyContext(explicit: readonly string[]): PolicyPatchContext { return { builtIn: POLICY_BUILT_IN, explicit: new Set(explicit), isValid: isValidPolicyValue }; } +/** + * Defaults an earlier version had, for the settings whose default changed + * since. Versions before explicitSettings saved the whole snapshot, defaults + * included, so a value equal to the old default was most likely never + * picked: senderFavicons was on until sender domains stopped going to the + * favicon service by default; the other three changed when the defaults were + * aligned with the webmail (0.1.59). + */ +const LEGACY_DEFAULTS: Readonly>> = { + senderFavicons: true, + includeGroupInUnified: false, + autoSelectReplyIdentity: true, + showBirthdayCalendar: true, +}; + +function isLegacyDefault(source: Record, key: string): boolean { + return key in LEGACY_DEFAULTS && source[key] === LEGACY_DEFAULTS[key as keyof PersistedSettings]; +} + /** * Settings from before explicitSettings existed: a value other than the * build default can only have come from the user, so it counts as chosen - * (webmail legacyExplicitSettings). + * (webmail legacyExplicitSettings). A value equal to that setting's old + * default doesn't (webmail migrateNotificationSoundDefault). */ export function legacyExplicitSettings(source: Record): string[] { return Object.keys(source).filter( - (key) => isPolicySettingKey(key) && source[key] !== undefined && source[key] !== POLICY_BUILT_IN[key], + (key) => + isPolicySettingKey(key) && + source[key] !== undefined && + source[key] !== POLICY_BUILT_IN[key] && + !isLegacyDefault(source, key), ); } +/** + * Moves a setting still at its old default, in settings from before + * explicitSettings existed, to the current default: the user never picked + * it (webmail migrateNotificationSoundDefault, migrateTintListRowsDefault). + */ +export function migrateLegacyDefaults>(source: T): T { + const out: Record = { ...source }; + for (const key of Object.keys(LEGACY_DEFAULTS)) { + if (isLegacyDefault(out, key)) out[key] = DEFAULT_PERSISTED[key as keyof PersistedSettings]; + } + return out as T; +} + /** Whether the organisation policy pins `key`, so the user can't change it. */ export function isSettingLockedByPolicy(key: string): boolean { return isLockedSetting(policyRules, key); @@ -942,9 +979,10 @@ export const useSettingsStore = create((set, get) => ({ const raw = await AsyncStorage.getItem(STORAGE_KEY); if (raw) { const parsed = JSON.parse(raw) as Partial; - const merged = mergeWithDefaults(parsed); + let merged = mergeWithDefaults(parsed); if (!Array.isArray(parsed.explicitSettings)) { merged.explicitSettings = legacyExplicitSettings(merged as unknown as Record); + merged = migrateLegacyDefaults(merged as unknown as Record) as unknown as PersistedSettings; } // The policy may have arrived (from its cache) before the settings. const patch = currentPolicyPatch(merged); @@ -1089,12 +1127,15 @@ export const useSettingsStore = create((set, get) => ({ return false; } if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return false; - const incoming = fromExportShape(parsed as Record); + let incoming = fromExportShape(parsed as Record); // A file without explicitSettings (older clients): what it sets to - // something other than the default was chosen. - const chosen = Array.isArray(incoming.explicitSettings) - ? incoming.explicitSettings.filter((key): key is string => typeof key === 'string') + // something other than the default was chosen, and what is still at an + // old default moves to the current one. + const legacy = !Array.isArray(incoming.explicitSettings); + const chosen = !legacy + ? (incoming.explicitSettings as unknown[]).filter((key): key is string => typeof key === 'string') : legacyExplicitSettings(incoming as Record); + if (legacy) incoming = migrateLegacyDefaults(incoming as Record) as typeof incoming; // Validate against the current state so keys absent from the file keep // their value instead of snapping back to the default. const merged = mergeWithDefaults({ From e2d4915edcd30ba4f41cd0821d63fc7339896d99 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:24:11 +0100 Subject: [PATCH 4/9] fix: keep a queued message without a drafts folder from passing as sent when its submission failed --- .../email-submission-refusals.test.ts | 28 +++++++++++++++++++ .../live-stalwart-send-queue.test.ts | 12 ++++++++ src/api/email.ts | 19 +++++++------ src/lib/send-queue.ts | 7 +++-- 4 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/api/__tests__/email-submission-refusals.test.ts b/src/api/__tests__/email-submission-refusals.test.ts index 5ebc227d..36cda74e 100644 --- a/src/api/__tests__/email-submission-refusals.test.ts +++ b/src/api/__tests__/email-submission-refusals.test.ts @@ -40,6 +40,7 @@ import { listScheduledEmails, RescheduleLostError, RescheduleRestoredError, rescheduleScheduledSend, sendEmail, } from '../email'; import { RecipientsRefusedError, jmapErrorType } from '../jmap-result'; +import { verdictFromCopies } from '../../lib/send-queue'; import { pruneEmailSubmissions, resetSubmissionPrune } from '../submissions'; import { createFakeSubmissionServer, RATE_LIMITED, RELAY_DENIED, type StalwartVersion, @@ -87,6 +88,33 @@ afterEach(() => { expect(fake.server?.violations ?? []).toEqual([]); }); +describe('a send from an account without a Drafts folder', () => { + // The copy is made in Sent right away. If the submission is refused and + // the answer is lost, that copy must not look sent to the offline queue, + // which takes a copy without $draft as proof the message went out. + it('leaves a copy a refused, unanswered submission made marked as a draft', async () => { + const server = setup('1.0', { forbiddenAccounts: ['shared-1'] }); + server.afterEachRequest(() => { throw new TypeError('Network request failed'); }); + + await expect(sendEmail({ ...OUTGOING, messageId: 'm1@example.com' }, 'id-1', 'sent', undefined, { accountId: 'shared-1' })) + .rejects.toThrow('Network request failed'); + + const copies = [...server.emails.values()]; + expect(copies).toEqual([expect.objectContaining({ mailboxIds: { sent: true }, keywords: { $seen: true, $draft: true } })]); + expect(verdictFromCopies(copies)).toEqual({ kind: 'resend', staleIds: [copies[0].id] }); + }); + + it('takes $draft off the copy once the submission went through', async () => { + const server = setup('1.0'); + + await sendEmail(OUTGOING, 'id-1', 'sent', undefined, {}); + + const copies = [...server.emails.values()]; + expect(copies).toEqual([expect.objectContaining({ mailboxIds: { sent: true }, keywords: { $seen: true } })]); + expect(verdictFromCopies(copies)).toEqual({ kind: 'sent', copyId: copies[0].id }); + }); +}); + describe('a send whose recipients the server refuses', () => { it.each(['1.0', '0.16.23', '0.16.10', '0.16.6'] as StalwartVersion[])( 'Stalwart %s: fails when every recipient was refused, and keeps the previous draft', diff --git a/src/api/__tests__/live-stalwart-send-queue.test.ts b/src/api/__tests__/live-stalwart-send-queue.test.ts index 0fd7e94b..f14b0881 100644 --- a/src/api/__tests__/live-stalwart-send-queue.test.ts +++ b/src/api/__tests__/live-stalwart-send-queue.test.ts @@ -101,6 +101,18 @@ live('live Stalwart: offline send queue', () => { expect(copies.map((c) => c.id)).toEqual([first.emailId]); }, 60_000); + it('takes $draft off a copy sent without a Drafts folder only once it went out', async () => { + const outgoing = message('no-drafts'); + // Created in Sent with $draft; onSuccessUpdateEmail takes it off. + const result = await sendEmail(outgoing, identity.id, sent, undefined, {}); + const copies = await findCopiesByMessageId(outgoing.messageId, lookup()); + expect(copies.map((c) => c.id)).toEqual([result.emailId]); + expect(copies[0].keywords?.$draft).toBeFalsy(); + expect(copies[0].keywords?.$seen).toBe(true); + const outcome = await runQueuedSend(newQueuedSend('q3', { ...payloadFor(outgoing), draftsMailboxId: undefined }, Date.now(), true), boxes); + expect(outcome.kind).toBe('alreadySent'); + }, 60_000); + it('sends a marked message whose only copy is its draft, and removes the draft', async () => { const outgoing = message('draft-only'); const draft = await saveDraftVersion(outgoing, drafts); diff --git a/src/api/email.ts b/src/api/email.ts index 31a3c724..5e8d1748 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -1781,8 +1781,12 @@ export async function sendEmail( emailCreate.mailboxIds = { [opts!.draftsMailboxId!]: true }; emailCreate.keywords = { $seen: true, $draft: true }; } else { + // No Drafts folder: created in Sent, but still with `$draft` until the + // submission succeeds, so a copy whose submission was refused (with the + // answer lost) never passes for a sent message. The send queue takes a + // copy without `$draft` as proof the message went out (verdictFromCopies). emailCreate.mailboxIds = { [sentMailboxId]: true }; - emailCreate.keywords = { $seen: true }; + emailCreate.keywords = { $seen: true, $draft: true }; } const createCall: [string, Record, string] = raw ? ['Email/import', { accountId, emails: { draft: emailCreate } }, '0'] @@ -1818,14 +1822,11 @@ export async function sendEmail( accountId, create: { 'sub-1': submissionCreate }, }; - if (viaDrafts) { - submissionArgs.onSuccessUpdateEmail = { - '#sub-1': { - mailboxIds: { [sentMailboxId]: true }, - [keywordPointer('$draft')]: null, - }, - }; - } + submissionArgs.onSuccessUpdateEmail = { + '#sub-1': viaDrafts + ? { mailboxIds: { [sentMailboxId]: true }, [keywordPointer('$draft')]: null } + : { [keywordPointer('$draft')]: null }, + }; let res = await jmapClient.request( [ diff --git a/src/lib/send-queue.ts b/src/lib/send-queue.ts index a4a4b0ca..e33c9016 100644 --- a/src/lib/send-queue.ts +++ b/src/lib/send-queue.ts @@ -130,9 +130,10 @@ export type CopyVerdict = /** * What the copies with the message's Message-ID say. A send creates the - * message as a draft and the submission's onSuccessUpdateEmail files it in - * Sent without `$draft` in the same request, so a copy without `$draft` went - * out (or is held for a scheduled time). Copies still marked `$draft` are + * message as a draft (in Drafts, or in Sent when the account has no Drafts + * folder) and only the submission's onSuccessUpdateEmail takes `$draft` off + * (and files it in Sent) in the same request, so a copy without `$draft` + * went out (or is held for a scheduled time). Copies still marked `$draft` are * the autosaved draft (`draftId`, kept until the send succeeds) and copies * a refused submission left behind with its answer lost: those go before * the message is sent again. From 1c33f8daef03103accd2b9310a96587cbd9aff25 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:24:12 +0100 Subject: [PATCH 5/9] fix: tell the user when a queued message gives up after unconfirmed attempts --- .../__tests__/outbox-send-queue.test.ts | 33 +++++++++++++++++++ src/stores/outbox-store.ts | 9 +++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/stores/__tests__/outbox-send-queue.test.ts b/src/stores/__tests__/outbox-send-queue.test.ts index 3985256d..46bc6715 100644 --- a/src/stores/__tests__/outbox-send-queue.test.ts +++ b/src/stores/__tests__/outbox-send-queue.test.ts @@ -154,6 +154,39 @@ describe('offline send queue', () => { expect(useOutboxStore.getState().sends).toEqual([]); }); + it('says so when a message fails after too many unconfirmed attempts, as for a refused one', async () => { + useNetworkStore.setState({ online: true, connected: true }); + runQueuedSend.mockRejectedValueOnce(new Error('Invalid envelope')); + useOutboxStore.getState().enqueueSend(payload('refused')); + await flushed(); + await flushed(); + expect(toast.error).toHaveBeenCalledTimes(1); + expect(toast.error.mock.calls[0][1]).toEqual({ message: 'refused: Invalid envelope' }); + + toast.error.mockClear(); + const id = useOutboxStore.getState().enqueueSend(payload('unconfirmed'))!; + useOutboxStore.setState({ sends: useOutboxStore.getState().sends.map((s) => (s.id === id ? { ...s, attempts: 2 } : s)) }); + runQueuedSend.mockRejectedValueOnce(named('SendUnconfirmedError', 'Send confirmation was not received')); + useOutboxStore.setState({ flushing: false }); + await useOutboxStore.getState().flush(); + await flushed(); + expect(useOutboxStore.getState().sends.find((s) => s.id === id)).toMatchObject({ status: 'failed', attempts: 3 }); + expect(toast.error).toHaveBeenCalledTimes(1); + expect(toast.error.mock.calls[0]).toEqual([ + 'A message in the Outbox could not be sent', { message: 'unconfirmed: Send confirmation was not received' }, + ]); + }); + + it('does not report an unconfirmed attempt that will be tried again', async () => { + useNetworkStore.setState({ online: true, connected: true }); + runQueuedSend.mockRejectedValueOnce(named('SendUnconfirmedError', 'Send confirmation was not received')); + useOutboxStore.getState().enqueueSend(payload()); + await flushed(); + await flushed(); + expect(useOutboxStore.getState().sends[0]).toMatchObject({ status: 'queued', attempts: 1 }); + expect(toast.error).not.toHaveBeenCalled(); + }); + it('stops after an expired session without failing the message', async () => { useNetworkStore.setState({ online: true, connected: true }); runQueuedSend.mockRejectedValueOnce(named('AuthenticationError', 'Session expired')); diff --git a/src/stores/outbox-store.ts b/src/stores/outbox-store.ts index 3dd97d8f..d0ea2aef 100644 --- a/src/stores/outbox-store.ts +++ b/src/stores/outbox-store.ts @@ -690,14 +690,17 @@ async function flushSends(accountId: string): Promise<'done' | 'transient'> { } catch (err) { const kind = classifySendFailure(err, isTransientNetworkError); const message = err instanceof Error ? err.message : String(err); - updateSend(accountId, afterFailure(started, kind, message)); + const next = afterFailure(started, kind, message); + updateSend(accountId, next); + // Refused, or unconfirmed too often: it waits for the user now, and + // nothing retries it, so say so whatever the reason. + if (next.status === 'failed') void notifyQueuedSendFailed(started, message); if (kind === 'auth') { useOutboxStore.setState({ paused: true }); break; } if (kind === 'offline' || kind === 'unconfirmed') return 'transient'; - // Refused: this one waits for the user; the others go on. - void notifyQueuedSendFailed(started, message); + // Refused: the others go on. } } return 'done'; From ac875ffe1d4d208281290ea3a0eafabc13ea04c4 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:27:57 +0100 Subject: [PATCH 6/9] fix: don't move a queued message to drafts when an earlier attempt already sent it --- locales/rn/en.json | 1 + src/api/__tests__/queued-send.test.ts | 27 ++++++- src/api/queued-send.ts | 78 +++++++++++++------ .../__tests__/outbox-send-queue.test.ts | 34 ++++++++ src/stores/outbox-store.ts | 29 +++++++ 5 files changed, 144 insertions(+), 25 deletions(-) diff --git a/locales/rn/en.json b/locales/rn/en.json index 6f58ab52..37bf230c 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -1004,6 +1004,7 @@ "section_personal": "My templates" }, "outbox": { + "already_sent": "This message was already sent", "discard": "Discard", "discard_body": "It will not be sent, and its text is lost.", "discard_title": "Discard this message?", diff --git a/src/api/__tests__/queued-send.test.ts b/src/api/__tests__/queued-send.test.ts index fa93f669..333b2c1f 100644 --- a/src/api/__tests__/queued-send.test.ts +++ b/src/api/__tests__/queued-send.test.ts @@ -18,7 +18,7 @@ vi.mock('../../lib/compose-send-account', () => ({ })); import { jmapClient } from '../jmap-client'; -import { findCopiesByMessageId, runQueuedSend } from '../queued-send'; +import { findCopiesByMessageId, runQueuedSend, wasQueuedSendSent } from '../queued-send'; import { newQueuedSend, type QueuedSendPayload } from '../../lib/send-queue'; const mockRequest = jmapClient.request as ReturnType; @@ -85,6 +85,31 @@ describe('runQueuedSend', () => { }); }); +describe('wasQueuedSendSent', () => { + it('finds a copy an earlier attempt sent, and removes the old draft', async () => { + mockRequest.mockResolvedValueOnce(found([ + { id: 'draft-1', keywords: { $draft: true }, messageId: ['m1@x.de'] }, + { id: 'sent-1', keywords: { $seen: true }, messageId: ['m1@x.de'] }, + ])); + expect(await wasQueuedSendSent(newQueuedSend('q', PAYLOAD, 0, true), [])).toBe(true); + expect(destroyEmails).toHaveBeenCalledWith(['draft-1'], 'acc-1'); + }); + + it('says not sent when only drafts are there, and removes nothing', async () => { + mockRequest.mockResolvedValueOnce(found([ + { id: 'draft-1', keywords: { $draft: true }, messageId: ['m1@x.de'] }, + { id: 'left-1', keywords: { $draft: true, $seen: true }, messageId: ['m1@x.de'] }, + ])); + expect(await wasQueuedSendSent(newQueuedSend('q', PAYLOAD, 0, true), [])).toBe(false); + expect(destroyEmails).not.toHaveBeenCalled(); + }); + + it('does not look for a message no attempt reached the server with', async () => { + expect(await wasQueuedSendSent(newQueuedSend('q', PAYLOAD, 0, false), [])).toBe(false); + expect(mockRequest).not.toHaveBeenCalled(); + }); +}); + describe('findCopiesByMessageId', () => { it('reads the recent messages of Sent and Drafts and keeps exact matches only', async () => { mockRequest.mockResolvedValueOnce(found([ diff --git a/src/api/queued-send.ts b/src/api/queued-send.ts index d9503310..c7641987 100644 --- a/src/api/queued-send.ts +++ b/src/api/queued-send.ts @@ -8,7 +8,7 @@ import type { Mailbox } from './types'; import { sendTargetFor } from '../lib/compose-send-account'; import { supportsSubmissionExtension } from '../lib/submission-options'; import { stripMessageIdBrackets } from '../lib/email-threading'; -import { holdForScheduled, verdictFromCopies, type FoundCopy, type QueuedSend } from '../lib/send-queue'; +import { holdForScheduled, verdictFromCopies, type CopyVerdict, type FoundCopy, type QueuedSend } from '../lib/send-queue'; /** How far before the queueing a copy can be dated (the server's clock may differ). */ const CLOCK_SKEW_MS = 10 * 60_000; @@ -68,34 +68,64 @@ export type QueuedSendOutcome = * went out; copies a refused attempt left in Drafts are removed before the * new attempt. */ -export async function runQueuedSend(send: QueuedSend, mailboxes: Mailbox[], now = Date.now()): Promise { +/** The account and folders a queued message goes through, as the composer works them out. */ +async function queuedSendTarget(send: QueuedSend, mailboxes: Mailbox[]) { const p = send.payload; const target = await sendTargetFor(p.identity, p.fromEmail, mailboxes); - const accountId = target.accountId ?? jmapClient.accountId; - const draftAccountId = p.draftAccountId ?? jmapClient.accountId; + return { + target, + accountId: target.accountId ?? jmapClient.accountId, + draftAccountId: p.draftAccountId ?? jmapClient.accountId, + sentMailboxId: target.sentMailboxId ?? p.sentMailboxId, + draftsMailboxId: target.accountId ? target.draftsMailboxId : p.draftsMailboxId, + }; +} - const sentMailboxId = target.sentMailboxId ?? p.sentMailboxId; - const draftsMailboxId = target.accountId ? target.draftsMailboxId : p.draftsMailboxId; +type Resolved = Awaited>; + +/** + * Whether an earlier, unanswered attempt at `send` went out. When it did, the + * old draft that send would have removed is removed now. + */ +async function earlierAttempt(send: QueuedSend, r: Resolved): Promise { + const p = send.payload; + const copies = await findCopiesByMessageId(p.outgoing.messageId!, { + accountId: r.target.accountId, + mailboxIds: [r.sentMailboxId, r.draftsMailboxId ?? ''], + since: send.createdAt, + }); + // The autosaved draft lives in this account only when the draft did. + const draftHere = p.draftId && r.draftAccountId === r.accountId ? p.draftId : undefined; + const verdict = verdictFromCopies(copies, draftHere); + if (verdict.kind === 'sent' && p.draftId && p.draftId !== verdict.copyId) { + // The send went out but its answer was lost, so the old draft it would + // have removed is still there. + await destroyEmails([p.draftId], r.draftAccountId).catch((err) => { + console.warn('[send-queue] could not remove the draft of a sent message', err); + }); + } + return verdict; +} + +/** + * Before a queued message is taken back (moved to Drafts): true when an + * earlier attempt that may have reached the server did send it. A message no + * attempt reached the server with is never looked up. + */ +export async function wasQueuedSendSent(send: QueuedSend, mailboxes: Mailbox[]): Promise { + if (!send.maybeSent || !send.payload.outgoing.messageId) return false; + const verdict = await earlierAttempt(send, await queuedSendTarget(send, mailboxes)); + return verdict.kind === 'sent'; +} + +export async function runQueuedSend(send: QueuedSend, mailboxes: Mailbox[], now = Date.now()): Promise { + const p = send.payload; + const resolved = await queuedSendTarget(send, mailboxes); + const { target, accountId, sentMailboxId, draftsMailboxId } = resolved; if (send.maybeSent && p.outgoing.messageId) { - const copies = await findCopiesByMessageId(p.outgoing.messageId, { - accountId: target.accountId, - mailboxIds: [sentMailboxId, draftsMailboxId ?? ''], - since: send.createdAt, - }); - // The autosaved draft lives in this account only when the draft did. - const draftHere = p.draftId && draftAccountId === accountId ? p.draftId : undefined; - const verdict = verdictFromCopies(copies, draftHere); - if (verdict.kind === 'sent') { - // The send went out but its answer was lost, so the old draft it would - // have removed is still there. - if (p.draftId && p.draftId !== verdict.copyId) { - await destroyEmails([p.draftId], draftAccountId).catch((err) => { - console.warn('[send-queue] could not remove the draft of a sent message', err); - }); - } - return { kind: 'alreadySent', accountId: target.accountId }; - } + const verdict = await earlierAttempt(send, resolved); + if (verdict.kind === 'sent') return { kind: 'alreadySent', accountId: target.accountId }; if (verdict.staleIds.length > 0) { await destroyEmails(verdict.staleIds, accountId); } diff --git a/src/stores/__tests__/outbox-send-queue.test.ts b/src/stores/__tests__/outbox-send-queue.test.ts index 46bc6715..11655c32 100644 --- a/src/stores/__tests__/outbox-send-queue.test.ts +++ b/src/stores/__tests__/outbox-send-queue.test.ts @@ -25,8 +25,10 @@ vi.mock('../../api/email', () => ({ })); const runQueuedSend = vi.fn(async (..._a: any[]): Promise => ({ kind: 'sent', result: { scheduled: false } })); +const wasQueuedSendSent = vi.fn(async (..._a: any[]) => false); vi.mock('../../api/queued-send', () => ({ runQueuedSend: (...a: any[]) => runQueuedSend(...a), + wasQueuedSendSent: (...a: any[]) => wasQueuedSendSent(...a), })); vi.mock('../email-store', () => ({ useEmailStore: { getState: () => ({ mailboxes: [{ id: 'sent', role: 'sent' }] }) }, @@ -221,6 +223,38 @@ describe('offline send queue', () => { expect(useOutboxStore.getState().sends[0]).toMatchObject({ id: again, status: 'failed' }); }); + it('does not turn a message an earlier attempt sent into a draft, and says it was sent', async () => { + const id = useOutboxStore.getState().enqueueSend(payload('went-out'), { maybeSent: true })!; + wasQueuedSendSent.mockResolvedValueOnce(true); + expect(await useOutboxStore.getState().cancelSend(id, { keepAsDraft: true })).toBe(true); + expect(wasQueuedSendSent).toHaveBeenCalledWith(expect.objectContaining({ id, maybeSent: true }), [{ id: 'sent', role: 'sent' }]); + expect(saveDraftVersion).not.toHaveBeenCalled(); + expect(useOutboxStore.getState().sends).toEqual([]); + await flushed(); + expect(toast.info).toHaveBeenCalledWith('This message was already sent', { message: 'went-out' }); + expect(patchKeywordsForEmails).toHaveBeenCalledWith(['orig-1'], { $answered: true }, undefined); + }); + + it('moves a marked message to Drafts when no earlier attempt sent it, and looks only for marked ones', async () => { + const marked = useOutboxStore.getState().enqueueSend(payload('marked'), { maybeSent: true })!; + expect(await useOutboxStore.getState().cancelSend(marked, { keepAsDraft: true })).toBe(true); + expect(wasQueuedSendSent).toHaveBeenCalledTimes(1); + expect(saveDraftVersion).toHaveBeenCalledTimes(1); + + const plain = useOutboxStore.getState().enqueueSend(payload('plain'))!; + expect(await useOutboxStore.getState().cancelSend(plain, { keepAsDraft: true })).toBe(true); + expect(wasQueuedSendSent).toHaveBeenCalledTimes(1); + expect(saveDraftVersion).toHaveBeenCalledTimes(2); + }); + + it('keeps a marked message in the Outbox when the lookup fails', async () => { + const id = useOutboxStore.getState().enqueueSend(payload('unknown'), { maybeSent: true })!; + wasQueuedSendSent.mockRejectedValueOnce(named('NetworkError', 'Network request failed')); + expect(await useOutboxStore.getState().cancelSend(id, { keepAsDraft: true })).toBe(false); + expect(saveDraftVersion).not.toHaveBeenCalled(); + expect(useOutboxStore.getState().sends[0]).toMatchObject({ id, status: 'failed' }); + }); + it('drops the queued messages with the rest when the server behind the account changed', async () => { useOutboxStore.getState().enqueueSend(payload()); expect(await useOutboxStore.getState().clearAccount(ACCOUNT)).toBe(1); diff --git a/src/stores/outbox-store.ts b/src/stores/outbox-store.ts index d0ea2aef..ab4c7350 100644 --- a/src/stores/outbox-store.ts +++ b/src/stores/outbox-store.ts @@ -562,6 +562,22 @@ export const useOutboxStore = create((set, get) => ({ if (!opts?.keepAsDraft) return true; const p = send.payload; try { + // An attempt that may have reached the server is looked for first, as + // before a resend: a message that went out must not come back as a + // draft. + if (send.maybeSent) { + const [{ wasQueuedSendSent }, { useEmailStore }] = await Promise.all([ + import('../api/queued-send'), + import('./email-store'), + ]); + if (await wasQueuedSendSent(send, useEmailStore.getState().mailboxes)) { + if (p.original) { + patchKeywordsForEmails([p.original.emailId], { [p.original.keyword]: true }, p.original.accountId).catch(() => undefined); + } + void notifyAlreadySent(send); + return true; + } + } if (!p.draftsMailboxId) throw new Error('No Drafts folder'); const { saveDraftVersion } = await import('../api/email'); await saveDraftVersion(p.outgoing, p.draftsMailboxId, p.draftId, undefined, p.draftAccountId); @@ -732,6 +748,19 @@ async function afterQueuedSend(send: QueuedSend, outcome: QueuedSendOutcomeLike) } } +/** Taking back a message found to have gone out already: nothing became a draft. */ +async function notifyAlreadySent(send: QueuedSend): Promise { + try { + const [{ toast }, { useLocaleStore }] = await Promise.all([import('./toast-store'), import('./locale-store')]); + const t = useLocaleStore.getState().t; + toast.info(t('outbox.already_sent', 'This message was already sent'), { + message: send.payload.outgoing.subject || undefined, + }); + } catch (err) { + console.warn('[outbox] could not report an already sent message', err); + } +} + async function notifyQueuedSendFailed(send: QueuedSend, message: string): Promise { try { const [{ toast }, { useLocaleStore }] = await Promise.all([import('./toast-store'), import('./locale-store')]); From 75213f6a551278c295a6e95246dc15a6cbf1e101 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:32:21 +0100 Subject: [PATCH 7/9] fix: send a message queued or retried while the outbox is already sending --- .../__tests__/outbox-send-queue.test.ts | 31 +++++++++++++++++++ src/stores/outbox-store.ts | 15 +++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/stores/__tests__/outbox-send-queue.test.ts b/src/stores/__tests__/outbox-send-queue.test.ts index 11655c32..c7555456 100644 --- a/src/stores/__tests__/outbox-send-queue.test.ts +++ b/src/stores/__tests__/outbox-send-queue.test.ts @@ -141,6 +141,37 @@ describe('offline send queue', () => { expect(runQueuedSend).toHaveBeenCalledTimes(1); }); + it('sends a message queued while another one is on its way', async () => { + let release!: () => void; + runQueuedSend.mockImplementationOnce(() => new Promise((r) => { release = () => r({ kind: 'sent', result: {} }); })); + useNetworkStore.setState({ online: true, connected: true }); + useOutboxStore.getState().enqueueSend(payload('first')); + await flushed(); + expect(runQueuedSend).toHaveBeenCalledTimes(1); + useOutboxStore.getState().enqueueSend(payload('second')); + release(); + await vi.waitFor(() => expect(useOutboxStore.getState().sends).toEqual([])); + expect(runQueuedSend.mock.calls.map(([s]) => s.payload.outgoing.subject)).toEqual(['first', 'second']); + }); + + it('sends a message retried while another one is on its way', async () => { + useNetworkStore.setState({ online: true, connected: true }); + runQueuedSend.mockRejectedValueOnce(new Error('Invalid envelope')); + const failed = useOutboxStore.getState().enqueueSend(payload('failed'))!; + await vi.waitFor(() => expect(useOutboxStore.getState().sends[0]?.status).toBe('failed')); + + let release!: () => void; + runQueuedSend.mockImplementationOnce(() => new Promise((r) => { release = () => r({ kind: 'sent', result: {} }); })); + useOutboxStore.getState().enqueueSend(payload('other')); + await flushed(); + expect(runQueuedSend).toHaveBeenCalledTimes(2); + const retried = useOutboxStore.getState().retrySend(failed); + release(); + await retried; + await vi.waitFor(() => expect(useOutboxStore.getState().sends).toEqual([])); + expect(runQueuedSend.mock.calls.map(([s]) => s.payload.outgoing.subject)).toEqual(['failed', 'other', 'failed']); + }); + it('parks a refused message with its reason and leaves it until the user tries again', async () => { useNetworkStore.setState({ online: true, connected: true }); runQueuedSend.mockRejectedValueOnce(new Error('Invalid envelope')); diff --git a/src/stores/outbox-store.ts b/src/stores/outbox-store.ts index ab4c7350..a052bb2c 100644 --- a/src/stores/outbox-store.ts +++ b/src/stores/outbox-store.ts @@ -258,6 +258,12 @@ function opFamily(op: OutboxOp): string { let retryTimer: ReturnType | null = null; let retryAttempt = 0; +/** + * A flush was asked for while one ran (a message queued or retried then): + * the running one works from lists it took when it started, so another + * follows it. + */ +let flushAgain = false; // Reads of a bucket from storage. A read still running when the account // changes or its queue is cleared must not put the old entries back, so @@ -436,7 +442,11 @@ export const useOutboxStore = create((set, get) => ({ pendingForEmail: (emailId) => get().entries.filter((e) => e.op.emailId === emailId), flush: async () => { - if (get().flushing || get().paused || !get().resumed || !get().hydrated) return; + if (get().flushing) { + flushAgain = true; + return; + } + if (get().paused || !get().resumed || !get().hydrated) return; const accountId = get().activeAccountId; if (!accountId) return; if (get().entries.length === 0 && !hasWaitingSends()) return; @@ -445,6 +455,7 @@ export const useOutboxStore = create((set, get) => ({ if (!useNetworkStore.getState().online) return; set({ flushing: true }); + flushAgain = false; let brokeTransient = false; let dropped = false; // An entry that took on a later edit while its op was out runs again. @@ -509,7 +520,7 @@ export const useOutboxStore = create((set, get) => ({ set({ flushing: false }); } if (brokeTransient && (get().entries.length > 0 || hasWaitingSends())) scheduleRetry(); - else if (changedInFlight) void get().flush(); + else if (changedInFlight || flushAgain) void get().flush(); if (dropped) void refreshAfterDrop(); }, From 2c5d95184e00c22979890857de53e22f3018e7e5 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:39:34 +0100 Subject: [PATCH 8/9] fix: wait for the saved session check instead of loading the account beside it when the network comes back at start --- src/stores/__tests__/auth-store.test.ts | 49 ++++++++++++++++++++++++ src/stores/auth-store.ts | 50 +++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/src/stores/__tests__/auth-store.test.ts b/src/stores/__tests__/auth-store.test.ts index 192d9f38..32f33420 100644 --- a/src/stores/__tests__/auth-store.test.ts +++ b/src/stores/__tests__/auth-store.test.ts @@ -391,6 +391,55 @@ describe('auth-store', () => { expect(useAccountStore.getState().getAccountById('acc-1')?.needsSignIn).toBeFalsy(); }); + // The network coming back (App.tsx) retries the session while the + // start from the saved session is still checking it: the retry used to + // load the account beside the check, and check the server while the + // start's reads were still landing. + it('waits for the check of the saved session when the network comes back during it', async () => { + registerAccount(); + mockLoadFromCache.mockResolvedValueOnce(true); + const check = deferred(); + mockRevalidate.mockReturnValueOnce(check.promise); + await useAuthStore.getState().restoreSession(); + + let retried: boolean | undefined; + const retry = useAuthStore.getState().retrySession().then((ok) => { retried = ok; }); + await new Promise((r) => setTimeout(r, 10)); + expect(mockLoadAccount).not.toHaveBeenCalled(); + expect(retried).toBeUndefined(); + + check.resolve({ apiUrl: 'https://mail.example.com/jmap/' }); + await retry; + expect(retried).toBe(true); + expect(useAuthStore.getState().session).not.toBeNull(); + expect(mockLoadAccount).not.toHaveBeenCalled(); + expect(mockRevalidate).toHaveBeenCalledTimes(1); + expect((resume.mock.calls as unknown[][]).filter(([id]) => id === 'acc-1')).toHaveLength(1); + }); + + it('loads the account once the check of the saved session failed without network', async () => { + registerAccount(); + mockLoadFromCache.mockResolvedValueOnce(true); + const check = deferred(); + mockRevalidate.mockReturnValueOnce(check.promise); + await useAuthStore.getState().restoreSession(); + + mockLoadAccount.mockResolvedValueOnce(true); + const retry = useAuthStore.getState().retrySession(); + await new Promise((r) => setTimeout(r, 10)); + expect(mockLoadAccount).not.toHaveBeenCalled(); + check.reject(new NetworkError('offline')); + + expect(await retry).toBe(true); + expect(mockLoadAccount).toHaveBeenCalledTimes(1); + expect(useAuthStore.getState().session).not.toBeNull(); + // The start is confirmed: a failed switch away does not check it again. + mockLoadAccount.mockRejectedValueOnce(new NetworkError('offline')); + useAccountStore.setState({ accounts: [...useAccountStore.getState().accounts, account('acc-2', false)] }); + await useAuthStore.getState().switchAccount('acc-2'); + expect(mockRevalidate).toHaveBeenCalledTimes(1); + }); + // A switch that fails puts the account back. Its saved session was // never confirmed (the switch overtook the check), so the check runs // again, and the outbox waits for it. diff --git a/src/stores/auth-store.ts b/src/stores/auth-store.ts index 38d50fad..cc31ab97 100644 --- a/src/stores/auth-store.ts +++ b/src/stores/auth-store.ts @@ -323,6 +323,26 @@ let restoreUnderWay: Promise | null = null; // connect, until the server confirms it (revalidateRestoredSession). Its // session is not live yet: its server was not checked and its outbox waits. let unconfirmedStart: string | null = null; +/** The reads that start sent with the saved session. */ +let unconfirmedStartReads: { accountId: string; promise: Promise } | null = null; +/** + * The check of the saved session under way. A session retry (the network + * coming back) during a start from the saved session waits for it instead of + * loading the account beside it. + */ +let revalidation: { accountId: string; promise: Promise } | null = null; + +function readsOfStart(accountId: string): Promise | undefined { + return unconfirmedStartReads?.accountId === accountId ? unconfirmedStartReads.promise : undefined; +} + +function startRevalidation(accountId: string, reads?: Promise): Promise { + const promise: Promise = revalidateRestoredSession(accountId, reads).finally(() => { + if (revalidation?.promise === promise) revalidation = null; + }); + revalidation = { accountId, promise }; + return promise; +} async function switchesSettled(): Promise { while (pendingSwitch) await pendingSwitch.settled; @@ -627,6 +647,7 @@ function applyConnectedState( // A session from the server, or a start from the saved one, which sets // this again right after. unconfirmedStart = null; + unconfirmedStartReads = null; } export const useAuthStore = create((set, get) => ({ @@ -1057,7 +1078,7 @@ export const useAuthStore = create((set, get) => ({ // still in the snapshot taken then, and its server was checked. jmapClient.restoreSnapshot(origin.client); if (origin.live) refetchFeatureStores(accountId); - else if (unconfirmedStart === accountId) void revalidateRestoredSession(accountId); + else if (unconfirmedStart === accountId) void startRevalidation(accountId); set({ isLoading: false }); return; } @@ -1072,7 +1093,7 @@ export const useAuthStore = create((set, get) => ({ if (origin.accountId && origin.live) resumeOutbox(origin.accountId); // A start from the saved session whose check this switch overtook. else if (origin.accountId && unconfirmedStart === origin.accountId) { - void revalidateRestoredSession(origin.accountId); + void startRevalidation(origin.accountId); } }; try { @@ -1245,7 +1266,8 @@ export const useAuthStore = create((set, get) => ({ unconfirmedStart = target.id; perfMark('restore-done', { cached: true }); const reads = reloadFeatureStores(); - void revalidateRestoredSession(target.id, reads); + unconfirmedStartReads = { accountId: target.id, promise: reads }; + void startRevalidation(target.id, reads); void syncAccountDisplayName(target.id); return true; } @@ -1358,6 +1380,20 @@ export const useAuthStore = create((set, get) => ({ if (!target) return false; // The server refused its sign-in: only a new one brings it back. if (target.needsSignIn) return false; + // A start from the saved session: its check brings the session up, after + // the reads that start sent. Loading the account beside it checked the + // server while those reads were still landing. Only a check that ended + // without network leaves the load below to try. + if (unconfirmedStart === activeAccountId) { + const running = revalidation?.accountId === activeAccountId + ? revalidation.promise + : startRevalidation(activeAccountId, readsOfStart(activeAccountId)); + await running; + if (!isCurrent() || get().activeAccountId !== activeAccountId) return false; + if (get().session) return true; + if (get().reauth || unconfirmedStart === activeAccountId) return false; + } + const startReads = readsOfStart(activeAccountId); try { const ok = await jmapClient.loadAccount(activeAccountId); if (!ok || !isCurrent()) return false; @@ -1369,6 +1405,13 @@ export const useAuthStore = create((set, get) => ({ }); const fresh = jmapClient.currentSession!; followStoredServer(activeAccountId); + if (startReads) { + // What the start read from the saved session lands before a server + // that was replaced drops the caches. + const recorded = await readServerFingerprint(activeAccountId).catch(() => null); + if (recorded !== sessionFingerprint(fresh)) await startReads; + if (!isCurrent()) return false; + } await checkServerChange(activeAccountId, fresh); if (!isCurrent()) return false; applyConnectedState(set, fresh, target.serverUrl, target.username, activeAccountId); @@ -1449,6 +1492,7 @@ async function revalidateRestoredSession(accountId: string, reads?: Promise Date: Sat, 31 Oct 2026 18:52:00 +0100 Subject: [PATCH 9/9] fix: stop saving pgp keys from messages on their own and offer to save a key only for the sender's address --- locales/rn/en.json | 3 + src/components/email/MessageContent.tsx | 2 +- src/components/email/SecureMessageBanner.tsx | 35 ++++- src/components/email/use-secure-message.ts | 3 +- src/lib/crypto/__tests__/banner-rows.test.ts | 21 ++- .../__tests__/secure-message-keys.test.ts | 137 ++++++++++++++++++ src/lib/crypto/banner-rows.ts | 27 +++- src/lib/crypto/keyring.ts | 37 +++-- src/lib/crypto/secure-message.ts | 53 +++++-- 9 files changed, 285 insertions(+), 33 deletions(-) create mode 100644 src/lib/crypto/__tests__/secure-message-keys.test.ts diff --git a/locales/rn/en.json b/locales/rn/en.json index 37bf230c..381918ba 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -707,8 +707,11 @@ "failed": "Could not decrypt: {reason}", "invalid": "Invalid signature ({kind})", "invalid_reason": "Invalid signature ({kind}): {reason}", + "key_offered": "The message contains a public key for {email}. Fingerprint: {fingerprint}", "locked": "Encrypted ({kind}). Unlock your key to read it.", "no_key": "Encrypted ({kind}) to a key that is not on this device.", + "save_key": "Save key for {email}", + "save_key_failed": "Could not save the key", "self_signed": "The certificate is self-signed, not issued by a known authority.", "signed": "Valid signature ({kind})", "signed_by": "Valid signature ({kind}) from {signer}", diff --git a/src/components/email/MessageContent.tsx b/src/components/email/MessageContent.tsx index 17318918..95f64b38 100644 --- a/src/components/email/MessageContent.tsx +++ b/src/components/email/MessageContent.tsx @@ -130,7 +130,7 @@ export function MessageContent({ /> )} - {(secure.loading || secure.result) && } + {(secure.loading || secure.result) && } diff --git a/src/components/email/SecureMessageBanner.tsx b/src/components/email/SecureMessageBanner.tsx index 9bb159df..7ef5e46f 100644 --- a/src/components/email/SecureMessageBanner.tsx +++ b/src/components/email/SecureMessageBanner.tsx @@ -7,16 +7,22 @@ import { spacing, radius, typography, type ThemePalette } from '../../theme/toke import { useColors } from '../../theme/colors'; import { useLocaleStore } from '../../stores/locale-store'; import { useCryptoKeysStore, type CryptoKeyRecord } from '../../stores/crypto-keys-store'; -import type { SecureStatus } from '../../lib/crypto/secure-message'; -import { secureBannerRows, type BannerTone as Tone } from '../../lib/crypto/banner-rows'; +import type { OfferedPgpKey, SecureStatus } from '../../lib/crypto/secure-message'; +import { offeredKeyRow, secureBannerRows, type BannerTone as Tone } from '../../lib/crypto/banner-rows'; +import { savePgpKeyFor } from '../../lib/crypto/keyring'; +import { toast } from '../../stores/toast-store'; -/** Status banner above a PGP / S/MIME message, with Unlock for a locked key. */ -export function SecureMessageBanner({ status, loading }: { status: SecureStatus | null; loading?: boolean }) { +/** + * Status banner above a PGP / S/MIME message, with Unlock for a locked key + * and Save for a public key the message carries for its sender. + */ +export function SecureMessageBanner({ status, loading, offeredKey }: { status: SecureStatus | null; loading?: boolean; offeredKey?: OfferedPgpKey }) { const c = useColors(); const styles = React.useMemo(() => makeStyles(c), [c]); const t = useLocaleStore((s) => s.t); const keys = useCryptoKeysStore((s) => s.keys); const [unlockRecord, setUnlockRecord] = React.useState(null); + const [saving, setSaving] = React.useState(false); if (loading) { return ( @@ -28,11 +34,23 @@ export function SecureMessageBanner({ status, loading }: { status: SecureStatus } if (!status) return null; const rows = secureBannerRows(status, t); - if (rows.length === 0) return null; const color = (tone: Tone) => (tone === 'ok' ? c.success : tone === 'warn' ? c.warning : tone === 'error' ? c.error : c.textMuted); const lockedRecord = status.lockedKeyIds?.length ? keys.find((k) => k.kind === status.kind && status.lockedKeyIds!.includes(k.id)) ?? null : null; + const offer = offeredKeyRow(offeredKey, keys, t); + if (rows.length === 0 && !offer) return null; + const saveOffered = async () => { + if (!offeredKey) return; + setSaving(true); + try { + await savePgpKeyFor(offeredKey.armored, offeredKey.email); + } catch (err) { + toast.error(t('encryption.banner.save_key_failed', 'Could not save the key'), { message: err instanceof Error ? err.message : String(err) }); + } finally { + setSaving(false); + } + }; return ( @@ -48,6 +66,13 @@ export function SecureMessageBanner({ status, loading }: { status: SecureStatus ); })} + {offer && ( + + + {offer.text} + + + )} (ownKeys('pgp') as PgpKeyRecord[]).map((k) => ({ id: k.id, publicKeyArmored: k.publicKeyArmored, unlocked: isUnlocked(k) })), pgpPublicKeys: allPgpPublicKeys, smimeKeys: () => (ownKeys('smime') as SmimeCertRecord[]).map((k) => ({ id: k.id, certificate: base64Decode(k.certificate), pkcs8: unlockedSmimeKey(k.id)?.pkcs8 })), - rememberPgpKey: rememberSignerPgpKey, rememberCertificate: rememberSignerCertificate, }; } diff --git a/src/lib/crypto/__tests__/banner-rows.test.ts b/src/lib/crypto/__tests__/banner-rows.test.ts index 6514982c..8a232241 100644 --- a/src/lib/crypto/__tests__/banner-rows.test.ts +++ b/src/lib/crypto/__tests__/banner-rows.test.ts @@ -1,6 +1,6 @@ // What the reader's banner says for each PGP / S/MIME outcome (#58). import { describe, expect, it } from 'vitest'; -import { secureBannerRows } from '../banner-rows'; +import { offeredKeyRow, secureBannerRows } from '../banner-rows'; const t = (_k: string, fallback: string, vars?: Record) => fallback.replace(/\{(\w+)\}/g, (_, name: string) => String(vars?.[name] ?? '')); @@ -29,3 +29,22 @@ describe('secureBannerRows', () => { expect(secureBannerRows({ kind: 'pgp', encrypted: false, decrypted: false, signature: 'none' }, t)).toEqual([]); }); }); + +describe('offeredKeyRow', () => { + const offer = { armored: 'k', fingerprint: 'abcd1234abcd1234abcd1234abcd1234abcd1234', email: 'a@example.org' }; + + it('offers to save the key for the sender, with its fingerprint', () => { + expect(offeredKeyRow(offer, [], t)).toEqual({ + key: 'offer', + tone: 'muted', + text: 'The message contains a public key for a@example.org. Fingerprint: ABCD 1234 ABCD 1234 ABCD 1234 ABCD 1234 ABCD 1234', + action: 'Save key for a@example.org', + }); + }); + + it('offers nothing without a key, or once that key is stored', () => { + expect(offeredKeyRow(undefined, [], t)).toBeNull(); + expect(offeredKeyRow(offer, [{ kind: 'pgp', id: offer.fingerprint }], t)).toBeNull(); + expect(offeredKeyRow(offer, [{ kind: 'smime', id: offer.fingerprint }], t)).not.toBeNull(); + }); +}); diff --git a/src/lib/crypto/__tests__/secure-message-keys.test.ts b/src/lib/crypto/__tests__/secure-message-keys.test.ts new file mode 100644 index 00000000..7ba6ec67 --- /dev/null +++ b/src/lib/crypto/__tests__/secure-message-keys.test.ts @@ -0,0 +1,137 @@ +// Keys that arrive in a message (#58): any sender can put a public key in a +// message, and the app used to store every one it found, for whatever address +// it named, whatever the signature said. Stored keys verify signatures and +// are encrypted to, so that let a sender plant a key for someone else. Now a +// key is only offered (when it names the From address) and saved by the +// user; an S/MIME certificate is kept only when it names the From address. +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as openpgp from 'openpgp'; +import fs from 'node:fs'; +import path from 'node:path'; +import { buildSecureMessage } from '../secure-send'; +import { openSecureMessage } from '../secure-message'; +import { importPgpKey, lockAll, savePgpKeyFor } from '../keyring'; +import { useCryptoKeysStore } from '../../../stores/crypto-keys-store'; +import { keyringAccess } from '../../../components/email/use-secure-message'; +import type { MimeMessageInput } from '../mime'; +import { makePgpKey, makeSmimeCert, useNodeCrypto, type TestCert } from './helpers'; + +const secure = vi.hoisted(() => new Map()); +vi.mock('expo-secure-store', () => ({ + WHEN_UNLOCKED_THIS_DEVICE_ONLY: 6, + setItemAsync: vi.fn(async (k: string, v: string) => { secure.set(k, v); }), + getItemAsync: vi.fn(async (k: string) => secure.get(k) ?? null), + deleteItemAsync: vi.fn(async (k: string) => { secure.delete(k); }), +})); + +const encode = (s: string) => new TextEncoder().encode(s); +const stored = () => useCryptoKeysStore.getState().keys; + +let alice: Awaited>; +let mallory: Awaited>; +let bob: Awaited>; + +beforeAll(async () => { + useNodeCrypto(); + [alice, mallory, bob] = await Promise.all([ + makePgpKey('alice@example.org'), makePgpKey('mallory@example.org'), makePgpKey('bob@example.org'), + ]); +}, 60_000); + +beforeEach(() => { + secure.clear(); + lockAll(); + useCryptoKeysStore.setState({ keys: [] }); +}); + +/** An inline cleartext-signed message from `from` whose text carries `key`. */ +async function inlineSigned(from: string, signer: openpgp.PrivateKey, key: string, tamper = false): Promise { + const signed = await openpgp.sign({ + message: await openpgp.createCleartextMessage({ text: `Here is my key.\n\n${key}` }), + signingKeys: signer, + }) as string; + const body = tamper ? signed.replace('Here is my key.', 'Here is my kex.') : signed; + return encode(`From: ${from}\r\nContent-Type: text/plain\r\n\r\n${body.replace(/\r?\n/g, '\r\n')}`); +} + +describe('a PGP key in a message', () => { + it('is offered, not stored, when it names the From address and the signer is not known yet', async () => { + const res = await openSecureMessage(await inlineSigned(alice.email, alice.unlocked, alice.publicKey), alice.email, keyringAccess()); + expect(res?.status.signature).toBe('unknown'); + const fingerprint = (await openpgp.readKey({ armoredKey: alice.publicKey })).getFingerprint(); + expect(res?.offeredKey).toMatchObject({ email: alice.email, fingerprint }); + expect(stored()).toEqual([]); + + // Stored only once the user saves it. + await savePgpKeyFor(res!.offeredKey!.armored, res!.offeredKey!.email); + expect(stored()).toEqual([expect.objectContaining({ kind: 'pgp', id: fingerprint, emails: [alice.email], source: 'message' })]); + }, 60_000); + + it('is neither stored nor offered when none of its user ids is the From address', async () => { + const res = await openSecureMessage(await inlineSigned(alice.email, alice.unlocked, mallory.publicKey), alice.email, keyringAccess()); + expect(res?.offeredKey).toBeUndefined(); + expect(stored()).toEqual([]); + await expect(savePgpKeyFor(mallory.publicKey, alice.email)).rejects.toThrow(/not for alice@example.org/); + expect(stored()).toEqual([]); + }, 60_000); + + it('is neither stored nor offered when the signature fails', async () => { + await importPgpKey(alice.publicKey); + const res = await openSecureMessage(await inlineSigned(alice.email, alice.unlocked, alice.publicKey, true), alice.email, keyringAccess()); + expect(res?.status.signature).toBe('invalid'); + expect(res?.offeredKey).toBeUndefined(); + expect(stored()).toHaveLength(1); + }, 60_000); + + it('is not stored from a decrypted message either', async () => { + await importPgpKey(bob.privateKey, bob.passphrase); + const input: MimeMessageInput = { + from: { email: alice.email }, + to: [{ email: bob.email }], + subject: 'key', + textBody: `My new key:\n\n${mallory.publicKey}`, + }; + const raw = await buildSecureMessage(input, { mode: 'pgp', sign: false, encrypt: true }, { pgp: { encryptionKeys: [bob.publicKey] } }); + const access = { ...keyringAccess(), pgpDecryptionKeys: () => [bob.unlocked], pgpOwnKeys: () => [{ id: 'bob', publicKeyArmored: bob.publicKey, unlocked: true }] }; + const res = await openSecureMessage(raw, alice.email, access); + expect(res?.status.decrypted).toBe(true); + expect(res?.body?.text).toContain('BEGIN PGP PUBLIC KEY BLOCK'); + expect(res?.offeredKey).toBeUndefined(); + expect(stored().map((k) => k.emails)).toEqual([[bob.email]]); + }, 60_000); +}); + +describe('an S/MIME signer certificate', () => { + let signer: TestCert; + beforeAll(async () => { + signer = await makeSmimeCert('alice@example.org'); + }, 60_000); + + const signed = () => buildSecureMessage( + { from: { email: signer.email }, to: [{ email: 'bob@example.org' }], subject: 's', textBody: 'signed' }, + { mode: 'smime', sign: true, encrypt: false }, + { smime: { signer: { pkcs8: signer.pkcs8, certificate: signer.certificate, chain: [], algorithm: 'RSA-2048' } } }, + ); + + it('is not kept when it does not name the From address, even with a valid signature', async () => { + const res = await openSecureMessage(await signed(), 'mallory@example.org', keyringAccess()); + expect(res?.status.signature).toBe('valid'); + expect(stored()).toEqual([]); + }, 60_000); + + it('is kept when it names the From address', async () => { + await openSecureMessage(await signed(), signer.email, keyringAccess()); + await vi.waitFor(() => expect(stored()).toEqual([expect.objectContaining({ kind: 'smime', emails: [signer.email], source: 'message' })])); + }, 60_000); +}); + +describe('the reader', () => { + it('hands the offered key to the banner, which saves it only on the button', () => { + const root = path.resolve(__dirname, '../../../..'); + const content = fs.readFileSync(path.join(root, 'src/components/email/MessageContent.tsx'), 'utf8'); + expect(content).toContain('offeredKey={secure.result?.offeredKey}'); + const banner = fs.readFileSync(path.join(root, 'src/components/email/SecureMessageBanner.tsx'), 'utf8'); + expect(banner).toMatch(/onPress=\{\(\) => \{ void saveOffered\(\); \}\}>\{offer\.action\}/); + expect(banner.match(/savePgpKeyFor\(/g)).toHaveLength(1); + }); +}); diff --git a/src/lib/crypto/banner-rows.ts b/src/lib/crypto/banner-rows.ts index d1d36171..00d20ff2 100644 --- a/src/lib/crypto/banner-rows.ts +++ b/src/lib/crypto/banner-rows.ts @@ -1,5 +1,6 @@ // Banner rows for a PGP / S/MIME status (#58). -import type { SecureStatus } from './secure-message'; +import type { OfferedPgpKey, SecureStatus } from './secure-message'; +import { formatFingerprint } from './key-labels'; export type BannerTone = 'ok' | 'warn' | 'error' | 'muted'; type Translate = (key: string, fallback: string, vars?: Record) => string; @@ -49,3 +50,27 @@ export function secureBannerRows(status: SecureStatus, t: Translate): BannerRow[ return rows; } +/** + * The row that offers to save the public key a message carries for its + * From address, with its fingerprint to compare, and the label of the save + * button. Null without an offer, or once a key with that fingerprint is + * stored. + */ +export function offeredKeyRow( + offer: OfferedPgpKey | undefined, + stored: ReadonlyArray<{ kind: string; id: string }>, + t: Translate, +): (BannerRow & { action: string }) | null { + if (!offer) return null; + if (stored.some((k) => k.kind === 'pgp' && k.id === offer.fingerprint)) return null; + return { + key: 'offer', + tone: 'muted', + text: t('encryption.banner.key_offered', 'The message contains a public key for {email}. Fingerprint: {fingerprint}', { + email: offer.email, + fingerprint: formatFingerprint(offer.fingerprint), + }), + action: t('encryption.banner.save_key', 'Save key for {email}', { email: offer.email }), + }; +} + diff --git a/src/lib/crypto/keyring.ts b/src/lib/crypto/keyring.ts index 3bd8a666..d13d022a 100644 --- a/src/lib/crypto/keyring.ts +++ b/src/lib/crypto/keyring.ts @@ -219,23 +219,38 @@ export function allPgpPublicKeys(): string[] { return records().filter((k): k is PgpKeyRecord => k.kind === 'pgp').map((k) => k.publicKeyArmored); } -/** Remember the key of someone who signed a message, unless it is known already. */ -export async function rememberSignerPgpKey(armored: string): Promise { - try { - const info = await inspectKey(armored); - if (info.isPrivate) return; - if (records().some((k) => k.kind === 'pgp' && k.id === info.fingerprint.toLowerCase())) return; - upsert(pgpRecord(info, false, 'message')); - } catch { - // a broken embedded key: ignore +/** + * Save a public key that came in a message from `email`, when the user asks + * to (the reader's banner offers it). Refused unless one of the key's user + * ids is that address: a sender must not plant a key for someone else. + */ +export async function savePgpKeyFor(armored: string, email: string): Promise { + const info = await inspectKey(armored); + if (info.isPrivate) throw new Error('Only a public key can be saved from a message'); + const address = email.trim().toLowerCase(); + if (!address || !info.userIDs.some((u) => u.email.trim().toLowerCase() === address)) { + throw new Error('The key is not for ' + email); } + const known = records().find((k): k is PgpKeyRecord => k.kind === 'pgp' && k.id === info.fingerprint.toLowerCase()); + if (known) return known; + const record = pgpRecord(info, false, 'message'); + upsert(record); + // Open messages verify again with the key. + notify(); + return record; } -export async function rememberSignerCertificate(der: Uint8Array): Promise { +/** + * Keep the certificate of a valid S/MIME signature on a message from + * `fromEmail`, unless it is known already. Only one that names that address + * (subject email or SAN rfc822Name) is kept. + */ +export async function rememberSignerCertificate(der: Uint8Array, fromEmail: string): Promise { try { const { describeCertificate } = await import('./smime/certificate'); const info = await describeCertificate(der); - if (!info.emailAddresses.length) return; + const from = fromEmail.trim().toLowerCase(); + if (!from || !info.emailAddresses.some((e) => e.toLowerCase() === from)) return; if (records().some((k) => k.kind === 'smime' && k.id === info.fingerprint)) return; upsert(smimeRecord(info, der, [], false, 'message')); } catch { diff --git a/src/lib/crypto/secure-message.ts b/src/lib/crypto/secure-message.ts index 46a4ad08..c2671ce6 100644 --- a/src/lib/crypto/secure-message.ts +++ b/src/lib/crypto/secure-message.ts @@ -9,7 +9,7 @@ import type { BodyPart, Email } from '../../api/types'; import { binaryToBytes, bytesToBinary, decodeBody, findNode, parseEntity, type MimeNode } from './mime'; import { detectPgp, decryptMessage, verifyDetached, verifyCleartext, encryptionKeyIds, keyHasId, canonicalSignedBytes, - extractPublicKeyBlock, PgpArgon2UnsupportedError, type PgpSignatureSummary, + extractPublicKeyBlock, inspectKey, PgpArgon2UnsupportedError, type PgpSignatureSummary, } from './pgp'; import type { SmimeKeyMaterial, SmimeSignatureStatus } from './smime/cms'; @@ -43,11 +43,24 @@ export interface SecureAttachment { bytes: Uint8Array; } +/** + * A public key in the message for its From address. Never saved on its own: + * any sender can put a key in a message, so the banner offers it and only + * the user saves it (keyring savePgpKeyFor). + */ +export interface OfferedPgpKey { + armored: string; + fingerprint: string; + /** The From address, which one of the key's user ids names. */ + email: string; +} + export interface SecureResult { status: SecureStatus; /** The decrypted/verified body, or null when there is none to show (locked, failed). */ body: { html: string | null; text: string | null } | null; attachments: SecureAttachment[]; + offeredKey?: OfferedPgpKey; } /** What the reader needs from the key store; injected so tests need no SecureStore. */ @@ -60,8 +73,8 @@ export interface SecureKeyAccess { pgpPublicKeys(): string[]; /** Own S/MIME keys; `pkcs8` set when unlocked. */ smimeKeys(): SmimeKeyMaterial[]; - rememberPgpKey?(armored: string): Promise; - rememberCertificate?(der: Uint8Array): Promise; + /** Keep the certificate of a valid signature, if it names `fromEmail`. */ + rememberCertificate?(der: Uint8Array, fromEmail: string): Promise; } type PartLike = Pick & { subParts?: PartLike[] }; @@ -164,8 +177,7 @@ async function openPgp(raw: Uint8Array, fromEmail: string | undefined, keys: Sec } applyPgpSignature(status, sig, fromEmail); const rendered = await renderPayload(payload); - await maybeRememberEmbeddedKey(rendered.body?.text, keys); - return { status, ...rendered }; + return withOfferedKey({ status, ...rendered }, rendered.body?.text, fromEmail); } if (info.kind === 'pgp-mime-signed') { applyPgpSignature(status, await verifySignedPart(info.signedBytes, info.signature, verificationKeys), fromEmail); @@ -174,8 +186,7 @@ async function openPgp(raw: Uint8Array, fromEmail: string | undefined, keys: Sec // inline-signed const v = await verifyCleartext({ cleartext: info.cleartext, verificationKeysArmored: verificationKeys }); applyPgpSignature(status, v.signature, fromEmail); - await maybeRememberEmbeddedKey(v.text, keys); - return { status, body: { html: null, text: v.text }, attachments: [] }; + return withOfferedKey({ status, body: { html: null, text: v.text }, attachments: [] }, v.text, fromEmail); } catch (err) { status.error = err instanceof PgpArgon2UnsupportedError ? err.message : err instanceof Error ? err.message : String(err); if (status.encrypted) status.decrypted = false; @@ -194,9 +205,27 @@ async function verifySignedPart(signed: Uint8Array, signature: string, verificat return retry.status === 'valid' ? retry : first; } -async function maybeRememberEmbeddedKey(text: string | null | undefined, keys: SecureKeyAccess): Promise { +/** + * Offers a public key the text carries, only when one of its user ids is the + * From address and the signature (if any) did not fail. Nothing is stored + * here: the webmail's bulwark-gpg plugin imports keys only from its + * settings, and a key that arrived in a message would otherwise be used to + * verify and encrypt for whatever address it claims. + */ +async function withOfferedKey(result: SecureResult, text: string | null | undefined, fromEmail: string | undefined): Promise { + const from = fromEmail?.trim().toLowerCase(); + if (!from || result.status.signature === 'invalid') return result; const block = extractPublicKeyBlock(text ?? ''); - if (block && keys.rememberPgpKey) await keys.rememberPgpKey(block); + if (!block) return result; + try { + const info = await inspectKey(block); + if (info.isPrivate) return result; + if (!info.userIDs.some((u) => u.email.trim().toLowerCase() === from)) return result; + return { ...result, offeredKey: { armored: info.publicKeyArmored, fingerprint: info.fingerprint.toLowerCase(), email: from } }; + } catch { + // a broken embedded key: nothing to offer + return result; + } } function applySmimeSignature(status: SecureStatus, sig: SmimeSignatureStatus): void { @@ -228,7 +257,7 @@ async function openSmime(raw: Uint8Array, fromEmail: string | undefined, keys: S const entity = binaryToBytes(signed.raw); const sig = await cms.smimeVerifyDetached(decodeBody(sigPart), entity, fromEmail); applySmimeSignature(status, sig); - if (sig.signatureValid && sig.signerCertificate) await keys.rememberCertificate?.(sig.signerCertificate); + if (sig.signatureValid && sig.signerCertificate && fromEmail) await keys.rememberCertificate?.(sig.signerCertificate, fromEmail); return { status, ...(await renderPayload(entity)) }; } @@ -236,7 +265,7 @@ async function openSmime(raw: Uint8Array, fromEmail: string | undefined, keys: S if (cms.isSignedData(der)) { const v = await cms.smimeVerifyOpaque(der, fromEmail); applySmimeSignature(status, v.status); - if (v.status.signatureValid && v.status.signerCertificate) await keys.rememberCertificate?.(v.status.signerCertificate); + if (v.status.signatureValid && v.status.signerCertificate && fromEmail) await keys.rememberCertificate?.(v.status.signerCertificate, fromEmail); return { status, ...(await renderPayload(v.mimeBytes)) }; } @@ -274,7 +303,7 @@ async function openSmime(raw: Uint8Array, fromEmail: string | undefined, keys: S if (innerSigned && cms.isSignedData(innerSigned)) { const v = await cms.smimeVerifyOpaque(innerSigned, fromEmail); applySmimeSignature(status, v.status); - if (v.status.signatureValid && v.status.signerCertificate) await keys.rememberCertificate?.(v.status.signerCertificate); + if (v.status.signatureValid && v.status.signerCertificate && fromEmail) await keys.rememberCertificate?.(v.status.signerCertificate, fromEmail); payload = v.mimeBytes; } else if (inner.type === 'multipart/signed' && /pkcs7-signature/i.test(inner.params.protocol || '')) { const full = parseEntity(bytesToBinary(decrypted));