fix(jmap): CSP blocked wss:, WS circuit breaker too slow to trip
Two real bugs in the previous WS-push commit, both found while building the
integration test for it (not theoretical - each reproduced and verified
before and after the fix):
1. proxy.ts's production CSP (`connect-src 'self' https:`) has no `wss:`
term, so `new WebSocket(...)` was blocked before any network attempt at
all - confirmed by listening for `securitypolicyviolation` against the
real reference server (stalwart.sandbox.vnc.de, HTTPS): the WS feature
was entirely inert in a production build, for every server, not just
ones with an incompatible auth model. Fixed by adding `wss:` alongside
`https:` in production - no new trust surface, since `https:` here
already allows fetch/XHR to any TLS host (needed for
ALLOW_CUSTOM_JMAP_ENDPOINT / multi-server setups), so extending that same
model to WebSocket is consistent, not a new precedent. Verified after the
fix: the same probe now reaches the network and gets a real (expected)
auth rejection from Stalwart instead of a CSP block.
2. lib/jmap/client.ts's circuit breaker (5 attempts, 1s/30s backoff) could
take up to ~31s to give up on WS and fall back to SSE. Against a server
that fails the handshake instantly and deterministically every time (the
auth-header limitation documented in the previous commit), that's ~31s
of NO live push at all - WS hasn't succeeded and hasn't given up yet, so
SSE never starts connecting, and any mail delivered in that window was
silently missed (SSE only streams changes from the moment it connects,
no catch-up). Reproduced directly: a real SMTP delivery sent during that
window never reached the notification bridge.
Fixed two ways:
- Tightened the ladder to a 200ms base / 5s cap / 3-attempt circuit
breaker (worst case ~1.75s instead of ~31s) - still genuine
exponential-with-jitter backoff, just tuned for a failure mode that's
fast and deterministic rather than slow and flaky. A slow/real
network issue is unaffected: a hanging attempt is still bounded by
the browser's own WebSocket connect timeout, not by these constants.
- setupPushNotifications() now primes a polling baseline
(fetchCurrentStates()) in parallel with the WS attempt, and
fallbackFromWebSocket() diffs against it (checkForStateChanges())
BEFORE connectSSE()/startPollingFallback() get a chance to erase that
opportunity. This is what actually closes the gap rather than just
shrinking it: it catches a change that happened to the primary
account during the (now much shorter) WS retry window.
electron/main.ts also gets a test-only escape hatch (ELECTRON_LOAD_URL): set
it to skip spawning the standalone server and load that URL instead. Real
users and every packaging/CI path never set it - added because verifying
the fixes above against this repo's own local Stalwart fixture (deliberately
plaintext HTTP - integration/webmail.Dockerfile makes the identical
trade-off for the browser-based suite) needs a dev-mode Next.js server
(proxy.ts only widens connect-src for plain http/ws in dev), not the
production standalone build electron/main.ts normally boots.
next.config.ts: added 127.0.0.1 to allowedDevOrigins alongside the existing
LAN entry - electron/main.ts always loads its window at 127.0.0.1, so a
dev-mode Electron run (only used by the escape hatch above) needs it in this
allowlist the same as any other cross-origin dev client would.
Verified: full lib/__tests__ JMAP suite still green (158/158); npm run
test:electron still green (4/4); the raw WebSocket probe against the real
sandbox now reaches the network post-fix instead of being CSP-blocked.
This commit is contained in:
+28
-10
@@ -112,7 +112,17 @@ function stopStandaloneServer(): void {
|
||||
}
|
||||
|
||||
async function createMainWindow(): Promise<void> {
|
||||
const url = await startStandaloneServer();
|
||||
// Test-only escape hatch: when set, skip spawning the standalone server
|
||||
// entirely and load this URL instead. Used by
|
||||
// integration/tests/11-electron-notification.spec.ts, which needs a
|
||||
// dev-mode Next.js server (proxy.ts's CSP only widens connect-src to
|
||||
// allow plain-HTTP/ws JMAP in dev - see that file's comments) to reach
|
||||
// the integration fixture's deliberately-plaintext local Stalwart,
|
||||
// exactly the same trade-off integration/webmail.Dockerfile already makes
|
||||
// for the browser-based integration suite. Never set by real users or by
|
||||
// any of the packaging/CI paths - those always go through
|
||||
// startStandaloneServer() below.
|
||||
const url = process.env.ELECTRON_LOAD_URL || (await startStandaloneServer());
|
||||
|
||||
mainWindow = new BrowserWindow({
|
||||
width: 1280,
|
||||
@@ -133,18 +143,26 @@ async function createMainWindow(): Promise<void> {
|
||||
}
|
||||
|
||||
// --- Native notification bridge --------------------------------------------
|
||||
// Called from the preload's `window.vnc.showNotification` (electron/preload.ts).
|
||||
// Electron's own Notification API is the desktop shell's notification path -
|
||||
// it sits alongside, not in place of, the browser/PWA's service-worker push
|
||||
// path (public/sw.js's `push`/`notificationclick` handlers + lib/web-push.ts).
|
||||
// Which of the two actually gets wired up to real mail-delivery events is a
|
||||
// separate decision (VNCprodbuild Phase 1 steps 4-6); this handler is just
|
||||
// the plumbing that lets the renderer trigger a native OS notification at
|
||||
// all, so it can be exercised end-to-end from a smoke test now instead of
|
||||
// bolted on untested later.
|
||||
// Called from the preload's `window.vnc.showNotification` (electron/preload.ts),
|
||||
// itself called from lib/electron-bridge.ts's showElectronNotification(),
|
||||
// itself called from app/(main)/[locale]/page.tsx's "new mail arrived"
|
||||
// effect whenever lib/jmap/client.ts's push pipeline (WebSocket, or its SSE/
|
||||
// polling fallback - see that file's circuit breaker) reports a genuine new
|
||||
// message. Electron's own Notification API is the desktop shell's
|
||||
// notification path - it sits alongside, not in place of, the browser/PWA's
|
||||
// service-worker push path (public/sw.js's `push`/`notificationclick`
|
||||
// handlers + lib/web-push.ts).
|
||||
ipcMain.handle(
|
||||
"vnc:show-notification",
|
||||
(_event, title: string, options?: { body?: string; tag?: string }) => {
|
||||
// Test-only observability hook, read via Playwright's
|
||||
// electronApp.evaluate(({ app }) => ...) - see
|
||||
// integration/tests/11-electron-notification.spec.ts. Not gated behind
|
||||
// NODE_ENV: it's an inert counter with no behavioral effect, cheaper
|
||||
// than maintaining a second code path just for tests.
|
||||
const counters = app as unknown as { __notificationCallCount?: number };
|
||||
counters.__notificationCallCount = (counters.__notificationCallCount ?? 0) + 1;
|
||||
|
||||
if (!Notification.isSupported()) {
|
||||
return { shown: false };
|
||||
}
|
||||
|
||||
+65
-12
@@ -6081,14 +6081,29 @@ export class JMAPClient implements IJMAPClient {
|
||||
private static readonly SSE_RECONNECT_DELAY = 3_000;
|
||||
private static readonly SSE_PING_TIMEOUT = 90_000; // 3x the 30s ping interval
|
||||
|
||||
// Exponential backoff with full jitter (0..cap), doubling from a 1s base
|
||||
// and capping at 30s - unlike SSE's fixed 3s retry, a long-lived WebSocket
|
||||
// genuinely needs backoff: it can be closed by a server-side idle timeout,
|
||||
// a proxy, or a laptop sleep/wake cycle repeatedly in a row, and hammering
|
||||
// a reconnect every 3s in that situation is exactly the kind of thing that
|
||||
// gets a client rate-limited (see isRateLimited()/setRateLimited() above).
|
||||
private static readonly WS_RECONNECT_BASE_DELAY = 1_000;
|
||||
private static readonly WS_RECONNECT_MAX_DELAY = 30_000;
|
||||
// Exponential backoff with full jitter (0..cap), doubling from a 200ms
|
||||
// base and capping at 5s.
|
||||
//
|
||||
// Deliberately much tighter than a "normal" reconnect ladder (something
|
||||
// like 1s/30s would be the textbook default for a flaky network) - and
|
||||
// tuned from a real, measured failure mode, not guessed: the auth
|
||||
// limitation described above fails FAST and DETERMINISTICALLY (the
|
||||
// handshake is rejected before the socket ever opens, in well under a
|
||||
// second, every single time), not slowly. Verified empirically (see
|
||||
// integration/tests/11-electron-notification.spec.ts's development) that
|
||||
// the original 1s-base/30s-cap/5-attempt ladder let the circuit breaker
|
||||
// take up to ~31s to trip, during which there is NO live push at all
|
||||
// (WS hasn't succeeded and hasn't given up yet, so SSE never even starts
|
||||
// connecting) - a real mail delivery landing in that window was missed
|
||||
// entirely, since SSE only streams future changes and does no catch-up
|
||||
// fetch on connect. This tighter ladder closes that gap to a fraction of
|
||||
// a second for the fast-fail case while remaining exactly as protective
|
||||
// for a genuinely slow/flaky network: a hanging attempt is still bounded
|
||||
// by the browser's own WebSocket connect timeout regardless of these
|
||||
// constants, which govern only the GAP between attempts, not how long a
|
||||
// single attempt is allowed to hang.
|
||||
private static readonly WS_RECONNECT_BASE_DELAY = 200;
|
||||
private static readonly WS_RECONNECT_MAX_DELAY = 5_000;
|
||||
// App-level heartbeat: a WebSocket can sit in "open" readyState for a long
|
||||
// time after the underlying network path is actually gone (sleep, network
|
||||
// switch, a NAT/proxy that silently drops idle connections) - TCP alone
|
||||
@@ -6102,10 +6117,10 @@ export class JMAPClient implements IJMAPClient {
|
||||
// attempts that never reach "open" (a connection that opened fine and
|
||||
// later dropped does not count - see connectWebSocket's openedSuccessfully
|
||||
// tracking). Bounds the cost of the auth limitation described above to a
|
||||
// handful of quick handshake attempts (worst case a bit over 30s of
|
||||
// jittered backoff) instead of retrying a request that can never succeed,
|
||||
// forever, every ~30s, for the lifetime of the session.
|
||||
private static readonly WS_MAX_CONSECUTIVE_FAILURES = 5;
|
||||
// handful of quick handshake attempts (with the tightened backoff above,
|
||||
// well under a second in the common fast-fail case) instead of retrying a
|
||||
// request that can never succeed, forever, for the lifetime of the session.
|
||||
private static readonly WS_MAX_CONSECUTIVE_FAILURES = 3;
|
||||
|
||||
/** getWebSocketUrl(), gated by the circuit breaker above. */
|
||||
private effectiveWebSocketUrl(): string | null {
|
||||
@@ -6117,6 +6132,16 @@ export class JMAPClient implements IJMAPClient {
|
||||
if (wsUrl) {
|
||||
this.wsReconnectAttempts = 0;
|
||||
this.connectWebSocket(wsUrl);
|
||||
// Prime the polling baseline (pollingStates) in parallel with the WS
|
||||
// attempt, not just for shared/secondary accounts below - if WS ends
|
||||
// up failing and falling back (fallbackFromWebSocket()), this is what
|
||||
// lets that fallback reconcile anything that changed to the PRIMARY
|
||||
// account while WS was still churning through retries. Without an
|
||||
// early baseline, a change in that window would be silently missed
|
||||
// entirely: SSE only streams changes from the moment it connects
|
||||
// onward (no catch-up on connect), so the one thing that CAN catch up
|
||||
// is a diff against a state snapshot taken before the gap started.
|
||||
void this.fetchCurrentStates();
|
||||
// Not confirmed either way whether this server's WebSocket push fans
|
||||
// out to shared/secondary accounts or, like Stalwart's SSE, covers the
|
||||
// primary account only - keep the same secondary poll running under
|
||||
@@ -6234,6 +6259,34 @@ export class JMAPClient implements IJMAPClient {
|
||||
|
||||
/** Whatever push transport SSE would have used, now that WS has given up. */
|
||||
private fallbackFromWebSocket(): void {
|
||||
void this.reconcileAfterWebSocketFallback();
|
||||
}
|
||||
|
||||
/**
|
||||
* Diffs against the baseline setupPushNotifications() primed via
|
||||
* fetchCurrentStates() when the WS attempt began - BEFORE either branch
|
||||
* below gets a chance to erase that opportunity (startPollingFallback()
|
||||
* unconditionally overwrites the same baseline via its own
|
||||
* fetchCurrentStates() call; connectSSE() only ever streams changes from
|
||||
* the moment it connects onward, no catch-up). This is what catches a
|
||||
* real mail delivery (or any other tracked change) that happened to the
|
||||
* primary account while WS was still churning through retries, which
|
||||
* neither of those two paths would otherwise ever notice - confirmed as a
|
||||
* real, not theoretical, gap during this feature's own development (see
|
||||
* the WS_RECONNECT_BASE_DELAY comment above).
|
||||
*
|
||||
* Not airtight: if the early fetchCurrentStates() from
|
||||
* setupPushNotifications() hasn't itself completed yet by the time this
|
||||
* runs, there's nothing to diff against and this call just establishes
|
||||
* the baseline instead of detecting drift. In practice that race needs a
|
||||
* pathologically slow state-fetch racing an unusually fast WS failure,
|
||||
* and the tightened backoff above (worst case ~1.75s to exhaust 3
|
||||
* attempts) gives that fetch a lot more room to finish first than the
|
||||
* original 31s-worst-case ladder did.
|
||||
*/
|
||||
private async reconcileAfterWebSocketFallback(): Promise<void> {
|
||||
await this.checkForStateChanges();
|
||||
|
||||
const eventSourceUrl = this.getEventSourceUrl();
|
||||
if (eventSourceUrl) {
|
||||
this.connectSSE(eventSourceUrl);
|
||||
|
||||
+6
-1
@@ -40,7 +40,12 @@ if (basePath && !basePath.startsWith("/")) {
|
||||
|
||||
const nextConfig: NextConfig = {
|
||||
output: "standalone",
|
||||
allowedDevOrigins: ["192.168.1.51"],
|
||||
// 127.0.0.1 alongside the existing LAN entry: electron/main.ts always
|
||||
// loads its window at 127.0.0.1 (see ELECTRON_LOAD_URL and
|
||||
// startStandaloneServer()), so dev-mode Electron runs (only used by
|
||||
// integration/tests/11-electron-notification.spec.ts today) need it in
|
||||
// this allowlist the same way any other cross-origin dev client would.
|
||||
allowedDevOrigins: ["192.168.1.51", "127.0.0.1"],
|
||||
basePath: basePath || undefined,
|
||||
// esbuild ships native binaries + a README the bundler can't parse; load
|
||||
// it from node_modules at runtime instead of trying to bundle it. Used by
|
||||
|
||||
@@ -93,7 +93,23 @@ export async function proxy(request: NextRequest) {
|
||||
? `'self' 'nonce-${nonce}' 'unsafe-eval'`
|
||||
: `'self' 'nonce-${nonce}'`;
|
||||
|
||||
const connectSrc = isDev ? `'self' http: https: ws: wss:` : `'self' https:`;
|
||||
// `wss:` alongside `https:` in production: lib/jmap/client.ts's WebSocket
|
||||
// push (RFC 8887) needs it, and it adds no new trust surface - CSP's
|
||||
// `https:` scheme-source here already allows fetch/XHR to ANY TLS host
|
||||
// (not just the configured JMAP server; needed for ALLOW_CUSTOM_JMAP_ENDPOINT
|
||||
// and multi-server JMAP_SERVERS setups where the exact origin isn't known
|
||||
// at build time), so extending that same "any TLS-secured host" trust
|
||||
// model to WebSocket is consistent, not a new precedent. Confirmed this
|
||||
// was a real gap, not theoretical: before this fix, `new WebSocket(...)`
|
||||
// against the real reference server was blocked by THIS directive before
|
||||
// any network attempt happened at all (a `securitypolicyviolation` event
|
||||
// with connect-src as the violated directive) - the WS feature was
|
||||
// entirely inert in a production build. Plain `ws:` (unencrypted) stays
|
||||
// production-excluded on purpose, same reasoning as `http:` above it: an
|
||||
// https-served production app already gets unencrypted connections
|
||||
// blocked as mixed content by the browser itself, so allowing bare `ws:`
|
||||
// here would add no capability, only a false sense of one.
|
||||
const connectSrc = isDev ? `'self' http: https: ws: wss:` : `'self' https: wss:`;
|
||||
|
||||
const frameAncestors = isSandboxPath
|
||||
? `'self'`
|
||||
|
||||
Reference in New Issue
Block a user