Fix #8077: Settings command overwrites entire JSON file, leaking environment variables (#8154)

Co-authored-by: Shreya Keshive <skeshive@gmail.com>
2026-04-08 12:20:38 -07:00 · 2025-09-13 14:31:15 +08:00
parent a96cc2f148
commit 2135dbb6a4
8 changed files with 379 additions and 26 deletions
--- a/npm-shrinkwrap.json
+++ b/npm-shrinkwrap.json
@@ -5201,6 +5201,12 @@
        "url": "https://github.com/sponsors/ljharb"
      }
    },
+    "node_modules/array-timsort": {
+      "version": "1.0.3",
+      "resolved": "https://registry.npmjs.org/array-timsort/-/array-timsort-1.0.3.tgz",
+      "integrity": "sha512-/+3GRL7dDAGEfM6TseQk/U+mi18TU2Ms9I3UlLdUMhz2hbvGNTKdj9xniwXfUqgYhHxRx0+8UnKkvlNwVU+cWQ==",
+      "license": "MIT"
+    },
    "node_modules/array.prototype.findlast": {
      "version": "1.2.5",
      "resolved": "https://registry.npmjs.org/array.prototype.findlast/-/array.prototype.findlast-1.2.5.tgz",
@@ -6293,6 +6299,22 @@
        "node": ">=18"
      }
    },
+    "node_modules/comment-json": {
+      "version": "4.2.5",
+      "resolved": "https://registry.npmjs.org/comment-json/-/comment-json-4.2.5.tgz",
+      "integrity": "sha512-bKw/r35jR3HGt5PEPm1ljsQQGyCrR8sFGNiN5L+ykDHdpO8Smxkrkla9Yi6NkQyUrb8V54PGhfMs6NrIwtxtdw==",
+      "license": "MIT",
+      "dependencies": {
+        "array-timsort": "^1.0.3",
+        "core-util-is": "^1.0.3",
+        "esprima": "^4.0.1",
+        "has-own-prop": "^2.0.0",
+        "repeat-string": "^1.6.1"
+      },
+      "engines": {
+        "node": ">= 6"
+      }
+    },
    "node_modules/component-emitter": {
      "version": "1.3.1",
      "resolved": "https://registry.npmjs.org/component-emitter/-/component-emitter-1.3.1.tgz",
@@ -6398,6 +6420,12 @@
      "dev": true,
      "license": "MIT"
    },
+    "node_modules/core-util-is": {
+      "version": "1.0.3",
+      "resolved": "https://registry.npmjs.org/core-util-is/-/core-util-is-1.0.3.tgz",
+      "integrity": "sha512-ZQBvi1DcpJ4GDqanjucZ2Hj3wEO5pZDS89BWbkcrvdxksJorwUDDZamX9ldFkp9aw2lmBDLgkObEA4DWNJ9FYQ==",
+      "license": "MIT"
+    },
    "node_modules/cors": {
      "version": "2.8.5",
      "resolved": "https://registry.npmjs.org/cors/-/cors-2.8.5.tgz",
@@ -7652,7 +7680,6 @@
      "version": "4.0.1",
      "resolved": "https://registry.npmjs.org/esprima/-/esprima-4.0.1.tgz",
      "integrity": "sha512-eGuFFw7Upda+g4p+QHvnW0RyTX/SVeJBDM/gCtMARO0cLuT2HcEKnTPvhjV6aGeqrCB/sbNop0Kszm0jsaWU4A==",
-      "dev": true,
      "license": "BSD-2-Clause",
      "bin": {
        "esparse": "bin/esparse.js",
@@ -8906,6 +8933,15 @@
        "node": ">=8"
      }
    },
+    "node_modules/has-own-prop": {
+      "version": "2.0.0",
+      "resolved": "https://registry.npmjs.org/has-own-prop/-/has-own-prop-2.0.0.tgz",
+      "integrity": "sha512-Pq0h+hvsVm6dDEa8x82GnLSYHOzNDt7f0ddFa3FqcQlgzEiptPqL+XrOJNavjOzSYiYWIrgeVYYgGlLmnxwilQ==",
+      "license": "MIT",
+      "engines": {
+        "node": ">=8"
+      }
+    },
    "node_modules/has-property-descriptors": {
      "version": "1.0.2",
      "resolved": "https://registry.npmjs.org/has-property-descriptors/-/has-property-descriptors-1.0.2.tgz",
@@ -12678,6 +12714,15 @@
        "url": "https://github.com/sponsors/sindresorhus"
      }
    },
