fix: fail requests under way when the desktop app closes its connections, and forget client certificates per account

This commit is contained in:
Linus Rath
2026-11-04 00:56:09 +01:00
parent 8549e3c4c6
commit 889f6ffbd1
8 changed files with 375 additions and 23 deletions
@@ -0,0 +1,129 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { JMAPClient, RequestTimeoutError } from '../jmap/client';
import { UploadXHR, uploadClient } from './helpers/upload-xhr';
/**
* The desktop app closed its connections under the page. A request under way then never settles
* by itself, so all that ends it is being told.
*/
const session = {
capabilities: { 'urn:ietf:params:jmap:core': {} },
accounts: { 'acct-1': { name: 'test', isPersonal: true, accountCapabilities: {} } },
primaryAccounts: { 'urn:ietf:params:jmap:mail': 'acct-1' },
apiUrl: 'https://mail.example.com/jmap/api',
downloadUrl: 'https://mail.example.com/jmap/download/{accountId}/{blobId}/{name}',
uploadUrl: 'https://mail.example.com/jmap/upload/{accountId}/',
eventSourceUrl: 'https://mail.example.com/jmap/eventsource',
};
let reset: () => void;
function withBridge(): void {
const listeners = new Set<() => void>();
reset = () => {
for (const listener of listeners) listener();
};
(window as { bulwarkDesktop?: unknown }).bulwarkDesktop = {
version: 1,
certificates: {
onConnectionsReset: (listener: () => void) => {
listeners.add(listener);
return () => listeners.delete(listener);
},
},
};
}
/** Takes the request, and nothing ever comes back. Only an abort ends it. */
const silent = (_url: unknown, init?: RequestInit) =>
new Promise<Response>((_resolve, reject) => {
init?.signal?.addEventListener('abort', () => reject(init.signal?.reason ?? new DOMException('aborted', 'AbortError')));
});
/** The one all of the client's calls go through. */
const requestOf = (client: JMAPClient) => (client as unknown as { request(calls: unknown[]): Promise<unknown> }).request.bind(client);
async function connectedClient(): Promise<JMAPClient> {
const client = new JMAPClient('https://mail.example.com', 'alice', 'secret');
vi.mocked(fetch).mockImplementationOnce(async () => new Response(JSON.stringify(session), { status: 200, headers: { 'Content-Type': 'application/json' } }));
await client.connect();
return client;
}
beforeEach(() => {
vi.stubGlobal('fetch', vi.fn());
});
afterEach(() => {
delete (window as { bulwarkDesktop?: unknown }).bulwarkDesktop;
vi.useRealTimers();
vi.unstubAllGlobals();
vi.restoreAllMocks();
});
describe('when the desktop app closes its connections', () => {
it('a send under way fails as "may have sent", at once, and isn\'t sent again', async () => {
withBridge();
const client = await connectedClient();
vi.mocked(fetch).mockClear();
vi.mocked(fetch).mockImplementation(silent);
const send = requestOf(client)([['EmailSubmission/set', { accountId: 'acct-1', create: { s: {} } }, '0']]);
await vi.waitFor(() => expect(fetch).toHaveBeenCalledTimes(1));
reset();
await expect(send).rejects.toBeInstanceOf(RequestTimeoutError);
await new Promise((resolve) => setTimeout(resolve, 1200));
expect(fetch, 'the client retries other failures of a read after 1 s: never this').toHaveBeenCalledTimes(1);
});
it('a read under way fails too, and nothing waits for its deadline', async () => {
withBridge();
const client = await connectedClient();
vi.mocked(fetch).mockClear();
vi.mocked(fetch).mockImplementation(silent);
const read = requestOf(client)([['Mailbox/get', { accountId: 'acct-1' }, '0']]);
await vi.waitFor(() => expect(fetch).toHaveBeenCalledTimes(1));
reset();
await expect(read).rejects.toBeInstanceOf(RequestTimeoutError);
});
it('a body still being read is caught as well', async () => {
withBridge();
const client = await connectedClient();
vi.mocked(fetch).mockClear();
let signal: AbortSignal | undefined;
vi.mocked(fetch).mockImplementation(async (_url, init) => {
signal = init?.signal ?? undefined;
return new Response('{}', { status: 200 });
});
await requestOf(client)([['Mailbox/get', { accountId: 'acct-1' }, '0']]).catch(() => {});
expect(signal?.aborted).toBe(false);
reset();
expect(signal?.reason).toBeInstanceOf(RequestTimeoutError);
});
it('an upload under way fails the same way, not five minutes later', async () => {
withBridge();
UploadXHR.instances = [];
vi.stubGlobal('XMLHttpRequest', UploadXHR);
const upload = uploadClient().uploadBlob(new File(['x'], 'a.txt', { type: 'text/plain' }), { onProgress: () => {} });
await vi.waitFor(() => expect(UploadXHR.instances).toHaveLength(1));
reset();
await expect(upload).rejects.toBeInstanceOf(RequestTimeoutError);
expect(UploadXHR.instances[0].abort).toHaveBeenCalledOnce();
expect(UploadXHR.instances).toHaveLength(1);
});
it('nothing changes in a browser, where a deadline is all there is', async () => {
const client = await connectedClient();
vi.mocked(fetch).mockClear();
vi.useFakeTimers();
vi.mocked(fetch).mockImplementation(silent);
const read = requestOf(client)([['Mailbox/get', { accountId: 'acct-1' }, '0']]);
const failed = expect(read).rejects.toBeInstanceOf(RequestTimeoutError);
await vi.advanceTimersByTimeAsync(0);
expect(vi.getTimerCount(), 'only the deadline').toBe(1);
await vi.advanceTimersByTimeAsync(10 * 60_000);
await failed;
});
});
+26
View File
@@ -0,0 +1,26 @@
import { getDesktopBridge } from './bridge';
/**
* The desktop app's connections were closed under the page: at the last sign-out with a client
* certificate in use, when a policy changed, or when Chromium's network process crashed. A
* request under way then never settles, as Electron's request proxy doesn't pass the loss on. So
* main says so, and whoever holds a request gives up on it. Nothing happens in a browser.
*/
let wiredTo: unknown = null;
const listeners = new Set<() => void>();
/** Null where there's nothing to listen to: in a browser, or on an older shell. */
export function onConnectionsReset(listener: () => void): (() => void) | null {
const bridge = getDesktopBridge();
const subscribe = bridge?.certificates?.onConnectionsReset;
if (!subscribe) return null;
if (wiredTo !== bridge) {
wiredTo = bridge;
subscribe(() => {
for (const each of [...listeners]) each();
});
}
listeners.add(listener);
return () => listeners.delete(listener);
}
+11 -5
View File
@@ -11,12 +11,18 @@ export function clearKeystoreGrants(): void {
}
/**
* Forgets which client certificate goes to which server, with what the app keeps of it in
* memory, so the next account at this computer gets asked. Nothing happens in a browser or on
* a shell without the call.
* Forgets which client certificate goes to which server. An account's own sign-out takes what
* was chosen under it. The last sign-out takes everything, with what the app keeps of it in
* memory, so the next person at this computer gets asked. That restarts the app's connections,
* so an account switch never comes here. Nothing happens in a browser or on a shell without the call.
*/
export function clearDesktopCertificateChoices(): void {
void Promise.resolve(getDesktopBridge()?.certificates?.clearChoices?.()).catch(() => {});
export function clearDesktopCertificateChoices(scope: { account?: string; last?: boolean }): void {
void Promise.resolve(getDesktopBridge()?.certificates?.clearChoices?.(scope)).catch(() => {});
}
/** Which account a certificate chosen from now on belongs to. */
export function noteDesktopCertificateAccount(account: string | null): void {
void Promise.resolve(getDesktopBridge()?.certificates?.setAccount?.(account)).catch(() => {});
}
/**
+10 -2
View File
@@ -324,8 +324,16 @@ export interface DesktopKeystoreBridge {
}
export interface DesktopCertificatesBridge {
/** Forgets the client certificates chosen for servers, for the next account. Only narrows. */
clearChoices: () => Promise<void>;
/**
* Forgets client certificates chosen for servers. `account`: those chosen while that account was
* active, at its own sign-out. `last`: all of them, and what the app keeps of them in memory, at
* the sign-out that leaves nobody. Only narrows.
*/
clearChoices: (scope: { account?: string; last?: boolean }) => Promise<void>;
/** The account a choice made from now on belongs to. */
setAccount: (account: string | null) => Promise<void>;
/** The app's connections were closed: a request under way will never settle by itself. */
onConnectionsReset: (listener: () => void) => () => void;
}
export interface DesktopPolicyBridge {
+28 -4
View File
@@ -1,5 +1,6 @@
import { recordJmapCall, reportJmapFailure } from "@/lib/error-reporting/client";
import { getDesktopBridge } from '@/lib/desktop/bridge';
import { onConnectionsReset } from '@/lib/desktop/connections-reset';
import { desktopJmapBase, desktopJmapOrigin, isDesktopProxiedServer, withDesktopBlobType } from '@/lib/desktop/jmap-route';
import { advertisesMailboxMetadata, bulwarkEntries, escapeMetadataPathSegment, MAX_METADATA_ENTRY_BYTES, METADATA_CAPABILITY, MetadataPatchError, MetadataRefusedError, MetadataTooLargeError, MetadataUnsupportedError, metadataValueBytes, namespaceFor } from "./bulwark-metadata";
import { IncompleteDeleteError } from "./delete-error";
@@ -1855,14 +1856,23 @@ export class JMAPClient implements IJMAPClient {
// "we gave up" from "the caller cancelled", and callers must be able to tell
// those apart (a cancelled send is not a failed send).
let timedOut = false;
const timer = setTimeout(() => {
const giveUp = () => {
timedOut = true;
controller.abort();
}, timeoutMs);
// The reason goes on to a body still being read after this returned.
controller.abort(new RequestTimeoutError(timeoutMs));
};
const timer = setTimeout(giveUp, timeoutMs);
// The desktop app closed its connections, so this will never settle by itself. It's what a
// deadline running out means: the request may have reached the server, only the answer is
// lost, and it isn't replayed (authenticatedFetch).
const stopWatching = onConnectionsReset(giveUp);
try {
return await fetch(url, { ...withJmapCredentials(init), headers, signal: controller.signal });
const response = await fetch(url, { ...withJmapCredentials(init), headers, signal: controller.signal });
if (stopWatching) setTimeout(stopWatching, timeoutMs);
return response;
} catch (error) {
stopWatching?.();
if (timedOut) throw new RequestTimeoutError(timeoutMs);
throw error;
} finally {
@@ -6363,6 +6373,7 @@ export class JMAPClient implements IJMAPClient {
const cleanup = () => {
clearTimeout(timer);
signal?.removeEventListener('abort', onAbort);
stopWatchingConnections?.();
xhr.onload = xhr.onerror = xhr.onabort = xhr.ontimeout = null;
xhr.upload.onprogress = null;
};
@@ -6379,6 +6390,7 @@ export class JMAPClient implements IJMAPClient {
};
const onAbort = () => abortWith(new DOMException('Upload aborted', 'AbortError'));
const onTimeout = () => abortWith(new RequestTimeoutError(JMAPClient.TRANSFER_TIMEOUT_MS));
const stopWatchingConnections = onConnectionsReset(onTimeout);
const resetWatchdog = () => {
clearTimeout(timer);
// XHR uses this budget for inactivity, unlike fetch's deadline until
@@ -11670,6 +11682,8 @@ export class JMAPClient implements IJMAPClient {
* Checking on the way back in makes the recovery immediate rather than
* leaving the app on a socket that will never deliver another byte.
*/
private stopWatchingConnections: (() => void) | null = null;
private recycleStaleSSE(): void {
if (this.intentionallyDisconnected) return;
if (!this.sseAbortController) return;
@@ -11704,6 +11718,14 @@ export class JMAPClient implements IJMAPClient {
}
};
window.addEventListener('online', this.onlineHandler);
// A stream under way when the desktop app closes its connections would stay silent.
this.stopWatchingConnections = onConnectionsReset(() => {
if (this.intentionallyDisconnected || !this.sseAbortController) return;
this.stopSSEPingMonitor();
this.sseAbortController.abort();
this.sseAbortController = null;
this.scheduleSSEReconnect();
});
}
}
@@ -11716,6 +11738,8 @@ export class JMAPClient implements IJMAPClient {
window.removeEventListener('online', this.onlineHandler);
this.onlineHandler = null;
}
this.stopWatchingConnections?.();
this.stopWatchingConnections = null;
}
onConnectionChange(callback: (connected: boolean) => void): void {
@@ -79,27 +79,43 @@ describe('sign-out with plugins', () => {
(window as { bulwarkDesktop?: unknown }).bulwarkDesktop = { version: 1, certificates: { clearChoices } };
};
it('are forgotten at sign-out', async () => {
it('all of them at the sign-out that leaves nobody', async () => {
const clearChoices = vi.fn().mockResolvedValue(undefined);
withBridge(clearChoices);
await useAuthStore.getState().logout();
expect(clearChoices).toHaveBeenCalledTimes(1);
expect(clearChoices.mock.calls[0][0]).toMatchObject({ last: true });
});
it('when all accounts sign out', async () => {
it('all of them when all accounts sign out', async () => {
const clearChoices = vi.fn().mockResolvedValue(undefined);
withBridge(clearChoices);
await useAuthStore.getState().logoutAll();
expect(clearChoices).toHaveBeenCalled();
expect(clearChoices).toHaveBeenCalledWith({ last: true });
});
it('and when switching to another account', () => {
it('only its own while another account stays', async () => {
const clearChoices = vi.fn().mockResolvedValue(undefined);
withBridge(clearChoices);
const accounts = useAccountStore.getState();
const entry = (id: string) => ({ id, label: id, serverUrl: 'https://mail.example.test', username: id, authMode: 'basic' as const, rememberMe: true, displayName: id, email: id, lastLoginAt: 1, isConnected: true, hasError: false, isDefault: false, cookieSlot: id === 'a1' ? 0 : 1, avatarColor: '#000' });
useAccountStore.setState({ ...accounts, accounts: [entry('a1'), entry('a2')], activeAccountId: 'a1' });
useAuthStore.setState({ isAuthenticated: true, activeAccountId: 'a1' });
await useAuthStore.getState().logout();
expect(clearChoices).toHaveBeenCalledWith({ account: 'a1', last: false });
expect(clearChoices.mock.calls.some(([scope]) => scope.last === true)).toBe(false);
});
it('nothing when switching to another account or searching across them', () => {
const clearChoices = vi.fn().mockResolvedValue(undefined);
const setAccount = vi.fn().mockResolvedValue(undefined);
(window as { bulwarkDesktop?: unknown }).bulwarkDesktop = { version: 1, certificates: { clearChoices, setAccount } };
useAuthStore.setState({ isAuthenticated: true, activeAccountId: 'a1' });
expect(clearChoices).not.toHaveBeenCalled();
useAuthStore.setState({ activeAccountId: 'a2' });
expect(clearChoices).toHaveBeenCalledTimes(1);
useAuthStore.setState({ activeAccountId: 'a1' });
expect(clearChoices).not.toHaveBeenCalled();
expect(setAccount.mock.calls.at(-1)).toEqual(['a1']);
expect(setAccount).toHaveBeenCalledWith('a2');
});
it('signs out all the same on a shell without the call, or one that fails', async () => {
@@ -0,0 +1,118 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
/**
* A token renewal holds the token lock for every window. A request that never answers, as one
* caught when the desktop app closes its connections, can't be left to hold it for good.
*/
const SERVER = 'https://mail.example.test';
function account(id: string, slot: number) {
return {
id,
label: id,
serverUrl: SERVER,
username: `${id}@example.test`,
authMode: 'oauth' as const,
rememberMe: true,
displayName: id,
email: `${id}@example.test`,
lastLoginAt: 1,
isConnected: false,
hasError: false,
isDefault: true,
cookieSlot: slot,
avatarColor: '#000',
};
}
describe('a token renewal that gets no answer', { timeout: 30_000 }, () => {
const held = new Set<string>();
let renewals = 0;
beforeEach(() => {
vi.resetModules();
held.clear();
renewals = 0;
Object.defineProperty(navigator, 'locks', {
configurable: true,
value: {
request: async (name: string, options: { ifAvailable?: boolean }, callback: (lock: unknown) => unknown) => {
if (options.ifAvailable && held.has(name)) return callback(null);
held.add(name);
try {
return await callback({});
} finally {
held.delete(name);
}
},
},
});
vi.stubGlobal('fetch', vi.fn((url: unknown, init?: RequestInit) => {
if (String(url).includes('/api/auth/token') && init?.method === 'PUT') {
renewals += 1;
return new Promise((_resolve, reject) => {
init.signal?.addEventListener('abort', () => reject(new DOMException('aborted', 'AbortError')));
});
}
return Promise.resolve(new Response('{}', { status: 200 }));
}));
});
afterEach(() => {
delete (window as { bulwarkDesktop?: unknown }).bulwarkDesktop;
delete (navigator as { locks?: unknown }).locks;
vi.clearAllTimers();
vi.useRealTimers();
vi.unstubAllGlobals();
vi.restoreAllMocks();
});
async function storeWithAccount() {
const { useAuthStore } = await import('../auth-store');
const { useAccountStore } = await import('../account-store');
useAccountStore.setState({ accounts: [account('A', 0)], activeAccountId: 'A', defaultAccountId: 'A' });
useAuthStore.setState({ isAuthenticated: true, isLoading: false, serverUrl: SERVER, username: 'A@example.test', activeAccountId: 'A', authMode: 'oauth' });
// After the imports: they don't finish on a stopped clock.
vi.useFakeTimers();
return useAuthStore;
}
it('lets go of the lock after its time, and counts as an outage', async () => {
const store = await storeWithAccount();
const done = vi.fn();
void store.getState().refreshAccessToken({ accountId: 'A' }).then(done);
await vi.advanceTimersByTimeAsync(0);
expect(renewals).toBe(1);
expect(held.size, 'it holds the lock while it runs').toBe(1);
await vi.advanceTimersByTimeAsync(29_000);
expect(done).not.toHaveBeenCalled();
await vi.advanceTimersByTimeAsync(1_000);
expect(held.size).toBe(0);
expect(done).toHaveBeenCalledWith(null);
expect(store.getState().isAuthenticated, 'nobody is signed out for it').toBe(true);
});
it('lets go at once when the desktop app says its connections went', async () => {
let reset = () => {};
(window as { bulwarkDesktop?: unknown }).bulwarkDesktop = {
version: 1,
certificates: {
setAccount: async () => {},
onConnectionsReset: (listener: () => void) => {
reset = listener;
return () => {};
},
},
};
const store = await storeWithAccount();
const done = vi.fn();
void store.getState().refreshAccessToken({ accountId: 'A' }).then(done);
await vi.advanceTimersByTimeAsync(0);
expect(held.size).toBe(1);
reset();
await vi.advanceTimersByTimeAsync(0);
expect(held.size).toBe(0);
expect(done).toHaveBeenCalledWith(null);
});
});
+31 -6
View File
@@ -18,7 +18,8 @@ import { generateAccountId, MAX_ACCOUNT_SLOTS } from '@/lib/account-utils';
import { replaceWindowLocation, getPathPrefix, getLocaleFromPath, apiFetch } from '@/lib/browser-navigation';
import { IS_DESKTOP_BUILD } from '@/lib/desktop/flag';
import { getDesktopBridge } from '@/lib/desktop/bridge';
import { clearDesktopCertificateChoices, clearDesktopCrashFiles, clearKeystoreGrants } from '@/lib/desktop/keystore-grants';
import { clearDesktopCertificateChoices, clearDesktopCrashFiles, clearKeystoreGrants, noteDesktopCertificateAccount } from '@/lib/desktop/keystore-grants';
import { onConnectionsReset } from '@/lib/desktop/connections-reset';
import { DESKTOP_CLIENT_ID_KEY } from '@/lib/desktop/browser-sign-in';
import { isEmbedded, notifyParent } from '@/lib/iframe-bridge';
import { snapshotAccount, restoreAccount, clearAllStores, evictAccount, evictAll, forgetAccountRows } from '@/lib/account-state-manager';
@@ -359,6 +360,26 @@ function trackSessionWrite<T>(slot: number, run: (signal: AbortSignal) => Promis
return request;
}
/** It holds the token lock, for every window: a request that never answers can't be left to hold it. */
const TOKEN_RENEWAL_TIMEOUT_MS = 30_000;
function renewalSignal(outer: AbortSignal): { signal: AbortSignal; done: () => void } {
const controller = new AbortController();
const stop = () => controller.abort();
if (outer.aborted) stop();
else outer.addEventListener('abort', stop, { once: true });
const timer = setTimeout(stop, TOKEN_RENEWAL_TIMEOUT_MS);
const stopWatching = onConnectionsReset(stop);
return {
signal: controller.signal,
done: () => {
clearTimeout(timer);
outer.removeEventListener('abort', stop);
stopWatching?.();
},
};
}
const TIMED_OUT = Symbol('timed out');
function withTimeout<T>(promise: Promise<T>, ms: number): Promise<T | typeof TIMED_OUT> {
@@ -649,8 +670,10 @@ function fetchSlotAccessToken(slot: number, opts: { force?: boolean; remember?:
const force = opts.force && !contended;
if (!IS_LITE) {
const query = `${force ? '&force=true' : ''}${opts.remember === false ? '&remember=false' : ''}`;
return apiFetch(`/api/auth/token?slot=${slot}${query}`, { method: 'PUT', signal })
.then((res) => noteSessionAnswer(res, slot));
const renewal = renewalSignal(signal);
return apiFetch(`/api/auth/token?slot=${slot}${query}`, { method: 'PUT', signal: renewal.signal })
.then((res) => noteSessionAnswer(res, slot))
.finally(renewal.done);
}
// Like the route's cookie cache: a restore resumes with the token it had,
// since a renewal before the refresh token's `nbf` is refused (#552).
@@ -2544,7 +2567,7 @@ export const useAuthStore = create<AuthState>()(
useSettingsStore.getState().disableSync();
clearKeystoreGrants();
clearDesktopCertificateChoices();
clearDesktopCertificateChoices({ account: accountId ?? undefined, last: useAccountStore.getState().accounts.length === 0 });
clearDesktopCrashFiles();
void authHooks.onAfterLogout.emit({
accountId: accountId ?? 'all',
@@ -2636,6 +2659,7 @@ export const useAuthStore = create<AuthState>()(
// mail never passed a question, so it waits for their next sign-in.
accountStore.removeAccount(remaining.id, 'evicted');
}
clearDesktopCertificateChoices({ last: true });
}
// Full logout. The credentials are cleared before the page state:
@@ -2680,6 +2704,7 @@ export const useAuthStore = create<AuthState>()(
evictAccount(accountId);
forgetAccountRows(accountId, get().client);
accountStore.removeAccount(accountId, reason);
clearDesktopCertificateChoices({ account: accountId });
forgetPrincipals(accountId);
clearPluginStorageForAccount(accountId);
},
@@ -2726,7 +2751,7 @@ export const useAuthStore = create<AuthState>()(
}
clearKeystoreGrants();
clearDesktopCertificateChoices();
clearDesktopCertificateChoices({ last: true });
clearDesktopCrashFiles();
void authHooks.onAfterLogout.emit({
accountId: 'all'
@@ -3874,9 +3899,9 @@ useAuthStore.subscribe((state, prev) => {
const ready = state.isAuthenticated && !!state.client && !!state.activeAccountId;
const wasReady = prev.isAuthenticated && !!prev.client && !!prev.activeAccountId;
if (ready && !wasReady) void authHooks.onAuthReady.emit({ accountId: state.activeAccountId });
if (state.activeAccountId !== prev.activeAccountId) noteDesktopCertificateAccount(state.activeAccountId);
if (state.activeAccountId && prev.activeAccountId && state.activeAccountId !== prev.activeAccountId) {
clearKeystoreGrants();
clearDesktopCertificateChoices();
clearDesktopCrashFiles();
void authHooks.onAccountSwitch.emit({ accountId: state.activeAccountId, previousAccountId: prev.activeAccountId });
}