fix(desktop): quarantine malformed connections.json entries per-entry instead of dropping or nuking the registry (#94246)

- normalizeRegistry now preserves every malformed entry (unknown kind, url-less remote/cloud, host-less ssh, mangled non-object items, and any entry whose normalization throws) under a capped 'quarantined' key that survives write cycles — healthy entries keep loading and user data is never silently deleted. - A whole-file parse failure preserves the original bytes in a connections.json.corrupt-<ts> sidecar BEFORE the drift reconciler or a save can overwrite the file with the degraded local-only registry. - Loads log a quarantine notice and sanitizeConnectionsRegistry surfaces reason+label summaries (never raw entries/token envelopes).
2026-08-26 07:43:54 -07:00
parent 31250da505
commit 62e2d6e1e4
3 changed files with 233 additions and 3 deletions
@@ -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 }')
+})
@@ -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<string>()
  const seenIds = new Set<string>()
  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<string, unknown>)) {
+        quarantine(String((item as Record<string, unknown>).reason || 'unknown'), (item as Record<string, unknown>).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<string, unknown>
    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
 }

 /**
@@ -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
+          : ''
+    }))
  }
 }