fix(cli): load environment variables before resolving settings placeholders - #28597

This test directly modifies process.env by deleting a property at the beginning and end of the test case. This pattern is susceptible to test leakage: if an assertion fails, the cleanup code at the end of the it block will not run, and the modified environment variable will persist for subsequent tests.

This violates the repository's testing convention, which states to avoid direct modification of process.env to prevent test leakage.

While testing loadEnvironment's side effects on process.env requires some direct interaction, it should be done safely. Please wrap the test's logic in a try...finally block to guarantee that the environment is cleaned up, even if assertions fail.

    it('should resolve environment variables loaded from .env files', () => {
      const envVar = 'TEST_VAR_FROM_DOT_ENV';
      // Ensure the environment is clean before the test.
      delete process.env[envVar];

      try {
        const workspaceEnvPath = path.join(MOCK_WORKSPACE_DIR, '.env');
        const userSettingsContent: TestSettings = {
          apiKey: `$${envVar}`,
        };

        (mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => {
          const normalized = normalizePath(p);
          return (
            normalized === normalizePath(USER_SETTINGS_PATH) ||
            normalized === normalizePath(workspaceEnvPath)
          );
        });

        (fs.readFileSync as Mock).mockImplementation(
          (p: fs.PathOrFileDescriptor) => {
            const normalized = normalizePath(p);
            if (normalized === normalizePath(USER_SETTINGS_PATH)) {
              return JSON.stringify(userSettingsContent);
            }
            if (normalized === normalizePath(workspaceEnvPath)) {
              return `${envVar}=resolved_from_dot_env`;
            }
            return '{}';
          },
        );

        const settings = loadSettings(MOCK_WORKSPACE_DIR);
        expect((settings.user.settings as TestSettings)['apiKey']).toBe(
          'resolved_from_dot_env',
        );
        expect((settings.merged as TestSettings)['apiKey']).toBe(
          'resolved_from_dot_env',
        );
      } finally {
        // Ensure cleanup happens even if the test fails.
        delete process.env[envVar];
      }
    });

I set up the test using the manual process.env deletion pattern to stay consistent with the adjacent tests (like "should resolve environment variables in user settings"), which also use this exact manual mutation and deletion pattern.

However, I agree that manually mutating process.env in this way is fragile if assertions fail. If we want to make the environment cleanup robust, rather than adding a verbose manual try...finally block to this test, a much cleaner Vitest option would be using vi.stubEnv().

To keep the scope of this PR focused strictly on the settings load-order fix, I feel it's probably best to leave the test matching the adjacent test patterns for now. We can then open a separate refactoring PR to update the settings test suite to use vi.stubEnv systematically. Let me know if you would like me to refactor this specific test using your recommended try...finally block, or the vi.stubEnv approach, or if you agree with me, to keep the test unchanged for consistency with adjacent tests.

Passing raw, unexpanded settings to isWorkspaceTrusted and loadEnvironment introduces a security and correctness risk. If settings like security.folderTrust.enabled, tools.sandbox, or advanced.ignoreLocalEnv contain environment variable placeholders (e.g., "$ENABLE_FOLDER_TRUST" or "$SANDBOX"), they will be evaluated as raw strings. Since non-empty strings are truthy in JavaScript, these placeholders will be treated as true regardless of their actual resolved values, or they will fail to match expected string values (such as selectedAuthType matching AuthType.USE_VERTEX_AI).

To prevent this, we should resolve environment variables against the pre-existing process.env (before loading .env files) using resolveEnvVarsInObject before performing the trust check and loading the environment.

  const initialTrustCheckSettings = resolveEnvVarsInObject(
    customDeepMerge(
      getMergeStrategyForPath,
      getDefaultsFromSchema(),
      systemDefaultsRawResult.rawSettings,
      userRawResult.rawSettings,
      systemRawResult.rawSettings,
    ) as Settings,
  );
  const isTrusted =
    isWorkspaceTrusted(initialTrustCheckSettings, workspaceDir)
      .isTrusted ?? false;

  // Create a temporary merged settings object to pass to loadEnvironment.
  const tempMergedSettings = resolveEnvVarsInObject(
    mergeSettings(
      systemRawResult.rawSettings,
      systemDefaultsRawResult.rawSettings,
      userRawResult.rawSettings,
      workspaceRawResult.rawSettings,
      isTrusted,
    ),
  );

  // loadEnvironment depends on settings so we have to create a temp version of
  // the settings to avoid a cycle. This loads environment variables from .env files!
  loadEnvironment(tempMergedSettings, workspaceDir);

