From e258a7b8c70fdaf95941a4a62bd9c693da0a2a7a Mon Sep 17 00:00:00 2001 From: xiao-test Date: Mon, 14 Sep 2026 17:13:36 +0800 Subject: [PATCH 01/19] perf(sync): read the backup database asynchronously Backing up a large database froze the whole UI: readFileSync made the main thread wait on disk, stalling every IPC, render and stream flush for seconds. The read now goes through fs.promises, which hands the work to the libuv threadpool and leaves the event loop free. The surrounding steps already used async fs, so this was the last synchronous holdout. Both APIs return a Buffer and Uint8Array(...) copies it either way, so the archive bytes are unchanged. One trade-off: the snapshot is now taken while other tasks can run, so commits landing during the read may be absent from the backup. The TRUNCATE checkpoint just before it keeps the copied file internally consistent; SQLite's own backup API would be the strict fix if that window ever matters. --- src/main/sync/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/sync/index.ts b/src/main/sync/index.ts index ae0fa3271..821007c6b 100644 --- a/src/main/sync/index.ts +++ b/src/main/sync/index.ts @@ -576,7 +576,7 @@ export class SyncService { this.ensureSqliteConfigStorageReady() this.checkpointDatabaseForBackup() const files: Record = {} - files[ZIP_PATHS.agentDb] = new Uint8Array(fs.readFileSync(this.DB_PATH)) + files[ZIP_PATHS.agentDb] = new Uint8Array(await fs.promises.readFile(this.DB_PATH)) files[ZIP_PATHS.appSettings] = await this.readSanitizedAppSettingsBackup() await this.addOptionalFile(files, ZIP_PATHS.customPrompts, this.CUSTOM_PROMPTS_PATH) await this.addOptionalFile(files, ZIP_PATHS.systemPrompts, this.SYSTEM_PROMPTS_PATH) From b30267861115d5cb40d3fa44807211a28924e637 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Tue, 15 Sep 2026 01:07:06 +0800 Subject: [PATCH 02/19] fix(sync): pin backup snapshot with WAL read lock The async agent.db read yielded the event loop between the draining checkpoint and the file copy, so a later auto or explicit checkpoint could rewrite the main database file mid-copy and corrupt the backup. Reading support files one await at a time could also mix instants across the archive, and copying the whole file into a fresh Uint8Array still stalled the main thread. Collect the backup under a data-layer withBackupReadLock: drain the WAL, then hold a read mark so no checkpoint can move pages into the file being copied while support files are read synchronously and the database is read asynchronously. Fall back to one blocking pass when the WAL cannot drain. Zip entries use zero-copy buffer views and are deflated in 4 MiB slices streamed to disk. Add consistency tests covering concurrent writes, auto checkpoints, encrypted databases, the blocked-checkpoint fallback, and multi-slice round trips. --- src/main/data/backupReadLock.ts | 46 ++++ src/main/data/mainDatabase.ts | 5 + src/main/sync/index.ts | 118 ++++++--- test/main/sync/backupConsistency.test.ts | 293 +++++++++++++++++++++++ test/main/sync/syncService.test.ts | 16 +- 5 files changed, 444 insertions(+), 34 deletions(-) create mode 100644 src/main/data/backupReadLock.ts create mode 100644 test/main/sync/backupConsistency.test.ts diff --git a/src/main/data/backupReadLock.ts b/src/main/data/backupReadLock.ts new file mode 100644 index 000000000..7523b3aa7 --- /dev/null +++ b/src/main/data/backupReadLock.ts @@ -0,0 +1,46 @@ +import type Database from 'better-sqlite3-multiple-ciphers' + +export type BackupReadLockOutcome = + | { acquired: true; result: T } + | { acquired: false; result?: undefined } + +type CheckpointDb = Pick + +async function drainWal(mainDb: CheckpointDb): Promise { + for (let attempt = 0; attempt < 3; attempt++) { + const rows = mainDb.pragma('wal_checkpoint(PASSIVE)') as Array<{ + busy: number + log: number + checkpointed: number + }> + const result = Array.isArray(rows) ? rows[0] : undefined + if (result && result.busy === 0 && result.checkpointed === result.log) { + return true + } + await new Promise((resolve) => setTimeout(resolve, 20)) + } + return false +} + +export async function withBackupReadLock( + mainDb: CheckpointDb | undefined, + openDb: () => Database.Database, + work: () => Promise +): Promise> { + if (!mainDb?.open || !(await drainWal(mainDb))) { + return { acquired: false } + } + const db = openDb() + db.exec('BEGIN') + try { + db.prepare('SELECT count(*) FROM sqlite_master').get() + const result = await work() + db.exec('COMMIT') + return { acquired: true, result } + } catch (error) { + db.exec('ROLLBACK') + throw error + } finally { + db.close() + } +} diff --git a/src/main/data/mainDatabase.ts b/src/main/data/mainDatabase.ts index 3624248fb..433bb7979 100644 --- a/src/main/data/mainDatabase.ts +++ b/src/main/data/mainDatabase.ts @@ -5,6 +5,7 @@ import type { DatabaseRepairReport, DatabaseSchemaDiagnosis } from '@shared/type import { DatabaseRepairService, SchemaInspector } from '@/data/schemaRepair' import type { SchemaTableSpec } from '@/data/schemaTypes' import { openSQLiteDatabase } from '@/data/databaseConnection' +import { withBackupReadLock, type BackupReadLockOutcome } from '@/data/backupReadLock' import { createMainSchemaCatalog, type MainSchemaCatalog } from '@/data/schemaCatalog' export { openSQLiteDatabase } from '@/data/databaseConnection' export { isDestructiveDatabaseError } from '@/data/databaseStartupRecovery' @@ -168,6 +169,10 @@ export class MainDatabase { return openSQLiteDatabase(dbPath, this.password) } + public async withBackupReadLock(work: () => Promise): Promise> { + return withBackupReadLock(this.db, () => this.openDatabaseConnection(), work) + } + public getDatabasePath(): string { return this.dbPath } diff --git a/src/main/sync/index.ts b/src/main/sync/index.ts index 821007c6b..f22c3d3a1 100644 --- a/src/main/sync/index.ts +++ b/src/main/sync/index.ts @@ -2,7 +2,7 @@ import { app, shell } from 'electron' import path from 'path' import fs from 'fs' import Database from 'better-sqlite3-multiple-ciphers' -import { zip, unzip, type AsyncZipOptions } from 'fflate' +import { unzip, Zip, AsyncZipDeflate } from 'fflate' import type { SyncBackupInfo, CloudSyncResult } from '@shared/types/sync' import { CloudStorageService } from './cloudStorageService' import type { DeepchatEventPublisher } from '@shared/contracts/events' @@ -14,6 +14,7 @@ import { type SyncBackupManifest } from './configImportService' import type { SyncSettings } from './settings' +import type { BackupReadLockOutcome } from '@/data/backupReadLock' import type { SettingsDatabase } from '@/settings/data/database' import type { ProviderDatabase } from '@/provider/data/database' @@ -68,16 +69,11 @@ const ZIP_PATHS = { manifest: 'manifest.json' } -const zipAsync = (files: Record, options: AsyncZipOptions) => - new Promise((resolve, reject) => { - zip(files, options, (error, data) => { - if (error) { - reject(error) - return - } - resolve(data) - }) - }) +const toUint8ArrayView = (buffer: Buffer): Uint8Array => + new Uint8Array(buffer.buffer, buffer.byteOffset, buffer.byteLength) + +const ZIP_LEVEL = 6 +const ZIP_SLICE_SIZE = 4 * 1024 * 1024 const unzipAsync = (data: Uint8Array) => new Promise>((resolve, reject) => { @@ -109,9 +105,9 @@ export interface SyncImportDatabasePort { } interface SyncDatabasePort { - getDatabase(): Database.Database getDatabasePassword(): string | undefined openDatabaseConnection(dbPath: string): Database.Database + withBackupReadLock(work: () => Promise): Promise> } export interface SyncImportResult { @@ -574,12 +570,7 @@ export class SyncService { this.emitBackupStatus('collecting') this.ensureSqliteConfigStorageReady() - this.checkpointDatabaseForBackup() - const files: Record = {} - files[ZIP_PATHS.agentDb] = new Uint8Array(await fs.promises.readFile(this.DB_PATH)) - files[ZIP_PATHS.appSettings] = await this.readSanitizedAppSettingsBackup() - await this.addOptionalFile(files, ZIP_PATHS.customPrompts, this.CUSTOM_PROMPTS_PATH) - await this.addOptionalFile(files, ZIP_PATHS.systemPrompts, this.SYSTEM_PROMPTS_PATH) + const files = await this.collectBackupFiles() const manifest = { version: CURRENT_SYNC_BACKUP_VERSION, @@ -595,8 +586,7 @@ export class SyncService { ) this.emitBackupStatus('compressing') - const zipData = await zipAsync(files, { level: 6 }) - await fs.promises.writeFile(tempZipPath, Buffer.from(zipData)) + await this.writeZipToDisk(files, tempZipPath) if (fs.existsSync(finalZipPath)) { await fs.promises.unlink(finalZipPath) @@ -709,11 +699,81 @@ export class SyncService { } } - private checkpointDatabaseForBackup(): void { - const db = this.database.getDatabase() - if (db?.open) { - db.pragma('wal_checkpoint(TRUNCATE)') + private async writeZipToDisk( + files: Record, + targetPath: string + ): Promise { + await new Promise((resolve, reject) => { + const output = fs.createWriteStream(targetPath) + const archive = new Zip() + let drain: Promise | null = null + output.on('error', reject) + archive.ondata = (error, chunk, final) => { + if (error) { + output.destroy() + reject(error) + return + } + if (!output.write(Buffer.from(chunk))) { + drain = new Promise((drained) => { + output.once('drain', () => drained()) + output.once('error', () => drained()) + }) + } + if (final) { + output.end(() => resolve()) + } + } + + void (async () => { + try { + for (const [name, data] of Object.entries(files)) { + const entry = new AsyncZipDeflate(name, { level: ZIP_LEVEL }) + archive.add(entry) + for (let offset = 0; offset < data.length; offset += ZIP_SLICE_SIZE) { + const end = Math.min(offset + ZIP_SLICE_SIZE, data.length) + entry.push(new Uint8Array(data.slice(offset, end)), false) + if (drain) { + await drain + drain = null + } + await new Promise((resume) => setImmediate(resume)) + } + entry.push(new Uint8Array(0), true) + } + archive.end() + } catch (error) { + output.destroy() + reject(error) + } + })() + }) + } + + private async collectBackupFiles(): Promise> { + const snapshot = await this.database.withBackupReadLock(async () => { + const files = this.readSupportFiles() + files[ZIP_PATHS.agentDb] = toUint8ArrayView(await fs.promises.readFile(this.DB_PATH)) + return files + }) + if (!snapshot.acquired) { + return this.readBackupFilesSynchronously() } + return snapshot.result + } + + private readBackupFilesSynchronously(): Record { + const files = this.readSupportFiles() + files[ZIP_PATHS.agentDb] = toUint8ArrayView(fs.readFileSync(this.DB_PATH)) + return files + } + + private readSupportFiles(): Record { + const files: Record = {} + files[ZIP_PATHS.appSettings] = this.readSanitizedAppSettingsBackup() + this.addOptionalFile(files, ZIP_PATHS.customPrompts, this.CUSTOM_PROMPTS_PATH) + this.addOptionalFile(files, ZIP_PATHS.systemPrompts, this.SYSTEM_PROMPTS_PATH) + return files } private resolveBackupVersion(manifest: SyncBackupManifest | null): number { @@ -753,18 +813,18 @@ export class SyncService { return baseName } - private async addOptionalFile( + private addOptionalFile( files: Record, zipPath: string, filePath: string - ): Promise { + ): void { if (fs.existsSync(filePath)) { - files[zipPath] = new Uint8Array(await fs.promises.readFile(filePath)) + files[zipPath] = toUint8ArrayView(fs.readFileSync(filePath)) } } - private async readSanitizedAppSettingsBackup(): Promise { - const raw = await fs.promises.readFile(this.APP_SETTINGS_PATH, 'utf-8') + private readSanitizedAppSettingsBackup(): Uint8Array { + const raw = fs.readFileSync(this.APP_SETTINGS_PATH, 'utf-8') const parsed = JSON.parse(raw) as Record const sanitized = this.removeMigratedAppSettings(parsed) return new Uint8Array(Buffer.from(JSON.stringify(sanitized, null, 2), 'utf-8')) diff --git a/test/main/sync/backupConsistency.test.ts b/test/main/sync/backupConsistency.test.ts new file mode 100644 index 000000000..bc43660f2 --- /dev/null +++ b/test/main/sync/backupConsistency.test.ts @@ -0,0 +1,293 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import fsModule from 'fs' +import { unzipSync } from 'fflate' +import Database from 'better-sqlite3-multiple-ciphers' +import { openSQLiteDatabase } from '@/data/databaseConnection' +import { withBackupReadLock } from '@/data/backupReadLock' + +vi.mock('../../../src/main/sync/cloudStorageService', () => ({ + CloudStorageService: vi.fn(() => ({})) +})) + +const AGENT_DB_ENTRY = 'database/agent.db' +const SEED_ROWS = 120 +const ROW_PAYLOAD = 'payload-'.repeat(64) + +const realFs = await vi.importActual('fs') +const path = await vi.importActual('path') +const osActual = await vi.importActual('os') +Object.assign(fsModule, realFs) +;(fsModule as unknown as { promises: unknown }).promises = ( + realFs as unknown as { promises: unknown } +).promises +const fs = realFs + +const { app } = await import('electron') +const { SyncService } = await import('../../../src/main/sync') + +let userDataDir: string +let syncDir: string +let dbPath: string +let writerDb: Database.Database | null = null +let sidecarConnections: Database.Database[] = [] +let postCheckpointImages: Buffer[] = [] +let getPathSpy: ReturnType + +beforeEach(() => { + userDataDir = fs.mkdtempSync(path.join(osActual.tmpdir(), 'deepchat-user-')) + syncDir = fs.mkdtempSync(path.join(osActual.tmpdir(), 'deepchat-sync-')) + dbPath = path.join(userDataDir, 'app_db', 'agent.db') + postCheckpointImages = [] + getPathSpy = vi.spyOn(app, 'getPath').mockImplementation((type: string) => { + if (type === 'userData') return userDataDir + return osActual.tmpdir() + }) + fs.mkdirSync(path.dirname(dbPath), { recursive: true }) + fs.writeFileSync(path.join(userDataDir, 'app-settings.json'), JSON.stringify({ theme: 'dark' })) +}) + +afterEach(() => { + for (const connection of sidecarConnections) { + try { + connection.exec('COMMIT') + } catch {} + connection.close() + } + sidecarConnections = [] + writerDb?.close() + writerDb = null + getPathSpy.mockRestore() + fs.rmSync(userDataDir, { recursive: true, force: true }) + fs.rmSync(syncDir, { recursive: true, force: true }) +}) + +function buildService(password: string | undefined): InstanceType { + const connection = openSQLiteDatabase(dbPath, password) + writerDb = connection + connection.exec('CREATE TABLE backup_probe (id INTEGER PRIMARY KEY, payload TEXT)') + const insert = connection.prepare('INSERT INTO backup_probe (payload) VALUES (?)') + connection.transaction(() => { + for (let index = 0; index < SEED_ROWS; index++) insert.run(ROW_PAYLOAD) + })() + + const checkpointingHandle = { + open: true, + pragma: (source: string, options?: unknown) => { + const result = connection.pragma(source, options as never) + if (source.startsWith('wal_checkpoint')) { + postCheckpointImages.push(fs.readFileSync(dbPath)) + } + return result + } + } + + return new SyncService( + { + getFolderPath: () => syncDir, + getEnabled: () => true, + getLastSyncTime: () => 0, + setLastSyncTime: () => undefined + } as never, + { + getDatabasePassword: () => password, + openDatabaseConnection: (target: string) => openSQLiteDatabase(target, password), + withBackupReadLock: (work: () => Promise) => + withBackupReadLock(checkpointingHandle, () => openSQLiteDatabase(dbPath, password), work) + } as never, + { + get appSettingsTable() { + return { hasConfigMigration: () => true } + } + } as never, + {} as never, + vi.fn() as never + ) +} + +function copiesInChunks(mutate: () => void) { + const originalReadFile = fs.promises.readFile.bind(fs.promises) + return vi.spyOn(fs.promises, 'readFile').mockImplementation((async ( + target: unknown, + options?: unknown + ) => { + if (target !== dbPath) { + return originalReadFile(target as never, options as never) + } + const handle = await fs.promises.open(dbPath, 'r') + try { + const { size } = await handle.stat() + const chunks: Buffer[] = [] + let offset = 0 + let mutated = false + while (offset < size) { + const length = Math.min(8192, size - offset) + const buffer = Buffer.alloc(length) + const { bytesRead } = await handle.read(buffer, 0, length, offset) + if (bytesRead === 0) break + chunks.push(buffer.subarray(0, bytesRead)) + offset += bytesRead + if (!mutated && offset >= size / 2) { + mutate() + mutated = true + } + } + return Buffer.concat(chunks) + } finally { + await handle.close() + } + }) as never) +} + +function writeDuringCopy() { + const connection = writerDb as Database.Database + const insert = connection.prepare('INSERT INTO backup_probe (payload) VALUES (?)') + connection.transaction(() => { + for (let index = 0; index < 1500; index++) insert.run(ROW_PAYLOAD) + })() + connection.pragma('wal_checkpoint(PASSIVE)') +} + +function archivedDatabaseEntry(fileName: string): Buffer { + const archive = fs.readFileSync(path.join(syncDir, fileName)) + const entries = unzipSync(new Uint8Array(archive)) as unknown as Record + return Buffer.from(entries[AGENT_DB_ENTRY]) +} + +function openCopiedImage(image: Buffer, password: string | undefined): Database.Database { + const restoredPath = path.join(userDataDir, 'restored.db') + fs.writeFileSync(restoredPath, image) + const db = new Database(restoredPath) + if (password) { + db.pragma("cipher='sqlcipher'") + db.pragma('legacy=4') + db.key(Buffer.from(password, 'utf8')) + } + return db +} + +function expectUsableProbeTable(image: Buffer, password: string | undefined): void { + const restored = openCopiedImage(image, password) + try { + expect(restored.pragma('integrity_check', { simple: true })).toBe('ok') + const { count } = restored.prepare('SELECT count(*) AS count FROM backup_probe').get() as { + count: number + } + expect(count).toBeGreaterThanOrEqual(SEED_ROWS) + } finally { + restored.close() + } +} + +describe('backup database image consistency', () => { + it('copies the checkpointed image even while other writes commit mid-read', async () => { + const service = buildService(undefined) + const copySpy = copiesInChunks(writeDuringCopy) + + const backup = await service.startBackup() + + expect(backup).not.toBeNull() + copySpy.mockRestore() + const archived = archivedDatabaseEntry((backup as { fileName: string }).fileName) + + expect(archived.equals(postCheckpointImages[postCheckpointImages.length - 1])).toBe(true) + expectUsableProbeTable(archived, undefined) + }) + + it('holds the image steady for an encrypted database too', async () => { + const password = 'backup-consistency-key' + const service = buildService(password) + const copySpy = copiesInChunks(writeDuringCopy) + + const backup = await service.startBackup() + + expect(backup).not.toBeNull() + copySpy.mockRestore() + const archived = archivedDatabaseEntry((backup as { fileName: string }).fileName) + expect(archived.equals(postCheckpointImages[postCheckpointImages.length - 1])).toBe(true) + + expect(() => { + const withoutKey = openCopiedImage(archived, undefined) + try { + withoutKey.prepare('SELECT count(*) AS count FROM backup_probe').get() + } finally { + withoutKey.close() + } + }).toThrow() + + expectUsableProbeTable(archived, password) + }) + + it('writes a multi-slice archive that still round-trips', async () => { + const service = buildService(undefined) + const connection = writerDb as Database.Database + const insert = connection.prepare('INSERT INTO backup_probe (payload) VALUES (?)') + const bigPayload = ROW_PAYLOAD.repeat(16) + connection.transaction(() => { + for (let index = 0; index < 700; index++) insert.run(bigPayload) + })() + + const backup = await service.startBackup() + + expect(backup).not.toBeNull() + const archivePath = path.join(syncDir, (backup as { fileName: string }).fileName) + expect(fs.statSync(archivePath).size).toBeGreaterThan(0) + const entries = unzipSync(new Uint8Array(fs.readFileSync(archivePath))) as unknown as Record< + string, + Uint8Array + > + expect(entries[AGENT_DB_ENTRY].length).toBeGreaterThan(4 * 1024 * 1024) + expect(Object.keys(entries).sort()).toEqual( + [AGENT_DB_ENTRY, 'configs/app-settings.json', 'manifest.json'].sort() + ) + expectUsableProbeTable(Buffer.from(entries[AGENT_DB_ENTRY]), undefined) + }) + + it('captures the supporting files alongside the image, not after it', async () => { + const service = buildService(undefined) + const settingsPath = path.join(userDataDir, 'app-settings.json') + fs.writeFileSync(settingsPath, JSON.stringify({ theme: 'before' })) + + const copySpy = copiesInChunks(() => { + writeDuringCopy() + fs.writeFileSync(settingsPath, JSON.stringify({ theme: 'after' })) + }) + + const backup = await service.startBackup() + + expect(backup).not.toBeNull() + copySpy.mockRestore() + const archive = fs.readFileSync(path.join(syncDir, (backup as { fileName: string }).fileName)) + const entries = unzipSync(new Uint8Array(archive)) as unknown as Record + expect(JSON.parse(Buffer.from(entries['configs/app-settings.json']).toString('utf-8'))).toEqual( + { + theme: 'before' + } + ) + expectUsableProbeTable(Buffer.from(entries[AGENT_DB_ENTRY]), undefined) + }) + + it('still produces a usable image when another reader blocks the checkpoint', async () => { + const service = buildService(undefined) + const connection = writerDb as Database.Database + connection.pragma('wal_checkpoint(PASSIVE)') + + const holder = openSQLiteDatabase(dbPath, undefined) + sidecarConnections.push(holder) + holder.exec('BEGIN') + holder.prepare('SELECT count(*) AS count FROM backup_probe').get() + const insert = connection.prepare('INSERT INTO backup_probe (payload) VALUES (?)') + connection.transaction(() => { + for (let index = 0; index < 40; index++) insert.run(ROW_PAYLOAD) + })() + + const copySpy = copiesInChunks(writeDuringCopy) + const backup = await service.startBackup() + + expect(backup).not.toBeNull() + copySpy.mockRestore() + const archived = archivedDatabaseEntry((backup as { fileName: string }).fileName) + + expect(postCheckpointImages.length).toBeGreaterThan(1) + expectUsableProbeTable(archived, undefined) + }) +}) diff --git a/test/main/sync/syncService.test.ts b/test/main/sync/syncService.test.ts index c65985d24..00b735097 100644 --- a/test/main/sync/syncService.test.ts +++ b/test/main/sync/syncService.test.ts @@ -4,6 +4,7 @@ import Database from 'better-sqlite3-multiple-ciphers' import { unzipSync, zipSync } from 'fflate' import * as fsMock from 'fs' import type { SettingsDatabase } from '@/settings/data/database' +import { withBackupReadLock } from '@/data/backupReadLock' const configImportMocks = vi.hoisted(() => ({ importLegacyConfig: vi.fn(), @@ -357,10 +358,15 @@ describe('SyncService backup import', () => { sqlitePresenter = { close: vi.fn(), reopen: vi.fn(), - getDatabase: vi.fn(() => ({ - open: true, - pragma: dbPragma - })), + withBackupReadLock: vi.fn((work: () => Promise) => + withBackupReadLock( + { open: true, pragma: dbPragma } as never, + () => { + throw new Error('backup read lock must not open a connection') + }, + work + ) + ), appSettingsTable: { hasConfigMigration: vi.fn(() => true) }, @@ -469,7 +475,7 @@ describe('SyncService backup import', () => { const files = unzipSync(new Uint8Array(fs.readFileSync(archivePath))) expect(files[ZIP_PATHS.agentDb]).toBeDefined() expect(files[ZIP_PATHS.mcpSettings]).toBeUndefined() - expect(dbPragma).toHaveBeenCalledWith('wal_checkpoint(TRUNCATE)') + expect(dbPragma).toHaveBeenCalledWith('wal_checkpoint(PASSIVE)') const manifest = JSON.parse(Buffer.from(files[ZIP_PATHS.manifest]).toString('utf-8')) expect(manifest).toMatchObject({ version: 2, From ba23455024ba8ad3c357fb954273d7291e057cff Mon Sep 17 00:00:00 2001 From: xiao-test Date: Tue, 15 Sep 2026 10:44:39 +0800 Subject: [PATCH 03/19] fix(sync): include WAL sidecar in fallback backup --- src/main/data/backupReadLock.ts | 4 ++- src/main/sync/index.ts | 36 ++++++++++++++++++++++-- test/main/sync/backupConsistency.test.ts | 26 ++++++++++++++--- 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/main/data/backupReadLock.ts b/src/main/data/backupReadLock.ts index 7523b3aa7..a0278ea8b 100644 --- a/src/main/data/backupReadLock.ts +++ b/src/main/data/backupReadLock.ts @@ -38,7 +38,9 @@ export async function withBackupReadLock( db.exec('COMMIT') return { acquired: true, result } } catch (error) { - db.exec('ROLLBACK') + if (db.inTransaction) { + db.exec('ROLLBACK') + } throw error } finally { db.close() diff --git a/src/main/sync/index.ts b/src/main/sync/index.ts index f22c3d3a1..8a41e5d02 100644 --- a/src/main/sync/index.ts +++ b/src/main/sync/index.ts @@ -61,6 +61,7 @@ const KNOWN_IMPORT_ERRORS = new Set([ const ZIP_PATHS = { agentDb: 'database/agent.db', + agentDbWal: 'database/agent.db-wal', chatDb: 'database/chat.db', appSettings: 'configs/app-settings.json', customPrompts: 'configs/custom_prompts.json', @@ -416,6 +417,7 @@ export class SyncService { this.copyFile(backupDbSource.path, this.DB_PATH) this.cleanupDatabaseSidecarFiles(this.DB_PATH) + this.restoreBackupWalSidecar(backupDbSource.path, this.DB_PATH) if (usesSqliteConfigStorage) { configImportService.finalizeSqliteConfigImport() } else { @@ -757,14 +759,30 @@ export class SyncService { return files }) if (!snapshot.acquired) { - return this.readBackupFilesSynchronously() + console.warn( + '[Sync] Backup could not drain the WAL (a reader is blocking the checkpoint); ' + + 'falling back to a best-effort copy that ships the WAL sidecar so committed ' + + 'transactions are not silently dropped' + ) + return this.readBackupFilesFallback() } return snapshot.result } - private readBackupFilesSynchronously(): Record { + private async readBackupFilesFallback(): Promise> { const files = this.readSupportFiles() - files[ZIP_PATHS.agentDb] = toUint8ArrayView(fs.readFileSync(this.DB_PATH)) + files[ZIP_PATHS.agentDb] = toUint8ArrayView(await fs.promises.readFile(this.DB_PATH)) + const walPath = `${this.DB_PATH}-wal` + try { + const walImage = await fs.promises.readFile(walPath) + if (walImage.length > 0) { + files[ZIP_PATHS.agentDbWal] = toUint8ArrayView(walImage) + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error + } + } return files } @@ -1066,6 +1084,17 @@ export class SyncService { // Shell windows no longer manage chat tabs; nothing to reset } + private restoreBackupWalSidecar(sourceDbPath: string, targetDbPath: string): void { + const sourceWalPath = `${sourceDbPath}-wal` + if (!fs.existsSync(sourceWalPath)) { + return + } + // Ships un-checkpointed transactions from a fallback archive; the next + // read-write open replays them. Placed after cleanupDatabaseSidecarFiles so + // stale sidecars from the previous database cannot mix with this image. + this.copyFile(sourceWalPath, `${targetDbPath}-wal`) + } + private cleanupDatabaseSidecarFiles(dbFilePath: string): void { const sidecarFiles = [`${dbFilePath}-wal`, `${dbFilePath}-shm`] for (const filePath of sidecarFiles) { @@ -1083,6 +1112,7 @@ export class SyncService { private restoreFromTempBackup(tempFiles: Record): void { if (tempFiles.db) { this.copyFile(tempFiles.db, this.DB_PATH) + this.cleanupDatabaseSidecarFiles(this.DB_PATH) } if (tempFiles.appSettings) { this.copyFile(tempFiles.appSettings, this.APP_SETTINGS_PATH) diff --git a/test/main/sync/backupConsistency.test.ts b/test/main/sync/backupConsistency.test.ts index bc43660f2..975df4d01 100644 --- a/test/main/sync/backupConsistency.test.ts +++ b/test/main/sync/backupConsistency.test.ts @@ -10,6 +10,7 @@ vi.mock('../../../src/main/sync/cloudStorageService', () => ({ })) const AGENT_DB_ENTRY = 'database/agent.db' +const AGENT_DB_WAL_ENTRY = 'database/agent.db-wal' const SEED_ROWS = 120 const ROW_PAYLOAD = 'payload-'.repeat(64) @@ -266,7 +267,7 @@ describe('backup database image consistency', () => { expectUsableProbeTable(Buffer.from(entries[AGENT_DB_ENTRY]), undefined) }) - it('still produces a usable image when another reader blocks the checkpoint', async () => { + it('keeps every committed row when another reader blocks the checkpoint', async () => { const service = buildService(undefined) const connection = writerDb as Database.Database connection.pragma('wal_checkpoint(PASSIVE)') @@ -285,9 +286,26 @@ describe('backup database image consistency', () => { expect(backup).not.toBeNull() copySpy.mockRestore() - const archived = archivedDatabaseEntry((backup as { fileName: string }).fileName) - expect(postCheckpointImages.length).toBeGreaterThan(1) - expectUsableProbeTable(archived, undefined) + + const archive = fs.readFileSync(path.join(syncDir, (backup as { fileName: string }).fileName)) + const entries = unzipSync(new Uint8Array(archive)) as unknown as Record + const walEntry = entries[AGENT_DB_WAL_ENTRY] + expect(walEntry).toBeDefined() + expect(walEntry.length).toBeGreaterThan(0) + + const restoredPath = path.join(userDataDir, 'restored-with-wal.db') + fs.writeFileSync(restoredPath, Buffer.from(entries[AGENT_DB_ENTRY])) + fs.writeFileSync(`${restoredPath}-wal`, Buffer.from(walEntry)) + const restored = new Database(restoredPath) + try { + expect(restored.pragma('integrity_check', { simple: true })).toBe('ok') + const { count } = restored.prepare('SELECT count(*) AS count FROM backup_probe').get() as { + count: number + } + expect(count).toBe(SEED_ROWS + 40 + 1500) + } finally { + restored.close() + } }) }) From fddf9574119f5d2e5efa43218656a9823d42f422 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Tue, 15 Sep 2026 11:52:13 +0800 Subject: [PATCH 04/19] fix(sync): keep BEGIN inside backup lock try block --- src/main/data/backupReadLock.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/data/backupReadLock.ts b/src/main/data/backupReadLock.ts index a0278ea8b..dd8163f69 100644 --- a/src/main/data/backupReadLock.ts +++ b/src/main/data/backupReadLock.ts @@ -31,8 +31,10 @@ export async function withBackupReadLock( return { acquired: false } } const db = openDb() - db.exec('BEGIN') try { + // Inside the try so a failing BEGIN cannot leak the snapshot connection; + // the inTransaction guard below then correctly skips the ROLLBACK. + db.exec('BEGIN') db.prepare('SELECT count(*) FROM sqlite_master').get() const result = await work() db.exec('COMMIT') From 5fd89d22fc763c19e6165084f1da0a3c51ee3966 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Tue, 15 Sep 2026 13:27:01 +0800 Subject: [PATCH 05/19] fix(sync): pin WAL snapshot during backup copy --- src/main/data/backupReadLock.ts | 53 ++++++++++--- src/main/sync/index.ts | 47 +++++++----- test/main/data/backupReadLock.test.ts | 98 ++++++++++++++++++++++++ test/main/sync/backupConsistency.test.ts | 91 +++++++++++++++++++++- test/main/sync/syncService.test.ts | 15 ++++ 5 files changed, 270 insertions(+), 34 deletions(-) create mode 100644 test/main/data/backupReadLock.test.ts diff --git a/src/main/data/backupReadLock.ts b/src/main/data/backupReadLock.ts index dd8163f69..c64e872a8 100644 --- a/src/main/data/backupReadLock.ts +++ b/src/main/data/backupReadLock.ts @@ -22,6 +22,42 @@ async function drainWal(mainDb: CheckpointDb): Promise { return false } +function rollbackSilently(db: Database.Database): void { + if (!db.inTransaction) { + return + } + try { + db.exec('ROLLBACK') + } catch (rollbackError) { + console.warn('[Backup] ROLLBACK failed while handling another error:', rollbackError) + } +} + +export class BackupSnapshotNotDrainedError extends Error {} + +export async function withBackupSnapshot( + openDb: () => Database.Database, + work: () => Promise, + guard?: () => Promise +): Promise { + const db = openDb() + try { + db.exec('BEGIN') + db.prepare('SELECT count(*) FROM sqlite_master').get() + if (guard && !(await guard())) { + throw new BackupSnapshotNotDrainedError() + } + const result = await work() + db.exec('COMMIT') + return result + } catch (error) { + rollbackSilently(db) + throw error + } finally { + db.close() + } +} + export async function withBackupReadLock( mainDb: CheckpointDb | undefined, openDb: () => Database.Database, @@ -30,21 +66,16 @@ export async function withBackupReadLock( if (!mainDb?.open || !(await drainWal(mainDb))) { return { acquired: false } } - const db = openDb() try { - // Inside the try so a failing BEGIN cannot leak the snapshot connection; - // the inTransaction guard below then correctly skips the ROLLBACK. - db.exec('BEGIN') - db.prepare('SELECT count(*) FROM sqlite_master').get() - const result = await work() - db.exec('COMMIT') + // A commit can land between the pre-drain above and the snapshot mark; those frames + // sit at or below the mark and could be backfilled mid-copy. Re-drain while holding + // the mark so nothing backfillable remains, or bail to the WAL-shipping fallback. + const result = await withBackupSnapshot(openDb, work, () => drainWal(mainDb)) return { acquired: true, result } } catch (error) { - if (db.inTransaction) { - db.exec('ROLLBACK') + if (error instanceof BackupSnapshotNotDrainedError) { + return { acquired: false } } throw error - } finally { - db.close() } } diff --git a/src/main/sync/index.ts b/src/main/sync/index.ts index 8a41e5d02..b1705ac0a 100644 --- a/src/main/sync/index.ts +++ b/src/main/sync/index.ts @@ -14,7 +14,7 @@ import { type SyncBackupManifest } from './configImportService' import type { SyncSettings } from './settings' -import type { BackupReadLockOutcome } from '@/data/backupReadLock' +import { withBackupSnapshot, type BackupReadLockOutcome } from '@/data/backupReadLock' import type { SettingsDatabase } from '@/settings/data/database' import type { ProviderDatabase } from '@/provider/data/database' @@ -708,7 +708,7 @@ export class SyncService { await new Promise((resolve, reject) => { const output = fs.createWriteStream(targetPath) const archive = new Zip() - let drain: Promise | null = null + let drain: Promise = Promise.resolve() output.on('error', reject) archive.ondata = (error, chunk, final) => { if (error) { @@ -733,16 +733,15 @@ export class SyncService { const entry = new AsyncZipDeflate(name, { level: ZIP_LEVEL }) archive.add(entry) for (let offset = 0; offset < data.length; offset += ZIP_SLICE_SIZE) { + await drain const end = Math.min(offset + ZIP_SLICE_SIZE, data.length) entry.push(new Uint8Array(data.slice(offset, end)), false) - if (drain) { - await drain - drain = null - } await new Promise((resume) => setImmediate(resume)) } + await drain entry.push(new Uint8Array(0), true) } + await drain archive.end() } catch (error) { output.destroy() @@ -761,7 +760,7 @@ export class SyncService { if (!snapshot.acquired) { console.warn( '[Sync] Backup could not drain the WAL (a reader is blocking the checkpoint); ' + - 'falling back to a best-effort copy that ships the WAL sidecar so committed ' + + 'falling back to a snapshot-pinned copy that ships the WAL sidecar so committed ' + 'transactions are not silently dropped' ) return this.readBackupFilesFallback() @@ -770,20 +769,28 @@ export class SyncService { } private async readBackupFilesFallback(): Promise> { - const files = this.readSupportFiles() - files[ZIP_PATHS.agentDb] = toUint8ArrayView(await fs.promises.readFile(this.DB_PATH)) - const walPath = `${this.DB_PATH}-wal` - try { - const walImage = await fs.promises.readFile(walPath) - if (walImage.length > 0) { - files[ZIP_PATHS.agentDbWal] = toUint8ArrayView(walImage) - } - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { - throw error + // The drain failed, so a checkpoint can still backfill mid-copy. Pin a read mark for + // the whole copy: while it is held the WAL cannot be reset, and any backfilled frames + // are at or below the mark, so the shipped db + WAL pair replays to one generation. + return withBackupSnapshot( + () => this.database.openDatabaseConnection(this.DB_PATH), + async () => { + const files = this.readSupportFiles() + files[ZIP_PATHS.agentDb] = toUint8ArrayView(await fs.promises.readFile(this.DB_PATH)) + const walPath = `${this.DB_PATH}-wal` + try { + const walImage = await fs.promises.readFile(walPath) + if (walImage.length > 0) { + files[ZIP_PATHS.agentDbWal] = toUint8ArrayView(walImage) + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error + } + } + return files } - } - return files + ) } private readSupportFiles(): Record { diff --git a/test/main/data/backupReadLock.test.ts b/test/main/data/backupReadLock.test.ts new file mode 100644 index 000000000..53b1b6c85 --- /dev/null +++ b/test/main/data/backupReadLock.test.ts @@ -0,0 +1,98 @@ +import { describe, it, expect, vi } from 'vitest' +import { withBackupReadLock, withBackupSnapshot } from '@/data/backupReadLock' + +interface FakeConnection { + exec: ReturnType + prepare: ReturnType + inTransaction: boolean + close: ReturnType +} + +function createConnection(overrides: Partial = {}): FakeConnection { + const connection: FakeConnection = { + exec: vi.fn(), + prepare: vi.fn(() => ({ get: vi.fn(() => ({ count: 0 })) })), + inTransaction: false, + close: vi.fn(), + ...overrides + } + if (!overrides.exec) { + connection.exec = vi.fn((sql: string) => { + if (sql === 'BEGIN') connection.inTransaction = true + if (sql === 'COMMIT' || sql === 'ROLLBACK') connection.inTransaction = false + }) + } + return connection +} + +describe('withBackupSnapshot', () => { + it('rolls back without masking the original error', async () => { + const connection = createConnection({ inTransaction: true }) + connection.exec.mockImplementation((sql: string) => { + if (sql === 'ROLLBACK') throw new Error('rollback failed') + }) + + await expect( + withBackupSnapshot( + () => connection as never, + async () => { + throw new Error('work failed') + } + ) + ).rejects.toThrow('work failed') + expect(connection.exec).toHaveBeenCalledWith('ROLLBACK') + expect(connection.close).toHaveBeenCalled() + }) + + it('skips the rollback when BEGIN never opened a transaction', async () => { + const connection = createConnection() + connection.exec.mockImplementation((sql: string) => { + if (sql === 'BEGIN') throw new Error('begin failed') + }) + + await expect( + withBackupSnapshot( + () => connection as never, + async () => undefined + ) + ).rejects.toThrow('begin failed') + expect(connection.exec).not.toHaveBeenCalledWith('ROLLBACK') + expect(connection.close).toHaveBeenCalled() + }) +}) + +describe('withBackupReadLock', () => { + it('reports not acquired without opening a connection when the WAL cannot drain', async () => { + const mainDb = { + open: true, + pragma: vi.fn(() => [{ busy: 1, log: 2, checkpointed: 1 }]) + } + const openDb = vi.fn() + + const outcome = await withBackupReadLock(mainDb as never, openDb, async () => 'result') + + expect(outcome).toEqual({ acquired: false }) + expect(openDb).not.toHaveBeenCalled() + }) + + it('re-drains under the snapshot mark and bails when a commit sneaked in', async () => { + const mainDb = { + open: true, + pragma: vi + .fn() + .mockReturnValueOnce([{ busy: 0, log: 1, checkpointed: 1 }]) + .mockReturnValue([{ busy: 0, log: 2, checkpointed: 1 }]) + } + const connection = createConnection() + + const outcome = await withBackupReadLock( + mainDb as never, + () => connection as never, + async () => 'result' + ) + + expect(outcome).toEqual({ acquired: false }) + expect(connection.exec).toHaveBeenCalledWith('ROLLBACK') + expect(connection.close).toHaveBeenCalled() + }) +}) diff --git a/test/main/sync/backupConsistency.test.ts b/test/main/sync/backupConsistency.test.ts index 975df4d01..ac6266a7e 100644 --- a/test/main/sync/backupConsistency.test.ts +++ b/test/main/sync/backupConsistency.test.ts @@ -62,7 +62,10 @@ afterEach(() => { fs.rmSync(syncDir, { recursive: true, force: true }) }) -function buildService(password: string | undefined): InstanceType { +function buildService( + password: string | undefined, + options: { forceBusyCheckpoint?: boolean; sneakCommitAfterDrain?: boolean } = {} +): InstanceType { const connection = openSQLiteDatabase(dbPath, password) writerDb = connection connection.exec('CREATE TABLE backup_probe (id INTEGER PRIMARY KEY, payload TEXT)') @@ -71,11 +74,21 @@ function buildService(password: string | undefined): InstanceType { - const result = connection.pragma(source, options as never) + pragma: (source: string, pragmaOptions?: unknown) => { + if (options.forceBusyCheckpoint && source.startsWith('wal_checkpoint')) { + return [{ busy: 1, log: 1, checkpointed: 0 }] + } + const result = connection.pragma(source, pragmaOptions as never) if (source.startsWith('wal_checkpoint')) { + const first = Array.isArray(result) ? result[0] : undefined + const drained = first && first.busy === 0 && first.checkpointed === first.log + if (options.sneakCommitAfterDrain && drained && !sneaked) { + sneaked = true + connection.prepare('UPDATE backup_probe SET payload = payload || payload').run() + } postCheckpointImages.push(fs.readFileSync(dbPath)) } return result @@ -308,4 +321,76 @@ describe('backup database image consistency', () => { restored.close() } }) + + it('pins the WAL against a mid-copy reset attempt when the drain fails', async () => { + const service = buildService(undefined, { forceBusyCheckpoint: true }) + const connection = writerDb as Database.Database + const insert = connection.prepare('INSERT INTO backup_probe (payload) VALUES (?)') + connection.transaction(() => { + for (let index = 0; index < 40; index++) insert.run(ROW_PAYLOAD) + })() + + // No busy-wait: the reset must be rejected immediately because the fallback's + // snapshot mark is held for the whole copy. + connection.pragma('busy_timeout = 0') + const copySpy = copiesInChunks(() => { + connection.transaction(() => { + for (let index = 0; index < 1500; index++) insert.run(ROW_PAYLOAD) + })() + // Without the snapshot mark this TRUNCATE resets the WAL mid-copy and the + // archive mixes generations; with the mark it must report busy instead. + connection.pragma('wal_checkpoint(TRUNCATE)') + }) + const backup = await service.startBackup() + + expect(backup).not.toBeNull() + copySpy.mockRestore() + + const archive = fs.readFileSync(path.join(syncDir, (backup as { fileName: string }).fileName)) + const entries = unzipSync(new Uint8Array(archive)) as unknown as Record + const walEntry = entries[AGENT_DB_WAL_ENTRY] + expect(walEntry).toBeDefined() + expect(walEntry.length).toBeGreaterThan(0) + + const restoredPath = path.join(userDataDir, 'restored-pinned-wal.db') + fs.writeFileSync(restoredPath, Buffer.from(entries[AGENT_DB_ENTRY])) + fs.writeFileSync(`${restoredPath}-wal`, Buffer.from(walEntry)) + const restored = new Database(restoredPath) + try { + expect(restored.pragma('integrity_check', { simple: true })).toBe('ok') + const { count } = restored.prepare('SELECT count(*) AS count FROM backup_probe').get() as { + count: number + } + expect(count).toBe(SEED_ROWS + 40 + 1500) + } finally { + restored.close() + } + }) + + it('keeps the image stable when a commit lands between the drain and the snapshot mark', async () => { + const service = buildService(undefined, { sneakCommitAfterDrain: true }) + const connection = writerDb as Database.Database + + // The sneaked UPDATE rewrites every page, so a backfill landing mid-copy mixes two + // different page layouts unless the sneaked frames are drained before the copy starts. + const copySpy = copiesInChunks(() => { + connection.pragma('wal_checkpoint(PASSIVE)') + }) + const backup = await service.startBackup() + + expect(backup).not.toBeNull() + copySpy.mockRestore() + const archived = archivedDatabaseEntry((backup as { fileName: string }).fileName) + + const restored = openCopiedImage(archived, undefined) + try { + expect(restored.pragma('integrity_check', { simple: true })).toBe('ok') + const row = restored.prepare('SELECT payload FROM backup_probe WHERE id = 1').get() as { + payload: string + } + expect(row.payload).toBe(ROW_PAYLOAD + ROW_PAYLOAD) + } finally { + restored.close() + } + }) }) diff --git a/test/main/sync/syncService.test.ts b/test/main/sync/syncService.test.ts index 00b735097..1ac320149 100644 --- a/test/main/sync/syncService.test.ts +++ b/test/main/sync/syncService.test.ts @@ -51,6 +51,7 @@ vi.mock('better-sqlite3-multiple-ciphers', async () => { class MockDatabase { private state: MockState + private inTx = false constructor( private readonly dbPath: string, @@ -59,7 +60,14 @@ vi.mock('better-sqlite3-multiple-ciphers', async () => { this.state = readState(dbPath) } + get inTransaction() { + return this.inTx + } + exec(sql: string) { + const normalized = sql.replace(/\s+/g, ' ').trim().toUpperCase() + if (normalized === 'BEGIN') this.inTx = true + if (normalized === 'COMMIT' || normalized === 'ROLLBACK') this.inTx = false for (const match of sql.matchAll(/CREATE TABLE IF NOT EXISTS\s+([a-zA-Z_][\w]*)/gi)) { this.ensureTable(match[1]) } @@ -107,6 +115,12 @@ vi.mock('better-sqlite3-multiple-ciphers', async () => { } } + if (normalizedSql === 'SELECT count(*) FROM sqlite_master') { + return { + get: () => ({ count: Object.keys(this.state.tables).length }) + } + } + const countMatch = normalizedSql.match(/^SELECT COUNT\(\*\) as count FROM "?([\w]+)"?$/i) if (countMatch) { return { @@ -371,6 +385,7 @@ describe('SyncService backup import', () => { hasConfigMigration: vi.fn(() => true) }, getDatabasePassword: vi.fn(() => undefined), + openDatabaseConnection: vi.fn((target: string) => new Database(target)), clearNewAgentData: vi.fn(), importLegacyChatDb: vi.fn(async () => ({ importedSessions: 0, From 10b539cc1f977426cf7dece62107c326f15efbee Mon Sep 17 00:00:00 2001 From: xiao-test Date: Tue, 15 Sep 2026 13:30:15 +0800 Subject: [PATCH 06/19] perf(sync): avoid copying zip chunks into buffers --- src/main/sync/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/sync/index.ts b/src/main/sync/index.ts index b1705ac0a..914dfa76d 100644 --- a/src/main/sync/index.ts +++ b/src/main/sync/index.ts @@ -716,7 +716,7 @@ export class SyncService { reject(error) return } - if (!output.write(Buffer.from(chunk))) { + if (!output.write(Buffer.from(chunk.buffer, chunk.byteOffset, chunk.byteLength))) { drain = new Promise((drained) => { output.once('drain', () => drained()) output.once('error', () => drained()) From e1b4a1d6782515590c03cdb6df7b65b2e4ec7032 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Tue, 15 Sep 2026 14:04:33 +0800 Subject: [PATCH 07/19] perf(sync): read backup support files asynchronously --- src/main/sync/index.ts | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/main/sync/index.ts b/src/main/sync/index.ts index 914dfa76d..bcd26f116 100644 --- a/src/main/sync/index.ts +++ b/src/main/sync/index.ts @@ -753,7 +753,7 @@ export class SyncService { private async collectBackupFiles(): Promise> { const snapshot = await this.database.withBackupReadLock(async () => { - const files = this.readSupportFiles() + const files = await this.readSupportFiles() files[ZIP_PATHS.agentDb] = toUint8ArrayView(await fs.promises.readFile(this.DB_PATH)) return files }) @@ -775,7 +775,7 @@ export class SyncService { return withBackupSnapshot( () => this.database.openDatabaseConnection(this.DB_PATH), async () => { - const files = this.readSupportFiles() + const files = await this.readSupportFiles() files[ZIP_PATHS.agentDb] = toUint8ArrayView(await fs.promises.readFile(this.DB_PATH)) const walPath = `${this.DB_PATH}-wal` try { @@ -793,11 +793,11 @@ export class SyncService { ) } - private readSupportFiles(): Record { + private async readSupportFiles(): Promise> { const files: Record = {} - files[ZIP_PATHS.appSettings] = this.readSanitizedAppSettingsBackup() - this.addOptionalFile(files, ZIP_PATHS.customPrompts, this.CUSTOM_PROMPTS_PATH) - this.addOptionalFile(files, ZIP_PATHS.systemPrompts, this.SYSTEM_PROMPTS_PATH) + files[ZIP_PATHS.appSettings] = await this.readSanitizedAppSettingsBackup() + await this.addOptionalFile(files, ZIP_PATHS.customPrompts, this.CUSTOM_PROMPTS_PATH) + await this.addOptionalFile(files, ZIP_PATHS.systemPrompts, this.SYSTEM_PROMPTS_PATH) return files } @@ -838,18 +838,22 @@ export class SyncService { return baseName } - private addOptionalFile( + private async addOptionalFile( files: Record, zipPath: string, filePath: string - ): void { - if (fs.existsSync(filePath)) { - files[zipPath] = toUint8ArrayView(fs.readFileSync(filePath)) + ): Promise { + try { + files[zipPath] = toUint8ArrayView(await fs.promises.readFile(filePath)) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error + } } } - private readSanitizedAppSettingsBackup(): Uint8Array { - const raw = fs.readFileSync(this.APP_SETTINGS_PATH, 'utf-8') + private async readSanitizedAppSettingsBackup(): Promise { + const raw = await fs.promises.readFile(this.APP_SETTINGS_PATH, 'utf-8') const parsed = JSON.parse(raw) as Record const sanitized = this.removeMigratedAppSettings(parsed) return new Uint8Array(Buffer.from(JSON.stringify(sanitized, null, 2), 'utf-8')) From 626a484da58dd6520d268f78a47db2b51cc0f89e Mon Sep 17 00:00:00 2001 From: xiao-test Date: Tue, 15 Sep 2026 15:18:10 +0800 Subject: [PATCH 08/19] fix(sync): harden backup error paths and fs races - Pass the error to output.destroy() on both zip failure paths so a pending backpressure drain gate is released; previously the producer coroutine stayed suspended and retained the files record (including the full agent.db image) for the process lifetime - Broaden the fallback warning: { acquired: false } now also means a commit landed during the drain window, not only a blocked checkpoint - Replace existsSync check-then-act with direct operation plus ENOENT handling across backup/restore helpers, closing delete-between-check- and-use races in settings, prompt, temp backup, WAL sidecar, and zip cleanup paths --- src/main/sync/index.ts | 82 +++++++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 33 deletions(-) diff --git a/src/main/sync/index.ts b/src/main/sync/index.ts index bcd26f116..57762dbfe 100644 --- a/src/main/sync/index.ts +++ b/src/main/sync/index.ts @@ -590,8 +590,12 @@ export class SyncService { this.emitBackupStatus('compressing') await this.writeZipToDisk(files, tempZipPath) - if (fs.existsSync(finalZipPath)) { + try { await fs.promises.unlink(finalZipPath) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error + } } this.emitBackupStatus('finalizing') await fs.promises.rename(tempZipPath, finalZipPath) @@ -606,8 +610,12 @@ export class SyncService { return { fileName: backupFileName, createdAt: timestamp, size: backupStats.size } } catch (error) { - if (fs.existsSync(tempZipPath)) { + try { await fs.promises.unlink(tempZipPath) + } catch (cleanupError) { + if ((cleanupError as NodeJS.ErrnoException).code !== 'ENOENT') { + console.warn('[Sync] Failed to remove partial backup archive:', cleanupError) + } } encounteredError = true this.emitBackupStatus('error', { @@ -712,7 +720,7 @@ export class SyncService { output.on('error', reject) archive.ondata = (error, chunk, final) => { if (error) { - output.destroy() + output.destroy(error) reject(error) return } @@ -744,7 +752,7 @@ export class SyncService { await drain archive.end() } catch (error) { - output.destroy() + output.destroy(error as Error) reject(error) } })() @@ -759,9 +767,9 @@ export class SyncService { }) if (!snapshot.acquired) { console.warn( - '[Sync] Backup could not drain the WAL (a reader is blocking the checkpoint); ' + - 'falling back to a snapshot-pinned copy that ships the WAL sidecar so committed ' + - 'transactions are not silently dropped' + '[Sync] Backup could not take a fully drained WAL snapshot (blocked checkpoint or a ' + + 'commit landed during the drain window); falling back to a snapshot-pinned copy ' + + 'that ships the WAL sidecar so committed transactions are not silently dropped' ) return this.readBackupFilesFallback() } @@ -1010,9 +1018,6 @@ export class SyncService { } private readSettingsFile(filePath: string): Record | null { - if (!fs.existsSync(filePath)) { - return null - } try { const parsed = JSON.parse(fs.readFileSync(filePath, 'utf-8')) if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { @@ -1020,20 +1025,22 @@ export class SyncService { } return parsed as Record } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return null + } console.error('Failed to read settings file for machine-local setting preservation:', error) throw new Error('sync.error.importFailed') } } private mergeAppSettingsPreservingMachineLocal(backupPath: string, targetPath: string): void { - if (!fs.existsSync(backupPath)) { - return - } - let backupSettingsRaw: string try { backupSettingsRaw = fs.readFileSync(backupPath, 'utf-8') } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return + } console.error('Failed to read backup app settings file:', error) throw new Error('sync.error.noValidBackup') } @@ -1078,11 +1085,15 @@ export class SyncService { } private createTempBackup(originalPath: string, name: string): string | null { - if (!fs.existsSync(originalPath)) { - return null - } const tempPath = path.join(app.getPath('temp'), `${name}.${Date.now()}.bak`) - this.copyFile(originalPath, tempPath) + try { + this.copyFile(originalPath, tempPath) + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return null + } + throw error + } return tempPath } @@ -1097,25 +1108,27 @@ export class SyncService { private restoreBackupWalSidecar(sourceDbPath: string, targetDbPath: string): void { const sourceWalPath = `${sourceDbPath}-wal` - if (!fs.existsSync(sourceWalPath)) { - return - } // Ships un-checkpointed transactions from a fallback archive; the next // read-write open replays them. Placed after cleanupDatabaseSidecarFiles so // stale sidecars from the previous database cannot mix with this image. - this.copyFile(sourceWalPath, `${targetDbPath}-wal`) + try { + this.copyFile(sourceWalPath, `${targetDbPath}-wal`) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error + } + } } private cleanupDatabaseSidecarFiles(dbFilePath: string): void { const sidecarFiles = [`${dbFilePath}-wal`, `${dbFilePath}-shm`] for (const filePath of sidecarFiles) { - if (!fs.existsSync(filePath)) { - continue - } try { fs.unlinkSync(filePath) } catch (error) { - console.warn('Failed to remove database sidecar file:', filePath, error) + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + console.warn('Failed to remove database sidecar file:', filePath, error) + } } } } @@ -1141,10 +1154,13 @@ export class SyncService { private cleanupTempFiles(paths: Array): void { for (const filePath of paths) { - if (filePath && fs.existsSync(filePath)) { - try { - fs.unlinkSync(filePath) - } catch (error) { + if (!filePath) { + continue + } + try { + fs.unlinkSync(filePath) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { console.warn('Failed to remove temp file:', filePath, error) } } @@ -1207,9 +1223,6 @@ export class SyncService { } private readPromptStore(filePath: string): PromptStore | null { - if (!fs.existsSync(filePath)) { - return null - } try { const content = fs.readFileSync(filePath, 'utf-8') const parsed = JSON.parse(content) @@ -1218,6 +1231,9 @@ export class SyncService { } return parsed as PromptStore } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return null + } console.warn('Failed to read prompt store:', filePath, error) return { prompts: [] } } From a673536785c0731ce7821701f03257e55dad9edb Mon Sep 17 00:00:00 2001 From: xiao-test Date: Wed, 16 Sep 2026 11:23:40 +0800 Subject: [PATCH 09/19] fix(auth): harden OAuth credential storage --- .../auth/openaiCodex/credentialStore.ts | 112 ++++++++++++--- src/main/provider/auth/openaiCodex/index.ts | 9 +- .../provider/auth/xaiGrok/credentialStore.ts | 107 +++++++++++--- src/main/provider/auth/xaiGrok/index.ts | 9 +- .../openaiCodexCredentialStore.test.ts | 131 ++++++++++++++++++ .../provider/xaiGrokCredentialStore.test.ts | 131 ++++++++++++++++++ test/setup.ts | 1 + 7 files changed, 446 insertions(+), 54 deletions(-) create mode 100644 test/main/provider/openaiCodexCredentialStore.test.ts create mode 100644 test/main/provider/xaiGrokCredentialStore.test.ts diff --git a/src/main/provider/auth/openaiCodex/credentialStore.ts b/src/main/provider/auth/openaiCodex/credentialStore.ts index 7bafdc043..5900b258e 100644 --- a/src/main/provider/auth/openaiCodex/credentialStore.ts +++ b/src/main/provider/auth/openaiCodex/credentialStore.ts @@ -30,8 +30,18 @@ type StoredCredentialEnvelope = updatedAt: number } +type EnvelopeReadResult = + | { state: 'missing' } + | { state: 'ok'; tokens: OpenAICodexTokenSet } + | { state: 'corrupt'; reason: string } + +function toErrorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} + export class OpenAICodexCredentialStore { private readonly filePath: string + private lastLoadError: string | null = null constructor(filePath?: string) { this.filePath = @@ -41,34 +51,31 @@ export class OpenAICodexCredentialStore { getStorageState(): OpenAICodexCredentialStorage { try { return safeStorage.isEncryptionAvailable() ? 'safeStorage' : 'file' - } catch { + } catch (error) { + console.warn( + '[OpenAICodexCredentialStore] Encryption availability check failed, using file storage:', + error + ) return 'file' } } - load(): OpenAICodexTokenSet | null { - try { - if (!fs.existsSync(this.filePath)) { - return null - } - - const envelope = JSON.parse(fs.readFileSync(this.filePath, 'utf-8')) as - | StoredCredentialEnvelope - | undefined - - if (!envelope || envelope.version !== 1) { - return null - } - - if (envelope.storage === 'file') { - return this.normalizeTokens(envelope.tokens) - } + getLoadError(): string | null { + return this.lastLoadError + } - const raw = safeStorage.decryptString(Buffer.from(envelope.wrapped, 'base64')) - return this.normalizeTokens(JSON.parse(raw) as OpenAICodexTokenSet) - } catch { + load(): OpenAICodexTokenSet | null { + const result = this.readEnvelope() + if (result.state === 'corrupt') { + this.lastLoadError = result.reason + console.warn( + `[OpenAICodexCredentialStore] Ignoring corrupted credential file: ${result.reason}` + ) return null } + + this.lastLoadError = null + return result.state === 'ok' ? result.tokens : null } save(tokens: OpenAICodexTokenSet): void { @@ -94,16 +101,75 @@ export class OpenAICodexCredentialStore { updatedAt: now } - fs.writeFileSync(this.filePath, JSON.stringify(envelope, null, 2), { + if (this.readEnvelope().state === 'corrupt') { + try { + fs.copyFileSync(this.filePath, `${this.filePath}.corrupt`) + } catch (error) { + console.warn( + '[OpenAICodexCredentialStore] Failed to back up corrupted credential file:', + error + ) + } + } + + const temporaryPath = `${this.filePath}.tmp` + fs.writeFileSync(temporaryPath, JSON.stringify(envelope, null, 2), { encoding: 'utf-8', mode: 0o600 }) + fs.renameSync(temporaryPath, this.filePath) + this.lastLoadError = null } clear(): void { try { fs.rmSync(this.filePath, { force: true }) - } catch {} + fs.rmSync(`${this.filePath}.corrupt`, { force: true }) + fs.rmSync(`${this.filePath}.tmp`, { force: true }) + this.lastLoadError = null + } catch (error) { + console.warn('[OpenAICodexCredentialStore] Failed to remove credential files:', error) + } + } + + private readEnvelope(): EnvelopeReadResult { + let raw: string + try { + raw = fs.readFileSync(this.filePath, 'utf-8') + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return { state: 'missing' } + } + return { state: 'corrupt', reason: `credential file is unreadable: ${toErrorMessage(error)}` } + } + + let envelope: StoredCredentialEnvelope | undefined + try { + envelope = JSON.parse(raw) as StoredCredentialEnvelope | undefined + } catch { + return { state: 'corrupt', reason: 'credential file is not valid JSON' } + } + + if (!envelope || envelope.version !== 1) { + return { state: 'corrupt', reason: 'credential envelope has an unsupported version' } + } + + if (envelope.storage === 'file') { + const tokens = this.normalizeTokens(envelope.tokens) + return tokens + ? { state: 'ok', tokens } + : { state: 'corrupt', reason: 'credential file holds an invalid token payload' } + } + + try { + const decrypted = safeStorage.decryptString(Buffer.from(envelope.wrapped, 'base64')) + const tokens = this.normalizeTokens(JSON.parse(decrypted) as OpenAICodexTokenSet) + return tokens + ? { state: 'ok', tokens } + : { state: 'corrupt', reason: 'credential file holds an invalid token payload' } + } catch (error) { + return { state: 'corrupt', reason: `credential decryption failed: ${toErrorMessage(error)}` } + } } private normalizeTokens(tokens: OpenAICodexTokenSet | undefined): OpenAICodexTokenSet | null { diff --git a/src/main/provider/auth/openaiCodex/index.ts b/src/main/provider/auth/openaiCodex/index.ts index 85ff09dbc..3193d817b 100644 --- a/src/main/provider/auth/openaiCodex/index.ts +++ b/src/main/provider/auth/openaiCodex/index.ts @@ -176,10 +176,11 @@ export class OpenAICodexAuth { return this.statusFromTokens(tokens) } + const statusError = this.lastError ?? this.store.getLoadError() return this.withStorage({ - state: this.lastError ? 'error' : 'signed-out', + state: statusError ? 'error' : 'signed-out', authenticated: false, - ...(this.lastError ? { error: this.lastError } : {}) + ...(statusError ? { error: statusError } : {}) }) } @@ -305,7 +306,7 @@ export class OpenAICodexAuth { const tokens = this.store.load() if (!tokens) { - throw new Error('OpenAI Codex sign-in is required') + throw new Error(this.store.getLoadError() ?? 'OpenAI Codex sign-in is required') } const current = @@ -322,7 +323,7 @@ export class OpenAICodexAuth { this.assertEnabled() const tokens = this.store.load() if (!tokens?.refreshToken) { - throw new Error('OpenAI Codex refresh token is unavailable') + throw new Error(this.store.getLoadError() ?? 'OpenAI Codex refresh token is unavailable') } const refreshed = await this.refreshAccessToken(tokens, true) diff --git a/src/main/provider/auth/xaiGrok/credentialStore.ts b/src/main/provider/auth/xaiGrok/credentialStore.ts index f146e0cac..e13de4a67 100644 --- a/src/main/provider/auth/xaiGrok/credentialStore.ts +++ b/src/main/provider/auth/xaiGrok/credentialStore.ts @@ -31,8 +31,18 @@ type StoredCredentialEnvelope = updatedAt: number } +type EnvelopeReadResult = + | { state: 'missing' } + | { state: 'ok'; tokens: XaiGrokTokenSet } + | { state: 'corrupt'; reason: string } + +function toErrorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} + export class XaiGrokCredentialStore { private readonly filePath: string + private lastLoadError: string | null = null constructor(filePath?: string) { this.filePath = @@ -42,34 +52,29 @@ export class XaiGrokCredentialStore { getStorageState(): XaiGrokCredentialStorage { try { return safeStorage.isEncryptionAvailable() ? 'safeStorage' : 'file' - } catch { + } catch (error) { + console.warn( + '[XaiGrokCredentialStore] Encryption availability check failed, using file storage:', + error + ) return 'file' } } - load(): XaiGrokTokenSet | null { - try { - if (!fs.existsSync(this.filePath)) { - return null - } - - const envelope = JSON.parse(fs.readFileSync(this.filePath, 'utf-8')) as - | StoredCredentialEnvelope - | undefined - - if (!envelope || envelope.version !== 1) { - return null - } - - if (envelope.storage === 'file') { - return this.normalizeTokens(envelope.tokens) - } + getLoadError(): string | null { + return this.lastLoadError + } - const raw = safeStorage.decryptString(Buffer.from(envelope.wrapped, 'base64')) - return this.normalizeTokens(JSON.parse(raw) as XaiGrokTokenSet) - } catch { + load(): XaiGrokTokenSet | null { + const result = this.readEnvelope() + if (result.state === 'corrupt') { + this.lastLoadError = result.reason + console.warn(`[XaiGrokCredentialStore] Ignoring corrupted credential file: ${result.reason}`) return null } + + this.lastLoadError = null + return result.state === 'ok' ? result.tokens : null } save(tokens: XaiGrokTokenSet): void { @@ -95,16 +100,72 @@ export class XaiGrokCredentialStore { updatedAt: now } - fs.writeFileSync(this.filePath, JSON.stringify(envelope, null, 2), { + if (this.readEnvelope().state === 'corrupt') { + try { + fs.copyFileSync(this.filePath, `${this.filePath}.corrupt`) + } catch (error) { + console.warn('[XaiGrokCredentialStore] Failed to back up corrupted credential file:', error) + } + } + + const temporaryPath = `${this.filePath}.tmp` + fs.writeFileSync(temporaryPath, JSON.stringify(envelope, null, 2), { encoding: 'utf-8', mode: 0o600 }) + fs.renameSync(temporaryPath, this.filePath) + this.lastLoadError = null } clear(): void { try { fs.rmSync(this.filePath, { force: true }) - } catch {} + fs.rmSync(`${this.filePath}.corrupt`, { force: true }) + fs.rmSync(`${this.filePath}.tmp`, { force: true }) + this.lastLoadError = null + } catch (error) { + console.warn('[XaiGrokCredentialStore] Failed to remove credential files:', error) + } + } + + private readEnvelope(): EnvelopeReadResult { + let raw: string + try { + raw = fs.readFileSync(this.filePath, 'utf-8') + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return { state: 'missing' } + } + return { state: 'corrupt', reason: `credential file is unreadable: ${toErrorMessage(error)}` } + } + + let envelope: StoredCredentialEnvelope | undefined + try { + envelope = JSON.parse(raw) as StoredCredentialEnvelope | undefined + } catch { + return { state: 'corrupt', reason: 'credential file is not valid JSON' } + } + + if (!envelope || envelope.version !== 1) { + return { state: 'corrupt', reason: 'credential envelope has an unsupported version' } + } + + if (envelope.storage === 'file') { + const tokens = this.normalizeTokens(envelope.tokens) + return tokens + ? { state: 'ok', tokens } + : { state: 'corrupt', reason: 'credential file holds an invalid token payload' } + } + + try { + const decrypted = safeStorage.decryptString(Buffer.from(envelope.wrapped, 'base64')) + const tokens = this.normalizeTokens(JSON.parse(decrypted) as XaiGrokTokenSet) + return tokens + ? { state: 'ok', tokens } + : { state: 'corrupt', reason: 'credential file holds an invalid token payload' } + } catch (error) { + return { state: 'corrupt', reason: `credential decryption failed: ${toErrorMessage(error)}` } + } } private normalizeTokens(tokens: XaiGrokTokenSet | undefined): XaiGrokTokenSet | null { diff --git a/src/main/provider/auth/xaiGrok/index.ts b/src/main/provider/auth/xaiGrok/index.ts index 4f25dbff1..69d3d3f48 100644 --- a/src/main/provider/auth/xaiGrok/index.ts +++ b/src/main/provider/auth/xaiGrok/index.ts @@ -169,10 +169,11 @@ export class XaiGrokAuth { return this.statusFromTokens(tokens) } + const statusError = this.lastError ?? this.store.getLoadError() return this.withStorage({ - state: this.lastError ? 'error' : 'signed-out', + state: statusError ? 'error' : 'signed-out', authenticated: false, - ...(this.lastError ? { error: this.lastError } : {}) + ...(statusError ? { error: statusError } : {}) }) } @@ -313,7 +314,7 @@ export class XaiGrokAuth { async getAccessToken(): Promise { const token = await this.ensureAccessToken() if (!token) { - throw new Error('xAI Grok OAuth sign-in is required') + throw new Error(this.store.getLoadError() ?? 'xAI Grok OAuth sign-in is required') } return token } @@ -322,7 +323,7 @@ export class XaiGrokAuth { this.assertEnabled() const tokens = this.store.load() if (!tokens?.refreshToken) { - throw new Error('xAI Grok OAuth refresh token is unavailable') + throw new Error(this.store.getLoadError() ?? 'xAI Grok OAuth refresh token is unavailable') } const refreshed = await this.refreshAccessToken(tokens, true) return refreshed.accessToken diff --git a/test/main/provider/openaiCodexCredentialStore.test.ts b/test/main/provider/openaiCodexCredentialStore.test.ts new file mode 100644 index 000000000..ec1ea3864 --- /dev/null +++ b/test/main/provider/openaiCodexCredentialStore.test.ts @@ -0,0 +1,131 @@ +import * as fs from 'fs' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { safeStorage } from 'electron' +import { + OpenAICodexCredentialStore, + type OpenAICodexTokenSet +} from '@/provider/auth/openaiCodex/credentialStore' + +const filePath = '/tmp/deepchat-openai-codex/credentials.json' + +const tokens: OpenAICodexTokenSet = { + accessToken: 'access-token', + refreshToken: 'refresh-token', + tokenType: 'Bearer', + expiresAt: Date.now() + 3_600_000, + updatedAt: Date.now() +} + +function fileEnvelope(value: OpenAICodexTokenSet): string { + return JSON.stringify({ version: 1, storage: 'file', tokens: value, updatedAt: 1 }) +} + +describe('OpenAICodexCredentialStore', () => { + let savedContent: string | null = null + + beforeEach(() => { + savedContent = null + vi.mocked(fs.readFileSync).mockImplementation(() => { + if (savedContent === null) { + throw Object.assign(new Error('ENOENT: no such file or directory'), { code: 'ENOENT' }) + } + return savedContent + }) + vi.mocked(fs.writeFileSync).mockImplementation((_, data) => { + savedContent = String(data) + }) + vi.mocked(fs.mkdirSync).mockImplementation(() => undefined as unknown as string) + vi.mocked(fs.renameSync).mockImplementation(() => {}) + vi.mocked(safeStorage.isEncryptionAvailable).mockReturnValue(false) + }) + + it('returns null without an error when the credential file is missing', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const store = new OpenAICodexCredentialStore(filePath) + + expect(store.load()).toBeNull() + expect(store.getLoadError()).toBeNull() + expect(warn).not.toHaveBeenCalled() + }) + + it('reports corrupted JSON instead of treating it as signed out', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + savedContent = '{broken' + const store = new OpenAICodexCredentialStore(filePath) + + expect(store.load()).toBeNull() + expect(store.getLoadError()).toContain('not valid JSON') + expect(warn).toHaveBeenCalled() + }) + + it('reports decryption failures instead of treating them as signed out', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(safeStorage.decryptString).mockImplementationOnce(() => { + throw new Error('keyring locked') + }) + savedContent = JSON.stringify({ + version: 1, + storage: 'safeStorage', + wrapped: Buffer.from('ciphertext').toString('base64'), + updatedAt: 1 + }) + const store = new OpenAICodexCredentialStore(filePath) + + expect(store.load()).toBeNull() + expect(store.getLoadError()).toContain('decryption failed') + }) + + it('returns tokens and clears the load error for a healthy envelope', () => { + savedContent = fileEnvelope(tokens) + const store = new OpenAICodexCredentialStore(filePath) + + expect(store.load()?.accessToken).toBe('access-token') + expect(store.getLoadError()).toBeNull() + }) + + it('writes through a temporary file and renames it into place', () => { + const store = new OpenAICodexCredentialStore(filePath) + + store.save(tokens) + + expect(fs.writeFileSync).toHaveBeenCalledWith( + `${filePath}.tmp`, + expect.any(String), + expect.objectContaining({ mode: 0o600 }) + ) + expect(fs.renameSync).toHaveBeenCalledWith(`${filePath}.tmp`, filePath) + expect(store.load()?.accessToken).toBe('access-token') + }) + + it('backs up a corrupted file before overwriting it', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + savedContent = '{broken' + const store = new OpenAICodexCredentialStore(filePath) + + store.save(tokens) + + expect(fs.copyFileSync).toHaveBeenCalledWith(filePath, `${filePath}.corrupt`) + expect(fs.renameSync).not.toHaveBeenCalledWith(filePath, `${filePath}.corrupt`) + expect(store.getLoadError()).toBeNull() + }) + + it('does not back up a healthy file before overwriting it', () => { + savedContent = fileEnvelope(tokens) + const store = new OpenAICodexCredentialStore(filePath) + + store.save({ ...tokens, accessToken: 'new-access-token' }) + + expect(fs.copyFileSync).not.toHaveBeenCalled() + expect(store.load()?.accessToken).toBe('new-access-token') + }) + + it('clear removes the credential file together with its backup and temp file', () => { + const store = new OpenAICodexCredentialStore(filePath) + + store.clear() + + expect(fs.rmSync).toHaveBeenCalledWith(filePath, { force: true }) + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.corrupt`, { force: true }) + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp`, { force: true }) + }) +}) diff --git a/test/main/provider/xaiGrokCredentialStore.test.ts b/test/main/provider/xaiGrokCredentialStore.test.ts new file mode 100644 index 000000000..2ff32c471 --- /dev/null +++ b/test/main/provider/xaiGrokCredentialStore.test.ts @@ -0,0 +1,131 @@ +import * as fs from 'fs' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { safeStorage } from 'electron' +import { + XaiGrokCredentialStore, + type XaiGrokTokenSet +} from '@/provider/auth/xaiGrok/credentialStore' + +const filePath = '/tmp/deepchat-xai-grok/credentials.json' + +const tokens: XaiGrokTokenSet = { + accessToken: 'access-token', + refreshToken: 'refresh-token', + tokenType: 'Bearer', + expiresAt: Date.now() + 3_600_000, + updatedAt: Date.now() +} + +function fileEnvelope(value: XaiGrokTokenSet): string { + return JSON.stringify({ version: 1, storage: 'file', tokens: value, updatedAt: 1 }) +} + +describe('XaiGrokCredentialStore', () => { + let savedContent: string | null = null + + beforeEach(() => { + savedContent = null + vi.mocked(fs.readFileSync).mockImplementation(() => { + if (savedContent === null) { + throw Object.assign(new Error('ENOENT: no such file or directory'), { code: 'ENOENT' }) + } + return savedContent + }) + vi.mocked(fs.writeFileSync).mockImplementation((_, data) => { + savedContent = String(data) + }) + vi.mocked(fs.mkdirSync).mockImplementation(() => undefined as unknown as string) + vi.mocked(fs.renameSync).mockImplementation(() => {}) + vi.mocked(safeStorage.isEncryptionAvailable).mockReturnValue(false) + }) + + it('returns null without an error when the credential file is missing', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const store = new XaiGrokCredentialStore(filePath) + + expect(store.load()).toBeNull() + expect(store.getLoadError()).toBeNull() + expect(warn).not.toHaveBeenCalled() + }) + + it('reports corrupted JSON instead of treating it as signed out', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + savedContent = '{broken' + const store = new XaiGrokCredentialStore(filePath) + + expect(store.load()).toBeNull() + expect(store.getLoadError()).toContain('not valid JSON') + expect(warn).toHaveBeenCalled() + }) + + it('reports decryption failures instead of treating them as signed out', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(safeStorage.decryptString).mockImplementationOnce(() => { + throw new Error('keyring locked') + }) + savedContent = JSON.stringify({ + version: 1, + storage: 'safeStorage', + wrapped: Buffer.from('ciphertext').toString('base64'), + updatedAt: 1 + }) + const store = new XaiGrokCredentialStore(filePath) + + expect(store.load()).toBeNull() + expect(store.getLoadError()).toContain('decryption failed') + }) + + it('returns tokens and clears the load error for a healthy envelope', () => { + savedContent = fileEnvelope(tokens) + const store = new XaiGrokCredentialStore(filePath) + + expect(store.load()?.accessToken).toBe('access-token') + expect(store.getLoadError()).toBeNull() + }) + + it('writes through a temporary file and renames it into place', () => { + const store = new XaiGrokCredentialStore(filePath) + + store.save(tokens) + + expect(fs.writeFileSync).toHaveBeenCalledWith( + `${filePath}.tmp`, + expect.any(String), + expect.objectContaining({ mode: 0o600 }) + ) + expect(fs.renameSync).toHaveBeenCalledWith(`${filePath}.tmp`, filePath) + expect(store.load()?.accessToken).toBe('access-token') + }) + + it('backs up a corrupted file before overwriting it', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + savedContent = '{broken' + const store = new XaiGrokCredentialStore(filePath) + + store.save(tokens) + + expect(fs.copyFileSync).toHaveBeenCalledWith(filePath, `${filePath}.corrupt`) + expect(fs.renameSync).not.toHaveBeenCalledWith(filePath, `${filePath}.corrupt`) + expect(store.getLoadError()).toBeNull() + }) + + it('does not back up a healthy file before overwriting it', () => { + savedContent = fileEnvelope(tokens) + const store = new XaiGrokCredentialStore(filePath) + + store.save({ ...tokens, accessToken: 'new-access-token' }) + + expect(fs.copyFileSync).not.toHaveBeenCalled() + expect(store.load()?.accessToken).toBe('new-access-token') + }) + + it('clear removes the credential file together with its backup and temp file', () => { + const store = new XaiGrokCredentialStore(filePath) + + store.clear() + + expect(fs.rmSync).toHaveBeenCalledWith(filePath, { force: true }) + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.corrupt`, { force: true }) + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp`, { force: true }) + }) +}) diff --git a/test/setup.ts b/test/setup.ts index 78cf0a1eb..611b30e08 100644 --- a/test/setup.ts +++ b/test/setup.ts @@ -276,6 +276,7 @@ vi.mock('fs', () => { unlinkSync: vi.fn(), readdirSync: vi.fn(), renameSync: vi.fn(), + copyFileSync: vi.fn(), constants: { F_OK: 0, X_OK: 1 From 042844357cd99a9ec4a04b5205ea3acfa68365bf Mon Sep 17 00:00:00 2001 From: xiao-test Date: Wed, 16 Sep 2026 11:50:09 +0800 Subject: [PATCH 10/19] test(auth): emulate fs rename and ENOENT in OAuth auth tests --- test/main/provider/auth/openaiCodex.test.ts | 24 +++++++++++++++++++-- test/main/provider/auth/xaiGrok.test.ts | 24 +++++++++++++++++++-- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/test/main/provider/auth/openaiCodex.test.ts b/test/main/provider/auth/openaiCodex.test.ts index abf32ae38..dd42f8152 100644 --- a/test/main/provider/auth/openaiCodex.test.ts +++ b/test/main/provider/auth/openaiCodex.test.ts @@ -30,7 +30,26 @@ describe('OpenAI Codex auth', () => { vi.mocked(fs.writeFileSync).mockImplementation((file, data) => { files.set(String(file), String(data)) }) - vi.mocked(fs.readFileSync).mockImplementation((file) => files.get(String(file)) || '') + vi.mocked(fs.readFileSync).mockImplementation((file) => { + const content = files.get(String(file)) + if (content === undefined) { + throw Object.assign(new Error('ENOENT: no such file or directory'), { code: 'ENOENT' }) + } + return content + }) + vi.mocked(fs.renameSync).mockImplementation((from, to) => { + const content = files.get(String(from)) + if (content !== undefined) { + files.delete(String(from)) + files.set(String(to), content) + } + }) + vi.mocked(fs.copyFileSync).mockImplementation((from, to) => { + const content = files.get(String(from)) + if (content !== undefined) { + files.set(String(to), content) + } + }) vi.mocked(fs.rmSync).mockImplementation((file) => { files.delete(String(file)) }) @@ -89,10 +108,11 @@ describe('OpenAI Codex auth', () => { recursive: true, mode: 0o700 }) - expect(fs.writeFileSync).toHaveBeenCalledWith(credentialPath, expect.any(String), { + expect(fs.writeFileSync).toHaveBeenCalledWith(`${credentialPath}.tmp`, expect.any(String), { encoding: 'utf-8', mode: 0o600 }) + expect(fs.renameSync).toHaveBeenCalledWith(`${credentialPath}.tmp`, credentialPath) expect(store.load()?.accessToken).toBe('access-token') store.clear() expect(store.load()).toBeNull() diff --git a/test/main/provider/auth/xaiGrok.test.ts b/test/main/provider/auth/xaiGrok.test.ts index a934c4f9a..ccf0aa03a 100644 --- a/test/main/provider/auth/xaiGrok.test.ts +++ b/test/main/provider/auth/xaiGrok.test.ts @@ -31,7 +31,26 @@ describe('xAI Grok OAuth', () => { vi.mocked(fs.writeFileSync).mockImplementation((file, data) => { files.set(String(file), String(data)) }) - vi.mocked(fs.readFileSync).mockImplementation((file) => files.get(String(file)) || '') + vi.mocked(fs.readFileSync).mockImplementation((file) => { + const content = files.get(String(file)) + if (content === undefined) { + throw Object.assign(new Error('ENOENT: no such file or directory'), { code: 'ENOENT' }) + } + return content + }) + vi.mocked(fs.renameSync).mockImplementation((from, to) => { + const content = files.get(String(from)) + if (content !== undefined) { + files.delete(String(from)) + files.set(String(to), content) + } + }) + vi.mocked(fs.copyFileSync).mockImplementation((from, to) => { + const content = files.get(String(from)) + if (content !== undefined) { + files.set(String(to), content) + } + }) vi.mocked(fs.rmSync).mockImplementation((file) => { files.delete(String(file)) }) @@ -123,10 +142,11 @@ describe('xAI Grok OAuth', () => { recursive: true, mode: 0o700 }) - expect(fs.writeFileSync).toHaveBeenCalledWith(credentialPath, expect.any(String), { + expect(fs.writeFileSync).toHaveBeenCalledWith(`${credentialPath}.tmp`, expect.any(String), { encoding: 'utf-8', mode: 0o600 }) + expect(fs.renameSync).toHaveBeenCalledWith(`${credentialPath}.tmp`, credentialPath) const loaded = store.load() expect(loaded?.accessToken).toBe('access-token') From f0f66a0dcb53d9edb78b1e214d7786c2d3a4f155 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Wed, 16 Sep 2026 11:56:37 +0800 Subject: [PATCH 11/19] fix(auth): prefer credential load error in status --- src/main/provider/auth/openaiCodex/index.ts | 2 +- src/main/provider/auth/xaiGrok/index.ts | 2 +- test/main/provider/auth/openaiCodex.test.ts | 14 +++++++++++ test/main/provider/auth/xaiGrok.test.ts | 28 +++++++++++++++++++++ 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/main/provider/auth/openaiCodex/index.ts b/src/main/provider/auth/openaiCodex/index.ts index 3193d817b..1d2b9f75e 100644 --- a/src/main/provider/auth/openaiCodex/index.ts +++ b/src/main/provider/auth/openaiCodex/index.ts @@ -176,7 +176,7 @@ export class OpenAICodexAuth { return this.statusFromTokens(tokens) } - const statusError = this.lastError ?? this.store.getLoadError() + const statusError = this.store.getLoadError() ?? this.lastError return this.withStorage({ state: statusError ? 'error' : 'signed-out', authenticated: false, diff --git a/src/main/provider/auth/xaiGrok/index.ts b/src/main/provider/auth/xaiGrok/index.ts index 69d3d3f48..6e1dda4fe 100644 --- a/src/main/provider/auth/xaiGrok/index.ts +++ b/src/main/provider/auth/xaiGrok/index.ts @@ -169,7 +169,7 @@ export class XaiGrokAuth { return this.statusFromTokens(tokens) } - const statusError = this.lastError ?? this.store.getLoadError() + const statusError = this.store.getLoadError() ?? this.lastError return this.withStorage({ state: statusError ? 'error' : 'signed-out', authenticated: false, diff --git a/test/main/provider/auth/openaiCodex.test.ts b/test/main/provider/auth/openaiCodex.test.ts index dd42f8152..cff153c18 100644 --- a/test/main/provider/auth/openaiCodex.test.ts +++ b/test/main/provider/auth/openaiCodex.test.ts @@ -269,4 +269,18 @@ describe('OpenAI Codex auth', () => { expect(auth.getStatus().state).toBe('disabled') await expect(auth.getAccessToken()).rejects.toThrow('disabled') }) + + it('prefers the credential-store load error over a stale login error', async () => { + const credentialPath = path.join(tempDir, 'credentials.json') + const store = new OpenAICodexCredentialStore(credentialPath) + const auth = new OpenAICodexAuth(store, vi.fn()) + + await auth.completeBrowserLoginFromCallbackUrl('https://example.com/callback') + + files.set(credentialPath, '{broken') + const status = auth.getStatus() + + expect(status.state).toBe('error') + expect(status.error).toContain('not valid JSON') + }) }) diff --git a/test/main/provider/auth/xaiGrok.test.ts b/test/main/provider/auth/xaiGrok.test.ts index ccf0aa03a..8fe9f05b1 100644 --- a/test/main/provider/auth/xaiGrok.test.ts +++ b/test/main/provider/auth/xaiGrok.test.ts @@ -290,4 +290,32 @@ describe('xAI Grok OAuth', () => { authenticated: false }) }) + + it('prefers the credential-store load error over a stale login error', async () => { + const credentialPath = path.join(tempDir, 'credentials.json') + const store = new XaiGrokCredentialStore(credentialPath) + store.save({ + accessToken: 'access-old', + refreshToken: 'refresh-1', + tokenType: 'Bearer', + expiresAt: Date.now() + 1000, + tokenEndpoint: 'https://auth.x.ai/oauth2/token', + updatedAt: Date.now() + }) + const auth = new XaiGrokAuth(store, vi.fn()) + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + throw new Error('network down') + }) + ) + + await expect(auth.ensureAccessToken()).rejects.toThrow('network down') + + files.set(credentialPath, '{broken') + const status = auth.getStatus() + + expect(status.state).toBe('error') + expect(status.error).toContain('not valid JSON') + }) }) From 590de99e027b823d5c336cd3c29d7e9f3703dbd9 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Wed, 16 Sep 2026 11:59:52 +0800 Subject: [PATCH 12/19] fix(auth): address credential store review findings --- .../provider/auth/openaiCodex/credentialStore.ts | 14 +++++++------- src/main/provider/auth/xaiGrok/credentialStore.ts | 14 +++++++------- .../provider/openaiCodexCredentialStore.test.ts | 15 +++++++++++++++ test/main/provider/xaiGrokCredentialStore.test.ts | 15 +++++++++++++++ 4 files changed, 44 insertions(+), 14 deletions(-) diff --git a/src/main/provider/auth/openaiCodex/credentialStore.ts b/src/main/provider/auth/openaiCodex/credentialStore.ts index 5900b258e..c1fdd4488 100644 --- a/src/main/provider/auth/openaiCodex/credentialStore.ts +++ b/src/main/provider/auth/openaiCodex/credentialStore.ts @@ -122,14 +122,14 @@ export class OpenAICodexCredentialStore { } clear(): void { - try { - fs.rmSync(this.filePath, { force: true }) - fs.rmSync(`${this.filePath}.corrupt`, { force: true }) - fs.rmSync(`${this.filePath}.tmp`, { force: true }) - this.lastLoadError = null - } catch (error) { - console.warn('[OpenAICodexCredentialStore] Failed to remove credential files:', error) + for (const artifact of [this.filePath, `${this.filePath}.corrupt`, `${this.filePath}.tmp`]) { + try { + fs.rmSync(artifact, { force: true }) + } catch (error) { + console.warn('[OpenAICodexCredentialStore] Failed to remove', artifact, error) + } } + this.lastLoadError = null } private readEnvelope(): EnvelopeReadResult { diff --git a/src/main/provider/auth/xaiGrok/credentialStore.ts b/src/main/provider/auth/xaiGrok/credentialStore.ts index e13de4a67..e5ce52d24 100644 --- a/src/main/provider/auth/xaiGrok/credentialStore.ts +++ b/src/main/provider/auth/xaiGrok/credentialStore.ts @@ -118,14 +118,14 @@ export class XaiGrokCredentialStore { } clear(): void { - try { - fs.rmSync(this.filePath, { force: true }) - fs.rmSync(`${this.filePath}.corrupt`, { force: true }) - fs.rmSync(`${this.filePath}.tmp`, { force: true }) - this.lastLoadError = null - } catch (error) { - console.warn('[XaiGrokCredentialStore] Failed to remove credential files:', error) + for (const artifact of [this.filePath, `${this.filePath}.corrupt`, `${this.filePath}.tmp`]) { + try { + fs.rmSync(artifact, { force: true }) + } catch (error) { + console.warn('[XaiGrokCredentialStore] Failed to remove', artifact, error) + } } + this.lastLoadError = null } private readEnvelope(): EnvelopeReadResult { diff --git a/test/main/provider/openaiCodexCredentialStore.test.ts b/test/main/provider/openaiCodexCredentialStore.test.ts index ec1ea3864..c89c43252 100644 --- a/test/main/provider/openaiCodexCredentialStore.test.ts +++ b/test/main/provider/openaiCodexCredentialStore.test.ts @@ -128,4 +128,19 @@ describe('OpenAICodexCredentialStore', () => { expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.corrupt`, { force: true }) expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp`, { force: true }) }) + + it('clear still attempts the backup and temp files when the main removal fails', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(fs.rmSync).mockImplementation((target) => { + if (String(target) === filePath) { + throw new Error('EISDIR: illegal operation on a directory') + } + }) + const store = new OpenAICodexCredentialStore(filePath) + + store.clear() + + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.corrupt`, { force: true }) + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp`, { force: true }) + }) }) diff --git a/test/main/provider/xaiGrokCredentialStore.test.ts b/test/main/provider/xaiGrokCredentialStore.test.ts index 2ff32c471..f49dbfb66 100644 --- a/test/main/provider/xaiGrokCredentialStore.test.ts +++ b/test/main/provider/xaiGrokCredentialStore.test.ts @@ -128,4 +128,19 @@ describe('XaiGrokCredentialStore', () => { expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.corrupt`, { force: true }) expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp`, { force: true }) }) + + it('clear still attempts the backup and temp files when the main removal fails', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(fs.rmSync).mockImplementation((target) => { + if (String(target) === filePath) { + throw new Error('EISDIR: illegal operation on a directory') + } + }) + const store = new XaiGrokCredentialStore(filePath) + + store.clear() + + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.corrupt`, { force: true }) + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp`, { force: true }) + }) }) From 075d82ad38f8987d9dfd40baa79108da965c3038 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Wed, 16 Sep 2026 13:29:16 +0800 Subject: [PATCH 13/19] fix(auth): address credential store review nits --- .../auth/openaiCodex/credentialStore.ts | 47 ++++++++++++++----- .../provider/auth/xaiGrok/credentialStore.ts | 44 ++++++++++++----- test/main/provider/auth/openaiCodex.test.ts | 37 ++++++++++++--- test/main/provider/auth/xaiGrok.test.ts | 37 ++++++++++++--- .../openaiCodexCredentialStore.test.ts | 47 ++++++++++++++++--- .../provider/xaiGrokCredentialStore.test.ts | 47 ++++++++++++++++--- test/setup.ts | 1 + 7 files changed, 214 insertions(+), 46 deletions(-) diff --git a/src/main/provider/auth/openaiCodex/credentialStore.ts b/src/main/provider/auth/openaiCodex/credentialStore.ts index c1fdd4488..831b94ea6 100644 --- a/src/main/provider/auth/openaiCodex/credentialStore.ts +++ b/src/main/provider/auth/openaiCodex/credentialStore.ts @@ -1,5 +1,6 @@ import * as fs from 'fs' import * as path from 'path' +import { randomBytes } from 'node:crypto' import { app, safeStorage } from 'electron' export type OpenAICodexCredentialStorage = 'safeStorage' | 'file' | 'none' @@ -33,6 +34,7 @@ type StoredCredentialEnvelope = type EnvelopeReadResult = | { state: 'missing' } | { state: 'ok'; tokens: OpenAICodexTokenSet } + | { state: 'undecryptable'; reason: string } | { state: 'corrupt'; reason: string } function toErrorMessage(error: unknown): string { @@ -66,10 +68,10 @@ export class OpenAICodexCredentialStore { load(): OpenAICodexTokenSet | null { const result = this.readEnvelope() - if (result.state === 'corrupt') { + if (result.state === 'corrupt' || result.state === 'undecryptable') { this.lastLoadError = result.reason console.warn( - `[OpenAICodexCredentialStore] Ignoring corrupted credential file: ${result.reason}` + `[OpenAICodexCredentialStore] Ignoring unreadable credential file: ${result.reason}` ) return null } @@ -112,12 +114,26 @@ export class OpenAICodexCredentialStore { } } - const temporaryPath = `${this.filePath}.tmp` - fs.writeFileSync(temporaryPath, JSON.stringify(envelope, null, 2), { - encoding: 'utf-8', - mode: 0o600 - }) - fs.renameSync(temporaryPath, this.filePath) + const temporaryPath = `${this.filePath}.tmp-${process.pid}-${randomBytes(6).toString('hex')}` + try { + const fd = fs.openSync(temporaryPath, 'w', 0o600) + try { + fs.writeFileSync(fd, JSON.stringify(envelope, null, 2), 'utf-8') + fs.fsyncSync(fd) + } finally { + fs.closeSync(fd) + } + fs.renameSync(temporaryPath, this.filePath) + } finally { + try { + fs.rmSync(temporaryPath, { force: true }) + } catch (error) { + console.warn( + '[OpenAICodexCredentialStore] Failed to remove temporary credential file:', + error + ) + } + } this.lastLoadError = null } @@ -161,14 +177,23 @@ export class OpenAICodexCredentialStore { : { state: 'corrupt', reason: 'credential file holds an invalid token payload' } } + let decrypted: string + try { + decrypted = safeStorage.decryptString(Buffer.from(envelope.wrapped, 'base64')) + } catch (error) { + return { + state: 'undecryptable', + reason: `credential decryption failed: ${toErrorMessage(error)}` + } + } + try { - const decrypted = safeStorage.decryptString(Buffer.from(envelope.wrapped, 'base64')) const tokens = this.normalizeTokens(JSON.parse(decrypted) as OpenAICodexTokenSet) return tokens ? { state: 'ok', tokens } : { state: 'corrupt', reason: 'credential file holds an invalid token payload' } - } catch (error) { - return { state: 'corrupt', reason: `credential decryption failed: ${toErrorMessage(error)}` } + } catch { + return { state: 'corrupt', reason: 'credential payload is not valid JSON' } } } diff --git a/src/main/provider/auth/xaiGrok/credentialStore.ts b/src/main/provider/auth/xaiGrok/credentialStore.ts index e5ce52d24..d88d2d9bc 100644 --- a/src/main/provider/auth/xaiGrok/credentialStore.ts +++ b/src/main/provider/auth/xaiGrok/credentialStore.ts @@ -1,5 +1,6 @@ import * as fs from 'fs' import * as path from 'path' +import { randomBytes } from 'node:crypto' import { app, safeStorage } from 'electron' export type XaiGrokCredentialStorage = 'safeStorage' | 'file' | 'none' @@ -34,6 +35,7 @@ type StoredCredentialEnvelope = type EnvelopeReadResult = | { state: 'missing' } | { state: 'ok'; tokens: XaiGrokTokenSet } + | { state: 'undecryptable'; reason: string } | { state: 'corrupt'; reason: string } function toErrorMessage(error: unknown): string { @@ -67,9 +69,9 @@ export class XaiGrokCredentialStore { load(): XaiGrokTokenSet | null { const result = this.readEnvelope() - if (result.state === 'corrupt') { + if (result.state === 'corrupt' || result.state === 'undecryptable') { this.lastLoadError = result.reason - console.warn(`[XaiGrokCredentialStore] Ignoring corrupted credential file: ${result.reason}`) + console.warn(`[XaiGrokCredentialStore] Ignoring unreadable credential file: ${result.reason}`) return null } @@ -108,12 +110,23 @@ export class XaiGrokCredentialStore { } } - const temporaryPath = `${this.filePath}.tmp` - fs.writeFileSync(temporaryPath, JSON.stringify(envelope, null, 2), { - encoding: 'utf-8', - mode: 0o600 - }) - fs.renameSync(temporaryPath, this.filePath) + const temporaryPath = `${this.filePath}.tmp-${process.pid}-${randomBytes(6).toString('hex')}` + try { + const fd = fs.openSync(temporaryPath, 'w', 0o600) + try { + fs.writeFileSync(fd, JSON.stringify(envelope, null, 2), 'utf-8') + fs.fsyncSync(fd) + } finally { + fs.closeSync(fd) + } + fs.renameSync(temporaryPath, this.filePath) + } finally { + try { + fs.rmSync(temporaryPath, { force: true }) + } catch (error) { + console.warn('[XaiGrokCredentialStore] Failed to remove temporary credential file:', error) + } + } this.lastLoadError = null } @@ -157,14 +170,23 @@ export class XaiGrokCredentialStore { : { state: 'corrupt', reason: 'credential file holds an invalid token payload' } } + let decrypted: string + try { + decrypted = safeStorage.decryptString(Buffer.from(envelope.wrapped, 'base64')) + } catch (error) { + return { + state: 'undecryptable', + reason: `credential decryption failed: ${toErrorMessage(error)}` + } + } + try { - const decrypted = safeStorage.decryptString(Buffer.from(envelope.wrapped, 'base64')) const tokens = this.normalizeTokens(JSON.parse(decrypted) as XaiGrokTokenSet) return tokens ? { state: 'ok', tokens } : { state: 'corrupt', reason: 'credential file holds an invalid token payload' } - } catch (error) { - return { state: 'corrupt', reason: `credential decryption failed: ${toErrorMessage(error)}` } + } catch { + return { state: 'corrupt', reason: 'credential payload is not valid JSON' } } } diff --git a/test/main/provider/auth/openaiCodex.test.ts b/test/main/provider/auth/openaiCodex.test.ts index cff153c18..27f3b0424 100644 --- a/test/main/provider/auth/openaiCodex.test.ts +++ b/test/main/provider/auth/openaiCodex.test.ts @@ -21,14 +21,27 @@ vi.mock('@/provider/auth/oauthLoopbackCallback', async (importOriginal) => { describe('OpenAI Codex auth', () => { let tempDir: string let files: Map + let fdPaths: Map beforeEach(() => { files = new Map() + fdPaths = new Map() + let nextFd = 1 tempDir = `/tmp/deepchat-codex-auth-${Date.now()}` vi.mocked(fs.existsSync).mockImplementation((file) => files.has(String(file))) vi.mocked(fs.mkdirSync).mockImplementation(() => undefined) + vi.mocked(fs.openSync).mockImplementation((file) => { + const fd = nextFd++ + fdPaths.set(fd, String(file)) + return fd + }) + vi.mocked(fs.closeSync).mockImplementation(() => {}) + vi.mocked(fs.fsyncSync).mockImplementation(() => {}) vi.mocked(fs.writeFileSync).mockImplementation((file, data) => { - files.set(String(file), String(data)) + const target = typeof file === 'number' ? fdPaths.get(file) : String(file) + if (target !== undefined) { + files.set(target, String(data)) + } }) vi.mocked(fs.readFileSync).mockImplementation((file) => { const content = files.get(String(file)) @@ -108,11 +121,11 @@ describe('OpenAI Codex auth', () => { recursive: true, mode: 0o700 }) - expect(fs.writeFileSync).toHaveBeenCalledWith(`${credentialPath}.tmp`, expect.any(String), { - encoding: 'utf-8', - mode: 0o600 - }) - expect(fs.renameSync).toHaveBeenCalledWith(`${credentialPath}.tmp`, credentialPath) + const temporaryPath = String(vi.mocked(fs.openSync).mock.calls[0][0]) + expect(temporaryPath).toMatch(/credentials\.json\.tmp-/) + expect(fs.openSync).toHaveBeenCalledWith(temporaryPath, 'w', 0o600) + expect(fs.fsyncSync).toHaveBeenCalled() + expect(fs.renameSync).toHaveBeenCalledWith(temporaryPath, credentialPath) expect(store.load()?.accessToken).toBe('access-token') store.clear() expect(store.load()).toBeNull() @@ -283,4 +296,16 @@ describe('OpenAI Codex auth', () => { expect(status.state).toBe('error') expect(status.error).toContain('not valid JSON') }) + + it('rejects backend auth requests with the credential-store load error', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const credentialPath = path.join(tempDir, 'credentials.json') + const store = new OpenAICodexCredentialStore(credentialPath) + const auth = new OpenAICodexAuth(store, vi.fn()) + + files.set(credentialPath, '{broken') + + await expect(auth.getBackendAuth()).rejects.toThrow(/not valid JSON/) + await expect(auth.forceRefreshBackendAuth()).rejects.toThrow(/not valid JSON/) + }) }) diff --git a/test/main/provider/auth/xaiGrok.test.ts b/test/main/provider/auth/xaiGrok.test.ts index 8fe9f05b1..5b88b1e07 100644 --- a/test/main/provider/auth/xaiGrok.test.ts +++ b/test/main/provider/auth/xaiGrok.test.ts @@ -22,14 +22,27 @@ function jsonResponse(value: unknown, init?: ResponseInit): Response { describe('xAI Grok OAuth', () => { let tempDir: string let files: Map + let fdPaths: Map beforeEach(() => { files = new Map() + fdPaths = new Map() + let nextFd = 1 tempDir = `/tmp/deepchat-xai-grok-auth-${Date.now()}` vi.mocked(fs.existsSync).mockImplementation((file) => files.has(String(file))) vi.mocked(fs.mkdirSync).mockImplementation(() => undefined) + vi.mocked(fs.openSync).mockImplementation((file) => { + const fd = nextFd++ + fdPaths.set(fd, String(file)) + return fd + }) + vi.mocked(fs.closeSync).mockImplementation(() => {}) + vi.mocked(fs.fsyncSync).mockImplementation(() => {}) vi.mocked(fs.writeFileSync).mockImplementation((file, data) => { - files.set(String(file), String(data)) + const target = typeof file === 'number' ? fdPaths.get(file) : String(file) + if (target !== undefined) { + files.set(target, String(data)) + } }) vi.mocked(fs.readFileSync).mockImplementation((file) => { const content = files.get(String(file)) @@ -142,11 +155,11 @@ describe('xAI Grok OAuth', () => { recursive: true, mode: 0o700 }) - expect(fs.writeFileSync).toHaveBeenCalledWith(`${credentialPath}.tmp`, expect.any(String), { - encoding: 'utf-8', - mode: 0o600 - }) - expect(fs.renameSync).toHaveBeenCalledWith(`${credentialPath}.tmp`, credentialPath) + const temporaryPath = String(vi.mocked(fs.openSync).mock.calls[0][0]) + expect(temporaryPath).toMatch(/credentials\.json\.tmp-/) + expect(fs.openSync).toHaveBeenCalledWith(temporaryPath, 'w', 0o600) + expect(fs.fsyncSync).toHaveBeenCalled() + expect(fs.renameSync).toHaveBeenCalledWith(temporaryPath, credentialPath) const loaded = store.load() expect(loaded?.accessToken).toBe('access-token') @@ -318,4 +331,16 @@ describe('xAI Grok OAuth', () => { expect(status.state).toBe('error') expect(status.error).toContain('not valid JSON') }) + + it('rejects access token requests with the credential-store load error', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const credentialPath = path.join(tempDir, 'credentials.json') + const store = new XaiGrokCredentialStore(credentialPath) + const auth = new XaiGrokAuth(store, vi.fn()) + + files.set(credentialPath, '{broken') + + await expect(auth.getAccessToken()).rejects.toThrow(/not valid JSON/) + await expect(auth.forceRefreshAccessToken()).rejects.toThrow(/not valid JSON/) + }) }) diff --git a/test/main/provider/openaiCodexCredentialStore.test.ts b/test/main/provider/openaiCodexCredentialStore.test.ts index c89c43252..77fffd7f0 100644 --- a/test/main/provider/openaiCodexCredentialStore.test.ts +++ b/test/main/provider/openaiCodexCredentialStore.test.ts @@ -88,27 +88,62 @@ describe('OpenAICodexCredentialStore', () => { store.save(tokens) - expect(fs.writeFileSync).toHaveBeenCalledWith( - `${filePath}.tmp`, - expect.any(String), - expect.objectContaining({ mode: 0o600 }) - ) - expect(fs.renameSync).toHaveBeenCalledWith(`${filePath}.tmp`, filePath) + const temporaryPath = String(vi.mocked(fs.openSync).mock.calls[0][0]) + expect(temporaryPath).toMatch(/^\/tmp\/deepchat-openai-codex\/credentials\.json\.tmp-/) + expect(fs.openSync).toHaveBeenCalledWith(temporaryPath, 'w', 0o600) + expect(fs.fsyncSync).toHaveBeenCalled() + expect(fs.renameSync).toHaveBeenCalledWith(temporaryPath, filePath) + expect(fs.rmSync).toHaveBeenCalledWith(temporaryPath, { force: true }) expect(store.load()?.accessToken).toBe('access-token') }) + it('removes the temporary file when the rename fails', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(fs.renameSync).mockImplementation(() => { + throw new Error('EPERM: operation not permitted') + }) + const store = new OpenAICodexCredentialStore(filePath) + + expect(() => store.save(tokens)).toThrow('EPERM') + const temporaryPath = String(vi.mocked(fs.openSync).mock.calls[0][0]) + expect(fs.rmSync).toHaveBeenCalledWith(temporaryPath, { force: true }) + }) + it('backs up a corrupted file before overwriting it', () => { vi.spyOn(console, 'warn').mockImplementation(() => {}) savedContent = '{broken' + let backupSnapshot: string | null = null + vi.mocked(fs.copyFileSync).mockImplementation(() => { + backupSnapshot = savedContent + }) const store = new OpenAICodexCredentialStore(filePath) store.save(tokens) expect(fs.copyFileSync).toHaveBeenCalledWith(filePath, `${filePath}.corrupt`) expect(fs.renameSync).not.toHaveBeenCalledWith(filePath, `${filePath}.corrupt`) + expect(backupSnapshot).toBe('{broken') expect(store.getLoadError()).toBeNull() }) + it('does not back up an undecryptable file before overwriting it', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(safeStorage.decryptString).mockImplementation(() => { + throw new Error('keyring locked') + }) + savedContent = JSON.stringify({ + version: 1, + storage: 'safeStorage', + wrapped: Buffer.from('ciphertext').toString('base64'), + updatedAt: 1 + }) + const store = new OpenAICodexCredentialStore(filePath) + + store.save(tokens) + + expect(fs.copyFileSync).not.toHaveBeenCalled() + }) + it('does not back up a healthy file before overwriting it', () => { savedContent = fileEnvelope(tokens) const store = new OpenAICodexCredentialStore(filePath) diff --git a/test/main/provider/xaiGrokCredentialStore.test.ts b/test/main/provider/xaiGrokCredentialStore.test.ts index f49dbfb66..147356505 100644 --- a/test/main/provider/xaiGrokCredentialStore.test.ts +++ b/test/main/provider/xaiGrokCredentialStore.test.ts @@ -88,27 +88,62 @@ describe('XaiGrokCredentialStore', () => { store.save(tokens) - expect(fs.writeFileSync).toHaveBeenCalledWith( - `${filePath}.tmp`, - expect.any(String), - expect.objectContaining({ mode: 0o600 }) - ) - expect(fs.renameSync).toHaveBeenCalledWith(`${filePath}.tmp`, filePath) + const temporaryPath = String(vi.mocked(fs.openSync).mock.calls[0][0]) + expect(temporaryPath).toMatch(/^\/tmp\/deepchat-xai-grok\/credentials\.json\.tmp-/) + expect(fs.openSync).toHaveBeenCalledWith(temporaryPath, 'w', 0o600) + expect(fs.fsyncSync).toHaveBeenCalled() + expect(fs.renameSync).toHaveBeenCalledWith(temporaryPath, filePath) + expect(fs.rmSync).toHaveBeenCalledWith(temporaryPath, { force: true }) expect(store.load()?.accessToken).toBe('access-token') }) + it('removes the temporary file when the rename fails', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(fs.renameSync).mockImplementation(() => { + throw new Error('EPERM: operation not permitted') + }) + const store = new XaiGrokCredentialStore(filePath) + + expect(() => store.save(tokens)).toThrow('EPERM') + const temporaryPath = String(vi.mocked(fs.openSync).mock.calls[0][0]) + expect(fs.rmSync).toHaveBeenCalledWith(temporaryPath, { force: true }) + }) + it('backs up a corrupted file before overwriting it', () => { vi.spyOn(console, 'warn').mockImplementation(() => {}) savedContent = '{broken' + let backupSnapshot: string | null = null + vi.mocked(fs.copyFileSync).mockImplementation(() => { + backupSnapshot = savedContent + }) const store = new XaiGrokCredentialStore(filePath) store.save(tokens) expect(fs.copyFileSync).toHaveBeenCalledWith(filePath, `${filePath}.corrupt`) expect(fs.renameSync).not.toHaveBeenCalledWith(filePath, `${filePath}.corrupt`) + expect(backupSnapshot).toBe('{broken') expect(store.getLoadError()).toBeNull() }) + it('does not back up an undecryptable file before overwriting it', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(safeStorage.decryptString).mockImplementation(() => { + throw new Error('keyring locked') + }) + savedContent = JSON.stringify({ + version: 1, + storage: 'safeStorage', + wrapped: Buffer.from('ciphertext').toString('base64'), + updatedAt: 1 + }) + const store = new XaiGrokCredentialStore(filePath) + + store.save(tokens) + + expect(fs.copyFileSync).not.toHaveBeenCalled() + }) + it('does not back up a healthy file before overwriting it', () => { savedContent = fileEnvelope(tokens) const store = new XaiGrokCredentialStore(filePath) diff --git a/test/setup.ts b/test/setup.ts index 611b30e08..cf8919866 100644 --- a/test/setup.ts +++ b/test/setup.ts @@ -272,6 +272,7 @@ vi.mock('fs', () => { openSync: vi.fn(), readSync: vi.fn(), closeSync: vi.fn(), + fsyncSync: vi.fn(), rmSync: vi.fn(), unlinkSync: vi.fn(), readdirSync: vi.fn(), From fd15074f449569aae48d52916d58c1bfd5523c66 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Wed, 16 Sep 2026 13:55:37 +0800 Subject: [PATCH 14/19] fix(auth): clear randomized credential temp files --- .../auth/openaiCodex/credentialStore.ts | 17 +++++++++++++- .../provider/auth/xaiGrok/credentialStore.ts | 17 +++++++++++++- test/main/provider/auth/openaiCodex.test.ts | 1 + test/main/provider/auth/xaiGrok.test.ts | 1 + .../openaiCodexCredentialStore.test.ts | 22 +++++++++++++++++++ .../provider/xaiGrokCredentialStore.test.ts | 22 +++++++++++++++++++ 6 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/main/provider/auth/openaiCodex/credentialStore.ts b/src/main/provider/auth/openaiCodex/credentialStore.ts index 831b94ea6..ded41e610 100644 --- a/src/main/provider/auth/openaiCodex/credentialStore.ts +++ b/src/main/provider/auth/openaiCodex/credentialStore.ts @@ -138,7 +138,22 @@ export class OpenAICodexCredentialStore { } clear(): void { - for (const artifact of [this.filePath, `${this.filePath}.corrupt`, `${this.filePath}.tmp`]) { + const directory = path.dirname(this.filePath) + const artifacts = [this.filePath, `${this.filePath}.corrupt`, `${this.filePath}.tmp`] + try { + const prefix = `${path.basename(this.filePath)}.tmp-` + for (const entry of fs.readdirSync(directory)) { + if (entry.startsWith(prefix)) { + artifacts.push(path.join(directory, entry)) + } + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + console.warn('[OpenAICodexCredentialStore] Failed to list credential directory:', error) + } + } + + for (const artifact of artifacts) { try { fs.rmSync(artifact, { force: true }) } catch (error) { diff --git a/src/main/provider/auth/xaiGrok/credentialStore.ts b/src/main/provider/auth/xaiGrok/credentialStore.ts index d88d2d9bc..f7676e2ec 100644 --- a/src/main/provider/auth/xaiGrok/credentialStore.ts +++ b/src/main/provider/auth/xaiGrok/credentialStore.ts @@ -131,7 +131,22 @@ export class XaiGrokCredentialStore { } clear(): void { - for (const artifact of [this.filePath, `${this.filePath}.corrupt`, `${this.filePath}.tmp`]) { + const directory = path.dirname(this.filePath) + const artifacts = [this.filePath, `${this.filePath}.corrupt`, `${this.filePath}.tmp`] + try { + const prefix = `${path.basename(this.filePath)}.tmp-` + for (const entry of fs.readdirSync(directory)) { + if (entry.startsWith(prefix)) { + artifacts.push(path.join(directory, entry)) + } + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + console.warn('[XaiGrokCredentialStore] Failed to list credential directory:', error) + } + } + + for (const artifact of artifacts) { try { fs.rmSync(artifact, { force: true }) } catch (error) { diff --git a/test/main/provider/auth/openaiCodex.test.ts b/test/main/provider/auth/openaiCodex.test.ts index 27f3b0424..51680ac74 100644 --- a/test/main/provider/auth/openaiCodex.test.ts +++ b/test/main/provider/auth/openaiCodex.test.ts @@ -66,6 +66,7 @@ describe('OpenAI Codex auth', () => { vi.mocked(fs.rmSync).mockImplementation((file) => { files.delete(String(file)) }) + vi.mocked(fs.readdirSync).mockImplementation(() => []) startOAuthLoopbackCallbackSessionMock.mockReset() vi.mocked(shell.openExternal).mockClear() delete process.env.DEEPCHAT_OPENAI_CODEX_DISABLED diff --git a/test/main/provider/auth/xaiGrok.test.ts b/test/main/provider/auth/xaiGrok.test.ts index 5b88b1e07..bcf080b1f 100644 --- a/test/main/provider/auth/xaiGrok.test.ts +++ b/test/main/provider/auth/xaiGrok.test.ts @@ -67,6 +67,7 @@ describe('xAI Grok OAuth', () => { vi.mocked(fs.rmSync).mockImplementation((file) => { files.delete(String(file)) }) + vi.mocked(fs.readdirSync).mockImplementation(() => []) vi.mocked(shell.openExternal).mockClear() delete process.env.DEEPCHAT_XAI_GROK_OAUTH_DISABLED delete process.env.XAI_GROK_ACCESS_TOKEN diff --git a/test/main/provider/openaiCodexCredentialStore.test.ts b/test/main/provider/openaiCodexCredentialStore.test.ts index 77fffd7f0..b71f867d3 100644 --- a/test/main/provider/openaiCodexCredentialStore.test.ts +++ b/test/main/provider/openaiCodexCredentialStore.test.ts @@ -36,6 +36,7 @@ describe('OpenAICodexCredentialStore', () => { }) vi.mocked(fs.mkdirSync).mockImplementation(() => undefined as unknown as string) vi.mocked(fs.renameSync).mockImplementation(() => {}) + vi.mocked(fs.readdirSync).mockImplementation(() => []) vi.mocked(safeStorage.isEncryptionAvailable).mockReturnValue(false) }) @@ -178,4 +179,25 @@ describe('OpenAICodexCredentialStore', () => { expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.corrupt`, { force: true }) expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp`, { force: true }) }) + + it('clear also removes randomized temporary credential files', () => { + vi.mocked(fs.readdirSync).mockImplementation( + () => + [ + 'credentials.json.tmp-1234-aabbccddeeff', + 'credentials.json.tmp-5678-001122334455', + 'unrelated.txt' + ] as unknown as fs.Dirent>[] + ) + const store = new OpenAICodexCredentialStore(filePath) + + store.clear() + + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp-1234-aabbccddeeff`, { force: true }) + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp-5678-001122334455`, { force: true }) + expect(fs.rmSync).not.toHaveBeenCalledWith( + expect.stringContaining('unrelated'), + expect.anything() + ) + }) }) diff --git a/test/main/provider/xaiGrokCredentialStore.test.ts b/test/main/provider/xaiGrokCredentialStore.test.ts index 147356505..a0b96b6e1 100644 --- a/test/main/provider/xaiGrokCredentialStore.test.ts +++ b/test/main/provider/xaiGrokCredentialStore.test.ts @@ -36,6 +36,7 @@ describe('XaiGrokCredentialStore', () => { }) vi.mocked(fs.mkdirSync).mockImplementation(() => undefined as unknown as string) vi.mocked(fs.renameSync).mockImplementation(() => {}) + vi.mocked(fs.readdirSync).mockImplementation(() => []) vi.mocked(safeStorage.isEncryptionAvailable).mockReturnValue(false) }) @@ -178,4 +179,25 @@ describe('XaiGrokCredentialStore', () => { expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.corrupt`, { force: true }) expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp`, { force: true }) }) + + it('clear also removes randomized temporary credential files', () => { + vi.mocked(fs.readdirSync).mockImplementation( + () => + [ + 'credentials.json.tmp-1234-aabbccddeeff', + 'credentials.json.tmp-5678-001122334455', + 'unrelated.txt' + ] as unknown as fs.Dirent>[] + ) + const store = new XaiGrokCredentialStore(filePath) + + store.clear() + + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp-1234-aabbccddeeff`, { force: true }) + expect(fs.rmSync).toHaveBeenCalledWith(`${filePath}.tmp-5678-001122334455`, { force: true }) + expect(fs.rmSync).not.toHaveBeenCalledWith( + expect.stringContaining('unrelated'), + expect.anything() + ) + }) }) From 74801f43b8cb5d82ff54e06c76025624112dc878 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Sat, 19 Sep 2026 10:20:21 +0800 Subject: [PATCH 15/19] fix(plugin): bind settings window to own plugin Plugin settings windows load plugin-bundled HTML with sandbox disabled, but the preload derived pluginId from location.search, which the page can rewrite via history.replaceState without a reload. Since the enable/disable/invokeAction routes trusted the caller-supplied pluginId, one plugin's settings page could control any other plugin. Deliver pluginId through webPreferences.additionalArguments so the page cannot tamper with it, record a webContentsId -> pluginId mapping when the window is created, and reject enable/disable/invokeAction calls from a plugin settings window that targets a different plugin. Other callers (main window, settings UI, CLI) stay unrestricted. --- src/main/app/composition.ts | 2 +- src/main/desktop/pluginSettingsWindow.ts | 11 ++- src/main/plugin/index.ts | 1 + src/main/plugin/routes.ts | 30 ++++++-- src/preload/plugin-settings-preload.ts | 7 +- test/main/plugin/pluginRoutes.test.ts | 89 ++++++++++++++++++++++++ test/main/routes/dispatcher.test.ts | 7 +- 7 files changed, 137 insertions(+), 10 deletions(-) create mode 100644 test/main/plugin/pluginRoutes.test.ts diff --git a/src/main/app/composition.ts b/src/main/app/composition.ts index db6cb5534..626a4ce98 100644 --- a/src/main/app/composition.ts +++ b/src/main/app/composition.ts @@ -2796,7 +2796,7 @@ export async function createMainProcessControl(dependencies: { recordSettingsActivity: (input) => settingsDatabase.recordSettingsActivity(input) }) const toolRoutes = createToolRoutes(toolService) - const pluginRoutes = createPluginRoutes(pluginService) + const pluginRoutes = createPluginRoutes(pluginService, pluginSettingsWindow) const skillRoutes = createSkillRoutes({ skillService, skillSyncService, diff --git a/src/main/desktop/pluginSettingsWindow.ts b/src/main/desktop/pluginSettingsWindow.ts index 7e6b091a4..ef5dce6fd 100644 --- a/src/main/desktop/pluginSettingsWindow.ts +++ b/src/main/desktop/pluginSettingsWindow.ts @@ -4,6 +4,7 @@ import type { PluginSettingsWindowPort } from '@/plugin' export class PluginSettingsWindow implements PluginSettingsWindowPort { private readonly windows = new Map() + private readonly pluginIdByWebContentsId = new Map() async open(input: { pluginId: string; title: string; entry: string }): Promise { const existing = this.windows.get(input.pluginId) @@ -23,11 +24,14 @@ export class PluginSettingsWindow implements PluginSettingsWindowPort { nodeIntegration: false, contextIsolation: true, preload: path.join(__dirname, '../preload/pluginSettings.mjs'), - sandbox: false + sandbox: false, + additionalArguments: [`--deepchat-plugin-id=${encodeURIComponent(input.pluginId)}`] } }) + const webContentsId = settingsWindow.webContents.id this.windows.set(input.pluginId, settingsWindow) + this.pluginIdByWebContentsId.set(webContentsId, input.pluginId) settingsWindow.webContents.setWindowOpenHandler(() => ({ action: 'deny' })) settingsWindow.on('ready-to-show', () => { if (!settingsWindow.isDestroyed()) { @@ -35,6 +39,7 @@ export class PluginSettingsWindow implements PluginSettingsWindowPort { } }) settingsWindow.on('closed', () => { + this.pluginIdByWebContentsId.delete(webContentsId) this.windows.delete(input.pluginId) }) @@ -45,6 +50,10 @@ export class PluginSettingsWindow implements PluginSettingsWindowPort { }) } + getPluginIdForWebContents(webContentsId: number): string | null { + return this.pluginIdByWebContentsId.get(webContentsId) ?? null + } + close(pluginId: string): void { const settingsWindow = this.windows.get(pluginId) if (settingsWindow && !settingsWindow.isDestroyed()) { diff --git a/src/main/plugin/index.ts b/src/main/plugin/index.ts index e74d9ffac..60e9dea61 100644 --- a/src/main/plugin/index.ts +++ b/src/main/plugin/index.ts @@ -80,6 +80,7 @@ export interface PluginSettingsWindowPort { open(input: { pluginId: string; title: string; entry: string }): Promise close(pluginId: string): void closeAll(): void + getPluginIdForWebContents(webContentsId: number): string | null } type PluginServiceDeps = { diff --git a/src/main/plugin/routes.ts b/src/main/plugin/routes.ts index 287fee2df..237ffdc84 100644 --- a/src/main/plugin/routes.ts +++ b/src/main/plugin/routes.ts @@ -11,10 +11,25 @@ import { pluginsInvokeActionRoute, pluginsListRoute } from '@shared/contracts/routes' -import { createRouteMap, type DeepchatRouteMap } from '@/routes/routeRegistry' -import type { PluginServicePort } from './index' +import { createRouteMap, type DeepchatRouteMap, type RouteContext } from '@/routes/routeRegistry' +import type { PluginServicePort, PluginSettingsWindowPort } from './index' + +export function createPluginRoutes( + pluginService: PluginServicePort, + settingsWindow: PluginSettingsWindowPort +): DeepchatRouteMap { + const assertPluginSettingsCallerOwns = (context: RouteContext, pluginId: string): void => { + if (context.caller.kind !== 'renderer') { + return + } + const ownerPluginId = settingsWindow.getPluginIdForWebContents(context.caller.webContentsId) + if (ownerPluginId != null && ownerPluginId !== pluginId) { + throw new Error( + `Plugin settings window for "${ownerPluginId}" cannot control plugin "${pluginId}"` + ) + } + } -export function createPluginRoutes(pluginService: PluginServicePort): DeepchatRouteMap { return createRouteMap([ [ pluginsInspectSourceRoute.name, @@ -87,8 +102,9 @@ export function createPluginRoutes(pluginService: PluginServicePort): DeepchatRo ], [ pluginsEnableRoute.name, - async (rawInput) => { + async (rawInput, context) => { const input = pluginsEnableRoute.input.parse(rawInput) + assertPluginSettingsCallerOwns(context, input.pluginId) return pluginsEnableRoute.output.parse({ result: await pluginService.enablePlugin(input.pluginId) }) @@ -96,8 +112,9 @@ export function createPluginRoutes(pluginService: PluginServicePort): DeepchatRo ], [ pluginsDisableRoute.name, - async (rawInput) => { + async (rawInput, context) => { const input = pluginsDisableRoute.input.parse(rawInput) + assertPluginSettingsCallerOwns(context, input.pluginId) return pluginsDisableRoute.output.parse({ result: await pluginService.disablePlugin(input.pluginId) }) @@ -105,8 +122,9 @@ export function createPluginRoutes(pluginService: PluginServicePort): DeepchatRo ], [ pluginsInvokeActionRoute.name, - async (rawInput) => { + async (rawInput, context) => { const input = pluginsInvokeActionRoute.input.parse(rawInput) + assertPluginSettingsCallerOwns(context, input.pluginId) return pluginsInvokeActionRoute.output.parse({ result: await pluginService.invokeAction(input.pluginId, input.actionId, input.payload) }) diff --git a/src/preload/plugin-settings-preload.ts b/src/preload/plugin-settings-preload.ts index f7bbd3046..cd4d171c1 100644 --- a/src/preload/plugin-settings-preload.ts +++ b/src/preload/plugin-settings-preload.ts @@ -9,8 +9,13 @@ import { } from '@shared/contracts/routes' import type { PluginSettingsApiStatus } from '@shared/types/plugin' +const PLUGIN_ID_ARG_PREFIX = '--deepchat-plugin-id=' + function readPluginId(): string { - const pluginId = new URL(window.location.href).searchParams.get('pluginId')?.trim() + const arg = process.argv.find((value) => value.startsWith(PLUGIN_ID_ARG_PREFIX)) + const pluginId = arg + ? decodeURIComponent(arg.slice(PLUGIN_ID_ARG_PREFIX.length)).trim() + : undefined if (!pluginId) { throw new Error('Plugin settings renderer is missing pluginId') } diff --git a/test/main/plugin/pluginRoutes.test.ts b/test/main/plugin/pluginRoutes.test.ts new file mode 100644 index 000000000..617cdcc46 --- /dev/null +++ b/test/main/plugin/pluginRoutes.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it, vi } from 'vitest' +import { + pluginsDisableRoute, + pluginsEnableRoute, + pluginsInvokeActionRoute +} from '@shared/contracts/routes' +import type { PluginServicePort, PluginSettingsWindowPort } from '@/plugin' +import { createPluginRoutes } from '@/plugin/routes' +import { createRendererRouteContext, type RouteContext } from '@/routes/routeRegistry' + +const actionResult = { ok: true } + +function setup(ownerPluginId: string | null) { + const pluginService = { + enablePlugin: vi.fn().mockResolvedValue(actionResult), + disablePlugin: vi.fn().mockResolvedValue(actionResult), + invokeAction: vi.fn().mockResolvedValue(actionResult) + } + const settingsWindow: PluginSettingsWindowPort = { + open: async () => {}, + close: () => {}, + closeAll: () => {}, + getPluginIdForWebContents: () => ownerPluginId + } + const routes = createPluginRoutes(pluginService as unknown as PluginServicePort, settingsWindow) + return { pluginService, routes } +} + +const pluginWindowContext = (): RouteContext => createRendererRouteContext(42, 7) + +describe('createPluginRoutes settings-window ownership', () => { + it('rejects disable for another plugin from a plugin settings window', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsDisableRoute.name) + + await expect(handler?.({ pluginId: 'plugin-b' }, pluginWindowContext())).rejects.toThrow( + /cannot control plugin/ + ) + expect(pluginService.disablePlugin).not.toHaveBeenCalled() + }) + + it('rejects enable for another plugin from a plugin settings window', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsEnableRoute.name) + + await expect(handler?.({ pluginId: 'plugin-b' }, pluginWindowContext())).rejects.toThrow( + /cannot control plugin/ + ) + expect(pluginService.enablePlugin).not.toHaveBeenCalled() + }) + + it('rejects invokeAction for another plugin from a plugin settings window', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsInvokeActionRoute.name) + + await expect( + handler?.({ pluginId: 'plugin-b', actionId: 'act' }, pluginWindowContext()) + ).rejects.toThrow(/cannot control plugin/) + expect(pluginService.invokeAction).not.toHaveBeenCalled() + }) + + it('allows a plugin settings window to control its own plugin', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsDisableRoute.name) + + await handler?.({ pluginId: 'plugin-a' }, pluginWindowContext()) + + expect(pluginService.disablePlugin).toHaveBeenCalledWith('plugin-a') + }) + + it('allows renderer callers that are not plugin settings windows', async () => { + const { pluginService, routes } = setup(null) + const handler = routes.get(pluginsDisableRoute.name) + + await handler?.({ pluginId: 'plugin-b' }, pluginWindowContext()) + + expect(pluginService.disablePlugin).toHaveBeenCalledWith('plugin-b') + }) + + it('allows non-renderer callers', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsEnableRoute.name) + const context: RouteContext = { caller: { kind: 'internal', component: 'scheduler' } } + + await handler?.({ pluginId: 'plugin-b' }, context) + + expect(pluginService.enablePlugin).toHaveBeenCalledWith('plugin-b') + }) +}) diff --git a/test/main/routes/dispatcher.test.ts b/test/main/routes/dispatcher.test.ts index 9fce8acc3..8166ef495 100644 --- a/test/main/routes/dispatcher.test.ts +++ b/test/main/routes/dispatcher.test.ts @@ -1616,7 +1616,12 @@ function createRuntime() { recordSettingsActivity: (input) => sqlitePresenter.recordSettingsActivity(input) }) const toolRoutes = createToolRoutes(toolService) - const pluginRoutes = createPluginRoutes(pluginService) + const pluginRoutes = createPluginRoutes(pluginService, { + open: async () => {}, + close: () => {}, + closeAll: () => {}, + getPluginIdForWebContents: () => null + }) const assertSessionActiveSkillsMutable = vi.fn().mockResolvedValue(undefined) const skillRoutes = createSkillRoutes({ skillService, From e53affd0e15ec95268680317fb9f637fc2f56e06 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Sat, 19 Sep 2026 10:34:22 +0800 Subject: [PATCH 16/19] test(renderer): fix plugin preload boundary test --- test/renderer/api/preloadBoundaries.test.ts | 89 ++++++++++++--------- 1 file changed, 49 insertions(+), 40 deletions(-) diff --git a/test/renderer/api/preloadBoundaries.test.ts b/test/renderer/api/preloadBoundaries.test.ts index 5795e2694..511bfa94f 100644 --- a/test/renderer/api/preloadBoundaries.test.ts +++ b/test/renderer/api/preloadBoundaries.test.ts @@ -229,49 +229,58 @@ describe('preload IPC boundaries', () => { it('backs plugin settings preload APIs with typed route bridge calls', async () => { const { ipcRenderer } = installElectronPreloadMock() - window.history.pushState({}, '', '/plugin-settings/?pluginId=plugin-1') - - await import('../../../src/preload/plugin-settings-preload') - - const deepchatPlugin = ( - window as Window & { - deepchatPlugin: { - getPluginId: () => string - getStatus: () => Promise<{ pluginId: string; enabled: boolean }> - enable: () => Promise - invokeAction: (actionId: string, payload?: Record) => Promise + const originalArgv = process.argv + process.argv = [...originalArgv, '--deepchat-plugin-id=plugin-1'] + + try { + await import('../../../src/preload/plugin-settings-preload') + + const deepchatPlugin = ( + window as Window & { + deepchatPlugin: { + getPluginId: () => string + getStatus: () => Promise<{ pluginId: string; enabled: boolean }> + enable: () => Promise + invokeAction: (actionId: string, payload?: Record) => Promise + } } - } - ).deepchatPlugin - - await expect(deepchatPlugin.getStatus()).resolves.toMatchObject({ - pluginId: 'plugin-1', - enabled: true - }) - - await deepchatPlugin.enable() - await deepchatPlugin.invokeAction('refresh', { force: true }) + ).deepchatPlugin - expect(deepchatPlugin.getPluginId()).toBe('plugin-1') - expect(ipcRenderer.invoke).toHaveBeenCalledWith(DEEPCHAT_ROUTE_INVOKE_CHANNEL, 'plugins.get', { - pluginId: 'plugin-1' - }) - expect(ipcRenderer.invoke).toHaveBeenCalledWith( - DEEPCHAT_ROUTE_INVOKE_CHANNEL, - 'plugins.enable', - { - pluginId: 'plugin-1' - } - ) - expect(ipcRenderer.invoke).toHaveBeenCalledWith( - DEEPCHAT_ROUTE_INVOKE_CHANNEL, - 'plugins.invokeAction', - { + await expect(deepchatPlugin.getStatus()).resolves.toMatchObject({ pluginId: 'plugin-1', - actionId: 'refresh', - payload: { force: true } - } - ) + enabled: true + }) + + await deepchatPlugin.enable() + await deepchatPlugin.invokeAction('refresh', { force: true }) + + expect(deepchatPlugin.getPluginId()).toBe('plugin-1') + expect(ipcRenderer.invoke).toHaveBeenCalledWith( + DEEPCHAT_ROUTE_INVOKE_CHANNEL, + 'plugins.get', + { + pluginId: 'plugin-1' + } + ) + expect(ipcRenderer.invoke).toHaveBeenCalledWith( + DEEPCHAT_ROUTE_INVOKE_CHANNEL, + 'plugins.enable', + { + pluginId: 'plugin-1' + } + ) + expect(ipcRenderer.invoke).toHaveBeenCalledWith( + DEEPCHAT_ROUTE_INVOKE_CHANNEL, + 'plugins.invokeAction', + { + pluginId: 'plugin-1', + actionId: 'refresh', + payload: { force: true } + } + ) + } finally { + process.argv = originalArgv + } }) it('replays a debug mode received before the splash renderer subscribes', async () => { From e66faa459227d8ad20303fc4ad0afa5027d4cdd9 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Sat, 19 Sep 2026 11:24:25 +0800 Subject: [PATCH 17/19] fix(plugin): harden plugin settings window --- src/main/desktop/pluginSettingsWindow.ts | 12 ++- src/main/plugin/routes.ts | 3 +- .../main/desktop/pluginSettingsWindow.test.ts | 87 +++++++++++++++++++ test/main/plugin/pluginRoutes.test.ts | 14 ++- 4 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 test/main/desktop/pluginSettingsWindow.test.ts diff --git a/src/main/desktop/pluginSettingsWindow.ts b/src/main/desktop/pluginSettingsWindow.ts index ef5dce6fd..fd2c045d6 100644 --- a/src/main/desktop/pluginSettingsWindow.ts +++ b/src/main/desktop/pluginSettingsWindow.ts @@ -1,5 +1,6 @@ import { BrowserWindow } from 'electron' import path from 'node:path' +import { pathToFileURL } from 'node:url' import type { PluginSettingsWindowPort } from '@/plugin' export class PluginSettingsWindow implements PluginSettingsWindowPort { @@ -30,9 +31,16 @@ export class PluginSettingsWindow implements PluginSettingsWindowPort { }) const webContentsId = settingsWindow.webContents.id + const entryPath = pathToFileURL(input.entry).pathname this.windows.set(input.pluginId, settingsWindow) this.pluginIdByWebContentsId.set(webContentsId, input.pluginId) settingsWindow.webContents.setWindowOpenHandler(() => ({ action: 'deny' })) + settingsWindow.webContents.on('will-navigate', (event, url) => { + const target = new URL(url) + if (target.protocol !== 'file:' || target.pathname !== entryPath) { + event.preventDefault() + } + }) settingsWindow.on('ready-to-show', () => { if (!settingsWindow.isDestroyed()) { settingsWindow.show() @@ -40,7 +48,9 @@ export class PluginSettingsWindow implements PluginSettingsWindowPort { }) settingsWindow.on('closed', () => { this.pluginIdByWebContentsId.delete(webContentsId) - this.windows.delete(input.pluginId) + if (this.windows.get(input.pluginId) === settingsWindow) { + this.windows.delete(input.pluginId) + } }) await settingsWindow.loadFile(input.entry, { diff --git a/src/main/plugin/routes.ts b/src/main/plugin/routes.ts index 237ffdc84..d1adad768 100644 --- a/src/main/plugin/routes.ts +++ b/src/main/plugin/routes.ts @@ -93,8 +93,9 @@ export function createPluginRoutes( ], [ pluginsGetRoute.name, - async (rawInput) => { + async (rawInput, context) => { const input = pluginsGetRoute.input.parse(rawInput) + assertPluginSettingsCallerOwns(context, input.pluginId) return pluginsGetRoute.output.parse({ plugin: await pluginService.getPlugin(input.pluginId) }) diff --git a/test/main/desktop/pluginSettingsWindow.test.ts b/test/main/desktop/pluginSettingsWindow.test.ts new file mode 100644 index 000000000..0e413d70c --- /dev/null +++ b/test/main/desktop/pluginSettingsWindow.test.ts @@ -0,0 +1,87 @@ +import { describe, expect, it, vi } from 'vitest' +import { BrowserWindow } from 'electron' +import { PluginSettingsWindow } from '@/desktop/pluginSettingsWindow' + +type Handler = (...args: any[]) => void + +type FakeWindow = { + webContents: { + id: number + on: ReturnType + setWindowOpenHandler: ReturnType + } + isDestroyed: ReturnType + on: ReturnType + show: ReturnType + focus: ReturnType + close: ReturnType + loadFile: ReturnType + handlers: Map + webContentsHandlers: Map +} + +function installBrowserWindowMock(): FakeWindow[] { + const created: FakeWindow[] = [] + ;(BrowserWindow as unknown as ReturnType).mockImplementation(() => { + const handlers = new Map() + const webContentsHandlers = new Map() + const win: FakeWindow = { + webContents: { + id: 100 + created.length, + on: vi.fn((event: string, cb: Handler) => webContentsHandlers.set(event, cb)), + setWindowOpenHandler: vi.fn() + }, + isDestroyed: vi.fn(() => false), + on: vi.fn((event: string, cb: Handler) => handlers.set(event, cb)), + show: vi.fn(), + focus: vi.fn(), + close: vi.fn(), + loadFile: vi.fn().mockResolvedValue(undefined), + handlers, + webContentsHandlers + } + created.push(win) + return win + }) + return created +} + +const input = { pluginId: 'p1', title: 'P1', entry: '/plugins/p1/settings.html' } + +describe('PluginSettingsWindow', () => { + it('keeps the reopened window record when a stale closed event fires late', async () => { + const created = installBrowserWindowMock() + const settingsWindow = new PluginSettingsWindow() + + await settingsWindow.open(input) + settingsWindow.close(input.pluginId) + await settingsWindow.open(input) + + created[0].handlers.get('closed')?.() + + settingsWindow.close(input.pluginId) + expect(created[1].close).toHaveBeenCalled() + expect(settingsWindow.getPluginIdForWebContents(created[1].webContents.id)).toBe('p1') + }) + + it('restricts navigation to the file entry', async () => { + const created = installBrowserWindowMock() + const settingsWindow = new PluginSettingsWindow() + + await settingsWindow.open(input) + + const onWillNavigate = created[0].webContentsHandlers.get('will-navigate') + expect(onWillNavigate).toBeDefined() + + const isAllowed = (url: string): boolean => { + const event = { preventDefault: vi.fn() } + onWillNavigate?.(event, url) + return event.preventDefault.mock.calls.length === 0 + } + + expect(isAllowed('file:///plugins/p1/settings.html?pluginId=p1')).toBe(true) + expect(isAllowed('file:///plugins/p1/settings.html#section')).toBe(true) + expect(isAllowed('file:///plugins/other/settings.html')).toBe(false) + expect(isAllowed('https://example.com/')).toBe(false) + }) +}) diff --git a/test/main/plugin/pluginRoutes.test.ts b/test/main/plugin/pluginRoutes.test.ts index 617cdcc46..a6e481ad0 100644 --- a/test/main/plugin/pluginRoutes.test.ts +++ b/test/main/plugin/pluginRoutes.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import { pluginsDisableRoute, pluginsEnableRoute, + pluginsGetRoute, pluginsInvokeActionRoute } from '@shared/contracts/routes' import type { PluginServicePort, PluginSettingsWindowPort } from '@/plugin' @@ -14,7 +15,8 @@ function setup(ownerPluginId: string | null) { const pluginService = { enablePlugin: vi.fn().mockResolvedValue(actionResult), disablePlugin: vi.fn().mockResolvedValue(actionResult), - invokeAction: vi.fn().mockResolvedValue(actionResult) + invokeAction: vi.fn().mockResolvedValue(actionResult), + getPlugin: vi.fn().mockResolvedValue({ id: 'plugin-a' }) } const settingsWindow: PluginSettingsWindowPort = { open: async () => {}, @@ -59,6 +61,16 @@ describe('createPluginRoutes settings-window ownership', () => { expect(pluginService.invokeAction).not.toHaveBeenCalled() }) + it('rejects get for another plugin from a plugin settings window', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsGetRoute.name) + + await expect(handler?.({ pluginId: 'plugin-b' }, pluginWindowContext())).rejects.toThrow( + /cannot control plugin/ + ) + expect(pluginService.getPlugin).not.toHaveBeenCalled() + }) + it('allows a plugin settings window to control its own plugin', async () => { const { pluginService, routes } = setup('plugin-a') const handler = routes.get(pluginsDisableRoute.name) From a33e1afe28161e122281a8188bf31ada4658edb7 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Sat, 19 Sep 2026 11:24:25 +0800 Subject: [PATCH 18/19] fix(auth): deny popups in oauth window --- src/main/provider/auth/index.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/provider/auth/index.ts b/src/main/provider/auth/index.ts index 52cf9f91b..c87db9099 100644 --- a/src/main/provider/auth/index.ts +++ b/src/main/provider/auth/index.ts @@ -397,6 +397,8 @@ export class OAuthService implements OAuthServicePort { const authUrl = this.buildAuthUrl(config) logger.info('Opening OAuth URL:', authUrl) + this.authWindow.webContents.setWindowOpenHandler(() => ({ action: 'deny' })) + // Load authorization page this.authWindow.loadURL(authUrl) this.authWindow.show() From d2cde3efc80c3fe29f20d37c2754bddf34ead0d3 Mon Sep 17 00:00:00 2001 From: xiao-test Date: Sat, 19 Sep 2026 12:01:49 +0800 Subject: [PATCH 19/19] test(desktop): constructible BrowserWindow mock --- test/main/desktop/pluginSettingsWindow.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/main/desktop/pluginSettingsWindow.test.ts b/test/main/desktop/pluginSettingsWindow.test.ts index 0e413d70c..ba9f980d3 100644 --- a/test/main/desktop/pluginSettingsWindow.test.ts +++ b/test/main/desktop/pluginSettingsWindow.test.ts @@ -22,7 +22,7 @@ type FakeWindow = { function installBrowserWindowMock(): FakeWindow[] { const created: FakeWindow[] = [] - ;(BrowserWindow as unknown as ReturnType).mockImplementation(() => { + vi.mocked(BrowserWindow).mockImplementation(function () { const handlers = new Map() const webContentsHandlers = new Map() const win: FakeWindow = { @@ -41,7 +41,7 @@ function installBrowserWindowMock(): FakeWindow[] { webContentsHandlers } created.push(win) - return win + return win as any }) return created }