Implemented unified secrets sanitization and env. redaction options (#15348)

This commit is contained in:
Christian Gunderman
2025-12-22 19:18:27 -08:00
committed by GitHub
parent 2ac9fe08f7
commit 3b1dbcd42d
18 changed files with 817 additions and 103 deletions
+24
View File
@@ -177,6 +177,7 @@ import {
SimpleExtensionLoader,
} from '../utils/extensionLoader.js';
import { McpClientManager } from '../tools/mcp-client-manager.js';
import type { EnvironmentSanitizationConfig } from '../services/environmentSanitization.js';
export type { FileFilteringOptions };
export {
@@ -284,6 +285,9 @@ export interface ConfigParameters {
enableExtensionReloading?: boolean;
allowedMcpServers?: string[];
blockedMcpServers?: string[];
allowedEnvironmentVariables?: string[];
blockedEnvironmentVariables?: string[];
enableEnvironmentVariableRedaction?: boolean;
noBrowser?: boolean;
summarizeToolOutput?: Record<string, SummarizeToolOutputSettings>;
folderTrust?: boolean;
@@ -340,6 +344,9 @@ export class Config {
private mcpClientManager?: McpClientManager;
private allowedMcpServers: string[];
private blockedMcpServers: string[];
private allowedEnvironmentVariables: string[];
private blockedEnvironmentVariables: string[];
private readonly enableEnvironmentVariableRedaction: boolean;
private promptRegistry!: PromptRegistry;
private resourceRegistry!: ResourceRegistry;
private agentRegistry!: AgentRegistry;
@@ -479,6 +486,10 @@ export class Config {
this.mcpServers = params.mcpServers;
this.allowedMcpServers = params.allowedMcpServers ?? [];
this.blockedMcpServers = params.blockedMcpServers ?? [];
this.allowedEnvironmentVariables = params.allowedEnvironmentVariables ?? [];
this.blockedEnvironmentVariables = params.blockedEnvironmentVariables ?? [];
this.enableEnvironmentVariableRedaction =
params.enableEnvironmentVariableRedaction ?? false;
this.userMemory = params.userMemory ?? '';
this.geminiMdFileCount = params.geminiMdFileCount ?? 0;
this.geminiMdFilePaths = params.geminiMdFilePaths ?? [];
@@ -547,6 +558,7 @@ export class Config {
terminalHeight: params.shellExecutionConfig?.terminalHeight ?? 24,
showColor: params.shellExecutionConfig?.showColor ?? false,
pager: params.shellExecutionConfig?.pager ?? 'cat',
sanitizationConfig: this.sanitizationConfig,
};
this.truncateToolOutputThreshold =
params.truncateToolOutputThreshold ??
@@ -1069,6 +1081,15 @@ export class Config {
return this.blockedMcpServers;
}
get sanitizationConfig(): EnvironmentSanitizationConfig {
return {
allowedEnvironmentVariables: this.allowedEnvironmentVariables,
blockedEnvironmentVariables: this.blockedEnvironmentVariables,
enableEnvironmentVariableRedaction:
this.enableEnvironmentVariableRedaction,
};
}
setMcpServers(mcpServers: Record<string, MCPServerConfig>): void {
this.mcpServers = mcpServers;
}
@@ -1488,6 +1509,9 @@ export class Config {
config.terminalHeight ?? this.shellExecutionConfig.terminalHeight,
showColor: config.showColor ?? this.shellExecutionConfig.showColor,
pager: config.pager ?? this.shellExecutionConfig.pager,
sanitizationConfig:
config.sanitizationConfig ??
this.shellExecutionConfig.sanitizationConfig,
};
}
getScreenReader(): boolean {
@@ -251,6 +251,11 @@ function createMockConfig(overrides: Partial<Config> = {}): Config {
getShellExecutionConfig: () => ({
terminalWidth: 90,
terminalHeight: 30,
sanitizationConfig: {
enableEnvironmentVariableRedaction: true,
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
},
}),
storage: {
getProjectTempDir: () => '/tmp',
@@ -1444,6 +1449,11 @@ describe('CoreToolScheduler request queueing', () => {
getShellExecutionConfig: () => ({
terminalWidth: 80,
terminalHeight: 24,
sanitizationConfig: {
enableEnvironmentVariableRedaction: true,
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
},
}),
isInteractive: () => false,
});
@@ -1556,6 +1566,11 @@ describe('CoreToolScheduler request queueing', () => {
getShellExecutionConfig: () => ({
terminalWidth: 80,
terminalHeight: 24,
sanitizationConfig: {
enableEnvironmentVariableRedaction: true,
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
},
}),
getToolRegistry: () => toolRegistry,
getHookSystem: () => undefined,
+4 -2
View File
@@ -7,12 +7,11 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process';
import { HookRunner } from './hookRunner.js';
import { HookEventName, HookType } from './types.js';
import { HookEventName, HookType, ConfigSource } from './types.js';
import type { HookConfig } from './types.js';
import type { HookInput } from './types.js';
import type { Readable, Writable } from 'node:stream';
import type { Config } from '../config/config.js';
import { ConfigSource } from './types.js';
// Mock type for the child_process spawn
type MockChildProcessWithoutNullStreams = ChildProcessWithoutNullStreams & {
@@ -70,6 +69,9 @@ describe('HookRunner', () => {
mockConfig = {
isTrustedFolder: vi.fn().mockReturnValue(true),
sanitizationConfig: {
enableEnvironmentVariableRedaction: true,
},
} as unknown as Config;
hookRunner = new HookRunner(mockConfig);
+2 -1
View File
@@ -18,6 +18,7 @@ import type {
} from './types.js';
import type { LLMRequest } from './hookTranslator.js';
import { debugLogger } from '../utils/debugLogger.js';
import { sanitizeEnvironment } from '../services/environmentSanitization.js';
import {
escapeShellArg,
getShellConfiguration,
@@ -238,7 +239,7 @@ export class HookRunner {
// Set up environment variables
const env = {
...process.env,
...sanitizeEnvironment(process.env, this.config.sanitizationConfig),
GEMINI_PROJECT_DIR: input.cwd,
CLAUDE_PROJECT_DIR: input.cwd, // For compatibility
};
@@ -0,0 +1,309 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { describe, expect, it } from 'vitest';
import {
ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES,
NEVER_ALLOWED_ENVIRONMENT_VARIABLES,
NEVER_ALLOWED_NAME_PATTERNS,
NEVER_ALLOWED_VALUE_PATTERNS,
sanitizeEnvironment,
} from './environmentSanitization.js';
const EMPTY_OPTIONS = {
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
enableEnvironmentVariableRedaction: true,
};
describe('sanitizeEnvironment', () => {
it('should allow safe, common environment variables', () => {
const env = {
PATH: '/usr/bin',
HOME: '/home/user',
USER: 'user',
SystemRoot: 'C:\\Windows',
LANG: 'en_US.UTF-8',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual(env);
});
it('should allow variables prefixed with GEMINI_CLI_', () => {
const env = {
GEMINI_CLI_FOO: 'bar',
GEMINI_CLI_BAZ: 'qux',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual(env);
});
it('should redact variables with sensitive names from the denylist', () => {
const env = {
CLIENT_ID: 'sensitive-id',
DB_URI: 'sensitive-uri',
DATABASE_URL: 'sensitive-url',
SAFE_VAR: 'is-safe',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual({
SAFE_VAR: 'is-safe',
});
});
it('should redact variables with names matching all sensitive patterns (case-insensitive)', () => {
const env = {
// Patterns
MY_API_TOKEN: 'token-value',
AppSecret: 'secret-value',
db_password: 'password-value',
ORA_PASSWD: 'password-value',
ANOTHER_KEY: 'key-value',
some_auth_var: 'auth-value',
USER_CREDENTIAL: 'cred-value',
AWS_CREDS: 'creds-value',
PRIVATE_STUFF: 'private-value',
SSL_CERT: 'cert-value',
// Safe variable
USEFUL_INFO: 'is-ok',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual({
USEFUL_INFO: 'is-ok',
});
});
it('should redact variables with values matching all private key patterns', () => {
const env = {
RSA_KEY: '-----BEGIN RSA PRIVATE KEY-----...',
OPENSSH_KEY: '-----BEGIN OPENSSH PRIVATE KEY-----...',
EC_KEY: '-----BEGIN EC PRIVATE KEY-----...',
PGP_KEY: '-----BEGIN PGP PRIVATE KEY-----...',
CERTIFICATE: '-----BEGIN CERTIFICATE-----...',
SAFE_VAR: 'is-safe',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual({
SAFE_VAR: 'is-safe',
});
});
it('should redact variables with values matching all token and credential patterns', () => {
const env = {
// GitHub
GITHUB_TOKEN_GHP: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
GITHUB_TOKEN_GHO: 'gho_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
GITHUB_TOKEN_GHU: 'ghu_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
GITHUB_TOKEN_GHS: 'ghs_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
GITHUB_TOKEN_GHR: 'ghr_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
GITHUB_PAT: 'github_pat_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
// Google
GOOGLE_KEY: 'AIzaSyxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
// AWS
AWS_KEY: 'AKIAxxxxxxxxxxxxxxxx',
// JWT
JWT_TOKEN: 'eyJhbGciOiJIUzI1NiJ9.e30.ZRrHA157xAA_7962-a_3rA',
// Stripe
STRIPE_SK_LIVE: 'sk_live_xxxxxxxxxxxxxxxxxxxxxxxx',
STRIPE_RK_LIVE: 'rk_live_xxxxxxxxxxxxxxxxxxxxxxxx',
STRIPE_SK_TEST: 'sk_test_xxxxxxxxxxxxxxxxxxxxxxxx',
STRIPE_RK_TEST: 'rk_test_xxxxxxxxxxxxxxxxxxxxxxxx',
// Slack
SLACK_XOXB: 'xoxb-xxxxxxxxxxxx-xxxxxxxxxxxx-xxxxxxxx',
SLACK_XOXA: 'xoxa-xxxxxxxxxxxx-xxxxxxxxxxxx-xxxxxxxx',
SLACK_XOXP: 'xoxp-xxxxxxxxxxxx-xxxxxxxxxxxx-xxxxxxxx',
SLACK_XOXB_2: 'xoxr-xxxxxxxxxxxx-xxxxxxxxxxxx-xxxxxxxx',
// URL Credentials
CREDS_IN_HTTPS_URL: 'https://user:password@example.com',
CREDS_IN_HTTP_URL: 'http://user:password@example.com',
CREDS_IN_FTP_URL: 'ftp://user:password@example.com',
CREDS_IN_SMTP_URL: 'smtp://user:password@example.com',
// Safe variable
SAFE_VAR: 'is-safe',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual({
SAFE_VAR: 'is-safe',
});
});
it('should not redact variables that look similar to sensitive patterns', () => {
const env = {
// Not a credential in URL
SAFE_URL: 'https://example.com/foo/bar',
// Not a real JWT
NOT_A_JWT: 'this.is.not.a.jwt',
// Too short to be a token
ALMOST_A_TOKEN: 'ghp_12345',
// Contains a sensitive word, but in a safe context in the value
PUBLIC_KEY_INFO: 'This value describes a public key',
// Variable names that could be false positives
KEYNOTE_SPEAKER: 'Dr. Jane Goodall',
CERTIFIED_DIVER: 'true',
AUTHENTICATION_FLOW: 'oauth',
PRIVATE_JET_OWNER: 'false',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual({
SAFE_URL: 'https://example.com/foo/bar',
NOT_A_JWT: 'this.is.not.a.jwt',
});
});
it('should not redact variables with undefined or empty values if name is safe', () => {
const env: NodeJS.ProcessEnv = {
EMPTY_VAR: '',
UNDEFINED_VAR: undefined,
ANOTHER_SAFE_VAR: 'value',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual({
EMPTY_VAR: '',
ANOTHER_SAFE_VAR: 'value',
});
});
it('should allow variables that do not match any redaction rules', () => {
const env = {
NODE_ENV: 'development',
APP_VERSION: '1.0.0',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual(env);
});
it('should handle an empty environment', () => {
const env = {};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual({});
});
it('should handle a mixed environment with allowed and redacted variables', () => {
const env = {
// Allowed
PATH: '/usr/bin',
HOME: '/home/user',
GEMINI_CLI_VERSION: '1.2.3',
NODE_ENV: 'production',
// Redacted by name
API_KEY: 'should-be-redacted',
MY_SECRET: 'super-secret',
// Redacted by value
GH_TOKEN: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
JWT: 'eyJhbGciOiJIUzI1NiJ9.e30.ZRrHA157xAA_7962-a_3rA',
// Allowed by name but redacted by value
RANDOM_VAR: '-----BEGIN CERTIFICATE-----...',
};
const sanitized = sanitizeEnvironment(env, EMPTY_OPTIONS);
expect(sanitized).toEqual({
PATH: '/usr/bin',
HOME: '/home/user',
GEMINI_CLI_VERSION: '1.2.3',
NODE_ENV: 'production',
});
});
it('should ensure all names in the sets are capitalized', () => {
for (const name of ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES) {
expect(name).toBe(name.toUpperCase());
}
for (const name of NEVER_ALLOWED_ENVIRONMENT_VARIABLES) {
expect(name).toBe(name.toUpperCase());
}
});
it('should ensure all of the regex in the patterns lists are case insensitive', () => {
for (const pattern of NEVER_ALLOWED_NAME_PATTERNS) {
expect(pattern.flags).toContain('i');
}
for (const pattern of NEVER_ALLOWED_VALUE_PATTERNS) {
expect(pattern.flags).toContain('i');
}
});
it('should allow variables specified in allowedEnvironmentVariables', () => {
const env = {
MY_TOKEN: 'secret-token',
OTHER_SECRET: 'another-secret',
};
const allowed = ['MY_TOKEN'];
const sanitized = sanitizeEnvironment(env, {
allowedEnvironmentVariables: allowed,
blockedEnvironmentVariables: [],
enableEnvironmentVariableRedaction: true,
});
expect(sanitized).toEqual({
MY_TOKEN: 'secret-token',
});
});
it('should block variables specified in blockedEnvironmentVariables', () => {
const env = {
SAFE_VAR: 'safe-value',
BLOCKED_VAR: 'blocked-value',
};
const blocked = ['BLOCKED_VAR'];
const sanitized = sanitizeEnvironment(env, {
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: blocked,
enableEnvironmentVariableRedaction: true,
});
expect(sanitized).toEqual({
SAFE_VAR: 'safe-value',
});
});
it('should prioritize allowed over blocked if a variable is in both (though user configuration should avoid this)', () => {
const env = {
CONFLICT_VAR: 'value',
};
const allowed = ['CONFLICT_VAR'];
const blocked = ['CONFLICT_VAR'];
const sanitized = sanitizeEnvironment(env, {
allowedEnvironmentVariables: allowed,
blockedEnvironmentVariables: blocked,
enableEnvironmentVariableRedaction: true,
});
expect(sanitized).toEqual({
CONFLICT_VAR: 'value',
});
});
it('should be case insensitive for allowed and blocked lists', () => {
const env = {
MY_TOKEN: 'secret-token',
BLOCKED_VAR: 'blocked-value',
};
const allowed = ['my_token'];
const blocked = ['blocked_var'];
const sanitized = sanitizeEnvironment(env, {
allowedEnvironmentVariables: allowed,
blockedEnvironmentVariables: blocked,
enableEnvironmentVariableRedaction: true,
});
expect(sanitized).toEqual({
MY_TOKEN: 'secret-token',
});
});
it('should not perform any redaction if enableEnvironmentVariableRedaction is false', () => {
const env = {
MY_API_TOKEN: 'token-value',
AppSecret: 'secret-value',
db_password: 'password-value',
RSA_KEY: '-----BEGIN RSA PRIVATE KEY-----...',
GITHUB_TOKEN_GHP: 'ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
SAFE_VAR: 'is-safe',
};
const options = {
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
enableEnvironmentVariableRedaction: false,
};
const sanitized = sanitizeEnvironment(env, options);
expect(sanitized).toEqual(env);
});
});
@@ -0,0 +1,191 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
export type EnvironmentSanitizationConfig = {
allowedEnvironmentVariables: string[];
blockedEnvironmentVariables: string[];
enableEnvironmentVariableRedaction: boolean;
};
export function sanitizeEnvironment(
processEnv: NodeJS.ProcessEnv,
config: EnvironmentSanitizationConfig,
): NodeJS.ProcessEnv {
if (!config.enableEnvironmentVariableRedaction) {
return { ...processEnv };
}
const results: NodeJS.ProcessEnv = {};
const allowedSet = new Set(
(config.allowedEnvironmentVariables || []).map((k) => k.toUpperCase()),
);
const blockedSet = new Set(
(config.blockedEnvironmentVariables || []).map((k) => k.toUpperCase()),
);
// Enable strict sanitization in GitHub actions.
const isStrictSanitization = !!processEnv['GITHUB_SHA'];
for (const key in processEnv) {
const value = processEnv[key];
if (
!shouldRedactEnvironmentVariable(
key,
value,
allowedSet,
blockedSet,
isStrictSanitization,
)
) {
results[key] = value;
}
}
return results;
}
export const ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES: ReadonlySet<string> =
new Set([
// Cross-platform
'PATH',
// Windows specific
'SYSTEMROOT',
'COMSPEC',
'PATHEXT',
'WINDIR',
'TEMP',
'TMP',
'USERPROFILE',
'SYSTEMDRIVE',
// Unix/Linux/macOS specific
'HOME',
'LANG',
'SHELL',
'TMPDIR',
'USER',
'LOGNAME',
// GitHub Action-related variables
'ADDITIONAL_CONTEXT',
'AVAILABLE_LABELS',
'BRANCH_NAME',
'DESCRIPTION',
'EVENT_NAME',
'GITHUB_ENV',
'IS_PULL_REQUEST',
'ISSUES_TO_TRIAGE',
'ISSUE_BODY',
'ISSUE_NUMBER',
'ISSUE_TITLE',
'PULL_REQUEST_NUMBER',
'REPOSITORY',
'TITLE',
'TRIGGERING_ACTOR',
]);
export const NEVER_ALLOWED_ENVIRONMENT_VARIABLES: ReadonlySet<string> = new Set(
[
'CLIENT_ID',
'DB_URI',
'CONNECTION_STRING',
'AWS_DEFAULT_REGION',
'AZURE_CLIENT_ID',
'AZURE_TENANT_ID',
'SLACK_WEBHOOK_URL',
'TWILIO_ACCOUNT_SID',
'DATABASE_URL',
'GOOGLE_CLOUD_PROJECT',
'GOOGLE_CLOUD_ACCOUNT',
'FIREBASE_PROJECT_ID',
],
);
export const NEVER_ALLOWED_NAME_PATTERNS = [
/TOKEN/i,
/SECRET/i,
/PASSWORD/i,
/PASSWD/i,
/KEY/i,
/AUTH/i,
/CREDENTIAL/i,
/CREDS/i,
/PRIVATE/i,
/CERT/i,
] as const;
export const NEVER_ALLOWED_VALUE_PATTERNS = [
/-----BEGIN (RSA|OPENSSH|EC|PGP) PRIVATE KEY-----/i,
/-----BEGIN CERTIFICATE-----/i,
// Credentials in URL
/(https?|ftp|smtp):\/\/[^:]+:[^@]+@/i,
// GitHub tokens (classic, fine-grained, OAuth, etc.)
/(ghp|gho|ghu|ghs|ghr|github_pat)_[a-zA-Z0-9_]{36,}/i,
// Google API keys
/AIzaSy[a-zA-Z0-9_\\-]{33}/i,
// Amazon AWS Access Key ID
/AKIA[A-Z0-9]{16}/i,
// Generic OAuth/JWT tokens
/eyJ[a-zA-Z0-9_-]*\.[a-zA-Z0-9_-]*\.[a-zA-Z0-9_-]*/i,
// Stripe API keys
/(s|r)k_(live|test)_[0-9a-zA-Z]{24}/i,
// Slack tokens (bot, user, etc.)
/xox[abpr]-[a-zA-Z0-9-]+/i,
] as const;
function shouldRedactEnvironmentVariable(
key: string,
value: string | undefined,
allowedSet?: Set<string>,
blockedSet?: Set<string>,
isStrictSanitization = false,
): boolean {
key = key.toUpperCase();
value = value?.toUpperCase();
// User overrides take precedence.
if (allowedSet?.has(key)) {
return false;
}
if (blockedSet?.has(key)) {
return true;
}
// These are never redacted.
if (
ALWAYS_ALLOWED_ENVIRONMENT_VARIABLES.has(key) ||
key.startsWith('GEMINI_CLI_')
) {
return false;
}
// These are always redacted.
if (NEVER_ALLOWED_ENVIRONMENT_VARIABLES.has(key)) {
return true;
}
// If in strict mode (e.g. GitHub Action), and not explicitly allowed, redact it.
if (isStrictSanitization) {
return true;
}
for (const pattern of NEVER_ALLOWED_NAME_PATTERNS) {
if (pattern.test(key)) {
return true;
}
}
// Redact if the value looks like a key/cert.
if (value) {
for (const pattern of NEVER_ALLOWED_VALUE_PATTERNS) {
if (pattern.test(value)) {
return true;
}
}
}
return false;
}
@@ -83,6 +83,11 @@ const shellExecutionConfig: ShellExecutionConfig = {
pager: 'cat',
showColor: false,
disableDynamicLineTrimming: true,
sanitizationConfig: {
enableEnvironmentVariableRedaction: true,
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
},
};
const createMockSerializeTerminalToObjectReturnValue = (
@@ -551,7 +556,13 @@ describe('ShellExecutionService', () => {
onOutputEventMock,
new AbortController().signal,
true,
{},
{
sanitizationConfig: {
enableEnvironmentVariableRedaction: true,
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
},
},
);
const result = await handle.result;
@@ -1070,7 +1081,13 @@ describe('ShellExecutionService child_process fallback', () => {
onOutputEventMock,
abortController.signal,
true,
{},
{
sanitizationConfig: {
enableEnvironmentVariableRedaction: true,
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
},
},
);
abortController.abort();
@@ -1258,7 +1275,13 @@ describe('ShellExecutionService execution method selection', () => {
onOutputEventMock,
abortController.signal,
false, // shouldUseNodePty
{},
{
sanitizationConfig: {
enableEnvironmentVariableRedaction: true,
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
},
},
);
// Simulate exit to allow promise to resolve
@@ -19,6 +19,10 @@ import {
serializeTerminalToObject,
type AnsiOutput,
} from '../utils/terminalSerializer.js';
import {
sanitizeEnvironment,
type EnvironmentSanitizationConfig,
} from './environmentSanitization.js';
const { Terminal } = pkg;
const SIGKILL_TIMEOUT_MS = 200;
@@ -80,6 +84,7 @@ export interface ShellExecutionConfig {
showColor?: boolean;
defaultFg?: string;
defaultBg?: string;
sanitizationConfig: EnvironmentSanitizationConfig;
// Used for testing
disableDynamicLineTrimming?: boolean;
scrollback?: number;
@@ -148,74 +153,6 @@ const getFullBufferText = (terminal: pkg.Terminal): string => {
return lines.join('\n');
};
function getSanitizedEnv(): NodeJS.ProcessEnv {
const isRunningInGithub =
process.env['GITHUB_SHA'] || process.env['SURFACE'] === 'Github';
if (!isRunningInGithub) {
// For local runs, we want to preserve the user's full environment.
return { ...process.env };
}
// For CI runs (GitHub), we sanitize the environment for security.
const env: NodeJS.ProcessEnv = {};
const essentialVars = [
// Cross-platform
'PATH',
// Windows specific
'Path',
'SYSTEMROOT',
'SystemRoot',
'COMSPEC',
'ComSpec',
'PATHEXT',
'WINDIR',
'TEMP',
'TMP',
'USERPROFILE',
'SYSTEMDRIVE',
'SystemDrive',
// Unix/Linux/macOS specific
'HOME',
'LANG',
'SHELL',
'TMPDIR',
'USER',
'LOGNAME',
// GitHub Action-related variables
'ADDITIONAL_CONTEXT',
'AVAILABLE_LABELS',
'BRANCH_NAME',
'DESCRIPTION',
'EVENT_NAME',
'GITHUB_ENV',
'IS_PULL_REQUEST',
'ISSUES_TO_TRIAGE',
'ISSUE_BODY',
'ISSUE_NUMBER',
'ISSUE_TITLE',
'PULL_REQUEST_NUMBER',
'REPOSITORY',
'TITLE',
'TRIGGERING_ACTOR',
];
for (const key of essentialVars) {
if (process.env[key] !== undefined) {
env[key] = process.env[key];
}
}
// Always carry over variables and secrets with GEMINI_CLI_*.
for (const key in process.env) {
if (key.startsWith('GEMINI_CLI_')) {
env[key] = process.env[key];
}
}
return env;
}
/**
* A centralized service for executing shell commands with robust process
* management, cross-platform compatibility, and streaming output capabilities.
@@ -265,6 +202,7 @@ export class ShellExecutionService {
cwd,
onOutputEvent,
abortSignal,
shellExecutionConfig.sanitizationConfig,
);
}
@@ -303,6 +241,7 @@ export class ShellExecutionService {
cwd: string,
onOutputEvent: (event: ShellOutputEvent) => void,
abortSignal: AbortSignal,
sanitizationConfig: EnvironmentSanitizationConfig,
): ShellExecutionHandle {
try {
const isWindows = os.platform() === 'win32';
@@ -317,7 +256,7 @@ export class ShellExecutionService {
shell: false,
detached: !isWindows,
env: {
...getSanitizedEnv(),
...sanitizeEnvironment(process.env, sanitizationConfig),
GEMINI_CLI: '1',
TERM: 'xterm-256color',
PAGER: 'cat',
@@ -531,7 +470,10 @@ export class ShellExecutionService {
cols,
rows,
env: {
...getSanitizedEnv(),
...sanitizeEnvironment(
process.env,
shellExecutionConfig.sanitizationConfig,
),
GEMINI_CLI: '1',
TERM: 'xterm-256color',
PAGER: shellExecutionConfig.pager ?? 'cat',
+49 -18
View File
@@ -35,6 +35,13 @@ import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';
import { coreEvents } from '../utils/events.js';
import type { EnvironmentSanitizationConfig } from '../services/environmentSanitization.js';
const EMPTY_CONFIG: EnvironmentSanitizationConfig = {
enableEnvironmentVariableRedaction: true,
allowedEnvironmentVariables: [],
blockedEnvironmentVariables: [],
};
vi.mock('@modelcontextprotocol/sdk/client/stdio.js');
vi.mock('@modelcontextprotocol/sdk/client/index.js');
@@ -124,7 +131,7 @@ describe('mcp-client', () => {
promptRegistry,
resourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
await client.connect();
@@ -204,7 +211,7 @@ describe('mcp-client', () => {
promptRegistry,
resourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
await client.connect();
@@ -255,7 +262,7 @@ describe('mcp-client', () => {
promptRegistry,
resourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
await client.connect();
@@ -310,7 +317,7 @@ describe('mcp-client', () => {
promptRegistry,
resourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
await client.connect();
@@ -369,7 +376,7 @@ describe('mcp-client', () => {
promptRegistry,
resourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
await client.connect();
@@ -442,7 +449,7 @@ describe('mcp-client', () => {
promptRegistry,
resourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
await client.connect();
@@ -518,7 +525,7 @@ describe('mcp-client', () => {
promptRegistry,
resourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
await client.connect();
@@ -601,7 +608,7 @@ describe('mcp-client', () => {
promptRegistry,
resourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
await client.connect();
@@ -681,7 +688,7 @@ describe('mcp-client', () => {
mockedPromptRegistry,
resourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
await client.connect();
@@ -730,7 +737,7 @@ describe('mcp-client', () => {
{} as PromptRegistry,
{} as ResourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
@@ -766,7 +773,7 @@ describe('mcp-client', () => {
{} as PromptRegistry,
{} as ResourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
@@ -821,7 +828,7 @@ describe('mcp-client', () => {
{} as PromptRegistry,
{} as ResourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
onToolsUpdatedSpy,
);
@@ -891,7 +898,7 @@ describe('mcp-client', () => {
{} as PromptRegistry,
{} as ResourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
@@ -961,7 +968,7 @@ describe('mcp-client', () => {
{} as PromptRegistry,
{} as ResourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
onToolsUpdatedSpy,
);
@@ -973,7 +980,7 @@ describe('mcp-client', () => {
{} as PromptRegistry,
{} as ResourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
onToolsUpdatedSpy,
);
@@ -1055,7 +1062,7 @@ describe('mcp-client', () => {
{} as PromptRegistry,
{} as ResourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
);
@@ -1119,7 +1126,7 @@ describe('mcp-client', () => {
{} as PromptRegistry,
{} as ResourceRegistry,
workspaceContext,
{} as Config,
{ sanitizationConfig: EMPTY_CONFIG } as Config,
false,
onToolsUpdatedSpy,
);
@@ -1170,6 +1177,7 @@ describe('mcp-client', () => {
httpUrl: 'http://test-server',
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
@@ -1187,6 +1195,7 @@ describe('mcp-client', () => {
headers: { Authorization: 'derp' },
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
@@ -1207,6 +1216,7 @@ describe('mcp-client', () => {
url: 'http://test-server',
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
expect(transport).toMatchObject({
@@ -1223,6 +1233,7 @@ describe('mcp-client', () => {
headers: { Authorization: 'derp' },
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
@@ -1242,6 +1253,7 @@ describe('mcp-client', () => {
type: 'http',
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
@@ -1259,6 +1271,7 @@ describe('mcp-client', () => {
type: 'sse',
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(SSEClientTransport);
@@ -1275,6 +1288,7 @@ describe('mcp-client', () => {
url: 'http://test-server',
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
@@ -1293,6 +1307,7 @@ describe('mcp-client', () => {
headers: { Authorization: 'Bearer token' },
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
@@ -1313,6 +1328,7 @@ describe('mcp-client', () => {
headers: { 'X-API-Key': 'key123' },
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(SSEClientTransport);
@@ -1332,6 +1348,7 @@ describe('mcp-client', () => {
url: 'http://test-server-url',
},
false,
EMPTY_CONFIG,
);
// httpUrl should take priority and create HTTP transport
@@ -1357,13 +1374,14 @@ describe('mcp-client', () => {
cwd: 'test/cwd',
},
false,
EMPTY_CONFIG,
);
expect(mockedTransport).toHaveBeenCalledWith({
command: 'test-command',
args: ['--foo', 'bar'],
cwd: 'test/cwd',
env: { ...process.env, FOO: 'bar' },
env: expect.objectContaining({ FOO: 'bar' }),
stderr: 'pipe',
});
});
@@ -1393,6 +1411,7 @@ describe('mcp-client', () => {
},
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
@@ -1425,6 +1444,7 @@ describe('mcp-client', () => {
},
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
@@ -1456,6 +1476,7 @@ describe('mcp-client', () => {
},
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(StreamableHTTPClientTransport);
@@ -1476,6 +1497,7 @@ describe('mcp-client', () => {
},
},
false,
EMPTY_CONFIG,
);
expect(transport).toBeInstanceOf(SSEClientTransport);
@@ -1495,6 +1517,7 @@ describe('mcp-client', () => {
},
},
false,
EMPTY_CONFIG,
),
).rejects.toThrow(
'URL must be provided in the config for Google Credentials provider',
@@ -1656,6 +1679,7 @@ describe('connectToMcpServer with OAuth', () => {
{ httpUrl: serverUrl, oauth: { enabled: true } },
false,
workspaceContext,
EMPTY_CONFIG,
);
expect(client).toBe(mockedClient);
@@ -1700,6 +1724,7 @@ describe('connectToMcpServer with OAuth', () => {
{ httpUrl: serverUrl, oauth: { enabled: true } },
false,
workspaceContext,
EMPTY_CONFIG,
);
expect(client).toBe(mockedClient);
@@ -1754,6 +1779,7 @@ describe('connectToMcpServer - HTTP→SSE fallback', () => {
{ url: 'http://test-server', type: 'http' },
false,
workspaceContext,
EMPTY_CONFIG,
),
).rejects.toThrow('Connection failed');
@@ -1772,6 +1798,7 @@ describe('connectToMcpServer - HTTP→SSE fallback', () => {
{ url: 'http://test-server', type: 'sse' },
false,
workspaceContext,
EMPTY_CONFIG,
),
).rejects.toThrow('Connection failed');
@@ -1789,6 +1816,7 @@ describe('connectToMcpServer - HTTP→SSE fallback', () => {
{ url: 'http://test-server' },
false,
workspaceContext,
EMPTY_CONFIG,
);
expect(client).toBe(mockedClient);
@@ -1810,6 +1838,7 @@ describe('connectToMcpServer - HTTP→SSE fallback', () => {
{ url: 'http://test-server' },
false,
workspaceContext,
EMPTY_CONFIG,
),
).rejects.toThrow('Server error');
@@ -1826,6 +1855,7 @@ describe('connectToMcpServer - HTTP→SSE fallback', () => {
{ url: 'http://test-server' },
false,
workspaceContext,
EMPTY_CONFIG,
);
expect(client).toBe(mockedClient);
@@ -1895,6 +1925,7 @@ describe('connectToMcpServer - OAuth with transport fallback', () => {
{ url: 'http://test-server', oauth: { enabled: true } },
false,
workspaceContext,
EMPTY_CONFIG,
);
expect(client).toBe(mockedClient);
+10 -1
View File
@@ -60,6 +60,10 @@ import { debugLogger } from '../utils/debugLogger.js';
import type { MessageBus } from '../confirmation-bus/message-bus.js';
import { coreEvents } from '../utils/events.js';
import type { ResourceRegistry } from '../resources/resource-registry.js';
import {
sanitizeEnvironment,
type EnvironmentSanitizationConfig,
} from '../services/environmentSanitization.js';
export const MCP_DEFAULT_TIMEOUT_MSEC = 10 * 60 * 1000; // default to 10 minutes
@@ -137,6 +141,7 @@ export class McpClient {
this.serverConfig,
this.debugMode,
this.workspaceContext,
this.cliConfig.sanitizationConfig,
);
this.registerNotificationHandlers();
@@ -820,6 +825,7 @@ export async function connectAndDiscover(
mcpServerConfig,
debugMode,
workspaceContext,
cliConfig.sanitizationConfig,
);
mcpClient.onerror = (error) => {
@@ -1329,6 +1335,7 @@ export async function connectToMcpServer(
mcpServerConfig: MCPServerConfig,
debugMode: boolean,
workspaceContext: WorkspaceContext,
sanitizationConfig: EnvironmentSanitizationConfig,
): Promise<Client> {
const mcpClient = new Client(
{
@@ -1395,6 +1402,7 @@ export async function connectToMcpServer(
mcpServerName,
mcpServerConfig,
debugMode,
sanitizationConfig,
);
try {
await mcpClient.connect(transport, {
@@ -1711,6 +1719,7 @@ export async function createTransport(
mcpServerName: string,
mcpServerConfig: MCPServerConfig,
debugMode: boolean,
sanitizationConfig: EnvironmentSanitizationConfig,
): Promise<Transport> {
const noUrl = !mcpServerConfig.url && !mcpServerConfig.httpUrl;
if (noUrl) {
@@ -1782,7 +1791,7 @@ export async function createTransport(
command: mcpServerConfig.command,
args: mcpServerConfig.args || [],
env: {
...process.env,
...sanitizeEnvironment(process.env, sanitizationConfig),
...(mcpServerConfig.env || {}),
} as Record<string, string>,
cwd: mcpServerConfig.cwd,
+7 -1
View File
@@ -254,7 +254,13 @@ export class ShellToolInvocation extends BaseToolInvocation<
},
combinedController.signal,
this.config.getEnableInteractiveShell(),
{ ...shellExecutionConfig, pager: 'cat' },
{
...shellExecutionConfig,
pager: 'cat',
sanitizationConfig:
shellExecutionConfig?.sanitizationConfig ??
this.config.sanitizationConfig,
},
);
if (pid && setPidCallback) {