[WRONG BRANCH] fix: isolate namespaced exec authorization - #331
Conversation
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review 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 (2)
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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
![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: b77c01ba74
ℹ️ 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".
| // declaration set, however: bare `exec` also authorizes legacy shell bridge aliases. | ||
| if (namespace) { | ||
| names.add(namespacedToolName(namespace, name)); | ||
| if (name === "exec") CATALOGS_WITH_NAMESPACED_EXEC.add(names); |
There was a problem hiding this comment.
When a routed request declares only a namespaced exec and the provider echoes its accepted bare child name, this WeakSet marker is lost in the production path: refreshUndeclaredToolGuard copies the collected names into the separate declaredWireToolNames set at src/server/responses/core.ts:3023-3027. Consequently CATALOGS_WITH_NAMESPACED_EXEC.has(declared) is false and the valid call is rejected with a 502 as undeclared. Carry this authority explicitly into the merged catalog (rather than attaching it to the temporary Set identity) and add coverage through handleResponses, which performs the merge.
AGENTS.md reference: src/AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
1 participant
Motivation
exec, allowing unnamespacedexec_command/shell_commandtool calls to be accepted even when the request did not declare a bareexecand thereby violating the request-local tool catalog boundary.Description
execto the declared-name set whenexecis declared inside anamespace, instead adding only the flattened namespaced wire name and tracking catalogs that contain a namespacedexecwith aWeakSet.execas authorized only if the catalog actually declared a bareexec, and avoid legacy-normalizingexec_command/shell_commandin the namespaced-only case.execdeclaration does not authorizeexec_commandorshell_commandand adjust related expectations/comments accordingly.Testing
bun test tests/responses-undeclared-tool-guard.test.tsand the focused suite passed (65 tests passed).bun run typecheckandbun run privacy:scan, both completed successfully.bun run testin this environment and observed the majority of suites pass, thoughtests/native-profile-drain-server.test.tstimed out in this environment; the targeted guard behavior is validated by the focused tests and a local Bun 1.4.0 run of the focused suite.Codex Task