Repository navigation
🤖 fix: load Codex .mcp.json env_vars and untyped url entries; flag custom_header without headerName - #6487
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3beec0c361
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Round 1 fixes are pushed: own-property env reads and |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56e610bb48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 56e610bb48
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
@codex review Round 2 fix is pushed: an |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Held: this PR waits for #6498. Do not merge it before #6498 lands. Saved plugin imports and enablement store only component names. A stdio server that an older parser skipped can come back under an enabled name and start without consent. This PR adds a new way for that to happen ( |
# Conflicts: # src/node/services/agentPlugins/installService.ts # src/node/services/mcpServerManager.ts
…; docs: document env_vars
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
# Conflicts: # src/node/services/agentPlugins/installService.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 362874dd0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…coder#6581) ## Summary Managed plugin stdio MCP servers no longer inherit the whole host environment. Xum starts them with a short allowlist of host variables plus the `env` their MCP configuration declares. Tokens such as `GITHUB_TOKEN` or `OPENAI_API_KEY` from the user's shell are no longer passed to a plugin server. This is non-inheritance, not a sandbox: the server still runs as the same OS user and can read what that user can read (for example `/proc/<xum pid>/environ` on Linux). The docs say so. User-configured servers and the bash tool keep the full environment. Closes coder#6509 ## Background `LocalBaseRuntime.exec` spread all of `process.env` under every stdio launch (found by the Codex security review of coder#6487, P1). A plugin that names no variables still received every secret in the shell. What other hosts do (sources read for this PR): 1. Codex (`openai/codex` @ `78dc250`): `codex-rs/rmcp-client/src/utils.rs` `create_env_for_mcp_server` clears the env and passes only `DEFAULT_ENV_VARS` (Unix) or `WINDOWS_CORE_ENV_VARS` (`codex-rs/protocol/src/shell_environment.rs`), the CA bundle keys (`network-proxy/src/certs.rs`), the names in `env_vars`, and the literal `env`. No proxy variables. 2. MCP TypeScript SDK (`packages/client/src/client/stdio.ts`): `getDefaultEnvironment()` passes `HOME LOGNAME PATH SHELL TERM USER` and drops values that start with `()`. 3. Agent Plugins 1.0 (MCP runtime page): the client chooses the base environment and can omit or sanitize ambient variables. ## Implementation 1. `childProcessEnv.ts`: `pickMinimalChildEnv` keeps the Codex lists plus CA bundle paths (`SSL_CERT_DIR` added, `CODEX_CA_CERTIFICATE` left out, `HOME` added on Windows for Git Bash). Windows names match case-insensitively. Exported bash functions are dropped. 2. `ExecOptions.hostEnv` (`"inherit"` default, or `"minimal"`). Only `LocalBaseRuntime` reads the host env, so remote runtimes ignore it. 3. `prepareStdioLaunch` returns `hostEnv: "minimal"` for servers with plugin provenance. Both spawn sites pass it: workspace startup (`startStdioInstance`) and the named Test connection (`runServerTest`). 4. Docs: `docs/config/mcp-servers.mdx` lists what a plugin server receives. ## Validation 1. `LocalBaseRuntime` test: a host variable set in `process.env` is invisible with `hostEnv: "minimal"` and visible by default. Declared `env` and `HOME` pass in both. 2. Manager tests for the startup path and the Test button: plugin servers get `"minimal"`, user servers get `"inherit"`. I removed each wiring line in a scratch edit and saw the matching test fail. ## Risks 1. Behavior change for plugins that read ambient variables. A plugin server that read a token from the shell (for example `GITHUB_PERSONAL_ACCESS_TOKEN`) fails at startup or on its first tool call. Servers started through `npx`/`uvx` or version-manager shims also lose `NVM_DIR`, `VOLTA_HOME`, `npm_config_registry`, `UV_*` and `XDG_CACHE_HOME`, as in Codex. Remedy: the plugin declares the value in `env`, or, after coder#6487 lands, in Codex `env_vars`. 2. Proxies: `HTTP(S)_PROXY`/`NO_PROXY` are not passed (Codex parity). Proxy URLs can carry credentials. A plugin server that downloads through a proxy fails. 3. coder#6487 must add its forwarded `env_vars` names on top of this base when it rebases, or those names will not reach the server. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$38.84`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=38.84 -->
… plugin servers (coder#6690) ## Summary Four follow-ups from the coder#6487 reviews for Codex `.mcp.json` plugin servers. Closes coder#6646. ## Changes 1. **Auth header names (item 1).** In `openaiMcpAuth.ts`, a `headerName` that is not a valid header name, or is `Host`, now gets its own warning ("must be a valid HTTP header name other than Host"). Before, it got "needs headerScheme custom_header", even when the scheme was custom_header. A `custom_header` scheme without a valid name is now dropped as well, not only when the name is missing. Nothing reads these fields yet, because Xum has no api_key support. 2. **OAuth fields on stdio servers (item 2).** Xum reads OAuth only for remote servers. A stdio entry with `oauth`, `scopes` or `oauth_resource` now gets a review warning that names them, with or without `env_vars`. Before, the fields were dropped silently. For this, the loader now reports translation warnings for stdio servers too. The remote OAuth handling moved, unchanged, into `remoteOAuthMessages`. 3. **Loader variables (item 3).** `env_vars` names that change how the server loads code (`LD_PRELOAD`, `LD_LIBRARY_PATH`, `DYLD_*`, `NODE_OPTIONS`) still load, but the review names them in a warning (defense in depth). 4. **Windows env names (item 4).** On Windows, env names are case-insensitive. The reserved-name check for `env_vars` and for `env` keys now ignores case there, so `plugin_data` cannot shadow `PLUGIN_DATA`. Duplicate `env_vars` names that differ only in case are merged, keeping the first spelling. Other platforms are unchanged. ## Validation New tests failed on main and pass now: - the header-name warnings and dropped scheme (`openaiMcpAuth.test.ts`); - the stdio OAuth and loader warnings through the real loader (`mcpConfig.test.ts`); - the Windows rules, with `process.platform` set to `win32` in the test, as other tests in the repo do. The same test checks that Linux keeps case-sensitive names. ## Risks Low. Items 1 to 3 only add or reword warnings. Item 4 refuses a server on Windows only when it names a case variant of a reserved variable. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$91.52`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=91.52 -->
This branch was successfully deployed
1 participant
Summary
Xum now loads three Codex
.mcp.jsonshapes that it used to skip or misreport. A stdio server withenv_varsloads and receives the named host variables. A remote entry with aurland notypeloads as streamable HTTP. Anapi_keyauth block withheaderScheme: custom_headerand noheaderNameis reported as malformed.Refs #6297. This PR covers three of its items: "Codex
env_varson stdio servers", "URL entries withouttype" and "custom_headerwithoutheaderName".Background
Codex review found these gaps after track A merged main (review threads
PRRT_kwDOPxxmWM6rv0UV,PRRT_kwDOPxxmWM6rv0UaandPRRT_kwDOPxxmWM6rv0Uh).env_varsas the way to pass selected host environment variables to a stdio server (OpenAI plugins guide). Xum rejected the field as unknown, so the server did not load.urlentry withouttypeas streamable HTTP (codex-mcp/src/plugin_config.rs). Xum skipped it as an unknown type.headerNameforcustom_header(fixed bearer tokens and API keys). Xum listed the block as an unsupported setting instead of a malformed one.Implementation
All three changes apply to the Codex dialect only. A root
mcp.jsonstays strict:env_varsand an untypedurlare still errors there. A new test covers that.env_vars. The Codex translator turnsenv_varsinto a newforwardEnvlist of names on the stdio server record. Xum copies the semantics from Codex:env_vars: nullandsource: nullcount as absent, as Codex's optional fields do.{ name, source }(McpServerEnvVarinconfig/src/mcp_types.rs).source: "remote". Codex refuses it for a plugin that runs in the local environment (bind_environment_env_varsinplugin_config.rs).env_varson a remote server. Codex does the same (throw_if_setinmcp_types.rs).prepareStdioLaunchcopies only the named variables from the host. It reads only own properties of the host environment, so a name such asconstructoris not an inherited value. It skips a name that the host does not set, as Codex'screate_env_for_mcp_serverdoes (rmcp-client/src/utils.rs). A configuredenvvalue wins over a host value.PLUGIN_ROOTandPLUGIN_DATAinenv_vars, as it already does inenv.Xum reads only the names at load time. It never stores or shows a host value.
The install consent preview shows the names, for example
node (host env: 'API_TOKEN'). The consent fingerprint includes the names in the order they first appear (duplicates are dropped), so an update that adds, removes or reorders a name needs new consent. The server signature includes them too, so a changed list restarts the server.Untyped
url. The translator infersstreamable-httpwhen an entry has aurland notype. This mirrors how it already infersstdiofrom acommand.custom_headerwithoutheaderName.readAuthnow adds a warning thatheaderNameis required, and it dropsheaderScheme. A badheaderNamealready used this warning class.Validation
origin/main. Then I made it pass.bun test(pinned Bun 1.4.3) ofmcpConfig.test.ts,openaiMcpAuth.test.ts,installService.test.tsandmcpServerManager.test.ts: 558 pass, 0 fail.make static-checkpasses.Resolved since the hold
forwardEnvis an included field in the fingerprint table (componentConsent.ts), so adding anenv_varsname changes the fingerprint and needs a review. The old update-onlystdioFingerprinthelper is gone, because main's update review uses the same consent fingerprint.env_varsnames are copied into the server's declaredenv, so they reach the server on top of that base. A new test launches a plugin server through the real host runtime and checks that a named variable arrives and an unnamed one does not.env_vars.Risks
The risk is low. The
env_varsand untyped-urlchanges apply only to Codex.mcp.jsonfiles, and those entries failed to load before. A stdio server withoutenv_varskeeps the same launch environment, fingerprint and signature, because each of them leaves out an unsetforwardEnv. AddingforwardEnvto the consent fingerprint does not change the fingerprint of an existing server, becausepickskips an unset field.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$0.00