+    "node_modules/repeat-string": {
+      "version": "1.6.1",
+      "resolved": "https://registry.npmjs.org/repeat-string/-/repeat-string-1.6.1.tgz",
+      "integrity": "sha512-PV0dzCYDNfRi1jCDbJzpW7jNNDRuCOG/jI5ctQcGKt/clZD+YcPS3yIlWuTJMmESC8aevCFmWJy5wjAFgNqN6w==",
+      "license": "MIT",
+      "engines": {
+        "node": ">=0.10"
+      }
+    },
    "node_modules/require-directory": {
      "version": "2.1.1",
      "resolved": "https://registry.npmjs.org/require-directory/-/require-directory-2.1.1.tgz",
@@ -16220,6 +16265,7 @@
        "@modelcontextprotocol/sdk": "^1.15.1",
        "@types/update-notifier": "^6.0.8",
        "command-exists": "^1.2.9",
+        "comment-json": "^4.2.5",
        "diff": "^7.0.0",
        "dotenv": "^17.1.0",
        "fzf": "^0.5.2",
--- a/packages/cli/package.json
+++ b/packages/cli/package.json
@@ -34,6 +34,7 @@
    "@modelcontextprotocol/sdk": "^1.15.1",
    "@types/update-notifier": "^6.0.8",
    "command-exists": "^1.2.9",
+    "comment-json": "^4.2.5",
    "diff": "^7.0.0",
    "dotenv": "^17.1.0",
    "fzf": "^0.5.2",
--- a/packages/cli/src/config/settings.ts
+++ b/packages/cli/src/config/settings.ts
@@ -29,6 +29,7 @@ import {
 } from './settingsSchema.js';
 import { resolveEnvVarsInObject } from '../utils/envVarResolver.js';
 import { customDeepMerge } from '../utils/deepMerge.js';
+import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js';

 function getMergeStrategyForPath(path: string[]): MergeStrategy | undefined {
  let current: SettingDefinition | undefined = undefined;
@@ -172,7 +173,9 @@ export interface SettingsError {

 export interface SettingsFile {
  settings: Settings;
+  originalSettings: Settings;
  path: string;
+  rawJson?: string;
 }

 function setNestedProperty(
@@ -406,6 +409,7 @@ export class LoadedSettings {
  setValue(scope: SettingScope, key: string, value: unknown): void {
    const settingsFile = this.forScope(scope);
    setNestedProperty(settingsFile.settings, key, value);
+    setNestedProperty(settingsFile.originalSettings, key, value);
    this._merged = this.computeMergedSettings();
    saveSettings(settingsFile);
  }
@@ -539,7 +543,10 @@ export function loadSettings(
    workspaceDir,
  ).getWorkspaceSettingsPath();

-  const loadAndMigrate = (filePath: string, scope: SettingScope): Settings => {
+  const loadAndMigrate = (
+    filePath: string,
+    scope: SettingScope,
+  ): { settings: Settings; rawJson?: string } => {
    try {
      if (fs.existsSync(filePath)) {
        const content = fs.readFileSync(filePath, 'utf-8');
@@ -554,7 +561,7 @@ export function loadSettings(
            message: 'Settings file is not a valid JSON object.',
            path: filePath,
          });
-          return {};
+          return { settings: {} };
        }

        let settingsObject = rawSettings as Record<string, unknown>;
@@ -582,7 +589,7 @@ export function loadSettings(
            settingsObject = migratedSettings;
          }
        }
-        return settingsObject as Settings;
+        return { settings: settingsObject as Settings, rawJson: content };
      }
    } catch (error: unknown) {
      settingsErrors.push({
@@ -590,23 +597,40 @@ export function loadSettings(
        path: filePath,
      });
    }
-    return {};
+    return { settings: {} };
  };

-  systemSettings = loadAndMigrate(systemSettingsPath, SettingScope.System);
-  systemDefaultSettings = loadAndMigrate(
+  const systemResult = loadAndMigrate(systemSettingsPath, SettingScope.System);
+  const systemDefaultsResult = loadAndMigrate(
    systemDefaultsPath,
    SettingScope.SystemDefaults,
  );
-  userSettings = loadAndMigrate(USER_SETTINGS_PATH, SettingScope.User);
+  const userResult = loadAndMigrate(USER_SETTINGS_PATH, SettingScope.User);

+  let workspaceResult: { settings: Settings; rawJson?: string } = {
+    settings: {} as Settings,
+    rawJson: undefined,
+  };
  if (realWorkspaceDir !== realHomeDir) {
-    workspaceSettings = loadAndMigrate(
+    workspaceResult = loadAndMigrate(
      workspaceSettingsPath,
      SettingScope.Workspace,
    );
  }

+  const systemOriginalSettings = structuredClone(systemResult.settings);
+  const systemDefaultsOriginalSettings = structuredClone(
+    systemDefaultsResult.settings,
+  );
+  const userOriginalSettings = structuredClone(userResult.settings);
+  const workspaceOriginalSettings = structuredClone(workspaceResult.settings);
+
+  // Environment variables for runtime use
+  systemSettings = resolveEnvVarsInObject(systemResult.settings);
+  systemDefaultSettings = resolveEnvVarsInObject(systemDefaultsResult.settings);
+  userSettings = resolveEnvVarsInObject(userResult.settings);
+  workspaceSettings = resolveEnvVarsInObject(workspaceResult.settings);
+
  // Support legacy theme names
  if (userSettings.ui?.theme === 'VS') {
    userSettings.ui.theme = DefaultLight.name;
@@ -642,11 +666,6 @@ export function loadSettings(
  // the settings to avoid a cycle
  loadEnvironment(tempMergedSettings);

-  // Now that the environment is loaded, resolve variables in the settings.
-  systemSettings = resolveEnvVarsInObject(systemSettings);
-  userSettings = resolveEnvVarsInObject(userSettings);
-  workspaceSettings = resolveEnvVarsInObject(workspaceSettings);
-
  // Create LoadedSettings first

  if (settingsErrors.length > 0) {
@@ -662,18 +681,26 @@ export function loadSettings(
    {
      path: systemSettingsPath,
      settings: systemSettings,
+      originalSettings: systemOriginalSettings,
+      rawJson: systemResult.rawJson,
    },
    {
      path: systemDefaultsPath,
      settings: systemDefaultSettings,
+      originalSettings: systemDefaultsOriginalSettings,
+      rawJson: systemDefaultsResult.rawJson,
    },
    {
      path: USER_SETTINGS_PATH,
      settings: userSettings,
+      originalSettings: userOriginalSettings,
+      rawJson: userResult.rawJson,
    },
    {
      path: workspaceSettingsPath,
      settings: workspaceSettings,
+      originalSettings: workspaceOriginalSettings,
+      rawJson: workspaceResult.rawJson,
    },
    isTrusted,
    migratedInMemorScopes,
@@ -688,17 +715,17 @@ export function saveSettings(settingsFile: SettingsFile): void {
      fs.mkdirSync(dirPath, { recursive: true });
    }

-    let settingsToSave = settingsFile.settings;
+    let settingsToSave = settingsFile.originalSettings;
    if (!MIGRATE_V2_OVERWRITE) {
      settingsToSave = migrateSettingsToV1(
        settingsToSave as Record<string, unknown>,
      ) as Settings;
    }

-    fs.writeFileSync(
+    // Use the format-preserving update function
+    updateSettingsFilePreservingFormat(
      settingsFile.path,
-      JSON.stringify(settingsToSave, null, 2),
-      'utf-8',
+      settingsToSave as Record<string, unknown>,
    );
  } catch (error) {
    console.error('Error saving user settings file:', error);
--- a/packages/cli/src/ui/components/SettingsDialog.test.tsx
+++ b/packages/cli/src/ui/components/SettingsDialog.test.tsx
@@ -58,10 +58,16 @@ const createMockSettings = (
  new LoadedSettings(
    {
      settings: { ui: { customThemes: {} }, mcpServers: {}, ...systemSettings },
+      originalSettings: {
+        ui: { customThemes: {} },
+        mcpServers: {},
+        ...systemSettings,
+      },
      path: '/system/settings.json',
    },
    {
      settings: {},
+      originalSettings: {},
      path: '/system/system-defaults.json',
    },
    {
@@ -70,6 +76,11 @@ const createMockSettings = (
        mcpServers: {},
        ...userSettings,
      },
+      originalSettings: {
+        ui: { customThemes: {} },
+        mcpServers: {},
+        ...userSettings,
+      },
      path: '/user/settings.json',
    },
    {
@@ -78,6 +89,11 @@ const createMockSettings = (
        mcpServers: {},
        ...workspaceSettings,
      },
+      originalSettings: {
+        ui: { customThemes: {} },
+        mcpServers: {},
+        ...workspaceSettings,
+      },
      path: '/workspace/settings.json',
    },
    true,
--- a/packages/cli/src/ui/components/ThemeDialog.test.tsx
+++ b/packages/cli/src/ui/components/ThemeDialog.test.tsx
@@ -21,10 +21,12 @@ const createMockSettings = (
  new LoadedSettings(
    {
      settings: { ui: { customThemes: {} }, ...systemSettings },
+      originalSettings: { ui: { customThemes: {} }, ...systemSettings },
      path: '/system/settings.json',
    },
    {
      settings: {},
+      originalSettings: {},
      path: '/system/system-defaults.json',
    },
    {
@@ -32,6 +34,10 @@ const createMockSettings = (
        ui: { customThemes: {} },
        ...userSettings,
      },
+      originalSettings: {
+        ui: { customThemes: {} },
+        ...userSettings,
+      },
      path: '/user/settings.json',
    },
    {
@@ -39,6 +45,10 @@ const createMockSettings = (
        ui: { customThemes: {} },
        ...workspaceSettings,
      },
+      originalSettings: {
+        ui: { customThemes: {} },
+        ...workspaceSettings,
+      },
      path: '/workspace/settings.json',
    },
    true,
--- a/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx
+++ b/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx
@@ -19,10 +19,10 @@ describe('<MarkdownDisplay />', () => {
  };

  const mockSettings = new LoadedSettings(
-    { path: '', settings: {} },
-    { path: '', settings: {} },
-    { path: '', settings: {} },
-    { path: '', settings: {} },
+    { path: '', settings: {}, originalSettings: {} },
+    { path: '', settings: {}, originalSettings: {} },
+    { path: '', settings: {}, originalSettings: {} },
+    { path: '', settings: {}, originalSettings: {} },
    true,
    new Set(),
  );
@@ -222,10 +222,14 @@ Another paragraph.
  it('hides line numbers in code blocks when showLineNumbers is false', () => {
    const text = '```javascript\nconst x = 1;\n```'.replace(/\n/g, EOL);
    const settings = new LoadedSettings(
-      { path: '', settings: {} },
-      { path: '', settings: {} },
-      { path: '', settings: { ui: { showLineNumbers: false } } },
-      { path: '', settings: {} },
+      { path: '', settings: {}, originalSettings: {} },
+      { path: '', settings: {}, originalSettings: {} },
+      {
+        path: '',
+        settings: { ui: { showLineNumbers: false } },
+        originalSettings: { ui: { showLineNumbers: false } },
+      },
+      { path: '', settings: {}, originalSettings: {} },
      true,
      new Set(),
    );
--- a/packages/cli/src/utils/commentJson.test.ts
+++ b/packages/cli/src/utils/commentJson.test.ts
@@ -0,0 +1,182 @@
+/**
+ * @license
+ * Copyright 2025 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import * as fs from 'node:fs';
+import * as path from 'node:path';
+import * as os from 'node:os';
+import { updateSettingsFilePreservingFormat } from './commentJson.js';
+
+describe('commentJson', () => {
+  let tempDir: string;
+  let testFilePath: string;
+
+  beforeEach(() => {
+    // Create a temporary directory for test files
+    tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'preserve-format-test-'));
+    testFilePath = path.join(tempDir, 'settings.json');
+  });
+
+  afterEach(() => {
+    // Clean up temporary directory
+    if (fs.existsSync(tempDir)) {
+      fs.rmSync(tempDir, { recursive: true, force: true });
+    }
+  });
+
+  describe('updateSettingsFilePreservingFormat', () => {
+    it('should preserve comments when updating settings', () => {
+      const originalContent = `{
+        // Model configuration
+        "model": "gemini-2.5-pro",
+        "ui": {
+          // Theme setting
+          "theme": "dark"
+        }
+      }`;
+
+      fs.writeFileSync(testFilePath, originalContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        model: 'gemini-3.0-ultra',
+      });
+
+      const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
+
+      expect(updatedContent).toContain('// Model configuration');
+      expect(updatedContent).toContain('// Theme setting');
+      expect(updatedContent).toContain('"model": "gemini-3.0-ultra"');
+      expect(updatedContent).toContain('"theme": "dark"');
+    });
+
+    it('should handle nested object updates', () => {
+      const originalContent = `{
+        "ui": {
+          "theme": "dark",
+          "showLineNumbers": true
+        }
+      }`;
+
+      fs.writeFileSync(testFilePath, originalContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        ui: {
+          theme: 'light',
+          showLineNumbers: true,
+        },
+      });
+
+      const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
+      expect(updatedContent).toContain('"theme": "light"');
+      expect(updatedContent).toContain('"showLineNumbers": true');
+    });
+
+    it('should add new fields while preserving existing structure', () => {
+      const originalContent = `{
+        // Existing config
+        "model": "gemini-2.5-pro"
+      }`;
+
+      fs.writeFileSync(testFilePath, originalContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        model: 'gemini-2.5-pro',
+        newField: 'newValue',
+      });
+
+      const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
+      expect(updatedContent).toContain('// Existing config');
+      expect(updatedContent).toContain('"newField": "newValue"');
+    });
+
+    it('should create file if it does not exist', () => {
+      updateSettingsFilePreservingFormat(testFilePath, {
+        model: 'gemini-2.5-pro',
+      });
+
+      expect(fs.existsSync(testFilePath)).toBe(true);
+      const content = fs.readFileSync(testFilePath, 'utf-8');
+      expect(content).toContain('"model": "gemini-2.5-pro"');
+    });
+
+    it('should handle complex real-world scenario', () => {
+      const complexContent = `{
+        // Settings
+        "model": "gemini-2.5-pro",
+        "mcpServers": {
+          // Active server
+          "context7": {
+            "headers": {
+              "API_KEY": "test-key" // API key
+            }
+          }
+        }
+      }`;
+
+      fs.writeFileSync(testFilePath, complexContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        model: 'gemini-3.0-ultra',
+        mcpServers: {
+          context7: {
+            headers: {
+              API_KEY: 'new-test-key',
+            },
+          },
+        },
+        newSection: {
+          setting: 'value',
+        },
+      });
+
+      const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
+
+      // Verify comments preserved
+      expect(updatedContent).toContain('// Settings');
+      expect(updatedContent).toContain('// Active server');
+      expect(updatedContent).toContain('// API key');
+
+      // Verify updates applied
+      expect(updatedContent).toContain('"model": "gemini-3.0-ultra"');
+      expect(updatedContent).toContain('"newSection"');
+      expect(updatedContent).toContain('"API_KEY": "new-test-key"');
+    });
+
+    it('should handle corrupted JSON files gracefully', () => {
+      const corruptedContent = `{
+        "model": "gemini-2.5-pro",
+        "ui": {
+          "theme": "dark"
+        // Missing closing brace
+      `;
+
+      fs.writeFileSync(testFilePath, corruptedContent, 'utf-8');
+
+      const consoleSpy = vi
+        .spyOn(console, 'error')
+        .mockImplementation(() => {});
+
+      expect(() => {
+        updateSettingsFilePreservingFormat(testFilePath, {
+          model: 'gemini-3.0-ultra',
+        });
+      }).not.toThrow();
+
+      expect(consoleSpy).toHaveBeenCalledWith(
+        'Error parsing settings file:',
+        expect.any(Error),
+      );
+      expect(consoleSpy).toHaveBeenCalledWith(
+        'Settings file may be corrupted. Please check the JSON syntax.',
+      );
+
+      const unchangedContent = fs.readFileSync(testFilePath, 'utf-8');
+      expect(unchangedContent).toBe(corruptedContent);
+
+      consoleSpy.mockRestore();
+    });
+  });
+});
--- a/packages/cli/src/utils/commentJson.ts
+++ b/packages/cli/src/utils/commentJson.ts
@@ -0,0 +1,67 @@
+/**
+ * @license
+ * Copyright 2025 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import * as fs from 'node:fs';
+import { parse, stringify } from 'comment-json';
+
+/**
+ * Updates a JSON file while preserving comments and formatting.
+ */
+export function updateSettingsFilePreservingFormat(
+  filePath: string,
+  updates: Record<string, unknown>,
+): void {
+  if (!fs.existsSync(filePath)) {
+    fs.writeFileSync(filePath, JSON.stringify(updates, null, 2), 'utf-8');
+    return;
+  }
+
+  const originalContent = fs.readFileSync(filePath, 'utf-8');
+
+  let parsed: Record<string, unknown>;
+  try {
+    parsed = parse(originalContent) as Record<string, unknown>;
+  } catch (error) {
+    console.error('Error parsing settings file:', error);
+    console.error(
+      'Settings file may be corrupted. Please check the JSON syntax.',
+    );
+    return;
+  }
+
+  const updatedStructure = applyUpdates(parsed, updates);
+  const updatedContent = stringify(updatedStructure, null, 2);
+
+  fs.writeFileSync(filePath, updatedContent, 'utf-8');
+}
+
+function applyUpdates(
+  current: Record<string, unknown>,
+  updates: Record<string, unknown>,
+): Record<string, unknown> {
+  const result = current;
+
+  for (const key of Object.getOwnPropertyNames(updates)) {
+    const value = updates[key];
+    if (
+      typeof value === 'object' &&
+      value !== null &&
+      !Array.isArray(value) &&
+      typeof result[key] === 'object' &&
+      result[key] !== null &&
+      !Array.isArray(result[key])
+    ) {
+      result[key] = applyUpdates(
+        result[key] as Record<string, unknown>,
+        value as Record<string, unknown>,
+      );
+    } else {
+      result[key] = value;
+    }
+  }
+
+  return result;
+}