🚨 High Severity: Load-Order Race Condition for .env Variables in Cloud Shell

There is a remaining race condition in loadEnvironment when resolving placeholders for selectedType (e.g., $MOCK_AUTH_TYPE) that are defined in the workspace .env file.

The Issue

1. loadEnvironment(tempMergedSettings, workspaceDir) is called. At this point, tempMergedSettings.security.auth.selectedType is still the literal string "$MOCK_AUTH_TYPE" because the .env file has not been loaded into process.env yet.
2. Inside loadEnvironment, setUpCloudShellEnvironment is called before the .env file is parsed and loaded into process.env.
3. setUpCloudShellEnvironment checks if selectedAuthType === AuthType.USE_VERTEX_AI (i.e., "vertex-ai"). Since it is still "$MOCK_AUTH_TYPE", the check fails, and the Cloud Shell override is not skipped.
4. GOOGLE_CLOUD_PROJECT is incorrectly overwritten to "cloudshell-gca".
5. Only after this does loadEnvironment parse the .env file and load MOCK_AUTH_TYPE=vertex-ai into process.env. But by then, GOOGLE_CLOUD_PROJECT has already been overwritten.

This breaks Vertex AI authentication in Cloud Shell for any user who defines their auth type placeholder in .env.

Why the Test Passed

The test should resolve placeholders in selected auth type during loadSettings and skip Cloud Shell override passed only because it used vi.stubEnv('MOCK_AUTH_TYPE', 'vertex-ai') to inject the variable into process.env before loadSettings was called, rather than simulating it being loaded from the .env file.

Recommended Fix

Inside loadEnvironment (around line 683), we should parse the .env file first, and use the parsed variables to resolve the placeholder in selectedAuthType before calling setUpCloudShellEnvironment.

For example, in loadEnvironment:

export function loadEnvironment(
  settings: Settings,
  workspaceDir: string,
  isWorkspaceTrustedFn = isWorkspaceTrusted,
): void {
  // ... (existing setup) ...

  const envFilePath = findEnvFile(workspaceDir, isTrusted, shouldIgnoreEnv);

  // 1. Parse the .env file first if it exists
  let parsedEnv: Record<string, string> = {};
  if (envFilePath) {
    try {
      const envFileContent = fs.readFileSync(envFilePath, 'utf-8');
      parsedEnv = dotenv.parse(envFileContent);
    } catch {
      // ignore
    }
  }

  // 2. Cloud Shell environment variable handling
  if (process.env['CLOUD_SHELL'] === 'true') {
    let selectedAuthType = settings.security?.auth?.selectedType;
    if (selectedAuthType) {
      // Resolve placeholders using both process.env and the parsed .env variables
      selectedAuthType = resolveEnvVarsInString(selectedAuthType, parsedEnv);
    }
    setUpCloudShellEnvironment(
      envFilePath,
      isTrusted,
      isSandboxed,
      selectedAuthType,
    );
  }

  // 3. Load environment variables into process.env (using the already parsed parsedEnv)
  if (envFilePath) {
    // ... (load parsedEnv into process.env as before) ...
  }
}

/gemini review

The refactored settings loading pipeline effectively resolves the race condition identified in the initial review. By decoupling the raw file loading from the environment variable expansion and validation, you have ensured that .env variables are fully available in process.env before they are used to resolve placeholders in settings or trigger Cloud Shell environment overrides.

Key Improvements:

• Load-Order Correctness: loadEnvironment now correctly parses the .env file and resolves the selectedAuthType before calling setUpCloudShellEnvironment.
• Decoupled Pipeline: The new resolveAndValidate helper ensures that settings are only expanded and validated after the environment is fully initialized, preventing the previous issue where variables were missing during the initial startup phase.
• Robust Testing: The added integration and unit tests correctly simulate the environment loading sequence, verifying that placeholders are resolved as expected in both standard and Cloud Shell scenarios.

This implementation is correct and addresses the high-severity issue identified.