diff --git a/docs/users/configuration/settings.md b/docs/users/configuration/settings.md index 9ef989b567b..09b93182150 100644 --- a/docs/users/configuration/settings.md +++ b/docs/users/configuration/settings.md @@ -724,7 +724,7 @@ For authentication-related variables (like `OPENAI_*`) and the recommended `.qwe > [!warning] > -> **Loader-affecting variables are always rejected:** Variables that make a spawned Node.js process or OS loader execute an attacker-chosen file — `NODE_OPTIONS`, `npm_config_node_options` (and npm's config-file redirects `npm_config_userconfig`, `npm_config_globalconfig`, `npm_config_script_shell`, `npm_config_prefix`), `NODE_PATH`, `LD_PRELOAD`, `LD_AUDIT`, `DYLD_INSERT_LIBRARIES`, `BASH_ENV`, `ZDOTDIR`, and exported bash function definitions (`BASH_FUNC_*`) — are never loaded from `.env` files (any scope, including `.qwen/.env` and user-level files) or from the top-level `settings.json` `env` section. A workspace-controlled value there could hijack module resolution for every subprocess Qwen Code spawns, so Qwen Code prints a warning when it rejects such a key (once per process, per key and source — in a multi-workspace daemon each workspace's rejection is reported separately). To use one of these variables, export it in the environment you launch Qwen Code from; sessions hosted by a `qwen serve` daemon deliberately do not inherit them, while direct editor (ACP) sessions and the plain CLI keep the exported value. Library _search_ paths (`LD_LIBRARY_PATH`, `DYLD_LIBRARY_PATH`) and the interactive-shell-only `ENV` are intentionally not on this list — rejecting them breaks mainstream toolchains (`ENV=production`, conda/CUDA library dirs) — but a project `.env` still cannot apply them on reload. This rejection applies to the top-level `env` section only: per-server `mcpServers[].env` and per-hook `hooks[].env` are intentionally scoped to that server or hook and still apply (both surfaces are gated by folder trust for workspace-provided configs). Separately, a project `.env` can never set `QWEN_CLI_ENTRY` (the daemon's session-process entrypoint), `NODE_EXTRA_CA_CERTS`, or `DEV` (the dev-harness launch marker); those stay settable from the shell environment or a user-level `.env`. Upgrade note: before this denylist existed, some of these keys could load from `.env` files or `settings.json` `env` on some paths; they are now rejected everywhere with a warning, and a `qwen serve` daemon no longer passes inherited values of them to session subprocesses. +> **Loader-affecting variables are always rejected:** Variables that make a spawned Node.js process or OS loader execute an attacker-chosen file — `NODE_OPTIONS`, `npm_config_node_options` (and npm's config-file redirects `npm_config_userconfig`, `npm_config_globalconfig`, `npm_config_script_shell`, `npm_config_prefix`), `NODE_PATH`, `OPENSSL_CONF` (dlopens an attacker OpenSSL engine at startup), `NODE_REPL_EXTERNAL_MODULE`, `npm_config_node_gyp`, `npm_config_init_module`, `LD_PRELOAD`, `LD_AUDIT`, `DYLD_INSERT_LIBRARIES`, `BASH_ENV`, `ZDOTDIR`, and exported bash function definitions (`BASH_FUNC_*`) — are never loaded from `.env` files (any scope, including `.qwen/.env` and user-level files) or from the top-level `settings.json` `env` section. A workspace-controlled value there could hijack module resolution for every subprocess Qwen Code spawns, so Qwen Code prints a warning when it rejects such a key (once per process, per key and source — in a multi-workspace daemon each workspace's rejection is reported separately). To use one of these variables, export it in the environment you launch Qwen Code from; sessions hosted by a `qwen serve` daemon deliberately do not inherit them, while direct editor (ACP) sessions and the plain CLI keep the exported value. Library _search_ paths (`LD_LIBRARY_PATH`, `DYLD_LIBRARY_PATH`) and the interactive-shell-only `ENV` are intentionally not on this list — rejecting them breaks mainstream toolchains (`ENV=production`, conda/CUDA library dirs) — but a project `.env` still cannot apply them on reload. This rejection applies to the top-level `env` section only: per-server `mcpServers[].env` and per-hook `hooks[].env` are intentionally scoped to that server or hook and still apply (both surfaces are gated by folder trust for workspace-provided configs). Separately, a project `.env` can never set `QWEN_CLI_ENTRY` (the daemon's session-process entrypoint), `QWEN_CDP_MCP_COMMAND` (the command the daemon spawns as the browser-automation MCP adapter), `QWEN_SERVE_CDP_TUNNEL_OVER_WS` (switches that tunnel surface on), `DEV` (the dev-harness launch marker), the TLS trust-anchor variables (`NODE_EXTRA_CA_CERTS`, `SSL_CERT_FILE`, `SSL_CERT_DIR`, `CURL_CA_BUNDLE`, `REQUESTS_CA_BUNDLE`, `GIT_SSL_CAINFO`, `GIT_SSL_CAPATH`, `npm_config_cafile`, `npm_config_ca`, `npm_config_strict_ssl`, `PIP_CERT` — an attacker CA there, or `npm_config_strict_ssl=false`, would enable MITM of the token-bearing traffic a session's `git`/`npm`/`pip`/`curl` calls make), the git command-execution variables (`GIT_SSH_COMMAND`, `GIT_SSH`, `GIT_EXEC_PATH`, `GIT_TEMPLATE_DIR`, `GIT_ASKPASS`, `GIT_PROXY_COMMAND`, `GIT_EDITOR`, `GIT_SEQUENCE_EDITOR`, `GIT_EXTERNAL_DIFF`, `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_SYSTEM`, `GIT_CONFIG_COUNT`, `GIT_CONFIG_PARAMETERS` and the numbered `GIT_CONFIG_KEY_`/`GIT_CONFIG_VALUE_` pairs — git runs these on any session `git` invocation — and `XDG_CONFIG_HOME`, which redirects the `$XDG_CONFIG_HOME/git/config` git merges with `~/.gitconfig`), the curl/wget rc-file redirects (`CURL_HOME`, `WGETRC` — their rc files can install an attacker proxy or CA), `PIP_CONFIG_FILE` (redirects all of pip's configuration — `index-url`, `trusted-host`, proxy, or cert settings in an attacker file send session pip traffic or credentials to attacker infrastructure), `SSH_ASKPASS` (git/ssh execute it as the fallback passphrase-prompt program on an auth challenge), `LESSOPEN` and `LESSCLOSE` (`less` executes them as input preprocessors on every file a session views), the node-gyp interpreter-selection variables (`NODE_GYP_FORCE_PYTHON`, `npm_config_python`, `PYTHON` — run as the build Python during native-addon installs — and `npm_config_git`, run as npm's git binary), the editor and startup hooks (`VISUAL`, `EDITOR` — git's editor fallback chain, also spawned by the CLI's own external-editor flows — and `PYTHONSTARTUP`, which CPython executes at interactive startup), or `BROWSER` (the CLI execs it via the secure browser launcher). Those stay settable from the shell environment or a user-level `.env`; unlike the loader list above they are rejected from project files only, so a value you export yourself is preserved. They are also frozen at boot from a user-level `.env`: a settings reload does not apply edits to them there — or their removal — until the process restarts. Upgrade note: before this denylist existed, some of these keys could load from `.env` files or `settings.json` `env` on some paths; they are now rejected everywhere with a warning, and a `qwen serve` daemon no longer passes inherited values of them to session subprocesses. ### Environment Variables Table diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 2e3fcf60d23..ca553844e94 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -558,7 +558,7 @@ provider decision with their normal tool policy and isolation boundary. - **Host header allowlist** — on **loopback** binds the daemon checks `Host:` matches `localhost:port` / `127.0.0.1:port` / `[::1]:port` / `host.docker.internal:port` (case-insensitive per RFC 7230 §5.4) to defend against DNS rebinding. **Non-loopback binds (`--hostname 0.0.0.0`) intentionally bypass the Host allowlist** — the operator has chosen the surface area, so the bearer-token gate is the sole authentication layer; reverse proxies / SNI / client cert pinning are the operator's responsibility, not the daemon's. If you need Host-based isolation on a non-loopback bind, terminate TLS + check Host at a front proxy. - **CORS denies any browser Origin by default** — returns `403` JSON. Pass **`--allow-origin `** (repeatable, T2.4 #4514) to opt specific browser origins through. Each value is either the literal `*` (any origin — boot refuses if no bearer token is configured; `--require-auth` on loopback is recommended for full hardening since `/health` and `/demo` remain pre-auth on loopback by default) or a canonical URL origin (`://[:]`, no trailing slash / path / userinfo). Matched origins receive proper CORS response headers (`Access-Control-Allow-Origin: `, `Vary: Origin`, plus standard methods / headers / max-age and exposed `Retry-After`); unmatched origins still get a 403 with the same envelope as the default wall. `caps.features.allow_origin` is advertised conditionally so SDK / webui clients can pre-flight whether the daemon honors cross-origin hits before issuing them. Example: `qwen serve --allow-origin http://localhost:3000 --allow-origin http://localhost:5173`. Loopback self-origin hits (e.g. the `/demo` page) are unaffected — a separate Origin-strip shim handles them regardless of `--allow-origin`. **Browser webuis without `--allow-origin` configured** still fall back to the same Stage 1 options as before: package as a native shell (Electron/Tauri) so no `Origin` header is sent, or front the daemon with a same-origin reverse proxy. - **Chrome extension browser automation is separate from framing.** `qwen serve --allow-origin chrome-extension://` lets the extension frame the Web Shell and connect to the daemon. Console/network/screenshot/click tools require an external CDP MCP adapter command: `QWEN_CDP_MCP_COMMAND=/path/to/cdp-mcp-adapter qwen serve --allow-origin chrome-extension://`. The main CLI package does not bundle a browser automation adapter; clients can check `caps.features.includes('browser_automation_mcp')` before presenting those tools as available. -- **A spawned `qwen --acp` child receives its owning runtime's effective environment.** The daemon freezes a process-env base, applies that workspace's settings/env-file overlay to a runtime-local snapshot, and never writes the overlay back to `process.env`; same-named keys in another runtime do not cross over. `QWEN_SERVER_TOKEN` is scrubbed before spawn because the agent does not need the daemon bearer. Loader-affecting variables (`NODE_OPTIONS`, `npm_config_node_options` and npm's config-file redirects, `NODE_PATH`, `LD_PRELOAD`, `LD_AUDIT`, `DYLD_INSERT_LIBRARIES`, `BASH_ENV`, `ZDOTDIR`, exported bash function definitions `BASH_FUNC_*`) are likewise never passed to session subprocesses — the daemon scrubs them from its own `process.env` and from the frozen base env that session-hosting children spawn with (the base env keeps them only under the `DEV=true` harness, whose `.ts` entries still need the tsx loader), and `.env` / `settings.json` `env` sources reject them (see [settings](./configuration/settings.md)); this applies to every session the daemon hosts. Base credentials such as `OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, `QWEN_*`, and `DASHSCOPE_API_KEY` otherwise pass through unless the runtime overlay changes them. **This is intentional, not a sandbox.** The agent runs as the same UID with shell-tool access, so anything in `~/.bashrc`, `~/.aws/credentials`, or `~/.npmrc` is reachable by prompt injection regardless. Environment isolation between runtimes is not an operating-system security boundary; do not run `qwen serve` under an identity that has credentials you would not trust the agent with. +- **A spawned `qwen --acp` child receives its owning runtime's effective environment.** The daemon freezes a process-env base, applies that workspace's settings/env-file overlay to a runtime-local snapshot, and never writes the overlay back to `process.env`; same-named keys in another runtime do not cross over. `QWEN_SERVER_TOKEN` is scrubbed before spawn because the agent does not need the daemon bearer. Loader-affecting variables (`NODE_OPTIONS`, `npm_config_node_options` and npm's config-file redirects, `NODE_PATH`, `OPENSSL_CONF`, `NODE_REPL_EXTERNAL_MODULE`, `npm_config_node_gyp`, `npm_config_init_module`, `LD_PRELOAD`, `LD_AUDIT`, `DYLD_INSERT_LIBRARIES`, `BASH_ENV`, `ZDOTDIR`, exported bash function definitions `BASH_FUNC_*`) are likewise never passed to session subprocesses — the daemon scrubs them from its own `process.env` and from the frozen base env that session-hosting children spawn with (the base env keeps them only under the `DEV=true` harness, whose `.ts` entries still need the tsx loader), and `.env` / `settings.json` `env` sources reject them (see [settings](./configuration/settings.md)); this applies to every session the daemon hosts. Base credentials such as `OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, `QWEN_*`, and `DASHSCOPE_API_KEY` otherwise pass through unless the runtime overlay changes them. **This is intentional, not a sandbox.** The agent runs as the same UID with shell-tool access, so anything in `~/.bashrc`, `~/.aws/credentials`, or `~/.npmrc` is reachable by prompt injection regardless. Environment isolation between runtimes is not an operating-system security boundary; do not run `qwen serve` under an identity that has credentials you would not trust the agent with. - **Agent text reads are child-local and follow the regular CLI permission rules, not the workspace filesystem boundary.** Direct `read_file` can reach host text paths outside every registered workspace: external paths default to confirmation, and allow rules or approval modes may approve them automatically. Approved reads use the configurable CLI output limits rather than the workspace filesystem's returned-output, full-snapshot, and large-text scan caps. This applies to every shared text-read consumer, so the pre-reads performed by write, edit, notebook, sed, and artifact operations lose those caps together with the workspace filesystem's read audit, symlink rejection, and read-side TOCTOU protections — see [the design doc](../design/daemon-local-text-reads.md) for the exact list. Because a confirmation payload is built by reading the file, an out-of-workspace diff is fanned out to **every** attached SSE subscriber before anyone approves it — in the interactive CLI that content is seen only by the person at the terminal. Treat authenticated daemon clients as the same security principal. HTTP filesystem routes remain workspace-scoped and still refuse these paths, agent discovery-tool behavior is unchanged, and final ACP `writeTextFile` content writes continue through the workspace filesystem. - **Per-subscriber bounded SSE queues** — a slow client that overflows its queue gets a `client_evicted` terminal frame and is closed; one stuck consumer can't pin the daemon. - **Per-session prompt admission cap** — defaults to 5 accepted-but-unsettled prompts per session. A buggy client cannot enqueue unbounded prompt promises or temporary SSE waits for one session. diff --git a/packages/cli/src/commands/channel/daemon-worker.test.ts b/packages/cli/src/commands/channel/daemon-worker.test.ts index 6ed3ded11fb..e3438898408 100644 --- a/packages/cli/src/commands/channel/daemon-worker.test.ts +++ b/packages/cli/src/commands/channel/daemon-worker.test.ts @@ -66,6 +66,7 @@ const mockResolveProxyUrl = vi.hoisted(() => vi.fn((_cliProxy?: string, settingsProxy?: string) => settingsProxy), ); const mockWriteStderrLine = vi.hoisted(() => vi.fn()); +const mockWriteStderrLineSafe = vi.hoisted(() => vi.fn()); const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); const mockSelectFirstModel = vi.hoisted(() => vi.fn( @@ -219,7 +220,7 @@ vi.mock('@qwen-code/qwen-code-core', () => ({ vi.mock('../../utils/stdioHelpers.js', () => ({ writeStderrLine: mockWriteStderrLine, - writeStderrLineSafe: vi.fn(), + writeStderrLineSafe: mockWriteStderrLineSafe, writeStdoutLine: mockWriteStdoutLine, })); @@ -2033,6 +2034,13 @@ describe('daemonWorkerCommand', () => { expect(process.env['NODE_OPTIONS']).toBeUndefined(); expect(process.env['npm_config_node-options']).toBeUndefined(); + // Pin the channel-boundary breadcrumb, not just the removal: a refactor + // onto the silent scrubInheritedLoaderEnv variant deletes the keys the + // same way but drops the operator diagnostic — the reason the *AndReport* + // helper exists. + expect(mockWriteStderrLineSafe).toHaveBeenCalledWith( + expect.stringContaining('scrubbed inherited loader env vars'), + ); }); it('scrubs daemon connection env when required env validation fails', async () => { diff --git a/packages/cli/src/config/environment.test.ts b/packages/cli/src/config/environment.test.ts index a67fb9f59ee..4231589a6ce 100644 --- a/packages/cli/src/config/environment.test.ts +++ b/packages/cli/src/config/environment.test.ts @@ -40,6 +40,42 @@ const TRACKED_ENV = [ 'Node_Options', 'ZDOTDIR', 'BASH_FUNC_id%%', + 'OPENSSL_CONF', + 'NODE_REPL_EXTERNAL_MODULE', + 'npm_config_node_gyp', + 'npm_config_init_module', + 'SSL_CERT_FILE', + 'GIT_SSH_COMMAND', + 'GIT_SSH', + 'GIT_CONFIG_COUNT', + 'GIT_CONFIG_PARAMETERS', + 'GIT_CONFIG_KEY_0', + 'GIT_CONFIG_VALUE_0', + 'GIT_EXEC_PATH', + 'GIT_TEMPLATE_DIR', + 'GIT_ASKPASS', + 'GIT_PROXY_COMMAND', + 'GIT_EDITOR', + 'GIT_SSL_CAPATH', + 'npm_config_cafile', + 'npm_config_ca', + 'npm_config_strict_ssl', + 'PIP_CERT', + 'PIP_CONFIG_FILE', + 'SSH_ASKPASS', + 'LESSOPEN', + 'LESSCLOSE', + 'CURL_HOME', + 'WGETRC', + 'PYTHON', + 'GIT_SEQUENCE_EDITOR', + 'XDG_CONFIG_HOME', + 'VISUAL', + 'EDITOR', + 'PYTHONSTARTUP', + 'BROWSER', + 'QWEN_CDP_MCP_COMMAND', + 'QWEN_SERVE_CDP_TUNNEL_OVER_WS', 'NODE_COMPILE_CACHE', 'NODE_DISABLE_COMPILE_CACHE', 'NODE_EXTRA_CA_CERTS', @@ -346,6 +382,225 @@ describe('loadEnvironment', () => { expect(process.env['RUNTIME_DOTENV']).toBe('allowed'); }); + // #8663 follow-up: pure-injection loader keys (dlopen/require/exec redirects) + // join the scrubbed loader set and are rejected from every .env scope. + it('never applies the follow-up code-injection loader keys from .env', () => { + const workspace = makeWorkspace(); + fs.writeFileSync( + path.join(workspace, '.env'), + [ + 'OPENSSL_CONF=/workspace-a/evil.cnf', + 'NODE_REPL_EXTERNAL_MODULE=/workspace-a/hook.js', + 'npm_config_node_gyp=/workspace-a/evil-gyp.js', + 'npm_config_init_module=/workspace-a/evil-init.js', + 'RUNTIME_DOTENV=allowed', + '', + ].join('\n'), + ); + + loadEnvironment(testSettings({}), workspace); + + expect(process.env['OPENSSL_CONF']).toBeUndefined(); + expect(process.env['NODE_REPL_EXTERNAL_MODULE']).toBeUndefined(); + expect(process.env['npm_config_node_gyp']).toBeUndefined(); + expect(process.env['npm_config_init_module']).toBeUndefined(); + expect(process.env['RUNTIME_DOTENV']).toBe('allowed'); + }); + + // #8663 follow-up: TLS trust anchors, the git command-exec family (incl. + // numbered GIT_CONFIG_KEY_/VALUE_ pairs), and node-gyp interpreter selection + // join the hardcoded reject-from-project-.env tier. + it('never applies the follow-up TLS/git/interpreter keys from a project .env', () => { + const workspace = makeWorkspace(); + fs.writeFileSync( + path.join(workspace, '.env'), + [ + 'SSL_CERT_FILE=/workspace-a/evil-ca.pem', + 'GIT_SSH_COMMAND=/workspace-a/evil-ssh.sh', + 'GIT_SSH=/workspace-a/evil-legacy-ssh.sh', + 'GIT_CONFIG_COUNT=1', + 'GIT_CONFIG_PARAMETERS=core.hooksPath=/workspace-a/evil-hooks', + 'GIT_CONFIG_KEY_0=core.hooksPath', + 'GIT_CONFIG_VALUE_0=/workspace-a/evil-hooks', + 'GIT_EXEC_PATH=/workspace-a/evil-exec', + 'GIT_TEMPLATE_DIR=/workspace-a/evil-templates', + 'GIT_ASKPASS=/workspace-a/evil-askpass', + 'GIT_PROXY_COMMAND=/workspace-a/evil-proxy.sh', + 'GIT_EDITOR=/workspace-a/evil-editor.sh', + 'GIT_SSL_CAPATH=/workspace-a/evil-capath', + 'npm_config_cafile=/workspace-a/evil-ca.pem', + 'npm_config_ca=/workspace-a/evil-ca-inline', + 'npm_config_strict_ssl=false', + 'PIP_CERT=/workspace-a/evil-ca.pem', + 'CURL_HOME=/workspace-a', + 'WGETRC=/workspace-a/evil-wgetrc', + 'PYTHON=/workspace-a/evil-python', + 'RUNTIME_DOTENV=allowed', + '', + ].join('\n'), + ); + + loadEnvironment(testSettings({}), workspace); + + expect(process.env['SSL_CERT_FILE']).toBeUndefined(); + expect(process.env['GIT_SSH_COMMAND']).toBeUndefined(); + expect(process.env['GIT_SSH']).toBeUndefined(); + expect(process.env['GIT_CONFIG_COUNT']).toBeUndefined(); + expect(process.env['GIT_CONFIG_PARAMETERS']).toBeUndefined(); + expect(process.env['GIT_CONFIG_KEY_0']).toBeUndefined(); + expect(process.env['GIT_CONFIG_VALUE_0']).toBeUndefined(); + expect(process.env['GIT_EXEC_PATH']).toBeUndefined(); + expect(process.env['GIT_TEMPLATE_DIR']).toBeUndefined(); + expect(process.env['GIT_ASKPASS']).toBeUndefined(); + expect(process.env['GIT_PROXY_COMMAND']).toBeUndefined(); + expect(process.env['GIT_EDITOR']).toBeUndefined(); + expect(process.env['GIT_SSL_CAPATH']).toBeUndefined(); + expect(process.env['npm_config_cafile']).toBeUndefined(); + expect(process.env['npm_config_ca']).toBeUndefined(); + expect(process.env['npm_config_strict_ssl']).toBeUndefined(); + expect(process.env['PIP_CERT']).toBeUndefined(); + expect(process.env['CURL_HOME']).toBeUndefined(); + expect(process.env['WGETRC']).toBeUndefined(); + expect(process.env['PYTHON']).toBeUndefined(); + expect(process.env['RUNTIME_DOTENV']).toBe('allowed'); + }); + + // #8663 review round: PIP_CONFIG_FILE redirects all of pip's configuration + // (index-url / trusted-host / proxy / cert) at an attacker file, SSH_ASKPASS + // is the askpass program git/ssh execute on an auth challenge, and LESSOPEN/ + // LESSCLOSE are run by `less` as input preprocessors. Each must be rejected + // on every application boundary — initial load and reload alike. + it('never applies pip config / ssh askpass / less preprocessor keys from a project .env, including reload', () => { + resetEnvironmentTrackingForTesting(); + const workspace = makeWorkspace(); + const envPath = path.join(workspace, '.env'); + fs.writeFileSync( + envPath, + [ + 'PIP_CONFIG_FILE=/workspace-a/pip.conf', + 'SSH_ASKPASS=/workspace-a/evil-askpass', + 'LESSOPEN=| /workspace-a/evil-lessopen.sh %s', + 'LESSCLOSE=/workspace-a/evil-lessclose.sh %s %s', + 'RUNTIME_DOTENV=allowed', + '', + ].join('\n'), + ); + + loadEnvironment(testSettings({}), workspace); + expect(process.env['PIP_CONFIG_FILE']).toBeUndefined(); + expect(process.env['SSH_ASKPASS']).toBeUndefined(); + expect(process.env['LESSOPEN']).toBeUndefined(); + expect(process.env['LESSCLOSE']).toBeUndefined(); + expect(process.env['RUNTIME_DOTENV']).toBe('allowed'); + + // A mid-session reload must not apply them either. + reloadEnvironment(testSettings({}), workspace); + expect(process.env['PIP_CONFIG_FILE']).toBeUndefined(); + expect(process.env['SSH_ASKPASS']).toBeUndefined(); + expect(process.env['LESSOPEN']).toBeUndefined(); + expect(process.env['LESSCLOSE']).toBeUndefined(); + expect(process.env['RUNTIME_DOTENV']).toBe('allowed'); + }); + + // The settings.env application (load and reload) and the daemon's + // per-workspace runtime env build consult the same hardcoded predicate. + it('rejects pip config / ssh askpass / less preprocessor keys from settings.env and the runtime env build', () => { + resetEnvironmentTrackingForTesting(); + const workspace = makeWorkspace(); + const settings = testSettings({ + env: { + PIP_CONFIG_FILE: '/workspace-a/pip.conf', + SSH_ASKPASS: '/workspace-a/evil-askpass', + LESSOPEN: '| /workspace-a/evil-lessopen.sh %s', + RUNTIME_SETTINGS_ONLY: 'from-settings', + }, + }); + + loadEnvironment(settings, workspace); + expect(process.env['PIP_CONFIG_FILE']).toBeUndefined(); + expect(process.env['SSH_ASKPASS']).toBeUndefined(); + expect(process.env['LESSOPEN']).toBeUndefined(); + expect(process.env['RUNTIME_SETTINGS_ONLY']).toBe('from-settings'); + + // Reload force-writes settings.env keys; the hardcoded gate must keep + // rejecting them there too. + reloadEnvironment(settings, workspace); + expect(process.env['PIP_CONFIG_FILE']).toBeUndefined(); + expect(process.env['SSH_ASKPASS']).toBeUndefined(); + expect(process.env['LESSOPEN']).toBeUndefined(); + expect(process.env['RUNTIME_SETTINGS_ONLY']).toBe('from-settings'); + + const snapshot = buildRuntimeEnvironment(settings, workspace, {}); + expect(snapshot.effectiveEnv['PIP_CONFIG_FILE']).toBeUndefined(); + expect(snapshot.effectiveEnv['SSH_ASKPASS']).toBeUndefined(); + expect(snapshot.effectiveEnv['LESSOPEN']).toBeUndefined(); + expect(snapshot.effectiveEnv['RUNTIME_SETTINGS_ONLY']).toBe( + 'from-settings', + ); + }); + + // #8663 round-4: git executes GIT_SEQUENCE_EDITOR on `git rebase -i` and + // merges `$XDG_CONFIG_HOME/git/config` with `~/.gitconfig` (bypassing the + // GIT_CONFIG_* blocks); $VISUAL/$EDITOR are git's editor fallback and the + // CLI's own editor launch; CPython executes PYTHONSTARTUP at interactive + // startup; the CLI execs $BROWSER via openBrowserSecurely; the daemon + // spawns QWEN_CDP_MCP_COMMAND as the browser-automation MCP adapter and + // QWEN_SERVE_CDP_TUNNEL_OVER_WS switches that tunnel surface on. + it('never applies the round-4 exec-redirect keys from project .env or settings.env, including reload', () => { + resetEnvironmentTrackingForTesting(); + const workspace = makeWorkspace(); + fs.writeFileSync( + path.join(workspace, '.env'), + [ + 'GIT_SEQUENCE_EDITOR=/workspace-a/evil-sequence.sh', + 'VISUAL=/workspace-a/evil-visual.sh', + 'EDITOR=/workspace-a/evil-editor.sh', + 'PYTHONSTARTUP=/workspace-a/evil-startup.py', + 'XDG_CONFIG_HOME=/workspace-a/.xdg', + 'BROWSER=/workspace-a/evil-browser.sh', + 'RUNTIME_DOTENV=allowed', + '', + ].join('\n'), + ); + const settings = testSettings({ + env: { + QWEN_CDP_MCP_COMMAND: '/workspace-a/evil-adapter', + QWEN_SERVE_CDP_TUNNEL_OVER_WS: '1', + RUNTIME_SETTINGS_ONLY: 'from-settings', + }, + }); + + const expectRound4Rejected = (env: Readonly) => { + expect(env['GIT_SEQUENCE_EDITOR']).toBeUndefined(); + expect(env['VISUAL']).toBeUndefined(); + expect(env['EDITOR']).toBeUndefined(); + expect(env['PYTHONSTARTUP']).toBeUndefined(); + expect(env['XDG_CONFIG_HOME']).toBeUndefined(); + expect(env['BROWSER']).toBeUndefined(); + expect(env['QWEN_CDP_MCP_COMMAND']).toBeUndefined(); + expect(env['QWEN_SERVE_CDP_TUNNEL_OVER_WS']).toBeUndefined(); + }; + + loadEnvironment(settings, workspace); + expectRound4Rejected(process.env); + expect(process.env['RUNTIME_DOTENV']).toBe('allowed'); + expect(process.env['RUNTIME_SETTINGS_ONLY']).toBe('from-settings'); + + // A mid-session reload must not apply them either. + reloadEnvironment(settings, workspace); + expectRound4Rejected(process.env); + expect(process.env['RUNTIME_DOTENV']).toBe('allowed'); + expect(process.env['RUNTIME_SETTINGS_ONLY']).toBe('from-settings'); + + // The daemon's per-workspace runtime env build consults the same gate. + const snapshot = buildRuntimeEnvironment(settings, workspace, {}); + expectRound4Rejected(snapshot.effectiveEnv); + expect(snapshot.effectiveEnv['RUNTIME_DOTENV']).toBe('allowed'); + expect(snapshot.effectiveEnv['RUNTIME_SETTINGS_ONLY']).toBe( + 'from-settings', + ); + }); + // The privileged /.qwen/.env scope deliberately bypasses // excludedEnvVars and is discovered before the plain .env, so exempting it // from the loader denylist must not ship green. @@ -770,6 +1025,62 @@ describe('loadEnvironment', () => { expect(process.env['tmpdir']).toBe('/workspace-a/first'); }); + // The numbered GIT_CONFIG_KEY_/VALUE_ pairs are hardcoded exclusions via + // prefix matching; the reload gate must freeze them exactly like their + // literal sibling GIT_CONFIG_COUNT, or a home `.env` edit rotates one half + // of the mechanism mid-session while the other half stays at the boot + // value. Home-scoped files are exempt from the reject-only tier at boot, + // so the boot value applies — and then freezes, edits and removals alike. + it('freezes the numbered GIT_CONFIG pairs on reload like GIT_CONFIG_COUNT', () => { + resetEnvironmentTrackingForTesting(); + const workspace = makeWorkspace(); + const homeEnvPath = path.join(process.env['HOME']!, '.env'); + fs.writeFileSync( + homeEnvPath, + [ + 'GIT_CONFIG_COUNT=1', + 'GIT_CONFIG_KEY_0=core.hooksPath', + 'GIT_CONFIG_VALUE_0=/home-a/hooks', + 'RUNTIME_DOTENV=allowed', + '', + ].join('\n'), + ); + + loadEnvironment(testSettings({}), workspace); + expect(process.env['GIT_CONFIG_COUNT']).toBe('1'); + expect(process.env['GIT_CONFIG_KEY_0']).toBe('core.hooksPath'); + expect(process.env['GIT_CONFIG_VALUE_0']).toBe('/home-a/hooks'); + expect(process.env['RUNTIME_DOTENV']).toBe('allowed'); + + fs.writeFileSync( + homeEnvPath, + [ + 'GIT_CONFIG_COUNT=2', + 'GIT_CONFIG_KEY_0=core.fsmonitor', + 'GIT_CONFIG_VALUE_0=/home-a/fsmonitor', + 'RUNTIME_DOTENV=rotated', + '', + ].join('\n'), + ); + reloadEnvironment(testSettings({}), workspace); + expect(process.env['GIT_CONFIG_COUNT']).toBe('1'); + expect(process.env['GIT_CONFIG_KEY_0']).toBe('core.hooksPath'); + expect(process.env['GIT_CONFIG_VALUE_0']).toBe('/home-a/hooks'); + // An ordinary key next to them still rotates — the freeze is key-scoped. + // The fixture value must actually change between reloads, or this check + // passes even when reload freezes every key. + expect(process.env['RUNTIME_DOTENV']).toBe('rotated'); + + // Removal is frozen too (symmetric with GIT_CONFIG_COUNT, documented in + // the settings.md upgrade note): a home `.env` deletion does not + // propagate on reload. + fs.writeFileSync(homeEnvPath, ['RUNTIME_DOTENV=allowed', ''].join('\n')); + reloadEnvironment(testSettings({}), workspace); + expect(process.env['GIT_CONFIG_COUNT']).toBe('1'); + expect(process.env['GIT_CONFIG_KEY_0']).toBe('core.hooksPath'); + expect(process.env['GIT_CONFIG_VALUE_0']).toBe('/home-a/hooks'); + }); + // The daemon reaches per-workspace .env files only through // buildRuntimeEnvironment (its loadSettings calls pass // skipLoadEnvironment), so the rejection report must fire from this loop diff --git a/packages/cli/src/config/environment.ts b/packages/cli/src/config/environment.ts index 57fab4868ae..d17d3266b05 100644 --- a/packages/cli/src/config/environment.ts +++ b/packages/cli/src/config/environment.ts @@ -60,7 +60,15 @@ const RELOAD_EXCLUDED_KEYS_CASEFOLDED: ReadonlySet = new Set( ); function isReloadExcludedKey(key: string): boolean { - return RELOAD_EXCLUDED_KEYS_CASEFOLDED.has(key.toLowerCase()); + return ( + RELOAD_EXCLUDED_KEYS_CASEFOLDED.has(key.toLowerCase()) || + // The hardcoded tier's pattern-matched keys (numbered + // GIT_CONFIG_KEY_/GIT_CONFIG_VALUE_ pairs) must freeze on reload + // together with their literal sibling GIT_CONFIG_COUNT — one mechanism, + // one gate. The literal hardcoded exclusions are already spread into + // RELOAD_EXCLUDED_KEYS, so this adds exactly the pattern coverage. + isHardcodedProjectEnvExclusion(key) + ); } const dotEnvSourcedKeys = new Set(); diff --git a/packages/cli/src/config/shared-env-keys.test.ts b/packages/cli/src/config/shared-env-keys.test.ts index 84cf30145bc..9504c1b3278 100644 --- a/packages/cli/src/config/shared-env-keys.test.ts +++ b/packages/cli/src/config/shared-env-keys.test.ts @@ -4,18 +4,23 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, afterEach } from 'vitest'; import { + acquireInheritedLoaderEnvScrub, + clearLoaderKeyRejectionReporterIfCurrent, ENV_ACP_REPEATED_TOOL_FAILURE_GUARD, HOME_ENV_BOOTSTRAP_KEYS, INHERITED_LOADER_ENV_KEYS, isHardcodedProjectEnvExclusion, isLoaderEnvKey, + type LoaderKeyRejectionReporter, PROJECT_ENV_HARDCODED_EXCLUSIONS, reportRejectedLoaderKeys, + resetInheritedLoaderEnvScrubForTesting, resetLoaderKeyRejectionReportingForTesting, scrubAndReportInheritedLoaderEnv, scrubInheritedLoaderEnv, + setLoaderKeyRejectionReporter, } from './shared-env-keys.js'; describe('PROJECT_ENV_HARDCODED_EXCLUSIONS', () => { @@ -75,6 +80,133 @@ describe('PROJECT_ENV_HARDCODED_EXCLUSIONS', () => { expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain('DEV'); }); + // The non-Node TLS trust-anchor vars reach the same MITM outcome as + // NODE_EXTRA_CA_CERTS for the curl/git/openssl/python tools a session + // shells out to; a project .env must not inject an attacker CA. + it('excludes the non-Node TLS trust-anchor vars', () => { + for (const key of [ + 'SSL_CERT_FILE', + 'SSL_CERT_DIR', + 'CURL_CA_BUNDLE', + 'REQUESTS_CA_BUNDLE', + 'GIT_SSL_CAINFO', + 'GIT_SSL_CAPATH', + 'npm_config_cafile', + 'npm_config_ca', + 'npm_config_strict_ssl', + 'PIP_CERT', + ]) { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain(key); + } + }); + + // npm treats underscore/hyphen spellings of a config key as the same key, + // so the hyphen twin of npm_config_strict_ssl must be excluded too. + it('excludes both spellings of the npm strict-ssl knob', () => { + expect(isHardcodedProjectEnvExclusion('npm_config_strict_ssl')).toBe(true); + expect(isHardcodedProjectEnvExclusion('npm_config_strict-ssl')).toBe(true); + }); + + // CURL_HOME/WGETRC redirect curl/wget at attacker rc files whose proxy/ + // cacert/insecure directives reach the same MITM outcome the TLS-anchor + // tier blocks — the config-file-redirect class, not a search path. + it('excludes the curl/wget rc-file redirect vars', () => { + for (const key of ['CURL_HOME', 'WGETRC']) { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain(key); + } + }); + + // git executes these on any session git invocation (SSH command, external + // diff, config-injected core.hooksPath); a project .env setting them is + // code execution as the daemon user. + it('excludes the git command-execution env family', () => { + for (const key of [ + 'GIT_SSH_COMMAND', + 'GIT_SSH', + 'GIT_EXEC_PATH', + 'GIT_TEMPLATE_DIR', + 'GIT_ASKPASS', + 'GIT_PROXY_COMMAND', + 'GIT_EDITOR', + 'GIT_EXTERNAL_DIFF', + 'GIT_CONFIG_GLOBAL', + 'GIT_CONFIG_SYSTEM', + 'GIT_CONFIG_COUNT', + 'GIT_CONFIG_PARAMETERS', + ]) { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain(key); + } + }); + + // node-gyp interpreter selection runs the pointed-at file as the build + // Python; a project .env must not redirect it. + it('excludes the node-gyp interpreter-selection and git-binary vars', () => { + for (const key of [ + 'NODE_GYP_FORCE_PYTHON', + 'npm_config_python', + 'PYTHON', + 'npm_config_git', + ]) { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain(key); + } + }); + + // PIP_CONFIG_FILE redirects all of pip's configuration (index-url / + // trusted-host / proxy / cert) at an attacker file; SSH_ASKPASS is the + // askpass fallback git/ssh execute on an auth challenge; less executes + // LESSOPEN (and LESSCLOSE when the preprocessor ran) as an input + // preprocessor. Each is code execution or credential diversion from a + // project .env. + it('excludes pip config, ssh askpass, and less preprocessor redirects', () => { + for (const key of [ + 'PIP_CONFIG_FILE', + 'SSH_ASKPASS', + 'LESSOPEN', + 'LESSCLOSE', + ]) { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain(key); + } + }); + + // SSH_ASKPASS_REQUIRE only selects *when* the askpass program runs; with + // SSH_ASKPASS project-blocked it has nothing to execute, so it stays + // settable from project files. + it('keeps SSH_ASKPASS_REQUIRE out of the hardcoded exclusions', () => { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).not.toContain( + 'SSH_ASKPASS_REQUIRE', + ); + expect(isHardcodedProjectEnvExclusion('SSH_ASKPASS_REQUIRE')).toBe(false); + }); + + // git executes GIT_SEQUENCE_EDITOR for the `git rebase -i` todo list like + // GIT_EDITOR, and merges `$XDG_CONFIG_HOME/git/config` with `~/.gitconfig` + // — a config-discovery redirect that bypasses the GIT_CONFIG_* blocks. + it('excludes the git sequence editor and XDG config redirect', () => { + for (const key of ['GIT_SEQUENCE_EDITOR', 'XDG_CONFIG_HOME']) { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain(key); + } + }); + + // git's editor fallback chain ($VISUAL/$EDITOR) and the CLI's own + // useLaunchEditor spawn these like the blocked GIT_EDITOR; CPython executes + // PYTHONSTARTUP at interactive startup; the CLI execs $BROWSER via + // openBrowserSecurely. Each is an exec redirect from a project .env. + it('excludes the editor, startup, and browser exec-redirect keys', () => { + for (const key of ['VISUAL', 'EDITOR', 'PYTHONSTARTUP', 'BROWSER']) { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain(key); + } + }); + + // QWEN_CDP_MCP_COMMAND is spawned by the daemon as the browser-automation + // MCP adapter and QWEN_SERVE_CDP_TUNNEL_OVER_WS switches that tunnel + // surface on — the same daemon-hijack class as QWEN_CLI_ENTRY. + it('excludes the serve CDP adapter command and tunnel switch', () => { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain('QWEN_CDP_MCP_COMMAND'); + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain( + 'QWEN_SERVE_CDP_TUNNEL_OVER_WS', + ); + }); + // Workspace settings.env QWEN_SERVER_TOKEN is an intentional fast-path // feature (fast-path.test.ts loads it without the full settings loader); // it stays reload-only rather than hardcoded-excluded. @@ -102,6 +234,76 @@ describe('isHardcodedProjectEnvExclusion', () => { expect(isHardcodedProjectEnvExclusion('QWEN_SERVER_TOKEN')).toBe(false); expect(isHardcodedProjectEnvExclusion('NODE_OPTIONS')).toBe(false); }); + + it('matches the newly added hardcoded exclusions case-insensitively', () => { + expect(isHardcodedProjectEnvExclusion('SSL_CERT_FILE')).toBe(true); + expect(isHardcodedProjectEnvExclusion('ssl_cert_file')).toBe(true); + expect(isHardcodedProjectEnvExclusion('GIT_SSH_COMMAND')).toBe(true); + expect(isHardcodedProjectEnvExclusion('git_ssh_command')).toBe(true); + expect(isHardcodedProjectEnvExclusion('GIT_SSH')).toBe(true); + expect(isHardcodedProjectEnvExclusion('git_ssh')).toBe(true); + expect(isHardcodedProjectEnvExclusion('GIT_CONFIG_PARAMETERS')).toBe(true); + expect(isHardcodedProjectEnvExclusion('git_config_parameters')).toBe(true); + expect(isHardcodedProjectEnvExclusion('PYTHON')).toBe(true); + expect(isHardcodedProjectEnvExclusion('python')).toBe(true); + expect(isHardcodedProjectEnvExclusion('npm_config_python')).toBe(true); + expect(isHardcodedProjectEnvExclusion('GIT_EXEC_PATH')).toBe(true); + expect(isHardcodedProjectEnvExclusion('git_template_dir')).toBe(true); + expect(isHardcodedProjectEnvExclusion('git_askpass')).toBe(true); + expect(isHardcodedProjectEnvExclusion('GIT_PROXY_COMMAND')).toBe(true); + expect(isHardcodedProjectEnvExclusion('git_editor')).toBe(true); + expect(isHardcodedProjectEnvExclusion('GIT_SSL_CAPATH')).toBe(true); + expect(isHardcodedProjectEnvExclusion('pip_cert')).toBe(true); + expect(isHardcodedProjectEnvExclusion('CURL_HOME')).toBe(true); + expect(isHardcodedProjectEnvExclusion('wgetrc')).toBe(true); + expect(isHardcodedProjectEnvExclusion('pip_config_file')).toBe(true); + expect(isHardcodedProjectEnvExclusion('SSH_ASKPASS')).toBe(true); + expect(isHardcodedProjectEnvExclusion('ssh_askpass')).toBe(true); + expect(isHardcodedProjectEnvExclusion('LESSOPEN')).toBe(true); + expect(isHardcodedProjectEnvExclusion('lessclose')).toBe(true); + expect(isHardcodedProjectEnvExclusion('GIT_SEQUENCE_EDITOR')).toBe(true); + expect(isHardcodedProjectEnvExclusion('git_sequence_editor')).toBe(true); + expect(isHardcodedProjectEnvExclusion('XDG_CONFIG_HOME')).toBe(true); + expect(isHardcodedProjectEnvExclusion('xdg_config_home')).toBe(true); + expect(isHardcodedProjectEnvExclusion('VISUAL')).toBe(true); + expect(isHardcodedProjectEnvExclusion('visual')).toBe(true); + expect(isHardcodedProjectEnvExclusion('EDITOR')).toBe(true); + expect(isHardcodedProjectEnvExclusion('editor')).toBe(true); + expect(isHardcodedProjectEnvExclusion('PYTHONSTARTUP')).toBe(true); + expect(isHardcodedProjectEnvExclusion('pythonstartup')).toBe(true); + expect(isHardcodedProjectEnvExclusion('BROWSER')).toBe(true); + expect(isHardcodedProjectEnvExclusion('browser')).toBe(true); + expect(isHardcodedProjectEnvExclusion('QWEN_CDP_MCP_COMMAND')).toBe(true); + expect(isHardcodedProjectEnvExclusion('qwen_cdp_mcp_command')).toBe(true); + expect( + isHardcodedProjectEnvExclusion('QWEN_SERVE_CDP_TUNNEL_OVER_WS'), + ).toBe(true); + expect( + isHardcodedProjectEnvExclusion('qwen_serve_cdp_tunnel_over_ws'), + ).toBe(true); + }); + + // Numbered GIT_CONFIG_KEY_/GIT_CONFIG_VALUE_ pairs are an unbounded + // index, matched by numeric suffix rather than literal membership. + it('matches numbered GIT_CONFIG_KEY_/VALUE_ pairs by numeric suffix', () => { + expect(isHardcodedProjectEnvExclusion('GIT_CONFIG_KEY_0')).toBe(true); + expect(isHardcodedProjectEnvExclusion('GIT_CONFIG_VALUE_0')).toBe(true); + expect(isHardcodedProjectEnvExclusion('git_config_key_12')).toBe(true); + expect(isHardcodedProjectEnvExclusion('GIT_CONFIG_VALUE_7')).toBe(true); + // Git only reads decimal-numbered pairs (GIT_CONFIG_KEY_%d), so keys + // with empty, nonnumeric, or trailing-garbage suffixes are + // project-defined variables Git never consumes and must stay settable. + expect(isHardcodedProjectEnvExclusion('GIT_CONFIG_KEY_')).toBe(false); + expect(isHardcodedProjectEnvExclusion('GIT_CONFIG_KEY_CACHE')).toBe(false); + expect(isHardcodedProjectEnvExclusion('git_config_value_cache')).toBe( + false, + ); + expect(isHardcodedProjectEnvExclusion('GIT_CONFIG_KEY_0X')).toBe(false); + // A key that merely starts with GIT_CONFIG but is not a KEY_/VALUE_ pair + // (and not a listed literal) is not excluded: GIT_CONFIG_NOSYSTEM only + // skips the system gitconfig read and injects nothing. + expect(isHardcodedProjectEnvExclusion('GIT_CONFIG_NOSYSTEM')).toBe(false); + }); }); describe('isLoaderEnvKey', () => { @@ -144,6 +346,47 @@ describe('isLoaderEnvKey', () => { expect(isLoaderEnvKey('bash_func_id%%')).toBe(true); }); + // Pure code-injection vectors with no benign cross-workspace inheritance: + // OPENSSL_CONF (dlopen an engine at crypto init), NODE_REPL_EXTERNAL_MODULE + // (require at REPL start), and the npm node-gyp/init script redirects. + it('matches the pure-injection loader keys added for the #8653 follow-up', () => { + expect(isLoaderEnvKey('OPENSSL_CONF')).toBe(true); + expect(isLoaderEnvKey('openssl_conf')).toBe(true); + expect(isLoaderEnvKey('NODE_REPL_EXTERNAL_MODULE')).toBe(true); + expect(isLoaderEnvKey('npm_config_node_gyp')).toBe(true); + expect(isLoaderEnvKey('npm_config_node-gyp')).toBe(true); + expect(isLoaderEnvKey('npm_config_init_module')).toBe(true); + expect(isLoaderEnvKey('npm_config_init-module')).toBe(true); + }); + + // The interpreter-selection vars stay in the reject-only hardcoded tier + // (they have a legitimate operator-shell use), NOT the scrubbed loader set. + it('does not scrub interpreter-selection or TLS-anchor keys', () => { + expect(isLoaderEnvKey('PYTHON')).toBe(false); + expect(isLoaderEnvKey('npm_config_python')).toBe(false); + expect(isLoaderEnvKey('NODE_GYP_FORCE_PYTHON')).toBe(false); + expect(isLoaderEnvKey('SSL_CERT_FILE')).toBe(false); + expect(isLoaderEnvKey('GIT_SSH_COMMAND')).toBe(false); + expect(isLoaderEnvKey('GIT_EXEC_PATH')).toBe(false); + expect(isLoaderEnvKey('GIT_TEMPLATE_DIR')).toBe(false); + expect(isLoaderEnvKey('npm_config_cafile')).toBe(false); + expect(isLoaderEnvKey('PIP_CERT')).toBe(false); + expect(isLoaderEnvKey('CURL_HOME')).toBe(false); + expect(isLoaderEnvKey('WGETRC')).toBe(false); + expect(isLoaderEnvKey('PIP_CONFIG_FILE')).toBe(false); + expect(isLoaderEnvKey('SSH_ASKPASS')).toBe(false); + expect(isLoaderEnvKey('LESSOPEN')).toBe(false); + expect(isLoaderEnvKey('LESSCLOSE')).toBe(false); + expect(isLoaderEnvKey('GIT_SEQUENCE_EDITOR')).toBe(false); + expect(isLoaderEnvKey('XDG_CONFIG_HOME')).toBe(false); + expect(isLoaderEnvKey('VISUAL')).toBe(false); + expect(isLoaderEnvKey('EDITOR')).toBe(false); + expect(isLoaderEnvKey('PYTHONSTARTUP')).toBe(false); + expect(isLoaderEnvKey('BROWSER')).toBe(false); + expect(isLoaderEnvKey('QWEN_CDP_MCP_COMMAND')).toBe(false); + expect(isLoaderEnvKey('QWEN_SERVE_CDP_TUNNEL_OVER_WS')).toBe(false); + }); + // Library search paths and the interactive-sh-only ENV are deliberately // reload-only: scrubbing them breaks mainstream toolchains. it('does not match search paths or the ENV convention', () => { @@ -163,6 +406,10 @@ describe('scrubInheritedLoaderEnv', () => { npm_config_node_options: '--import file:///other-checkout/hook.mjs', npm_config_userconfig: '/other-checkout/.npmrc', NODE_PATH: '/other-checkout/node_modules', + OPENSSL_CONF: '/evil.cnf', + NODE_REPL_EXTERNAL_MODULE: '/evil.mjs', + npm_config_node_gyp: '/evil-gyp.js', + npm_config_init_module: '/evil-init.js', LD_PRELOAD: '/evil.so', LD_AUDIT: '/evil-audit.so', DYLD_INSERT_LIBRARIES: '/evil.dylib', @@ -189,6 +436,10 @@ describe('scrubInheritedLoaderEnv', () => { 'npm_config_node_options', 'npm_config_userconfig', 'NODE_PATH', + 'OPENSSL_CONF', + 'NODE_REPL_EXTERNAL_MODULE', + 'npm_config_node_gyp', + 'npm_config_init_module', 'LD_PRELOAD', 'LD_AUDIT', 'DYLD_INSERT_LIBRARIES', @@ -254,8 +505,12 @@ describe('scrubInheritedLoaderEnv', () => { 'LD_PRELOAD', 'NODE_OPTIONS', 'NODE_PATH', + 'NODE_REPL_EXTERNAL_MODULE', + 'OPENSSL_CONF', 'ZDOTDIR', 'npm_config_globalconfig', + 'npm_config_init_module', + 'npm_config_node_gyp', 'npm_config_node_options', 'npm_config_prefix', 'npm_config_script_shell', @@ -377,3 +632,282 @@ describe('scrubAndReportInheritedLoaderEnv', () => { expect(breadcrumb).toBe(''); }); }); + +// #8663 follow-up (concurrency): overlapping embedded daemons in one process +// share process.env; a per-daemon scrub+restore races and re-poisons the +// survivor. acquireInheritedLoaderEnvScrub reference-counts so only the last +// release restores. +describe('acquireInheritedLoaderEnvScrub', () => { + afterEach(() => { + resetInheritedLoaderEnvScrubForTesting(); + delete process.env['NODE_OPTIONS']; + delete process.env['LD_PRELOAD']; + }); + + it('does not restore loader vars while a second holder is still active', () => { + resetInheritedLoaderEnvScrubForTesting(); + const poison = '--import file:///workspace-a/register.mjs'; + process.env['NODE_OPTIONS'] = poison; + + const write = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + // Daemon A boots and scrubs the shared env. + const daemonA = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + expect(daemonA.removedKeys).toContain('NODE_OPTIONS'); + expect(process.env['NODE_OPTIONS']).toBeUndefined(); + + // Daemon B boots into the already-scrubbed env: nothing left to remove. + const daemonB = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + expect(daemonB.removedKeys).toEqual([]); + expect(process.env['NODE_OPTIONS']).toBeUndefined(); + + // A closes first. Its restore must NOT re-poison B's still-live + // session subprocesses — the regression this guard closes. + daemonA.release(); + expect(process.env['NODE_OPTIONS']).toBeUndefined(); + + // Only when the last holder releases is the original value restored. + daemonB.release(); + expect(process.env['NODE_OPTIONS']).toBe(poison); + } finally { + write.mockRestore(); + } + }); + + it('release is idempotent and a later assignment wins over the restore', () => { + resetInheritedLoaderEnvScrubForTesting(); + process.env['LD_PRELOAD'] = '/evil.so'; + const write = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + const handle = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + expect(process.env['LD_PRELOAD']).toBeUndefined(); + // A legitimate re-assignment before release must survive the restore. + process.env['LD_PRELOAD'] = '/legit.so'; + handle.release(); + handle.release(); // idempotent + expect(process.env['LD_PRELOAD']).toBe('/legit.so'); + } finally { + write.mockRestore(); + } + }); + + // Regression: the embedding host can assign loader keys between two + // acquires. The nested scrub deletes that assignment, and only the + // snapshot taken at the nested acquire lets the final release bring it + // back — without it the host's value is silently lost, corrupting the + // shared env of the embedding process. + it('restores a host assignment made between acquires (A -> assign -> B -> release)', () => { + resetInheritedLoaderEnvScrubForTesting(); + const write = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + // Daemon A boots and scrubs the shared env. + const daemonA = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + expect(process.env['NODE_OPTIONS']).toBeUndefined(); + + // The embedding host assigns a loader key while A holds the scrub. + process.env['NODE_OPTIONS'] = '--max-old-space-size=4096'; + + // Daemon B boots and its scrub removes the host assignment. + const daemonB = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + expect(daemonB.removedKeys).toContain('NODE_OPTIONS'); + expect(process.env['NODE_OPTIONS']).toBeUndefined(); + + // B closes first; A still holds the scrub, so nothing restores yet. + daemonB.release(); + expect(process.env['NODE_OPTIONS']).toBeUndefined(); + + // The final release brings the host assignment back. + daemonA.release(); + expect(process.env['NODE_OPTIONS']).toBe('--max-old-space-size=4096'); + } finally { + write.mockRestore(); + } + }); + + // The snapshot must track the NEWEST value observed at any acquire + // boundary: restoring the first acquire's stale value instead would + // overwrite a host re-assignment the same way losing it would. + it('restores the newest host assignment, not the stale first-acquire snapshot', () => { + resetInheritedLoaderEnvScrubForTesting(); + process.env['NODE_OPTIONS'] = '--stale'; + const write = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + const daemonA = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + expect(process.env['NODE_OPTIONS']).toBeUndefined(); + + process.env['NODE_OPTIONS'] = '--current'; + const daemonB = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + expect(daemonB.removedKeys).toContain('NODE_OPTIONS'); + expect(process.env['NODE_OPTIONS']).toBeUndefined(); + + daemonA.release(); + daemonB.release(); + expect(process.env['NODE_OPTIONS']).toBe('--current'); + } finally { + write.mockRestore(); + } + }); + + // The release that drops the refcount back to zero clears the snapshot; + // without that clear, the next cycle's final release would restore a key + // the host removed between cycles, re-injecting a stale loader value into + // the shared env. + it('does not restore a prior cycle snapshot for a key the host removed', () => { + resetInheritedLoaderEnvScrubForTesting(); + process.env['NODE_OPTIONS'] = '--cycle-one'; + const write = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + const firstCycle = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + firstCycle.release(); + expect(process.env['NODE_OPTIONS']).toBe('--cycle-one'); + + // The host removes the key entirely between cycles. + delete process.env['NODE_OPTIONS']; + + const secondCycle = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + expect(secondCycle.removedKeys).not.toContain('NODE_OPTIONS'); + secondCycle.release(); + expect(process.env['NODE_OPTIONS']).toBeUndefined(); + } finally { + write.mockRestore(); + } + }); + + // The test-only reset must drop a leaked cycle's snapshot along with the + // refcount, or the next test's first release would re-inject the leaked + // value into process.env. + it('reset drops a leaked snapshot before the next acquire', () => { + resetInheritedLoaderEnvScrubForTesting(); + process.env['LD_PRELOAD'] = '/leaked.so'; + const write = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + // Simulate a holder that never releases before the reset runs. + acquireInheritedLoaderEnvScrub(process.env, 'qwen serve', 'daemon'); + resetInheritedLoaderEnvScrubForTesting(); + delete process.env['LD_PRELOAD']; + + const handle = acquireInheritedLoaderEnvScrub( + process.env, + 'qwen serve', + 'daemon', + ); + expect(handle.removedKeys).toEqual([]); + handle.release(); + expect(process.env['LD_PRELOAD']).toBeUndefined(); + } finally { + write.mockRestore(); + } + }); + + // A loader key present with an undefined value is scrubbed but has + // nothing to restore; snapshotting it would let the final release write + // `undefined` back into the env. + it('does not snapshot loader keys whose value is undefined', () => { + resetInheritedLoaderEnvScrubForTesting(); + const env: NodeJS.ProcessEnv = { + NODE_OPTIONS: undefined, + PATH: '/usr/bin', + }; + const write = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + const handle = acquireInheritedLoaderEnvScrub( + env, + 'qwen serve', + 'daemon', + ); + expect(handle.removedKeys).toEqual(['NODE_OPTIONS']); + expect(env).not.toHaveProperty('NODE_OPTIONS'); + handle.release(); + expect(env).not.toHaveProperty('NODE_OPTIONS'); + expect(env['PATH']).toBe('/usr/bin'); + } finally { + write.mockRestore(); + } + }); +}); + +describe('clearLoaderKeyRejectionReporterIfCurrent', () => { + afterEach(() => { + setLoaderKeyRejectionReporter(undefined); + resetLoaderKeyRejectionReportingForTesting(); + }); + + it('clears only when the given reporter is still the active one', () => { + resetLoaderKeyRejectionReportingForTesting(); + const calls: string[] = []; + const reporterA: LoaderKeyRejectionReporter = () => calls.push('A'); + const reporterB: LoaderKeyRejectionReporter = () => calls.push('B'); + + const write = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + // Two co-resident daemons install their reporters in turn. + setLoaderKeyRejectionReporter(reporterA); + setLoaderKeyRejectionReporter(reporterB); + + // Daemon A's close must not drop daemon B's reporter. + clearLoaderKeyRejectionReporterIfCurrent(reporterA); + reportRejectedLoaderKeys('/ws-b/.env', ['NODE_OPTIONS']); + expect(calls).toEqual(['B']); + + // Daemon B's close clears it; further rejections fall back to stderr. + clearLoaderKeyRejectionReporterIfCurrent(reporterB); + resetLoaderKeyRejectionReportingForTesting(); + reportRejectedLoaderKeys('/ws-b/.env', ['LD_PRELOAD']); + expect(calls).toEqual(['B']); + } finally { + write.mockRestore(); + } + }); +}); diff --git a/packages/cli/src/config/shared-env-keys.ts b/packages/cli/src/config/shared-env-keys.ts index 095a0c95206..30c1792715f 100644 --- a/packages/cli/src/config/shared-env-keys.ts +++ b/packages/cli/src/config/shared-env-keys.ts @@ -48,11 +48,135 @@ export const PROJECT_ENV_HARDCODED_EXCLUSIONS = [ // NODE_EXTRA_CA_CERTS reaches the same outcome by adding a TLS trust // anchor instead of disabling verification. 'NODE_EXTRA_CA_CERTS', + // The non-Node TLS trust-anchor vars reach the SAME MITM outcome for the + // curl/git/openssl/python tools a session subprocess routinely shells out + // to: they are honored unconditionally as a CA bundle/dir, and npm/pip + // honor their own equivalents (npm_config_cafile / npm_config_ca / PIP_CERT + // as attacker CAs, npm_config_strict_ssl=false disables verification with + // no CA at all; GIT_SSL_CAPATH is git's directory-form twin of + // GIT_SSL_CAINFO). A project `.env` pointing any of them at an attacker CA + // lets an untrusted repo silently intercept token-bearing traffic + // (git/npm/pip fetches) for every workspace's sessions — the exact outcome + // NODE_EXTRA_CA_CERTS is blocked for. Like NODE_EXTRA_CA_CERTS these stay + // reject-from-project-`.env` only: a value the operator set in their own + // login shell or home `.env` is their trusted choice and is preserved. + 'SSL_CERT_FILE', + 'SSL_CERT_DIR', + 'CURL_CA_BUNDLE', + 'REQUESTS_CA_BUNDLE', + 'GIT_SSL_CAINFO', + 'GIT_SSL_CAPATH', + 'npm_config_cafile', + 'npm_config_ca', + // npm treats underscore/hyphen spellings of a config key as the same key + // (see canonicalLoaderKey below), so both forms of strict-ssl are listed. + 'npm_config_strict_ssl', + 'npm_config_strict-ssl', + 'PIP_CERT', + // CURL_HOME / WGETRC redirect curl/wget at attacker rc files + // (`$CURL_HOME/.curlrc`, the file WGETRC names) whose `proxy`, `cacert`, or + // `insecure` directives intercept or downgrade the same token-bearing + // traffic — the config-file-redirect class this list already blocks for npm + // (`npm_config_userconfig`), git (`GIT_CONFIG_GLOBAL`), and OpenSSL + // (`OPENSSL_CONF`). + 'CURL_HOME', + 'WGETRC', + // PIP_CONFIG_FILE redirects all of pip's configuration at the file it + // names — index-url, trusted-host, proxy, cert, or client-cert in an + // attacker file sends session pip traffic or credentials to attacker + // infrastructure — the same config-file-redirect class as + // npm_config_userconfig and GIT_CONFIG_GLOBAL. + 'PIP_CONFIG_FILE', + // The git command-execution env family: git runs these on any invocation in + // a session subprocess. GIT_SSH_COMMAND / GIT_SSH (its documented legacy + // counterpart, still exec'd by git for SSH transports) / GIT_EXTERNAL_DIFF + // execute a command directly; GIT_ASKPASS / GIT_PROXY_COMMAND / GIT_EDITOR + // / GIT_SEQUENCE_EDITOR are exec'd conditionally (credential prompt, proxy + // transport, editor, `git rebase -i` todo-list edit); + // GIT_EXEC_PATH redirects git's own remote-helper/subcommand lookup at an + // attacker directory and GIT_TEMPLATE_DIR plants hooks that run after the + // next clone/init; GIT_CONFIG_* injects arbitrary config (`core.hooksPath`, + // `core.fsmonitor`, …) that turns a routine `git commit` into + // attacker-code execution as the daemon user — GIT_CONFIG_PARAMETERS + // carries the same injection as one quoted string, so it is blocked with + // GIT_CONFIG_COUNT and the numbered pairs. core/utils/git-branches.ts + // scrubs the config-injection subset from the repo's own git invocations, + // so a project `.env` setting this family contradicts that model. Numbered + // GIT_CONFIG_KEY_/GIT_CONFIG_VALUE_ pairs are matched by numeric + // suffix below. + 'GIT_SSH_COMMAND', + 'GIT_SSH', + 'GIT_EXEC_PATH', + 'GIT_TEMPLATE_DIR', + 'GIT_ASKPASS', + 'GIT_PROXY_COMMAND', + 'GIT_EDITOR', + 'GIT_SEQUENCE_EDITOR', + 'GIT_EXTERNAL_DIFF', + 'GIT_CONFIG_GLOBAL', + 'GIT_CONFIG_SYSTEM', + // git merges `$XDG_CONFIG_HOME/git/config` with `~/.gitconfig`, so a + // project `.env` redirecting XDG_CONFIG_HOME plants the same config + // injection (`core.hooksPath`, …) the GIT_CONFIG_* keys block — without + // naming a git config file at all. + 'XDG_CONFIG_HOME', + 'GIT_CONFIG_COUNT', + 'GIT_CONFIG_PARAMETERS', + // git falls back to executing $SSH_ASKPASS for passphrase prompts (its + // askpass order is GIT_ASKPASS > core.askPass > SSH_ASKPASS, and ssh runs + // it whenever SSH_ASKPASS_REQUIRE=force or no terminal is available), so a + // project `.env` pointing it at an attacker script is code execution on + // any git/ssh auth challenge. SSH_ASKPASS_REQUIRE stays settable: it only + // selects *when* the askpass program runs, and with SSH_ASKPASS + // project-blocked it has no program to execute. + 'SSH_ASKPASS', + // less executes $LESSOPEN as an input preprocessor on every file a session + // views (and $LESSCLOSE on exit whenever the preprocessor ran), so a + // project `.env` setting either is attacker command execution the first + // time a session runs `less`. + 'LESSOPEN', + 'LESSCLOSE', + // node-gyp interpreter selection: node-gyp's find-python.js executes + // NODE_GYP_FORCE_PYTHON / npm_config_python / PYTHON as the build Python, + // so a project `.env` pointing them at an attacker script is code execution + // during any native-addon `npm install` in another workspace's session. + // Unlike the pure-injection loader keys these have a legitimate + // operator-shell use (selecting a real interpreter), so they are + // reject-from-project-`.env` only, not scrubbed from the launch env. + 'NODE_GYP_FORCE_PYTHON', + 'npm_config_python', + 'PYTHON', + // CPython executes $PYTHONSTARTUP at interactive startup — the Python + // analogue of NODE_REPL_EXTERNAL_MODULE. It stays reject-only (like + // PYTHON) because operators legitimately set it in their own shells. + 'PYTHONSTARTUP', + // git's documented editor fallback chain (GIT_EDITOR → core.editor → + // $VISUAL → $EDITOR) executes these exactly like the blocked GIT_EDITOR, + // and the CLI's own useLaunchEditor spawns them from ordinary interactive + // flows. + 'VISUAL', + 'EDITOR', + // npm runs `$npm_config_git` as the git binary for install-from-git and + // similar flows, so a project `.env` pointing it at an attacker script is + // the same exec redirect as the interpreter keys above. + 'npm_config_git', + // The CLI itself execs $BROWSER via openBrowserSecurely (core/utils/ + // secure-browser-launcher.ts) before any CI/DISPLAY gate, so a project + // `.env` pointing it at an attacker script runs on any browser-launch + // flow. + 'BROWSER', // QWEN_CLI_ENTRY is the script path daemon-spawned session processes run. // A project `.env` or settings.env fixing it turns // `cd && qwen serve` into code execution as the daemon // via an attacker-chosen ACP entrypoint, for every workspace's sessions. 'QWEN_CLI_ENTRY', + // QWEN_CDP_MCP_COMMAND is the command the daemon spawns as the + // browser-automation MCP adapter, and QWEN_SERVE_CDP_TUNNEL_OVER_WS + // switches that tunnel surface on. A project `.env` or settings.env fixing + // either hijacks the daemon the same way QWEN_CLI_ENTRY does; values the + // operator set in the daemon's launch env still apply. + 'QWEN_CDP_MCP_COMMAND', + 'QWEN_SERVE_CDP_TUNNEL_OVER_WS', // DEV gates the daemon's inherited-loader-env scrub (run-qwen-serve.ts); // only the dev harness (scripts/dev.js) stamps it into the launch env. A // project file setting it would silently keep loader vars in the base env @@ -69,8 +193,29 @@ const HARDCODED_PROJECT_ENV_EXCLUSIONS: ReadonlySet = new Set( PROJECT_ENV_HARDCODED_EXCLUSIONS.map((key) => key.toLowerCase()), ); +// Command-scope git config injection uses numbered GIT_CONFIG_KEY_ / +// GIT_CONFIG_VALUE_ pairs read up to GIT_CONFIG_COUNT — an unbounded index, +// so match them by numeric suffix rather than listing literals. Git only +// reads decimal-numbered pairs (GIT_CONFIG_KEY_%d up to the count), so a key +// with an empty or nonnumeric suffix (e.g. GIT_CONFIG_KEY_CACHE) is a +// project-defined variable Git never consumes and it must stay settable. +// (core/utils/git-branches.ts scrubs the same family by bare prefix; that +// over-scrub is harmless, but a denylist rejection freezes the key, so this +// gate matches precisely.) GIT_CONFIG_COUNT alone already neutralizes the +// pairs, but rejecting the pairs too matches the repo's existing git scrub. +const HARDCODED_PROJECT_ENV_EXCLUSION_PATTERNS = [ + /^git_config_key_\d+$/u, + /^git_config_value_\d+$/u, +] as const; + export function isHardcodedProjectEnvExclusion(key: string): boolean { - return HARDCODED_PROJECT_ENV_EXCLUSIONS.has(key.toLowerCase()); + const lowerKey = key.toLowerCase(); + return ( + HARDCODED_PROJECT_ENV_EXCLUSIONS.has(lowerKey) || + HARDCODED_PROJECT_ENV_EXCLUSION_PATTERNS.some((pattern) => + pattern.test(lowerKey), + ) + ); } export const HOME_ENV_BOOTSTRAP_KEYS = [ @@ -127,6 +272,20 @@ export const INHERITED_LOADER_ENV_KEYS = [ 'npm_config_script_shell', 'npm_config_prefix', 'NODE_PATH', + // OPENSSL_CONF points Node's startup crypto init at an attacker `.cnf` + // whose `nodejs_conf` section can dlopen an arbitrary engine/provider `.so` + // before any user code runs — a pure code-injection vector with no benign + // cross-workspace inheritance, so it is scrubbed like NODE_OPTIONS. + 'OPENSSL_CONF', + // NODE_REPL_EXTERNAL_MODULE makes a spawned `node` REPL require() an + // attacker file at startup. npm_config_node_gyp overrides the node-gyp + // *script* npm's shim runs verbatim (`"$npm_config_node_gyp" "$@"`), and + // npm_config_init_module is require()d by `npm init` (even `-y`). All three + // are executable/module redirects with no legitimate login-shell use, so + // they join the scrubbed loader set rather than the reject-only tier. + 'NODE_REPL_EXTERNAL_MODULE', + 'npm_config_node_gyp', + 'npm_config_init_module', 'LD_PRELOAD', 'LD_AUDIT', 'DYLD_INSERT_LIBRARIES', @@ -166,10 +325,15 @@ export function isLoaderEnvKey(key: string): boolean { return canonical.startsWith('bash-func-') || LOADER_ENV_KEYS.has(canonical); } -export function scrubInheritedLoaderEnv(env: NodeJS.ProcessEnv): string[] { +export function scrubInheritedLoaderEnv( + env: NodeJS.ProcessEnv, + snapshotInto?: Map, +): string[] { const removedKeys: string[] = []; for (const key of Object.keys(env)) { if (isLoaderEnvKey(key)) { + const value = env[key]; + if (snapshotInto && value !== undefined) snapshotInto.set(key, value); delete env[key]; removedKeys.push(key); } @@ -185,8 +349,9 @@ export function scrubAndReportInheritedLoaderEnv( env: NodeJS.ProcessEnv, commandLabel: string, processLabel: string, + snapshotInto?: Map, ): string[] { - const removedKeys = scrubInheritedLoaderEnv(env); + const removedKeys = scrubInheritedLoaderEnv(env, snapshotInto); if (removedKeys.length > 0) { writeStderrLineSafe( `${commandLabel}: scrubbed inherited loader env vars from the ` + @@ -197,6 +362,76 @@ export function scrubAndReportInheritedLoaderEnv( return removedKeys; } +// Concurrent embedded daemons in one process (a documented supported config — +// see acp-bridge/src/bridgeOptions.ts `childEnvOverrides`) share this +// process's `process.env`. A per-daemon scrub+restore over that shared object +// races: the second daemon boots into an already-scrubbed env (nothing to +// scrub, nothing to restore), then the first daemon's close() restores the +// loader vars into the shared env and re-poisons the survivor's session +// subprocesses — reopening #8653 for the still-live daemon. Coordinate the +// scrub of `process.env` process-globally: each acquire snapshots the loader +// values present at that boundary, and only the last release (refcount back +// to zero) restores them. +let sharedProcessEnvScrubDepth = 0; +const sharedProcessEnvScrubOriginals = new Map(); + +export interface InheritedLoaderEnvScrubHandle { + /** Loader keys this acquire removed from the shared env (empty for a nested acquire whose env was already scrubbed). */ + readonly removedKeys: readonly string[]; + /** Idempotent; restores the snapshotted originals only when the last holder releases. */ + release(): void; +} + +// Scrubs loader vars from a shared, live env (the daemon passes its own +// `process.env`), reference-counted so it is safe to call from overlapping +// daemon instances in one process. The caller owns the `process.env` +// reference so the serve-surface process.env guard still sees the access; +// one-shot scrubs of a private env object (ACP child / channel worker boot) +// use the same reporting scrub without a snapshot. Concurrent callers must +// pass the same shared env object for the snapshot/restore refcount to be +// correct. +export function acquireInheritedLoaderEnvScrub( + env: NodeJS.ProcessEnv, + commandLabel: string, + processLabel: string, +): InheritedLoaderEnvScrubHandle { + sharedProcessEnvScrubDepth++; + // Snapshot on every acquire, not just the first: the embedding host can + // assign loader keys between acquires, and the scrub below deletes them + // with no record — the final release would then leave the assignment + // absent or restore a stale pre-scrub value, corrupting the shared env. + // The newest value observed at any acquire boundary is the one the final + // restore must bring back. The snapshot is recorded inside the scrub's + // single pass over the shared env below. + const removedKeys = scrubAndReportInheritedLoaderEnv( + env, + commandLabel, + processLabel, + sharedProcessEnvScrubOriginals, + ); + let released = false; + return { + removedKeys, + release() { + if (released) return; + released = true; + sharedProcessEnvScrubDepth--; + if (sharedProcessEnvScrubDepth > 0) return; + for (const [key, value] of sharedProcessEnvScrubOriginals) { + // A later legitimate assignment wins over the restore. + if (!Object.hasOwn(env, key)) env[key] = value; + } + sharedProcessEnvScrubOriginals.clear(); + }, + }; +} + +/** Test-only: reset the shared process-env scrub refcount/snapshot. */ +export function resetInheritedLoaderEnvScrubForTesting(): void { + sharedProcessEnvScrubDepth = 0; + sharedProcessEnvScrubOriginals.clear(); +} + // Loader keys rejected from .env/settings.env used to apply on some // application paths before the denylist existed; dropping them silently // would send upgrade investigations everywhere except here. Report once per @@ -223,6 +458,19 @@ export function setLoaderKeyRejectionReporter( loaderKeyRejectionReporter = reporter; } +// Overlapping daemons in one process each install their own reporter at boot +// and clear it on close. An unconditional clear lets the first daemon's +// close() drop the survivor's reporter, silently routing its fresh rejections +// to the stderr fallback. Clear only when we are still the active reporter, so +// a co-resident daemon that installed after us keeps its own. +export function clearLoaderKeyRejectionReporterIfCurrent( + reporter: LoaderKeyRejectionReporter, +): void { + if (loaderKeyRejectionReporter === reporter) { + loaderKeyRejectionReporter = undefined; + } +} + // candidateKeys is the raw key list of a parsed source (e.g. // Object.keys(parsedEnv)); the intersection with the loader denylist happens // here so every application site reports with identical matching semantics. diff --git a/packages/cli/src/serve/fast-path.test.ts b/packages/cli/src/serve/fast-path.test.ts index 6763297fa2c..d2341edd38f 100644 --- a/packages/cli/src/serve/fast-path.test.ts +++ b/packages/cli/src/serve/fast-path.test.ts @@ -1895,6 +1895,51 @@ describe('serve fast path environment bootstrap', () => { } }); + // The fast-path settings.env loop rejects hardcoded exclusions through the + // case-folded isHardcodedProjectEnvExclusion predicate. Every other + // settings.env fixture uses loader/allowlisted keys, so a regression to + // exact-case membership would ship green — this pins a lowercase hardcoded + // key (the entrypoint hijack) being rejected from settings.env. + it('never applies a case-variant hardcoded exclusion from settings.env on the fast path', () => { + useTempQwenHome(); + const trackedKeys = [ + 'QWEN_CLI_ENTRY', + 'qwen_cli_entry', + 'node_extra_ca_certs', + ] as const; + const previous: Record = {}; + for (const key of trackedKeys) { + previous[key] = process.env[key]; + delete process.env[key]; + } + tempWorkspace = realpathSync( + mkdtempSync(join(os.tmpdir(), 'qws-fast-path-settings-hardcoded-')), + ); + + try { + loadServeFastPathEnvironment( + { + env: { + qwen_cli_entry: '/workspace-a/evil-entry-lower.js', + node_extra_ca_certs: '/workspace-a/evil-ca.pem', + }, + }, + tempWorkspace, + ); + expect(process.env['qwen_cli_entry']).toBeUndefined(); + expect(process.env['QWEN_CLI_ENTRY']).toBeUndefined(); + expect(process.env['node_extra_ca_certs']).toBeUndefined(); + } finally { + for (const key of trackedKeys) { + if (previous[key] === undefined) { + delete process.env[key]; + } else { + process.env[key] = previous[key]; + } + } + } + }); + // Daemon-side loadSettings() skips the .env load for untrusted workspaces // and only re-runs it later for trusted ones, so a loader key rejected at // boot would vanish without a breadcrumb unless the fast path reports it. @@ -2011,6 +2056,11 @@ describe('serve fast path environment bootstrap', () => { writeFileSync(join(secondWorkspace, '.env'), 'LD_PRELOAD=/hijack.so\n'); try { + // The stash is a module-global; drain any residue left by earlier tests + // so this exact-match assertion does not depend on test declaration + // order (partial `-t` selection, --sequence.shuffle, or a new load- + // bearing test inserted before this one). + consumeServeFastPathRejectedLoaderKeys(); loadServeFastPathEnvironment({}, firstWorkspace); loadServeFastPathEnvironment( { env: { LD_PRELOAD: '/hijack.so', NODE_OPTIONS: '--inspect' } }, diff --git a/packages/cli/src/serve/process-env-guard.test.ts b/packages/cli/src/serve/process-env-guard.test.ts index dd800f0fc4d..f642a02b9a5 100644 --- a/packages/cli/src/serve/process-env-guard.test.ts +++ b/packages/cli/src/serve/process-env-guard.test.ts @@ -155,13 +155,12 @@ const allowedProcessEnvAccesses = normalizeAllowances([ 'computed:QWEN_SERVE_PROMPT_DEADLINE_MS_ENV': 1, 'computed:QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS_ENV': 1, 'computed:RUNTIME_STARTUP_TIMEOUT_ENV': 1, - 'computed:key': 1, 'key:DEV': 1, 'key:QWEN_CODE_IDE_WORKSPACE_PATH': 1, 'key:QWEN_SERVE_NO_MCP_POOL': 1, 'key:QWEN_SERVE_NO_PERSISTENT_REGISTRATION': 1, 'key:VITEST_WORKER_ID': 1, - whole: 7, + whole: 5, }, }, ], diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 8dd0b731907..eb9f6de7eb9 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -21,9 +21,11 @@ import express, { import { writeStderrLine, writeStdoutLine } from '../utils/stdioHelpers.js'; import { isWithinRoot } from '../config/path-comparison.js'; import { - scrubAndReportInheritedLoaderEnv, + acquireInheritedLoaderEnvScrub, + clearLoaderKeyRejectionReporterIfCurrent, scrubInheritedLoaderEnv, setLoaderKeyRejectionReporter, + type LoaderKeyRejectionReporter, } from '../config/shared-env-keys.js'; import { DEFAULT_COMPACTED_REPLAY_MAX_BYTES, @@ -1966,6 +1968,10 @@ interface DaemonLoggerLifecycleCallbacks { // startup fails after the scrub — the close() path is unreachable then, // and an embedded caller must not keep a permanently scrubbed env. scrubApplied(restoreScrubbedLoaderEnv: () => void): void; + // Called with the loader-key rejection reporter this run installed, so the + // startup-failure catch can clear it only when it is still the active one — + // a co-resident daemon that installed after us must keep its own reporter. + reporterInstalled(reporter: LoaderKeyRejectionReporter): void; } /** @@ -2029,6 +2035,7 @@ export async function runQwenServe( let daemonLog: DaemonLogger | undefined; let owner: 'startup' | 'handle' | 'signal' = 'startup'; let restoreScrubbedLoaderEnv: (() => void) | undefined; + let installedLoaderRejectionReporter: LoaderKeyRejectionReporter | undefined; try { return await runQwenServeImpl(optsIn, deps, { initialized: (logger) => { @@ -2043,12 +2050,18 @@ export async function runQwenServe( scrubApplied: (restore) => { restoreScrubbedLoaderEnv = restore; }, + reporterInstalled: (reporter) => { + installedLoaderRejectionReporter = reporter; + }, }); } catch (error) { // Startup failed after the scrub and (when the logger was up) the // reporter install; the close() path that reverts both is unreachable. - if (daemonLog) { - setLoaderKeyRejectionReporter(undefined); + // Clear only our own reporter so a co-resident daemon keeps its own. + if (installedLoaderRejectionReporter) { + clearLoaderKeyRejectionReporterIfCurrent( + installedLoaderRejectionReporter, + ); } if (daemonLog && owner === 'startup') { const startupLog = daemonLog; @@ -2102,9 +2115,6 @@ async function runQwenServeImpl( ); } preResolveServeFastPathHomeEnvOverrides(); - // Snapshot before any scrub: close() restores the host's launch - // environment from this copy, not from the (possibly scrubbed) base env. - const launchEnv = { ...process.env }; const baseEnv: NodeJS.ProcessEnv = { ...process.env, ...(optsIn.memoryProjectScope !== undefined @@ -2130,20 +2140,19 @@ async function runQwenServeImpl( Object.freeze(baseEnv); // The daemon process itself is done with loader vars either way: // session-shell subprocesses run here with process.env while their cwd is - // another workspace. The scrub is reverted on close() so an embedded - // caller reusing the host process gets its launch environment back. - const scrubbedLoaderEnvKeys = scrubAndReportInheritedLoaderEnv( + // another workspace. The scrub is reference-counted (see + // acquireInheritedLoaderEnvScrub) so overlapping embedded daemons in one + // process do not restore each other's loader vars mid-flight, and reverted + // on close() so an embedded caller reusing the host process gets its launch + // environment back. + const loaderEnvScrub = acquireInheritedLoaderEnvScrub( process.env, 'qwen serve', 'daemon', ); + const scrubbedLoaderEnvKeys = loaderEnvScrub.removedKeys; const restoreScrubbedLoaderEnv = (): void => { - for (const key of scrubbedLoaderEnvKeys) { - if (Object.hasOwn(process.env, key)) continue; - const value = launchEnv[key]; - if (value === undefined) continue; - process.env[key] = value; - } + loaderEnvScrub.release(); }; loggerLifecycle.scrubApplied(restoreScrubbedLoaderEnv); @@ -2666,12 +2675,20 @@ async function runQwenServeImpl( // capability checks, settings reloads); boot stderr is long gone by then, // so fresh loader-key rejections must land in the durable daemon log or // they vanish without a diagnostic. - setLoaderKeyRejectionReporter((source, freshKeys) => { + const loaderRejectionReporter: LoaderKeyRejectionReporter = ( + source, + freshKeys, + ) => { daemonLog.warn( 'rejected loader-affecting env keys; they were not applied', - { source, rejectedKeys: freshKeys }, + { + source, + rejectedKeys: freshKeys, + }, ); - }); + }; + setLoaderKeyRejectionReporter(loaderRejectionReporter); + loggerLifecycle.reporterInstalled(loaderRejectionReporter); // Boot stderr rarely survives desktop/systemd daemon launches, so persist // the scrub decision in the durable daemon log as well. if (scrubbedLoaderEnvKeys.length > 0) { @@ -7230,7 +7247,9 @@ async function runQwenServeImpl( daemonLog.info('daemon stopped'); } }); - setLoaderKeyRejectionReporter(undefined); + clearLoaderKeyRejectionReporterIfCurrent( + loaderRejectionReporter, + ); await daemonLog.close(); } restoreScrubbedLoaderEnv();