From d543c8339acd51a4cf8ade23e896249d3321fc1f Mon Sep 17 00:00:00 2001 From: Marat Boshernitsan Date: Tue, 19 Aug 2025 13:16:06 -0700 Subject: [PATCH] fix(core): harden user account caching (#6501) --- packages/core/src/utils/user_account.test.ts | 120 +++++++++++++++++-- packages/core/src/utils/user_account.ts | 116 ++++++++++-------- 2 files changed, 177 insertions(+), 59 deletions(-) diff --git a/packages/core/src/utils/user_account.test.ts b/packages/core/src/utils/user_account.test.ts index a0c861cc..35231ca3 100644 --- a/packages/core/src/utils/user_account.test.ts +++ b/packages/core/src/utils/user_account.test.ts @@ -99,18 +99,37 @@ describe('user_account', () => { it('should handle corrupted JSON by starting fresh', async () => { fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); fs.writeFileSync(accountsFile(), 'not valid json'); - const consoleDebugSpy = vi - .spyOn(console, 'debug') + const consoleLogSpy = vi + .spyOn(console, 'log') .mockImplementation(() => {}); await cacheGoogleAccount('test1@google.com'); - expect(consoleDebugSpy).toHaveBeenCalled(); + expect(consoleLogSpy).toHaveBeenCalled(); expect(JSON.parse(fs.readFileSync(accountsFile(), 'utf-8'))).toEqual({ active: 'test1@google.com', old: [], }); }); + + it('should handle valid JSON with incorrect schema by starting fresh', async () => { + fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); + fs.writeFileSync( + accountsFile(), + JSON.stringify({ active: 'test1@google.com', old: 'not-an-array' }), + ); + const consoleLogSpy = vi + .spyOn(console, 'log') + .mockImplementation(() => {}); + + await cacheGoogleAccount('test2@google.com'); + + expect(consoleLogSpy).toHaveBeenCalled(); + expect(JSON.parse(fs.readFileSync(accountsFile(), 'utf-8'))).toEqual({ + active: 'test2@google.com', + old: [], + }); + }); }); describe('getCachedGoogleAccount', () => { @@ -139,14 +158,21 @@ describe('user_account', () => { it('should return null and log if file is corrupted', () => { fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); fs.writeFileSync(accountsFile(), '{ "active": "test@google.com"'); // Invalid JSON - const consoleDebugSpy = vi - .spyOn(console, 'debug') + const consoleLogSpy = vi + .spyOn(console, 'log') .mockImplementation(() => {}); const account = getCachedGoogleAccount(); expect(account).toBeNull(); - expect(consoleDebugSpy).toHaveBeenCalled(); + expect(consoleLogSpy).toHaveBeenCalled(); + }); + + it('should return null if active key is missing', () => { + fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); + fs.writeFileSync(accountsFile(), JSON.stringify({ old: [] })); + const account = getCachedGoogleAccount(); + expect(account).toBeNull(); }); }); @@ -177,6 +203,56 @@ describe('user_account', () => { expect(stored.active).toBeNull(); expect(stored.old).toEqual([]); }); + + it('should handle corrupted JSON by creating a fresh file', async () => { + fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); + fs.writeFileSync(accountsFile(), 'not valid json'); + const consoleLogSpy = vi + .spyOn(console, 'log') + .mockImplementation(() => {}); + + await clearCachedGoogleAccount(); + + expect(consoleLogSpy).toHaveBeenCalled(); + const stored = JSON.parse(fs.readFileSync(accountsFile(), 'utf-8')); + expect(stored.active).toBeNull(); + expect(stored.old).toEqual([]); + }); + + it('should be idempotent if active account is already null', async () => { + fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); + fs.writeFileSync( + accountsFile(), + JSON.stringify({ active: null, old: ['old1@google.com'] }, null, 2), + ); + + await clearCachedGoogleAccount(); + + const stored = JSON.parse(fs.readFileSync(accountsFile(), 'utf-8')); + expect(stored.active).toBeNull(); + expect(stored.old).toEqual(['old1@google.com']); + }); + + it('should not add a duplicate to the old list', async () => { + fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); + fs.writeFileSync( + accountsFile(), + JSON.stringify( + { + active: 'active@google.com', + old: ['active@google.com'], + }, + null, + 2, + ), + ); + + await clearCachedGoogleAccount(); + + const stored = JSON.parse(fs.readFileSync(accountsFile(), 'utf-8')); + expect(stored.active).toBeNull(); + expect(stored.old).toEqual(['active@google.com']); + }); }); describe('getLifetimeGoogleAccounts', () => { @@ -193,12 +269,12 @@ describe('user_account', () => { it('should return 0 if the file is corrupted', () => { fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); fs.writeFileSync(accountsFile(), 'invalid json'); - const consoleDebugSpy = vi - .spyOn(console, 'debug') + const consoleLogSpy = vi + .spyOn(console, 'log') .mockImplementation(() => {}); expect(getLifetimeGoogleAccounts()).toBe(0); - expect(consoleDebugSpy).toHaveBeenCalled(); + expect(consoleLogSpy).toHaveBeenCalled(); }); it('should return 1 if there is only an active account', () => { @@ -233,5 +309,31 @@ describe('user_account', () => { ); expect(getLifetimeGoogleAccounts()).toBe(3); }); + + it('should handle valid JSON with incorrect schema by returning 0', () => { + fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); + fs.writeFileSync( + accountsFile(), + JSON.stringify({ active: null, old: 1 }), + ); + const consoleLogSpy = vi + .spyOn(console, 'log') + .mockImplementation(() => {}); + + expect(getLifetimeGoogleAccounts()).toBe(0); + expect(consoleLogSpy).toHaveBeenCalled(); + }); + + it('should not double count if active account is also in old list', () => { + fs.mkdirSync(path.dirname(accountsFile()), { recursive: true }); + fs.writeFileSync( + accountsFile(), + JSON.stringify({ + active: 'test1@google.com', + old: ['test1@google.com', 'test2@google.com'], + }), + ); + expect(getLifetimeGoogleAccounts()).toBe(2); + }); }); }); diff --git a/packages/core/src/utils/user_account.ts b/packages/core/src/utils/user_account.ts index 6701dfe3..18b7dcf4 100644 --- a/packages/core/src/utils/user_account.ts +++ b/packages/core/src/utils/user_account.ts @@ -5,7 +5,7 @@ */ import path from 'node:path'; -import { promises as fsp, existsSync, readFileSync } from 'node:fs'; +import { promises as fsp, readFileSync } from 'node:fs'; import * as os from 'os'; import { GEMINI_DIR, GOOGLE_ACCOUNTS_FILENAME } from './paths.js'; @@ -18,21 +18,66 @@ function getGoogleAccountsCachePath(): string { return path.join(os.homedir(), GEMINI_DIR, GOOGLE_ACCOUNTS_FILENAME); } -async function readAccounts(filePath: string): Promise { +/** + * Parses and validates the string content of an accounts file. + * @param content The raw string content from the file. + * @returns A valid UserAccounts object. + */ +function parseAndValidateAccounts(content: string): UserAccounts { + const defaultState = { active: null, old: [] }; + if (!content.trim()) { + return defaultState; + } + + const parsed = JSON.parse(content); + + // Inlined validation logic + if (typeof parsed !== 'object' || parsed === null) { + console.log('Invalid accounts file schema, starting fresh.'); + return defaultState; + } + const { active, old } = parsed as Partial; + const isValid = + (active === undefined || active === null || typeof active === 'string') && + (old === undefined || + (Array.isArray(old) && old.every((i) => typeof i === 'string'))); + + if (!isValid) { + console.log('Invalid accounts file schema, starting fresh.'); + return defaultState; + } + + return { + active: parsed.active ?? null, + old: parsed.old ?? [], + }; +} + +function readAccountsSync(filePath: string): UserAccounts { + const defaultState = { active: null, old: [] }; try { - const content = await fsp.readFile(filePath, 'utf-8'); - if (!content.trim()) { - return { active: null, old: [] }; - } - return JSON.parse(content) as UserAccounts; + const content = readFileSync(filePath, 'utf-8'); + return parseAndValidateAccounts(content); } catch (error) { if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { - // File doesn't exist, which is fine. - return { active: null, old: [] }; + return defaultState; } - // File is corrupted or not valid JSON, start with a fresh object. - console.debug('Could not parse accounts file, starting fresh.', error); - return { active: null, old: [] }; + console.log('Error during sync read of accounts, starting fresh.', error); + return defaultState; + } +} + +async function readAccounts(filePath: string): Promise { + const defaultState = { active: null, old: [] }; + try { + const content = await fsp.readFile(filePath, 'utf-8'); + return parseAndValidateAccounts(content); + } catch (error) { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { + return defaultState; + } + console.log('Could not parse accounts file, starting fresh.', error); + return defaultState; } } @@ -56,52 +101,23 @@ export async function cacheGoogleAccount(email: string): Promise { } export function getCachedGoogleAccount(): string | null { - try { - const filePath = getGoogleAccountsCachePath(); - if (existsSync(filePath)) { - const content = readFileSync(filePath, 'utf-8').trim(); - if (!content) { - return null; - } - const accounts: UserAccounts = JSON.parse(content); - return accounts.active; - } - return null; - } catch (error) { - console.debug('Error reading cached Google Account:', error); - return null; - } + const filePath = getGoogleAccountsCachePath(); + const accounts = readAccountsSync(filePath); + return accounts.active; } export function getLifetimeGoogleAccounts(): number { - try { - const filePath = getGoogleAccountsCachePath(); - if (!existsSync(filePath)) { - return 0; - } - - const content = readFileSync(filePath, 'utf-8').trim(); - if (!content) { - return 0; - } - const accounts: UserAccounts = JSON.parse(content); - let count = accounts.old.length; - if (accounts.active) { - count++; - } - return count; - } catch (error) { - console.debug('Error reading lifetime Google Accounts:', error); - return 0; + const filePath = getGoogleAccountsCachePath(); + const accounts = readAccountsSync(filePath); + const allAccounts = new Set(accounts.old); + if (accounts.active) { + allAccounts.add(accounts.active); } + return allAccounts.size; } export async function clearCachedGoogleAccount(): Promise { const filePath = getGoogleAccountsCachePath(); - if (!existsSync(filePath)) { - return; - } - const accounts = await readAccounts(filePath); if (accounts.active) {