diff --git a/apps/desktop/electron/connection-registry.test.ts b/apps/desktop/electron/connection-registry.test.ts index 3f9a785758..9053d4d045 100644 --- a/apps/desktop/electron/connection-registry.test.ts +++ b/apps/desktop/electron/connection-registry.test.ts @@ -1808,3 +1808,116 @@ test('migrateV1ToRegistry carries v1 remote headers into the registry entry', () 'CF-Access-Client-Id': { encoding: 'safeStorage', value: 'id' } }) }) + +// --- normalizeRegistry per-entry quarantine (#94246 remainder) --- +// +// One malformed entry must never cost the user the rest of the registry, and +// malformed entries are USER DATA: they are preserved under `quarantined` +// (with the raw entry verbatim) instead of being silently deleted on the next +// registry write. "Only deleting connections.json recovers" was the reported +// failure shape; the recovery must never be data loss. + +test('normalizeRegistry quarantines malformed entries instead of silently dropping them', () => { + const registry = normalizeRegistry({ + version: 2, + primary: 'a', + connections: [ + { id: 'local', kind: 'local', label: 'This device' }, + { id: 'a', kind: 'remote', label: 'Homelab', url: 'http://10.0.0.5:9119' }, + { id: 'c', kind: 'remote', label: 'No URL entry' }, + { kind: 'nonsense', label: 'Mystery box', extra: 'still my data' }, + { id: 's', kind: 'ssh', label: 'No host ssh' } + ] + }) + + // Healthy entries all load. + assert.deepEqual( + registry.connections.map(c => c.id), + ['local', 'a'] + ) + assert.equal(registry.primary, 'a') + + // The malformed ones are preserved verbatim, with reasons. + assert.equal((registry.quarantined || []).length, 3) + + const reasons = registry.quarantined!.map(q => q.reason).sort() + + assert.deepEqual(reasons, ['entry-missing-ssh-host', 'entry-missing-url', 'entry-unrecognized-kind']) + + const mystery = registry.quarantined!.find(q => q.reason === 'entry-unrecognized-kind') + + assert.deepEqual(mystery!.entry, { kind: 'nonsense', label: 'Mystery box', extra: 'still my data' }) +}) + +test('normalizeRegistry preserves previously quarantined entries across round trips', () => { + const first = normalizeRegistry({ + version: 2, + connections: [{ id: 'c', kind: 'remote', label: 'No URL entry' }] + }) + + assert.equal((first.quarantined || []).length, 1) + + // Simulate write → read → normalize again (what every registry save does). + const second = normalizeRegistry(JSON.parse(JSON.stringify(first))) + + assert.equal((second.quarantined || []).length, 1) + assert.deepEqual(second.quarantined![0].entry, { id: 'c', kind: 'remote', label: 'No URL entry' }) +}) + +test('normalizeRegistry quarantines an entry that explodes during normalization (no whole-load abort)', () => { + const poisoned: any = { id: 'boom', kind: 'remote', url: 'http://10.0.0.9:9119' } + + Object.defineProperty(poisoned, 'label', { + enumerable: true, + get() { + throw new Error('poisoned entry') + } + }) + + const registry = normalizeRegistry({ + version: 2, + primary: 'a', + connections: [poisoned, { id: 'a', kind: 'remote', label: 'Homelab', url: 'http://10.0.0.5:9119' }] + }) + + // The healthy entry still loads and keeps primary; the poisoned one is + // quarantined rather than aborting the whole registry load. + assert.deepEqual( + registry.connections.filter(c => c.kind === 'remote').map(c => c.id), + ['a'] + ) + assert.equal(registry.primary, 'a') + assert.equal((registry.quarantined || []).length, 1) + assert.equal(registry.quarantined![0].reason, 'entry-normalization-failed') +}) + +test('normalizeRegistry keeps a clean registry free of the quarantined key and caps quarantine growth', () => { + const clean = normalizeRegistry({ + version: 2, + connections: [{ id: 'a', kind: 'remote', label: 'Homelab', url: 'http://10.0.0.5:9119' }] + }) + + assert.equal('quarantined' in clean, false) + + const flooded = normalizeRegistry({ + version: 2, + connections: Array.from({ length: 100 }, (_, i) => ({ id: `q${i}`, kind: 'remote', label: `No URL ${i}` })) + }) + + assert.ok((flooded.quarantined || []).length <= 20) +}) + +test('normalizeRegistry quarantines non-object junk items that could still be user data', () => { + const registry = normalizeRegistry({ + version: 2, + connections: ['{ mangled json fragment }', null, false, { id: 'a', kind: 'remote', label: 'A', url: 'http://x:1' }] + }) + + assert.deepEqual( + registry.connections.map(c => c.kind), + ['local', 'remote'] + ) + // null/false carry no data and are dropped; the string is preserved. + assert.equal((registry.quarantined || []).length, 1) + assert.equal(registry.quarantined![0].entry, '{ mangled json fragment }') +}) diff --git a/apps/desktop/electron/connection-registry.ts b/apps/desktop/electron/connection-registry.ts index 582d599532..2757a28e01 100644 --- a/apps/desktop/electron/connection-registry.ts +++ b/apps/desktop/electron/connection-registry.ts @@ -73,6 +73,22 @@ export interface RegistryConnection { remoteProfile?: string } +/** + * A registry entry that failed normalization (#94246). The raw entry is USER + * DATA — it is preserved verbatim here (and re-persisted on every write) + * instead of being silently dropped, so a malformed/corrupt entry never + * requires "delete connections.json" recovery and never loses the user's + * connection material. + */ +export interface QuarantinedRegistryEntry { + reason: string + entry: unknown +} + +/** Upper bound on preserved quarantine entries so a pathological file cannot + * grow the registry without limit. Oldest-first within one load pass. */ +export const REGISTRY_QUARANTINE_CAP = 20 + export interface ConnectionRegistry { version: typeof REGISTRY_VERSION /** id of the connection that owns the window/primary backend. */ @@ -83,6 +99,8 @@ export interface ConnectionRegistry { * so registries written before multi-source switching still normalize. */ lastUsed: string connections: RegistryConnection[] + /** Entries preserved from a malformed load — absent when empty. */ + quarantined?: QuarantinedRegistryEntry[] } // ── Labels and ids ────────────────────────────────────────────────────────── @@ -1043,16 +1061,56 @@ export function normalizeRegistry(raw: unknown): ConnectionRegistry { const seenLabels = new Set() const seenIds = new Set() const connections: RegistryConnection[] = [] + const quarantined: QuarantinedRegistryEntry[] = [] + + const quarantine = (reason: string, entry: unknown) => { + if (quarantined.length < REGISTRY_QUARANTINE_CAP) { + quarantined.push({ reason, entry }) + } + } + + // Entries quarantined by a previous load are user data too — carry them + // through every subsequent normalize/write cycle rather than dropping them + // the first time the file is rewritten. + if (Array.isArray(parsed.quarantined)) { + for (const item of parsed.quarantined) { + if (item && typeof item === 'object' && 'entry' in (item as Record)) { + quarantine(String((item as Record).reason || 'unknown'), (item as Record).entry) + } + } + } + + // Best-effort plain-data copy for entries that blew up mid-normalization — + // the raw object may carry whatever poisoned it, so never persist it as-is. + const safeEntryCopy = (item: unknown) => { + try { + return JSON.parse(JSON.stringify(item)) + } catch { + return { unserializable: true } + } + } for (const item of rawConnections) { - if (!item || typeof item !== 'object') { + if (!item) { + continue // null/false/'' carry no user data + } + + if (typeof item !== 'object') { + // A string/number here is usually a mangled hand-edit — still user data. + quarantine('entry-malformed', item) + continue } + // One bad entry must never abort the whole registry load (#94246): any + // unexpected throw quarantines THIS entry and the loop moves on. + try { const entry = item as Record const kind = entry.kind if (kind !== 'local' && kind !== 'remote' && kind !== 'cloud' && kind !== 'ssh') { + quarantine('entry-unrecognized-kind', item) + continue } @@ -1086,6 +1144,8 @@ export function normalizeRegistry(raw: unknown): ConnectionRegistry { const url = String(entry.url || '').trim() if (!url) { + quarantine('entry-missing-url', item) + continue } @@ -1111,6 +1171,8 @@ export function normalizeRegistry(raw: unknown): ConnectionRegistry { const ssh = normalizeSshConfig({ ...entry, mode: 'ssh' }) if (!ssh) { + quarantine('entry-missing-ssh-host', item) + continue } @@ -1119,6 +1181,9 @@ export function normalizeRegistry(raw: unknown): ConnectionRegistry { } connections.push(clean) + } catch { + quarantine('entry-normalization-failed', safeEntryCopy(item)) + } } if (!connections.some(c => c.kind === 'local')) { @@ -1129,13 +1194,19 @@ export function normalizeRegistry(raw: unknown): ConnectionRegistry { const primary = connections.some(c => c.id === storedPrimary) ? storedPrimary : LOCAL_CONNECTION_ID const storedLastUsed = String(parsed.lastUsed || '').trim() - return { + const normalized: ConnectionRegistry = { version: REGISTRY_VERSION, primary, launchMode: parsed.launchMode === 'last-used' ? 'last-used' : 'primary', lastUsed: connections.some(c => c.id === storedLastUsed) ? storedLastUsed : primary, connections } + + if (quarantined.length > 0) { + normalized.quarantined = quarantined + } + + return normalized } /** diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 3c42a749bd..fcaff6ceb1 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -9021,9 +9021,21 @@ function readDesktopConnectionsRegistry() { tightenSecretFileMode(DESKTOP_CONNECTIONS_REGISTRY_PATH) registry = normalizeRegistry(JSON.parse(fs.readFileSync(DESKTOP_CONNECTIONS_REGISTRY_PATH, 'utf8'))) } catch { + // Whole-file corruption (truncated write, mangled hand-edit). The + // degraded local-only registry keeps boot working, but the file BYTES are + // the user's connection data — preserve them in a sidecar BEFORE any + // later write (drift reconcile, connection save) overwrites the file + // (#94246: recovery must never be data loss). + preserveCorruptRegistrySidecar() registry = normalizeRegistry(null) } + if (registry?.quarantined?.length) { + rememberLog( + `[connections] ${registry.quarantined.length} malformed registry entr${registry.quarantined.length === 1 ? 'y was' : 'ies were'} quarantined (kept under "quarantined" in connections.json); healthy connections loaded normally.` + ) + } + // Heal v1 -> v2 drift: the v1 global route names a remote this registry has // never heard of, so the live descriptor resolves to no connectionId and the // launch pick sends the window somewhere else. Persist so the repair is a @@ -9052,6 +9064,31 @@ function readDesktopConnectionsRegistry() { return registry } +// Copy an unparseable connections.json aside (once per corruption event) so a +// later registry write can never destroy the only copy of the user's saved +// connections (#94246). Best effort: failure to preserve must not block boot. +function preserveCorruptRegistrySidecar() { + try { + const rawText = fs.readFileSync(DESKTOP_CONNECTIONS_REGISTRY_PATH, 'utf8') + + if (!rawText.trim()) { + return + } + + const sidecar = `${DESKTOP_CONNECTIONS_REGISTRY_PATH}.corrupt-${new Date().toISOString().replace(/[:.]/g, '-')}` + + if (!fs.existsSync(sidecar)) { + fs.writeFileSync(sidecar, rawText, { mode: 0o600 }) + } + + rememberLog( + `[connections] connections.json could not be parsed; preserved the original file at ${sidecar} and continuing with a local-only registry. No connection data was deleted.` + ) + } catch { + // The read itself failed (missing file, permissions) — nothing to save. + } +} + function writeDesktopConnectionsRegistry(registry) { fs.mkdirSync(path.dirname(DESKTOP_CONNECTIONS_REGISTRY_PATH), { recursive: true }) // Owner-only for the same reason as connection.json: entries carry @@ -9098,7 +9135,16 @@ function sanitizeConnectionsRegistry(registry = readDesktopConnectionsRegistry() launchMode: registry.launchMode, lastUsed: registry.lastUsed, secureTokenStorage, - connections: registry.connections.map(sanitizeRegistryConnection) + connections: registry.connections.map(sanitizeRegistryConnection), + // Surface quarantined-entry NOTICES only (reason + best-effort label) — + // the raw entries can carry token envelopes and stay in the file (#94246). + quarantined: (registry.quarantined || []).map(q => ({ + reason: String(q?.reason || 'unknown'), + label: + q && q.entry && typeof q.entry === 'object' && typeof (q.entry as any).label === 'string' + ? (q.entry as any).label + : '' + })) } }