fix(responses): stop a namespaced MCP exec from authorizing bare shell aliases - #2966
Conversation
…l aliases
A namespaced tool named `exec` was aliased into the declared-name set under its
bare name as well as its flattened wire name. That bare entry is not just a name:
`normalizeDeclaredToolName` treats `declared.has("exec")` as the switch that maps
an emitted `exec_command`, `shell_command`, or `apply_patch` onto the code-mode
shell tool. An MCP server advertising its own `exec` therefore handed the turn a
bare shell tool the request never declared, and the undeclared-tool guard accepted
all three helper names.
Withhold only that one bare alias when `exec` is declared inside a namespace. The
flattened name still authorizes the tool, a namespaced call still matches by its
full wire name, and every other inner name keeps the bare alias it had before. A
request that separately declares a top-level `exec` is unchanged.
…c alias `buildToolBridgeMaps` also aliases a namespaced tool under its bare name when the caller's `tool_choice` selected it unambiguously, which the bridge needs to route the call back. `refreshUndeclaredToolGuard` merges that map into the same declared set, so a request whose only `exec` is an MCP tool selected by a bare `tool_choice` re-entered the normalization switch and the three helper names were authorized again. Admit that one alias only when the caller's own catalog declared a bare `exec`. Selecting an MCP `exec` is not a declaration of the code-mode shell tool. Bridge routing is untouched, so the namespaced call still returns under its bare selector.
Codex groups ordinary top-level tools under the reserved `functions` namespace. The namespace parser deliberately lowers those children without an MCP namespace, but the undeclared-tool collector treated `functions.exec` like an MCP tool and withheld its bare name. On a replay continuation, outbound and bridge-map names are intentionally not allowed to widen the current caller snapshot. The resulting set contained only `functions__exec`, so legitimate bare `exec` and its normalized helper calls were refused with 502. Treat the reserved namespace as the top-level container it is. Genuine MCP namespaces still withhold the bare exec alias, and a focused replay regression proves the current catalog authorizes bare exec after history expansion.
A namespaced MCP exec selected through a unique bare tool_choice was kept out of the guard's declared-name set, but its upstream bare call was then rejected with a 502. Restore only the request-bounded bare selector to its namespaced identity before guard inspection while keeping code-mode helper normalization disabled.
|
Warning Review limit reachedNext included review available in 23 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ 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. |
|
✅ Deterministic PR hygiene checks passed. |
![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: 67f09620be
ℹ️ 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".
| ? [[alias, { | ||
| namespace: identity.namespace, | ||
| name: identity.name, | ||
| kind: identity.freeform ? "custom" as const : "function" as const, |
There was a problem hiding this comment.
When a bare tool_choice selects a namespaced function and the routed provider responds with a same-named custom_tool_call (or vice versa), this alias is still restored and accepted. Although the map records kind, restoreRoutedNamespaceCalls matches only the name, after which the undeclared-tool guard sees the authorized namespace__name and relays an item type the client never declared, potentially with the wrong input/arguments shape. Make this restoration conditional on the response item type matching identity.kind, leaving mismatches for the guard to reject.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 76 / 80설명 이 PR은 MCP로 선언된 이름공간 고치는 축은 두 갈래입니다. 메인테이너 수리 커밋이 기여자 #2953의 “전부 버리기”가 깨뜨린 정당한 경로를 되돌립니다. bare 라인 - 문제 src/server/responses/core.ts src/server/responses-undeclared-tool-guard.ts src/types/tools.ts tests/responses-undeclared-tool-guard.test.ts - 대소문자·유니코드·구분자·중첩 네임스페이스 우회와 셸 별칭 502, bare 선택 성공 케이스가 본문 감사 목록과 맞습니다. 메인테이너의 판단이 필요한 지점
너의 추천 보안 픽으로 이 댓글은 grok-bot이 작성했습니다 |
2 participants
Summary
Carries #2953 by @luvs01, plus one repair commit. A namespaced MCP tool named
execcould lend itsnormalization authority to the bare shell aliases
apply_patch,exec_commandandshell_command,which the undeclared-tool guard is supposed to reject.
Opened as a maintainer PR because pushing the repair to the contributor branch reset its
review-readiness checklist and returned it to draft; that attestation is the author's. The cherry-pick
preserves @luvs01's authorship across all three original commits.
Carries #2953. Close it as carried once this lands.
Review boundary
This is an authorization boundary, so it wants a security review rather than a routine pass. The
audited properties, each probed directly:
execstays out of the normalization-authority set, and bridge maps cannot restore it.apply_patch,exec_commandandshell_commandremain fail-closed with 502.The repair
The original also rejected a legitimate case: when a namespaced tool is selected through a bare
tool_choice, the bridge creates an unambiguous bareexecalias for it. That alias was discardedwith everything else, so an upstream response calling bare
execreturned 502 despite having invokedthe sole declared MCP tool.
An authorized bare selector is now restored to its namespaced identity before the guard runs, using
the request-bounded
toolBridgeMaps.toolNsMap. Only an unambiguous single-candidate mapping isrestored; ambiguity still fails closed, and bare
execstill holds no normalization authority.Verification
Based on
dev@dca16949b.bun test tests/responses-undeclared-tool-guard.test.ts tests/namespace-tool-compat.test.ts tests/core-lab-boundary.test.ts→ 132 pass, 0 failbun x tsc --noEmit→ cleanThe new end-to-end case selects the sole namespaced
execby baretool_choiceand asserts theupstream bare
execsucceeds, alongside assertions that the three shell aliases still return 502.tests/core-lab-boundary.test.tsis in the gate becausesrc/server/responses/core.tsis a core-pathfile that must not reach
src/lab.Checklist