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'); + }); +});