From 2aaa415a253ca4f2cc9064f77ede0eee302e5b49 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 11:45:36 +0000 Subject: [PATCH] Treat stdio `env` values as secrets, like HTTP headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A config entry's `env` routinely holds API tokens — directly or via `${VAR}` substitution — but only `headers` were protected. Resolved `env` values were written to `sessions.json` in plaintext, printed verbatim by `mcpc --json @session` / `connect --json`, and passed to the bridge in its command line, where `ps` exposed them. They now follow the exact same path as headers: stored in the OS keychain (`session::env`), redacted to `` in `sessions.json` and all `--json` output, and delivered to the bridge over IPC after spawn. The bridge merges them back into the stdio transport config, so servers still get their environment unchanged. Sessions written before this change keep working — their on-disk plaintext is used as is; recreate a session to move its values into the keychain. Rebuilt on main after the draft branch was squashed in as #316. Fixes #341 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rmdd458q2Bx68bPxuUtQGa --- CHANGELOG.md | 4 + CLAUDE.md | 10 +- README.md | 3 +- docs/REFERENCE.md | 4 + src/bridge/index.ts | 22 +++ src/cli/commands/connect.ts | 32 ++-- src/cli/commands/sessions.ts | 19 +-- src/cli/help-text.ts | 13 +- src/lib/auth/keychain.ts | 31 ++++ src/lib/bridge-manager.ts | 96 +++++++++-- src/lib/sessions.ts | 22 ++- src/lib/types.ts | 6 +- src/lib/utils.ts | 31 +++- test/e2e/server/stdio-server.mjs | 41 ++++- test/e2e/suites/stdio/env-security.test.sh | 189 +++++++++++++++++++++ test/unit/lib/auth/keychain.test.ts | 25 +++ test/unit/lib/utils.test.ts | 55 ++++++ 17 files changed, 545 insertions(+), 58 deletions(-) create mode 100755 test/e2e/suites/stdio/env-security.test.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index bdb075ec..920a8134 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `mcpc help tools/list` and other MCP method names now show the command's help instead of failing with "Unknown command" — they already worked as aliases everywhere else. +### Security + +- A stdio server's `env` values are no longer stored in plaintext: they are kept in the OS keychain (like HTTP headers), shown as `` in `sessions.json` and `--json` output, and passed to the bridge over IPC so they never appear in the process list. Sessions created before this change keep working; recreate them to move their already-stored values out of `sessions.json`. + ## [0.6.0] - 2026-08-02 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index f84266d6..19d33874 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -276,7 +276,7 @@ Implements [MCP security best practices](https://modelcontextprotocol.io/specifi - Credentials stored in OS keychain (encrypted by system), with `0600` fallback file - No credentials logged even in verbose mode — only log presence/absence (e.g., `refreshToken: present`) -- Headers sent to bridge via IPC after socket connect, never as command-line arguments (visible in `ps`) +- Headers and stdio `env` values sent to bridge via IPC after socket connect, never as command-line arguments (visible in `ps`) - `sessions.json` and `profiles.json` file permissions: `0600` (user-only) **Transport security:** @@ -307,7 +307,7 @@ When making changes, follow these rules to maintain the security posture: - Always use `ensureDir()` for creating directories (defaults to `0700`); use `mode: 0o600` for files containing secrets - Use `execFile()` (array args) instead of `exec()` (shell string) when spawning processes - Escape any user-controlled or server-controlled data before embedding in HTML responses -- Send sensitive data (headers, tokens) via IPC socket, never via CLI arguments or environment variables +- Send sensitive data (headers, stdio `env` values, tokens) via IPC socket, never via CLI arguments or environment variables - Read all keychain values needed to start a bridge in the CLI **before** `spawn()`. After spawn the bridge arms a short IPC-credential timeout; on macOS a Keychain password dialog can block longer than that timeout, so a post-spawn keychain read races the bridge timer and causes ENOENT (#55). The CLI is the only process attached to a TTY and can show the dialog without the user wondering why a background process is asking. Bridge-side keychain access is permitted only on the OAuth token refresh paths (the `oauth-token-manager` callbacks and the id-jag provider callbacks in `src/bridge/index.ts`), where it is needed to persist rotated refresh tokens for long-running sessions - Validate and sanitize all external input (URLs, session names, profile names) before use - Default to HTTPS; only allow HTTP for localhost/127.0.0.1 @@ -468,7 +468,8 @@ Environment variable substitution supported: `${VAR_NAME}` - Bearer tokens passed via `--header "Authorization: Bearer ${TOKEN}"` are NOT stored as profiles - All session headers are stored in the OS keychain as one JSON blob per session (keychain account: `session::headers`) -- Bridge loads them automatically when making requests (delivered over IPC after spawn, never via argv) +- A stdio server's `env` values get the same treatment (keychain account: `session::env`) — config `env` routinely holds API tokens, directly or via `${VAR}` substitution +- Bridge loads both automatically when connecting (delivered over IPC after spawn, never via argv) **CLI Commands:** @@ -576,7 +577,8 @@ On failure, the error message includes instructions on how to login. This ensure // Account: auth-profile:mcp.apify.com:personal:tokens // Value: {"access_token": "...", "refresh_token": "...", "expires_at": ...} // Other accounts: auth-profile:::client (registered OAuth client), -// session::headers (per-session headers), session::proxy-bearer-token +// session::headers (per-session headers), session::env (stdio env vars), +// session::proxy-bearer-token ``` ## State and Data Storage diff --git a/README.md b/README.md index a14ce80e..a107ecde 100644 --- a/README.md +++ b/README.md @@ -1262,7 +1262,7 @@ For **stdio servers:** - `command` (required) - Command to execute (e.g., `node`, `npx`, `python`) - `args` (optional) - Array of command arguments -- `env` (optional) - Environment variables for the process +- `env` (optional) - Environment variables for the process (treated as secrets: stored in the OS keychain and shown as `` in session output) > **Note:** Stdio servers inherit only a minimal env whitelist from the shell > (`PATH`, `HOME`, `SHELL`, …). Other vars — `NODE_EXTRA_CA_CERTS`, `HTTPS_PROXY`, @@ -1341,6 +1341,7 @@ MCP enables arbitrary tool execution and data access - treat servers like you tr | ---------------------- | ----------------------------------------------- | | **OAuth tokens** | Stored in OS keychain (headless fallback: `credentials.json`, `0600`) | | **HTTP headers** | Stored in OS keychain per-session | +| **stdio `env` values** | Stored in OS keychain per-session | | **Bridge credentials** | Passed via Unix socket IPC, kept in memory only | | **Process arguments** | No secrets visible in `ps aux` | | **x402 private key** | Stored in OS keychain (fallback: `wallets.json`, `0600`) | diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index cd10ee8d..079727dd 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -162,6 +162,7 @@ JSON output (--json): `[{ protocolVersion?, supportedVersions?, capabilities?, serverInfo?, instructions?, _meta?, toolNames?, _mcpc: { ... } }]` Schema: https://modelcontextprotocol.io/specification/2025-11-25/schema#initializeresult https://modelcontextprotocol.io/specification/2026-07-28/schema#discoverresult + Secrets in `server` (`headers`, `env`) are always shown as "". ``` ## `mcpc close` @@ -196,6 +197,7 @@ JSON output (--json): `{ protocolVersion?, supportedVersions?, capabilities?, serverInfo?, instructions?, _meta?, toolNames?, _mcpc: { ... } }` Schema: https://modelcontextprotocol.io/specification/2025-11-25/schema#initializeresult https://modelcontextprotocol.io/specification/2026-07-28/schema#discoverresult + Secrets in `server` (`headers`, `env`) are always shown as "". ``` ## `mcpc login` @@ -510,6 +512,7 @@ JSON output (--json): `{ protocolVersion?, supportedVersions?, capabilities?, serverInfo?, instructions?, _meta?, toolNames?, _mcpc: { ... } }` Schema: https://modelcontextprotocol.io/specification/2025-11-25/schema#initializeresult https://modelcontextprotocol.io/specification/2026-07-28/schema#discoverresult + Secrets in `server` (`headers`, `env`) are always shown as "". ``` ### `mcpc @ close` @@ -544,6 +547,7 @@ JSON output (--json): `{ protocolVersion?, supportedVersions?, capabilities?, serverInfo?, instructions?, _meta?, toolNames?, _mcpc: { ... } }` Schema: https://modelcontextprotocol.io/specification/2025-11-25/schema#initializeresult https://modelcontextprotocol.io/specification/2026-07-28/schema#discoverresult + Secrets in `server` (`headers`, `env`) are always shown as "". ``` ### `mcpc @ grep` diff --git a/src/bridge/index.ts b/src/bridge/index.ts index 92ab2c33..3a0e3c94 100644 --- a/src/bridge/index.ts +++ b/src/bridge/index.ts @@ -131,6 +131,10 @@ class BridgeProcess { // HTTP headers (received via IPC, stored in memory only) private headers: Record | null = null; + // Environment variables for a stdio server (received via IPC, stored in memory only). + // Kept off the command line so their values never show up in `ps` output. + private serverEnv: Record | null = null; + // Bearer token the proxy server requires (received via IPC, stored in memory only). // Read by the CLI before spawn and sent over IPC — never read from the keychain here, // keeping the bridge's only keychain access on the OAuth-refresh path (see #55). @@ -211,6 +215,7 @@ class BridgeProcess { logger.debug(` clientSecret: ${credentials.clientSecret ? 'present' : 'absent'}`); logger.debug(` privateKey: ${credentials.privateKeyPem ? 'present' : 'absent'}`); logger.debug(` headers: ${credentials.headers ? Object.keys(credentials.headers).length : 0}`); + logger.debug(` env: ${credentials.env ? Object.keys(credentials.env).length : 0}`); logger.debug(` proxyBearerToken: ${credentials.proxyBearerToken ? 'present' : 'absent'}`); logger.debug(` idJag: ${credentials.idJag ? 'present' : 'absent'}`); @@ -357,6 +362,15 @@ class BridgeProcess { logger.debug(`Stored headers "${Object.keys(this.headers).join(', ')}" in memory`); } + // Store stdio env variables if provided (merged into the transport config on connect) + if (credentials.env) { + this.serverEnv = { + ...this.serverEnv, + ...credentials.env, + }; + logger.debug(`Stored env variables "${Object.keys(this.serverEnv).join(', ')}" in memory`); + } + // Store the proxy bearer token if provided (used by startProxyServer) if (credentials.proxyBearerToken) { this.proxyBearerToken = credentials.proxyBearerToken; @@ -658,6 +672,14 @@ class BridgeProcess { serverConfig = await this.updateTransportAuth(); } + // Restore the stdio server's env variables. They are stripped from the command-line + // config by the CLI and delivered over IPC instead, so their values (often API tokens) + // are never visible in `ps` output. + if (this.serverEnv && serverConfig.command) { + serverConfig.env = { ...serverConfig.env, ...this.serverEnv }; + logger.debug(`Added ${Object.keys(this.serverEnv).length} env variables to transport`); + } + logger.debug('Building MCP client config...'); logger.debug(` this.authProvider is set: ${!!this.authProvider}`); logger.debug(` this.x402Wallet is set: ${!!this.x402Wallet}`); diff --git a/src/cli/commands/connect.ts b/src/cli/commands/connect.ts index 5d133e8a..b1b467fe 100644 --- a/src/cli/commands/connect.ts +++ b/src/cli/commands/connect.ts @@ -14,7 +14,8 @@ import { generateSessionName, normalizeServerUrl, validateProfileName, - redactHeaders, + redactValues, + redactServerConfigSecrets, AuthError, ClientError, isAuthenticationError, @@ -47,6 +48,7 @@ import { import { startBridge, StartBridgeOptions, stopBridge } from '../../lib/bridge-manager.js'; import { storeKeychainSessionHeaders, + storeKeychainSessionEnv, storeKeychainProxyBearerToken, } from '../../lib/auth/keychain.js'; import { getWallet } from '../../lib/wallets.js'; @@ -152,7 +154,7 @@ type ConnectSessionOptions = { /** * Connect to a session via the bridge and build a populated ConnectResultEntry from its - * server details and tools list. The entry's `_mcpc.server` headers are redacted. + * server details and tools list. The entry's `_mcpc.server` headers and env are redacted. */ async function buildConnectResultEntry( sessionName: string, @@ -177,12 +179,7 @@ async function buildConnectResultEntry( const tools = (await client.listAllTools()).tools; const server: ServerConfig | undefined = context.serverConfig - ? { - ...context.serverConfig, - ...(context.serverConfig.headers && { - headers: redactHeaders(context.serverConfig.headers), - }), - } + ? redactServerConfigSecrets(context.serverConfig) : undefined; return { @@ -374,6 +371,17 @@ export async function connectSession( await storeKeychainSessionHeaders(name, headers); } + // Same for a stdio server's env variables: they routinely hold API tokens (directly or + // via `${VAR}` substitution in the config file), so they go to the keychain too + let env: Record | undefined; + if (serverConfig.env && Object.keys(serverConfig.env).length > 0) { + env = { ...serverConfig.env }; + logger.debug( + `Storing ${Object.keys(env).length} env variables for session ${name} in keychain` + ); + await storeKeychainSessionEnv(name, env); + } + // Store proxy bearer token in keychain (if provided) if (options.proxyBearerToken) { logger.debug(`Storing proxy bearer token for session ${name} in keychain`); @@ -390,12 +398,13 @@ export async function connectSession( } // Create or update session record (without pid - that comes from startBridge) - // Store serverConfig with headers redacted (actual values in keychain) + // Store serverConfig with headers and env redacted (actual values in keychain) const isReconnect = !!existingSession; - const { headers: _originalHeaders, ...baseTransportConfig } = serverConfig; + const { headers: _originalHeaders, env: _originalEnv, ...baseTransportConfig } = serverConfig; const sessionTransportConfig: ServerConfig = { ...baseTransportConfig, - ...(headers && { headers: redactHeaders(headers) }), + ...(headers && { headers: redactValues(headers) }), + ...(env && { env: redactValues(env) }), }; const sessionUpdate: Parameters[1] = { @@ -429,6 +438,7 @@ export async function connectSession( serverConfig, verbose: options.verbose || false, ...(headers && { headers }), + ...(env && { env }), ...(profileName && { profileName }), ...(proxyConfig && { proxyConfig }), ...(options.x402 && { x402: options.x402 }), diff --git a/src/cli/commands/sessions.ts b/src/cli/commands/sessions.ts index 96cd022e..cbbcd69b 100644 --- a/src/cli/commands/sessions.ts +++ b/src/cli/commands/sessions.ts @@ -10,7 +10,7 @@ import { OutputMode, isProcessAlive, getServerHost, - redactHeaders, + redactServerConfigSecrets, ClientError, } from '../../lib/index.js'; import { DISCONNECTED_THRESHOLD_MILLIS } from '../../lib/types.js'; @@ -37,6 +37,7 @@ import { StartBridgeOptions, stopBridge, reconnectCrashedSessions, + resolveSessionEnv, } from '../../lib/bridge-manager.js'; import chalk from 'chalk'; import { createLogger } from '../../lib/logger.js'; @@ -326,13 +327,8 @@ export async function showServerDetails( // 2026-07-28 ones. `ServerDetails` reconciles the two — see its doc comment. // https://modelcontextprotocol.io/specification/2025-11-25/schema#initializeresult // https://modelcontextprotocol.io/specification/2026-07-28/schema#discoverresult - // Build _mcpc.server with redacted headers for security - const server: ServerConfig = { - ...context.serverConfig, - ...(context.serverConfig?.headers && { - headers: redactHeaders(context.serverConfig.headers), - }), - }; + // Build _mcpc.server with redacted headers and env values for security + const server: ServerConfig = redactServerConfigSecrets({ ...context.serverConfig }); // The bridge log path is useful debug context for callers — only meaningful for // session targets (those starting with "@"); ad-hoc URL/config targets have no @@ -406,9 +402,11 @@ export async function restartSession( throw new ClientError(`Session ${name} has no server configuration`); } - // Load headers from keychain if present + // Load headers and stdio env variables from keychain if present. The copies in + // sessions.json only carry key names — their values are redacted. const { readKeychainSessionHeaders } = await import('../../lib/auth/keychain.js'); const headers = await readKeychainSessionHeaders(name); + const env = await resolveSessionEnv(name, serverConfig.env); // Resolve auth profile: use stored profile, or auto-detect a "default" profile. // This handles the case where user creates a session without auth, then later runs @@ -432,9 +430,10 @@ export async function restartSession( // the session ID, the session is marked as expired. const bridgeOptions: StartBridgeOptions = { sessionName: name, - serverConfig: { ...serverConfig, ...(headers && { headers }) }, + serverConfig: { ...serverConfig, ...(headers && { headers }), ...(env && { env }) }, verbose: options.verbose || false, ...(headers && { headers }), + ...(env && { env }), ...(profileName && { profileName }), ...(session.proxy && { proxyConfig: session.proxy }), ...(session.x402 && { x402: session.x402 }), diff --git a/src/cli/help-text.ts b/src/cli/help-text.ts index 939300d1..d21f2246 100644 --- a/src/cli/help-text.ts +++ b/src/cli/help-text.ts @@ -42,6 +42,13 @@ const SERVER_DETAILS_SCHEMA_URLS = [ `${SCHEMA_BASE}#discoverresult`, ]; +/** + * Stated with the shape so an agent reading the output never mistakes the redaction + * sentinel for a real credential: the values live in the OS keychain, not here. + */ +const SERVER_DETAILS_SECRETS_NOTE = + 'Secrets in `server` (`headers`, `env`) are always shown as "".'; + /** * Standard "JSON output (--json):" block for every command that prints server details: * `connect` (an array of entries), `restart` (the restarted session), and the `mcpc @@ -57,7 +64,11 @@ export function serverDetailsJsonHelp(returns: 'object' | 'array'): string { : '`InitializeResult` or `DiscoverResult` object'; const shape = returns === 'array' ? `\`[${SERVER_DETAILS_JSON_SHAPE}]\`` : `\`${SERVER_DETAILS_JSON_SHAPE}\``; - return jsonHelp(`${subject} ${SERVER_DETAILS_JSON_META}`, shape, SERVER_DETAILS_SCHEMA_URLS); + return `${jsonHelp( + `${subject} ${SERVER_DETAILS_JSON_META}`, + shape, + SERVER_DETAILS_SCHEMA_URLS + )} ${SERVER_DETAILS_SECRETS_NOTE}\n`; } /** diff --git a/src/lib/auth/keychain.ts b/src/lib/auth/keychain.ts index 53f30086..107590b1 100644 --- a/src/lib/auth/keychain.ts +++ b/src/lib/auth/keychain.ts @@ -233,6 +233,8 @@ const oauthIdJagAccount = (serverUrl: string, profileName: string): string => const sessionHeadersAccount = (sessionName: string): string => `session:${sessionName}:headers`; +const sessionEnvAccount = (sessionName: string): string => `session:${sessionName}:env`; + const proxyBearerTokenAccount = (sessionName: string): string => `session:${sessionName}:proxy-bearer-token`; @@ -418,6 +420,35 @@ export async function removeKeychainSessionHeaders(sessionName: string): Promise return keychainDelete(sessionHeadersAccount(sessionName)); } +/** + * Store stdio environment variables for a session. Treated as secrets: config `env` + * values commonly hold API tokens (directly or via `${VAR}` substitution). + */ +export async function storeKeychainSessionEnv( + sessionName: string, + env: Record +): Promise { + logger.debug(`Storing env variables for session ${sessionName}`); + await keychainSet(sessionEnvAccount(sessionName), JSON.stringify(env)); +} + +/** Read stdio environment variables for a session. */ +export async function readKeychainSessionEnv( + sessionName: string +): Promise | undefined> { + logger.debug(`Retrieving env variables for session ${sessionName}`); + return keychainGetParsed>( + sessionEnvAccount(sessionName), + 'session env variables' + ); +} + +/** Delete stdio environment variables for a session. */ +export async function removeKeychainSessionEnv(sessionName: string): Promise { + logger.debug(`Deleting env variables for session ${sessionName}`); + return keychainDelete(sessionEnvAccount(sessionName)); +} + /** Store the bearer token used to authenticate requests to the proxy server. */ export async function storeKeychainProxyBearerToken( sessionName: string, diff --git a/src/lib/bridge-manager.ts b/src/lib/bridge-manager.ts index 1497ceaf..c3948f52 100644 --- a/src/lib/bridge-manager.ts +++ b/src/lib/bridge-manager.ts @@ -29,6 +29,7 @@ import { invalidateProcessAliveCache, isSessionExpiredError, enrichErrorMessage, + REDACTED_VALUE, } from './utils.js'; import { updateSession, getSession } from './sessions.js'; import { createLogger } from './logger.js'; @@ -45,6 +46,7 @@ import { readKeychainClientCredentials, readKeychainIdJagCredentials, readKeychainSessionHeaders, + readKeychainSessionEnv, readKeychainProxyBearerToken, } from './auth/keychain.js'; import { getAuthProfile } from './auth/profiles.js'; @@ -115,6 +117,7 @@ export interface StartBridgeOptions { verbose?: boolean; profileName?: string; // Auth profile name for token refresh headers?: Record; // Headers to send via IPC (caller stores in keychain) + env?: Record; // stdio env vars to send via IPC (caller stores in keychain) proxyConfig?: ProxyConfig; // Proxy server configuration mcpSessionId?: string; // MCP session ID for resumption (Streamable HTTP only) protocolVersion?: string; // Protocol version negotiated by the resumed session (only pass with mcpSessionId) @@ -131,9 +134,9 @@ export interface StartBridgeResult { * Start a bridge process for a session * Spawns the bridge process and sends auth credentials via IPC * - * SECURITY: All headers are treated as potentially sensitive: - * 1. Caller stores headers in OS keychain before calling this function - * 2. Headers are sent to bridge via IPC after startup + * SECURITY: All headers and stdio env variables are treated as potentially sensitive: + * 1. Caller stores them in the OS keychain before calling this function + * 2. They are sent to the bridge via IPC after startup * 3. Never exposed in process listings * * NOTE: This function does NOT manage session storage. The caller is responsible for: @@ -149,6 +152,7 @@ export async function startBridge(options: StartBridgeOptions): Promise 0) || proxyBearerToken) { + } else if ( + (headers && Object.keys(headers).length > 0) || + (env && Object.keys(env).length > 0) || + proxyBearerToken + ) { args.push('--profile', 'dummy'); } @@ -453,12 +464,43 @@ async function waitForProcessExit(pid: number, timeoutMillis: number): Promise` and the real ones + * live in the OS keychain. Sessions written before env was treated as a secret still hold + * plaintext values on disk; those are used as-is so an upgrade never breaks a live session + * (recreate the session to move them into the keychain). + */ +export async function resolveSessionEnv( + sessionName: string, + storedEnv: Record | undefined +): Promise | undefined> { + if (!storedEnv || Object.keys(storedEnv).length === 0) return undefined; + + // Keychain values win; a legacy plaintext value is used as-is, and a `` + // placeholder is never passed to the server as if it were the real value. + const env = { + ...(await readKeychainSessionEnv(sessionName)), + ...Object.fromEntries(Object.entries(storedEnv).filter(([, v]) => v !== REDACTED_VALUE)), + }; + + const missingKeys = Object.keys(storedEnv).filter((key) => !(key in env)); + if (missingKeys.length > 0) { + throw new ClientError( + `Missing env variable(s) in keychain for session ${sessionName}: ${missingKeys.join(', ')}. ` + + `The session may need to be recreated with "mcpc ${sessionName} close" followed by a new connect.` + ); + } + return env; +} + /** * Restart a bridge process for a session * Used for automatic recovery when connection to bridge fails * - * Headers persist in keychain across bridge restarts, so they are - * retrieved here and passed to startBridge() which sends them via IPC. + * Headers and stdio env variables persist in the keychain across bridge restarts, + * so they are retrieved here and passed to startBridge() which sends them via IPC. */ export async function restartBridge(sessionName: string): Promise { logger.debug(`Trying to restart bridge for ${sessionName}...`); @@ -476,9 +518,11 @@ export async function restartBridge(sessionName: string): Promise | undefined; @@ -496,6 +540,9 @@ export async function restartBridge(sessionName: string): Promise, - proxyBearerToken?: string + proxyBearerToken?: string, + env?: Record ): Promise { // Build credentials object const credentials: AuthCredentials = { @@ -624,6 +675,13 @@ async function loadAuthCredentials( logger.debug(`Including ${Object.keys(headers).length} headers in credentials`); } + // Add stdio env variables if provided — they reach the bridge over IPC so their + // values stay out of the bridge's command line + if (env) { + credentials.env = env; + logger.debug(`Including ${Object.keys(env).length} env variables in credentials`); + } + // Add the proxy bearer token if provided, so the bridge configures its proxy // server's auth from the IPC credentials instead of reading the keychain. if (proxyBearerToken) { @@ -649,8 +707,12 @@ async function sendAuthCredentialsToBridge( (credentials.refreshToken ? ' (with refresh token)' : '') + (credentials.accessToken ? ' (with access token)' : '') + (credentials.headers ? ` (with ${Object.keys(credentials.headers).length} headers)` : '') + - (!credentials.refreshToken && !credentials.accessToken && !credentials.headers - ? ' (minimal - no tokens or headers)' + (credentials.env ? ` (with ${Object.keys(credentials.env).length} env variables)` : '') + + (!credentials.refreshToken && + !credentials.accessToken && + !credentials.headers && + !credentials.env + ? ' (minimal - no tokens, headers or env variables)' : '') ); diff --git a/src/lib/sessions.ts b/src/lib/sessions.ts index 7c1a35e3..8af1e2f5 100644 --- a/src/lib/sessions.ts +++ b/src/lib/sessions.ts @@ -19,7 +19,11 @@ import { import { withFileLock } from './file-lock.js'; import { createLogger } from './logger.js'; import { ClientError } from './errors.js'; -import { removeKeychainSessionHeaders, removeKeychainProxyBearerToken } from './auth/keychain.js'; +import { + removeKeychainSessionHeaders, + removeKeychainSessionEnv, + removeKeychainProxyBearerToken, +} from './auth/keychain.js'; const logger = createLogger('sessions'); @@ -239,6 +243,14 @@ export async function deleteSession(sessionName: string): Promise { // Ignore errors - headers may not exist } + // Delete stdio env variables from keychain (if any) + try { + await removeKeychainSessionEnv(sessionName); + logger.debug(`Deleted env variables from keychain for session: ${sessionName}`); + } catch { + // Ignore errors - env variables may not exist + } + // Delete proxy bearer token from keychain (if any) try { await removeKeychainProxyBearerToken(sessionName); @@ -324,6 +336,14 @@ export async function consolidateSessions( // Ignore errors - headers may not exist } + // Delete stdio env variables from keychain (if any) + try { + await removeKeychainSessionEnv(name); + logger.debug(`Deleted env variables from keychain for session: ${name}`); + } catch { + // Ignore errors - env variables may not exist + } + // Delete proxy bearer token from keychain (if any) try { await removeKeychainProxyBearerToken(name); diff --git a/src/lib/types.ts b/src/lib/types.ts index 70430921..418f0aff 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -172,7 +172,7 @@ export interface SessionNotifications { */ export interface SessionData { name: string; - server: ServerConfig; // Transport configuration (header values redacted to "") + server: ServerConfig; // Transport configuration (header and env values redacted to "") profileName?: string; // Name of auth profile (for OAuth servers) /** * x402 auto-payment scheme preference. Presence enables x402 for the session; @@ -381,6 +381,10 @@ export interface AuthCredentials { idJag?: IdJagCredentials; // HTTP headers (from --header flags, stored in keychain) headers?: Record; + // Environment variables for a stdio server (from the config entry's `env`, stored in + // keychain). Sent over IPC rather than on the bridge's command line so resolved + // secrets never show up in `ps` output — same treatment as headers. + env?: Record; // Bearer token the bridge's proxy server requires (from --proxy-bearer-token). // Read by the CLI before spawn and delivered via IPC so the bridge never reads it // from the keychain itself — keeping the bridge's only keychain access on the diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 9afb4bcf..3ea4bd16 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -9,6 +9,7 @@ import { homedir, tmpdir } from 'os'; import { join, resolve, isAbsolute } from 'path'; import { mkdir, access, constants, rename, lstat, chmod } from 'fs/promises'; import { ClientError, ServerError } from './errors.js'; +import type { ServerConfig } from './types.js'; /** * Safety cap on pages fetched by fetchAllPages(). Generous — real servers @@ -607,23 +608,37 @@ export function generateRequestId(): string { } /** - * Sentinel value used to replace sensitive header values when storing in sessions.json + * Sentinel value used to replace sensitive values when storing in sessions.json */ -export const REDACTED_HEADER_VALUE = ''; +export const REDACTED_VALUE = ''; /** - * Redact header values for secure storage - * Replaces all header values with "" sentinel + * Redact a record of sensitive values for secure storage + * Replaces all values with the "" sentinel, keeping the keys */ -export function redactHeaders(headers: Record): Record { - if (Object.keys(headers).length === 0) return headers; +export function redactValues(record: Record): Record { + if (Object.keys(record).length === 0) return record; const redacted: Record = {}; - for (const key of Object.keys(headers)) { - redacted[key] = REDACTED_HEADER_VALUE; + for (const key of Object.keys(record)) { + redacted[key] = REDACTED_VALUE; } return redacted; } +/** + * Redact every secret-bearing part of a server config: HTTP `headers` and stdio `env`. + * Both routinely carry credentials — `env` values come from `${VAR}` substitution in + * mcp.json — so neither is ever written to sessions.json or printed in `--json` output. + * The real values live in the OS keychain and reach the bridge over IPC. + */ +export function redactServerConfigSecrets(config: T): T { + return { + ...config, + ...(config.headers && { headers: redactValues(config.headers) }), + ...(config.env && { env: redactValues(config.env) }), + }; +} + /** * Check if an error message indicates MCP session expiration. * Used to detect when a server has invalidated a session so it can be marked as expired. diff --git a/test/e2e/server/stdio-server.mjs b/test/e2e/server/stdio-server.mjs index 22b04695..8cf3f938 100644 --- a/test/e2e/server/stdio-server.mjs +++ b/test/e2e/server/stdio-server.mjs @@ -1,10 +1,43 @@ #!/usr/bin/env node // Minimal stdio MCP server used by e2e tests to create a live stdio session // without any network access (the official @modelcontextprotocol/sdk is a local -// dependency). It only needs to complete the MCP initialize handshake so the -// bridge reports the session as "live". -import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; +// dependency). It completes the MCP initialize handshake so the bridge reports +// the session as "live", and exposes a single `echo-env` tool that returns the +// value of one of its own environment variables — used by the env-security +// suite to prove that a config entry's `env` still reaches the server process +// even though its values are kept out of sessions.json and the process list. +import { Server } from '@modelcontextprotocol/sdk/server/index.js'; import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; +import { + CallToolRequestSchema, + ListToolsRequestSchema, +} from '@modelcontextprotocol/sdk/types.js'; + +const server = new Server( + { name: 'e2e-stdio', version: '1.0.0' }, + { capabilities: { tools: {} } } +); + +server.setRequestHandler(ListToolsRequestSchema, async () => ({ + tools: [ + { + name: 'echo-env', + description: 'Return the value of one of the server process environment variables', + inputSchema: { + type: 'object', + properties: { name: { type: 'string', description: 'Environment variable name' } }, + required: ['name'], + }, + }, + ], +})); + +server.setRequestHandler(CallToolRequestSchema, async (request) => { + if (request.params.name !== 'echo-env') { + throw new Error(`Unknown tool: ${request.params.name}`); + } + const name = String(request.params.arguments?.name ?? ''); + return { content: [{ type: 'text', text: process.env[name] ?? '' }] }; +}); -const server = new McpServer({ name: 'e2e-stdio', version: '1.0.0' }); await server.connect(new StdioServerTransport()); diff --git a/test/e2e/suites/stdio/env-security.test.sh b/test/e2e/suites/stdio/env-security.test.sh new file mode 100755 index 00000000..ece096f7 --- /dev/null +++ b/test/e2e/suites/stdio/env-security.test.sh @@ -0,0 +1,189 @@ +#!/bin/bash +# Test: stdio env security (no leak in process list, sessions.json, or --json output) +# A config entry's `env` routinely holds API tokens — directly or via ${VAR} +# substitution — so its values get the same treatment as HTTP headers: keychain +# storage, IPC delivery to the bridge, and "" everywhere else (#341). + +source "$(dirname "$0")/../../lib/framework.sh" +test_init "stdio/env-security" --isolated + +# The secret is injected into the config via ${FAKE_SECRET_TOKEN} substitution, +# exactly like the report in #341. +export FAKE_SECRET_TOKEN="sk-stdio-env-secret-$(date +%s)" + +STDIO_SERVER="$(to_native_path "$PROJECT_ROOT/test/e2e/server/stdio-server.mjs")" +CONFIG_FILE="$(to_native_path "$TEST_TMP/env-security-config.json")" +cat > "$CONFIG_FILE" </dev/null 2>&1" +test_pass + +test_case "env values still reach the stdio server process" +run_mcpc "$SESSION" tools-call echo-env "name:=SECRET_TOKEN" +assert_success +assert_contains "$STDOUT" "$FAKE_SECRET_TOKEN" +test_pass + +# ============================================================================= +# Test: the secret is not exposed anywhere it can be read back +# ============================================================================= + +test_case "secret env value not visible in ps output" +ps_output=$(ps aux 2>/dev/null || ps -ef 2>/dev/null || echo "") +if echo "$ps_output" | grep -q "$FAKE_SECRET_TOKEN"; then + test_fail "Secret env value found in the process list! It must be sent over IPC, not argv." +fi +test_pass + +test_case "env values are redacted in sessions.json" +sessions_file="$MCPC_HOME_DIR/sessions.json" +assert_file_exists "$sessions_file" +sessions_content=$(cat "$sessions_file") + +if echo "$sessions_content" | grep -q "$FAKE_SECRET_TOKEN"; then + test_fail "Secret env value found in sessions.json! Env values must be redacted." +fi + +# The key names are kept (they are needed on restart), the values are not +stored_env=$(jq -r ".sessions[\"$SESSION\"].server.env.SECRET_TOKEN" "$sessions_file") +if [[ "$stored_env" != "" ]]; then + test_fail "Expected for env.SECRET_TOKEN in sessions.json, got: $stored_env" +fi +test_pass + +test_case "session info --json redacts env values" +run_mcpc --json "$SESSION" +assert_success +assert_json_valid "$STDOUT" +if echo "$STDOUT" | grep -q "$FAKE_SECRET_TOKEN"; then + test_fail "Secret env value found in 'mcpc --json @session' output!" +fi +json_env=$(json_get "._mcpc.server.env.SECRET_TOKEN") +if [[ "$json_env" != "" ]]; then + test_fail "Expected for _mcpc.server.env.SECRET_TOKEN, got: $json_env" +fi +test_pass + +test_case "session list --json redacts env values" +run_mcpc --json +assert_success +assert_json_valid "$STDOUT" +if echo "$STDOUT" | grep -q "$FAKE_SECRET_TOKEN"; then + test_fail "Secret env value found in 'mcpc --json' session list!" +fi +test_pass + +test_case "bridge log doesn't leak env values" +BRIDGE_LOG="$MCPC_HOME_DIR/logs/bridge-$SESSION.log" +if [[ -f "$BRIDGE_LOG" ]]; then + if grep -q "$FAKE_SECRET_TOKEN" "$BRIDGE_LOG"; then + test_fail "Secret env value found in the bridge log!" + fi +fi +test_pass + +test_case "verbose output doesn't leak env values" +run_mcpc --verbose "$SESSION" ping +assert_success +if echo "$STDOUT$STDERR" | grep -q "$FAKE_SECRET_TOKEN"; then + test_fail "Secret env value found in verbose output!" +fi +test_pass + +# ============================================================================= +# Test: env survives a restart (restored from the keychain, not sessions.json) +# ============================================================================= + +test_case "env values survive an explicit restart" +run_mcpc "$SESSION" restart +assert_success +wait_for "$MCPC $SESSION ping >/dev/null 2>&1" + +run_mcpc "$SESSION" tools-call echo-env "name:=SECRET_TOKEN" +assert_success +assert_contains "$STDOUT" "$FAKE_SECRET_TOKEN" +test_pass + +test_case "env values survive a bridge crash" +run_mcpc --json +bridge_pid=$(json_get ".sessions[] | select(.name == \"$SESSION\") | .pid") +if [[ -n "$bridge_pid" && "$bridge_pid" != "null" ]]; then + _kill_tree "$bridge_pid" + wait_for "! kill -0 $bridge_pid 2>/dev/null" 10 || true + + run_mcpc "$SESSION" tools-call echo-env "name:=SECRET_TOKEN" + assert_success + assert_contains "$STDOUT" "$FAKE_SECRET_TOKEN" +fi +test_pass + +test_case "cleanup: close session" +run_mcpc "$SESSION" close +assert_success +_SESSIONS_CREATED=("${_SESSIONS_CREATED[@]/$SESSION}") +test_pass + +# ============================================================================= +# Test: a session written before env was a secret (plaintext values in +# sessions.json) keeps working — the upgrade never breaks it +# ============================================================================= + +test_case "legacy session with plaintext env still works after restart" +LEGACY_SESSION=$(session_name "env-old") +LEGACY_SECRET="sk-legacy-env-secret-$(date +%s)" + +run_mcpc connect "$CONFIG_FILE:env-secret" "$LEGACY_SESSION" +assert_success +_SESSIONS_CREATED+=("$LEGACY_SESSION") +wait_for "$MCPC $LEGACY_SESSION ping >/dev/null 2>&1" + +# Simulate the pre-fix on-disk state: plaintext env values in sessions.json. +# The keychain entry is left in place but overridden by the plaintext copy, +# which is what an upgraded session looks like from the CLI's point of view. +sessions_file="$MCPC_HOME_DIR/sessions.json" +tmp_sessions="$TEST_TMP/sessions-legacy.json" +jq --arg s "$LEGACY_SESSION" --arg v "$LEGACY_SECRET" \ + '.sessions[$s].server.env.SECRET_TOKEN = $v' "$sessions_file" > "$tmp_sessions" +mv "$tmp_sessions" "$sessions_file" + +run_mcpc "$LEGACY_SESSION" restart +assert_success +wait_for "$MCPC $LEGACY_SESSION ping >/dev/null 2>&1" + +# The plaintext value is honoured (not dropped, not passed through as "") +run_mcpc "$LEGACY_SESSION" tools-call echo-env "name:=SECRET_TOKEN" +assert_success +assert_contains "$STDOUT" "$LEGACY_SECRET" +test_pass + +test_case "cleanup: close legacy session" +run_mcpc "$LEGACY_SESSION" close +assert_success +_SESSIONS_CREATED=("${_SESSIONS_CREATED[@]/$LEGACY_SESSION}") +test_pass + +unset FAKE_SECRET_TOKEN +test_done diff --git a/test/unit/lib/auth/keychain.test.ts b/test/unit/lib/auth/keychain.test.ts index 32cf7b49..81fc9ac8 100644 --- a/test/unit/lib/auth/keychain.test.ts +++ b/test/unit/lib/auth/keychain.test.ts @@ -135,6 +135,31 @@ describe('OS keychain available', () => { expect(await readKeychainSessionHeaders('s')).toEqual(headers); }); + it('stores and retrieves session env variables', async () => { + const { storeKeychainSessionEnv, readKeychainSessionEnv } = await loadKeychain(); + + const env = { SECRET_TOKEN: 'sk_totally_fake_12345', DEBUG: 'mcp:*' }; + await storeKeychainSessionEnv('s', env); + expect(await readKeychainSessionEnv('s')).toEqual(env); + }); + + it('keeps session env variables separate from session headers', async () => { + const { + storeKeychainSessionEnv, + storeKeychainSessionHeaders, + readKeychainSessionEnv, + readKeychainSessionHeaders, + removeKeychainSessionEnv, + } = await loadKeychain(); + + await storeKeychainSessionHeaders('s', { Authorization: 'Bearer tok' }); + await storeKeychainSessionEnv('s', { SECRET_TOKEN: 'env-secret' }); + + expect(await removeKeychainSessionEnv('s')).toBe(true); + expect(await readKeychainSessionEnv('s')).toBeUndefined(); + expect(await readKeychainSessionHeaders('s')).toEqual({ Authorization: 'Bearer tok' }); + }); + it('stores and retrieves proxy bearer token', async () => { const { storeKeychainProxyBearerToken, readKeychainProxyBearerToken } = await loadKeychain(); diff --git a/test/unit/lib/utils.test.ts b/test/unit/lib/utils.test.ts index fd11cc41..fed6dbbd 100644 --- a/test/unit/lib/utils.test.ts +++ b/test/unit/lib/utils.test.ts @@ -28,6 +28,9 @@ import { isProcessAlive, generateRequestId, fetchAllPages, + redactValues, + redactServerConfigSecrets, + REDACTED_VALUE, } from '../../../src/lib/utils.js'; import { ServerError } from '../../../src/lib/errors.js'; import { DEFAULT_AUTH_PROFILE } from '../../../src/lib/auth/oauth-utils.js'; @@ -656,3 +659,55 @@ describe('fetchAllPages', () => { ).rejects.toThrow(/pagination cursor/); }); }); + +describe('redactValues', () => { + it('replaces every value with the redaction sentinel, keeping the keys', () => { + expect(redactValues({ Authorization: 'Bearer secret', 'X-Api-Key': 'abc' })).toEqual({ + Authorization: REDACTED_VALUE, + 'X-Api-Key': REDACTED_VALUE, + }); + }); + + it('returns an empty record unchanged', () => { + expect(redactValues({})).toEqual({}); + }); +}); + +describe('redactServerConfigSecrets', () => { + it('redacts HTTP header values', () => { + const config = redactServerConfigSecrets({ + url: 'https://mcp.example.com', + headers: { Authorization: 'Bearer secret' }, + }); + expect(config).toEqual({ + url: 'https://mcp.example.com', + headers: { Authorization: REDACTED_VALUE }, + }); + }); + + it('redacts stdio env values (they hold API tokens via ${VAR} substitution)', () => { + const config = redactServerConfigSecrets({ + command: 'node', + args: ['server.js'], + env: { SECRET_TOKEN: 'sk_totally_fake_12345', DEBUG: 'mcp:*' }, + }); + expect(config).toEqual({ + command: 'node', + args: ['server.js'], + env: { SECRET_TOKEN: REDACTED_VALUE, DEBUG: REDACTED_VALUE }, + }); + }); + + it('leaves non-secret fields untouched and omits absent ones', () => { + expect(redactServerConfigSecrets({ url: 'https://mcp.example.com', timeout: 30 })).toEqual({ + url: 'https://mcp.example.com', + timeout: 30, + }); + }); + + it('does not mutate the input config', () => { + const original = { command: 'node', env: { SECRET: 'value' } }; + redactServerConfigSecrets(original); + expect(original.env.SECRET).toBe('value'); + }); +});