fix(gateway): normalize MCP schemas for Codex loopback#70763
Conversation
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟠 Codex MCP per-server auto-approval enabled for any `openclaw` loopback URL (potential tool-approval bypass + token exposure)
Description
Vulnerable code: if (isOpenClawLoopbackMcpServer(name, server)) {
next.default_tools_approval_mode = "approve";
}RecommendationConstrain auto-approval to the internally-generated OpenClaw MCP server only, rather than any config entry named Suggested mitigations (choose one or combine):
Example (flag-based): // when creating the OpenClaw-injected server
const injected = { url, headers, __openclaw_internal: true };
function normalizeCodexServerConfig(name: string, server: BundleMcpServerConfig) {
const next: Record<string, unknown> = {};
applyCommonServerConfig(next, server);
if (server.__openclaw_internal === true) {
next.default_tools_approval_mode = "approve";
}
// ...
}Also consider not sending OpenClaw session credentials ( Analyzed PR: #70763 at commit Last updated on: 2026-04-23T20:47:43Z |
PR SummaryMedium Risk Overview Updates Codex CLI bundle-MCP injection to mark the generated Pins the Docker Codex CLI smoke lane to Adds an optional Docker live schema-probe plugin + preflight assertion to validate real loopback Reviewed by Cursor Bugbot for commit 3410e8d. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR fixes MCP schema normalization so that Confidence Score: 5/5Safe to merge; the core schema fix is correct and the only open concern (IPv6 loopback) was flagged in a previous thread. No new P0 or P1 findings beyond what was already raised. The mcp-http.schema.ts restructure correctly ensures properties is injected unconditionally for all object schemas, the test coverage is thorough, and the surrounding tooling changes (version pin, approval mode, service tier) appear intentional. No files require special attention beyond the previously flagged IPv6 gap in src/agents/cli-runner/bundle-mcp.ts. Reviews (2): Last reviewed commit: "docs: update config baseline" | Re-trigger Greptile |
| function isOpenClawLoopbackMcpServer(name: string, server: BundleMcpServerConfig): boolean { | ||
| return ( | ||
| name === "openclaw" && | ||
| typeof server.url === "string" && | ||
| /^https?:\/\/(?:127\.0\.0\.1|localhost):\d+\/mcp(?:[?#].*)?$/.test(server.url) | ||
| ); | ||
| } |
There was a problem hiding this comment.
isOpenClawLoopbackMcpServer
The regex in isOpenClawLoopbackMcpServer accepts 127.0.0.1 and localhost but not [::1]. The sibling normalizeOpenClawLoopbackUrl already handles [::1] as a third loopback variant. If the server ever binds to ::1, the default_tools_approval_mode = "approve" injection will be silently skipped, which could cause Codex to stall on approval prompts.
| function isOpenClawLoopbackMcpServer(name: string, server: BundleMcpServerConfig): boolean { | |
| return ( | |
| name === "openclaw" && | |
| typeof server.url === "string" && | |
| /^https?:\/\/(?:127\.0\.0\.1|localhost):\d+\/mcp(?:[?#].*)?$/.test(server.url) | |
| ); | |
| } | |
| function isOpenClawLoopbackMcpServer(name: string, server: BundleMcpServerConfig): boolean { | |
| return ( | |
| name === "openclaw" && | |
| typeof server.url === "string" && | |
| /^https?:\/\/(?:127\.0\.0\.1|localhost|\[::1\]):\d+\/mcp(?:[?#].*)?$/.test(server.url) | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cli-runner/bundle-mcp.ts
Line: 144-150
Comment:
**IPv6 loopback not covered by `isOpenClawLoopbackMcpServer`**
The regex in `isOpenClawLoopbackMcpServer` accepts `127.0.0.1` and `localhost` but not `[::1]`. The sibling `normalizeOpenClawLoopbackUrl` already handles `[::1]` as a third loopback variant. If the server ever binds to `::1`, the `default_tools_approval_mode = "approve"` injection will be silently skipped, which could cause Codex to stall on approval prompts.
```suggestion
function isOpenClawLoopbackMcpServer(name: string, server: BundleMcpServerConfig): boolean {
return (
name === "openclaw" &&
typeof server.url === "string" &&
/^https?:\/\/(?:127\.0\.0\.1|localhost|\[::1\]):\d+\/mcp(?:[?#].*)?$/.test(server.url)
);
}
```
How can I resolve this? If you propose a fix, please make it concise.db9e187 to
3410e8d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3410e8d. Configure here.
| function normalizeCodexServerConfig( | ||
| name: string, | ||
| server: BundleMcpServerConfig, | ||
| ): Record<string, unknown> { |
There was a problem hiding this comment.
Parameter name shadows loop variable in same function
Low Severity
The newly added name parameter on normalizeCodexServerConfig shadows the existing for (const [name, value] of ...) loop variable on line 165. While the outer name (server config name) is only used before the loop and the inner name (header name) is used correctly within it, this shadowing creates a maintenance hazard — any future code added after the loop that intends to reference the server config name would silently get the last header name instead.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3410e8d. Configure here.
|
Landed via rebase. Verification:
Merged as |
Summary
propertiesis always emitted, including no-argument tools with{ type: "object" }default_tools_approval_mode = "approve"instead of relying on global Codex approval settings@openai/codex@0.124.0, keep the default oncodex-cli/gpt-5.5, and make the Docker lane prefer staged Codex OAuth unlessOPENCLAW_LIVE_CLI_BACKEND_AUTH=api-keyis explicittools/listSupersedes #69956. The schema fix is the minimal part of that PR; the already-landed auto-failover commits are intentionally not included here. Thanks @Chevron7Locked for the original MCP schema fix direction.
Verification
pnpm test src/agents/cli-backends.test.ts src/agents/cli-runner/bundle-mcp.test.ts src/gateway/gateway-cli-backend.live-helpers.test.ts src/gateway/server-startup.test.ts src/gateway/mcp-http.test.tsOPENCLAW_VITEST_NO_OUTPUT_TIMEOUT_MS=180000 OPENCLAW_VITEST_NO_OUTPUT_RETRY=1 pnpm check:changedOPENCLAW_SKIP_DOCKER_BUILD=1 OPENCLAW_LIVE_CLI_BACKEND_MCP_SCHEMA_PROBE=1 OPENCLAW_LIVE_CLI_BACKEND_USE_CI_SAFE_CODEX_CONFIG=0 OPENCLAW_LIVE_CLI_BACKEND_IMAGE_PROBE=0 OPENCLAW_LIVE_CLI_BACKEND_DEBUG=1 pnpm test:docker:live-cli-backend:codexDocker proof used Codex CLI
0.124.0, modelcodex-cli/gpt-5.5, staged.codex/auth.json, livetools/listschema validation, direct loopbackcroncall, and model-drivencrontool call on the first attempt.