From 47948e371267d0421aa1b96a059bb7caad47617c Mon Sep 17 00:00:00 2001 From: shrutip90 Date: Mon, 22 Sep 2025 17:06:43 -0700 Subject: [PATCH] fix(cli): Handle formatting errors in trustedFolders.json similar to settings file (#9167) --- .../cli/src/config/trustedFolders.test.ts | 67 ++++++++++++++++++- packages/cli/src/config/trustedFolders.ts | 50 ++++++++++---- 2 files changed, 103 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/config/trustedFolders.test.ts b/packages/cli/src/config/trustedFolders.test.ts index 150bffbdf4..122ba95b15 100644 --- a/packages/cli/src/config/trustedFolders.test.ts +++ b/packages/cli/src/config/trustedFolders.test.ts @@ -5,7 +5,7 @@ */ import * as osActual from 'node:os'; -import { ideContextStore } from '@google/gemini-cli-core'; +import { FatalConfigError, ideContextStore } from '@google/gemini-cli-core'; import { describe, it, @@ -24,6 +24,7 @@ import { getTrustedFoldersPath, TrustLevel, isWorkspaceTrusted, + resetTrustedFoldersForTesting, } from './trustedFolders.js'; import type { Settings } from './settings.js'; @@ -55,6 +56,7 @@ describe('Trusted Folders Loading', () => { let mockFsWriteFileSync: Mocked; beforeEach(() => { + resetTrustedFoldersForTesting(); vi.resetAllMocks(); mockFsExistsSync = vi.mocked(fs.existsSync); mockStripJsonComments = vi.mocked(stripJsonComments); @@ -208,6 +210,7 @@ describe('isWorkspaceTrusted', () => { }; beforeEach(() => { + resetTrustedFoldersForTesting(); vi.spyOn(process, 'cwd').mockImplementation(() => mockCwd); vi.spyOn(fs, 'readFileSync').mockImplementation((p) => { if (p === getTrustedFoldersPath()) { @@ -226,6 +229,35 @@ describe('isWorkspaceTrusted', () => { Object.keys(mockRules).forEach((key) => delete mockRules[key]); }); + it('should throw a fatal error if the config is malformed', () => { + mockCwd = '/home/user/projectA'; + // This mock needs to be specific to this test to override the one in beforeEach + vi.spyOn(fs, 'readFileSync').mockImplementation((p) => { + if (p === getTrustedFoldersPath()) { + return '{"foo": "bar",}'; // Malformed JSON with trailing comma + } + return '{}'; + }); + expect(() => isWorkspaceTrusted(mockSettings)).toThrow(FatalConfigError); + expect(() => isWorkspaceTrusted(mockSettings)).toThrow( + /Please fix the configuration file/, + ); + }); + + it('should throw a fatal error if the config is not a JSON object', () => { + mockCwd = '/home/user/projectA'; + vi.spyOn(fs, 'readFileSync').mockImplementation((p) => { + if (p === getTrustedFoldersPath()) { + return 'null'; + } + return '{}'; + }); + expect(() => isWorkspaceTrusted(mockSettings)).toThrow(FatalConfigError); + expect(() => isWorkspaceTrusted(mockSettings)).toThrow( + /not a valid JSON object/, + ); + }); + it('should return true for a directly trusted folder', () => { mockCwd = '/home/user/projectA'; mockRules['/home/user/projectA'] = TrustLevel.TRUST_FOLDER; @@ -298,7 +330,9 @@ describe('isWorkspaceTrusted', () => { describe('isWorkspaceTrusted with IDE override', () => { afterEach(() => { + vi.clearAllMocks(); ideContextStore.clear(); + resetTrustedFoldersForTesting(); }); const mockSettings: Settings = { @@ -359,3 +393,34 @@ describe('isWorkspaceTrusted with IDE override', () => { }); }); }); + +describe('Trusted Folders Caching', () => { + beforeEach(() => { + resetTrustedFoldersForTesting(); + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.readFileSync).mockReturnValue('{}'); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should cache the loaded folders object', () => { + const readSpy = vi.spyOn(fs, 'readFileSync'); + + // First call should read the file + loadTrustedFolders(); + expect(readSpy).toHaveBeenCalledTimes(1); + + // Second call should use the cache + loadTrustedFolders(); + expect(readSpy).toHaveBeenCalledTimes(1); + + // Resetting should clear the cache + resetTrustedFoldersForTesting(); + + // Third call should read the file again + loadTrustedFolders(); + expect(readSpy).toHaveBeenCalledTimes(2); + }); +}); diff --git a/packages/cli/src/config/trustedFolders.ts b/packages/cli/src/config/trustedFolders.ts index dcc27d6904..c2a74b3a01 100644 --- a/packages/cli/src/config/trustedFolders.ts +++ b/packages/cli/src/config/trustedFolders.ts @@ -8,6 +8,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { homedir } from 'node:os'; import { + FatalConfigError, getErrorMessage, isWithinRoot, ideContextStore, @@ -114,9 +115,23 @@ export class LoadedTrustedFolders { } } +let loadedTrustedFolders: LoadedTrustedFolders | undefined; + +/** + * FOR TESTING PURPOSES ONLY. + * Resets the in-memory cache of the trusted folders configuration. + */ +export function resetTrustedFoldersForTesting(): void { + loadedTrustedFolders = undefined; +} + export function loadTrustedFolders(): LoadedTrustedFolders { + if (loadedTrustedFolders) { + return loadedTrustedFolders; + } + const errors: TrustedFoldersError[] = []; - const userConfig: Record = {}; + let userConfig: Record = {}; const userPath = getTrustedFoldersPath(); @@ -124,12 +139,19 @@ export function loadTrustedFolders(): LoadedTrustedFolders { try { if (fs.existsSync(userPath)) { const content = fs.readFileSync(userPath, 'utf-8'); - const parsed = JSON.parse(stripJsonComments(content)) as Record< - string, - TrustLevel - >; - if (parsed) { - Object.assign(userConfig, parsed); + const parsed: unknown = JSON.parse(stripJsonComments(content)); + + if ( + typeof parsed !== 'object' || + parsed === null || + Array.isArray(parsed) + ) { + errors.push({ + message: 'Trusted folders file is not a valid JSON object.', + path: userPath, + }); + } else { + userConfig = parsed as Record; } } } catch (error: unknown) { @@ -139,10 +161,11 @@ export function loadTrustedFolders(): LoadedTrustedFolders { }); } - return new LoadedTrustedFolders( + loadedTrustedFolders = new LoadedTrustedFolders( { path: userPath, config: userConfig }, errors, ); + return loadedTrustedFolders; } export function saveTrustedFolders( @@ -181,11 +204,12 @@ function getWorkspaceTrustFromLocalConfig( } if (folders.errors.length > 0) { - for (const error of folders.errors) { - console.error( - `Error loading trusted folders config from ${error.path}: ${error.message}`, - ); - } + const errorMessages = folders.errors.map( + (error) => `Error in ${error.path}: ${error.message}`, + ); + throw new FatalConfigError( + `${errorMessages.join('\n')}\nPlease fix the configuration file and try again.`, + ); } const isTrusted = folders.isPathTrusted(process.cwd());