fix(a2a-server): guard deep-merge against null settings and clone nested values

Address review feedback on #28094:

- Normalize parsed user/workspace settings to `{}` when a file is empty
  or contains `null`/a primitive (JSON.parse("null") === null). This
  prevents a server crash both in the deep merge (Object.entries(null))
  and at the earlier folderTrust / later policyPaths property accesses.
- During the merge, deep-clone a nested source object when the target
  value is not a plain object, instead of assigning it by reference. This
  stops the merged result from sharing references with the workspace
  settings object, matching the behavior in cli/src/utils/deepMerge.ts.

Adds unit tests for deepMergeSettings (null-safety, reference cloning)
and loadSettings (null settings files do not crash).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: AriaZhao-coder

packages/a2a-server/src/config/settings.test.ts

@@ -8,7 +8,11 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
 import * as fs from 'node:fs';
 import * as path from 'node:path';
 import * as os from 'node:os';
-import { loadSettings, USER_SETTINGS_PATH } from './settings.js';
+import {
+  deepMergeSettings,
+  loadSettings,
+  USER_SETTINGS_PATH,
+} from './settings.js';
 import { debugLogger, checkPathTrust } from '@google/gemini-cli-core';
 
 const mocks = vi.hoisted(() => {
@@ -215,6 +219,35 @@ describe('loadSettings', () => {
     expect(result.showMemoryUsage).toBe(true);
   });
 
+  it('does not crash when the user settings file contains null', () => {
+    // JSON.parse("null") returns null, which would otherwise crash downstream
+    // property access and the deep merge.
+    fs.writeFileSync(USER_SETTINGS_PATH, 'null');
+
+    expect(() => loadSettings(mockWorkspaceDir)).not.toThrow();
+    const result = loadSettings(mockWorkspaceDir);
+    expect(result).toEqual({
+      policyPaths: undefined,
+      adminPolicyPaths: undefined,
+    });
+  });
+
+  it('does not crash when the workspace settings file contains null', () => {
+    fs.writeFileSync(
+      USER_SETTINGS_PATH,
+      JSON.stringify({ showMemoryUsage: true }),
+    );
+    const workspaceSettingsPath = path.join(
+      mockGeminiWorkspaceDir,
+      'settings.json',
+    );
+    fs.writeFileSync(workspaceSettingsPath, 'null');
+
+    expect(() => loadSettings(mockWorkspaceDir, true)).not.toThrow();
+    const result = loadSettings(mockWorkspaceDir, true);
+    expect(result.showMemoryUsage).toBe(true);
+  });
+
   describe('security', () => {
     it('should NOT load workspace settings if workspace is NOT trusted', () => {
       const userSettings = { showMemoryUsage: false };
@@ -289,3 +322,42 @@ describe('loadSettings', () => {
     });
   });
 });
+
+describe('deepMergeSettings', () => {
+  it('does not throw when either side is null', () => {
+    const nullValue = null as unknown as Record<string, unknown>;
+    expect(() => deepMergeSettings(nullValue, { a: 1 })).not.toThrow();
+    expect(() => deepMergeSettings({ a: 1 }, nullValue)).not.toThrow();
+    expect(() => deepMergeSettings(nullValue, nullValue)).not.toThrow();
+
+    expect(deepMergeSettings(nullValue, { a: 1 })).toEqual({ a: 1 });
+    expect(deepMergeSettings({ a: 1 }, nullValue)).toEqual({ a: 1 });
+    expect(deepMergeSettings(nullValue, nullValue)).toEqual({});
+  });
+
+  it('deep-clones nested source objects so the result does not share references', () => {
+    // `target` has no `nested` key, so `source.nested` would previously be
+    // assigned by reference, letting later mutations of the result leak back
+    // into the source object.
+    const source = { nested: { keep: true } };
+    const result = deepMergeSettings<Record<string, unknown>>({}, source);
+
+    expect(result['nested']).toEqual({ keep: true });
+    expect(result['nested']).not.toBe(source.nested);
+
+    // Mutating the merged result must not affect the original source object.
+    (result['nested'] as Record<string, unknown>)['keep'] = false;
+    expect(source.nested.keep).toBe(true);
+  });
+
+  it('deep-clones nested objects even when the target value is a primitive', () => {
+    const source = { section: { value: 1 } };
+    const result = deepMergeSettings<Record<string, unknown>>(
+      { section: 'not-an-object' },
+      source,
+    );
+
+    expect(result['section']).toEqual({ value: 1 });
+    expect(result['section']).not.toBe(source.section);
+  });
+});

packages/a2a-server/src/config/settings.ts

@@ -87,7 +87,9 @@ export function loadSettings(
       const parsedUserSettings = JSON.parse(
         stripJsonComments(userContent),
       ) as Settings;
-      userSettings = resolveEnvVarsInObject(parsedUserSettings);
+      userSettings = normalizeSettings(
+        resolveEnvVarsInObject(parsedUserSettings),
+      );
     }
   } catch (error: unknown) {
     settingsErrors.push({
@@ -121,7 +123,9 @@ export function loadSettings(
         const parsedWorkspaceSettings = JSON.parse(
           stripJsonComments(projectContent),
         ) as Settings;
-        workspaceSettings = resolveEnvVarsInObject(parsedWorkspaceSettings);
+        workspaceSettings = normalizeSettings(
+          resolveEnvVarsInObject(parsedWorkspaceSettings),
+        );
       }
     } catch (error: unknown) {
       settingsErrors.push({
@@ -207,18 +211,35 @@ function isPlainObject(value: unknown): value is Record<string, unknown> {
   return typeof value === 'object' && value !== null && !Array.isArray(value);
 }
 
+/**
+ * Normalizes a parsed settings value into a plain `Settings` object. An empty
+ * settings file or one containing `null` or a primitive parses to a non-object
+ * (e.g. `JSON.parse("null")` returns `null`); returning `{}` in those cases
+ * keeps downstream property access and merging safe instead of crashing.
+ */
+function normalizeSettings(value: unknown): Settings {
+  return isPlainObject(value) ? (value as Settings) : {};
+}
+
 /**
  * Deeply merges two settings objects. Nested plain objects are merged
  * recursively so that the `source` settings override only the specific keys
  * they define, instead of replacing an entire nested section. Arrays and
  * primitive values in `source` replace those in `target`, and `undefined`
  * values in `source` are ignored so they never clobber an existing value.
+ *
+ * Exported for testing.
  */
-function deepMergeSettings<T extends object>(target: T, source: T): T {
-  // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
-  const output = { ...target } as Record<string, unknown>;
+export function deepMergeSettings<T extends object>(target: T, source: T): T {
+  // Defensively default to empty objects when either side is not a plain
+  // object. An empty or `null` settings file parses to `null`
+  // (`JSON.parse("null")`), and calling `Object.entries(null)` would throw and
+  // crash the server.
+  const t = isPlainObject(target) ? target : {};
+  const s = isPlainObject(source) ? source : {};
+  const output: Record<string, unknown> = { ...t };
 
-  for (const [key, sourceValue] of Object.entries(source)) {
+  for (const [key, sourceValue] of Object.entries(s)) {
     // Skip dangerous keys that JSON.parse can create as own properties, to
     // prevent prototype pollution.
     if (key === '__proto__' || key === 'constructor' || key === 'prototype') {
@@ -230,6 +251,12 @@ function deepMergeSettings<T extends object>(target: T, source: T): T {
     const targetValue = output[key];
     if (isPlainObject(targetValue) && isPlainObject(sourceValue)) {
       output[key] = deepMergeSettings(targetValue, sourceValue);
+    } else if (isPlainObject(sourceValue)) {
+      // The source value is a nested object but the target value is not, so
+      // recursively clone it. Assigning `sourceValue` directly would make the
+      // merged result share references with the original `source` object,
+      // allowing later mutations to leak back into it.
+      output[key] = deepMergeSettings({}, sourceValue);
     } else {
       output[key] = sourceValue;
     }