fix(ai,policy-opa): use own-property checks when resolving per-tool approvals#16900
Merged
Conversation
…pprovals Tool sets, tool contexts, and per-tool approval maps are indexed by names that come from model output or client-supplied message history. Plain bracket access resolves names that match inherited object properties (constructor, toString, valueOf, __proto__) to values on Object.prototype, which can slip past the "unknown tool" / "unconfigured approval" guards. Add a getOwn helper (own-property check) and route tool/context/approval lookups through it, build the HITL approval-matching maps with a null prototype, and add own-property guards in policy-opa wrapMcpTools and shadow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Additional Suggestion:
refineToolInput map lookup uses plain bracket access instead of getOwn, so a tool named after an Object.prototype member (e.g. toString, valueOf) resolves to the inherited prototype method rather than undefined, silently corrupting or crashing tool-input refinement.
The per-tool `refineToolInput` map was read with plain bracket access, so a tool named after an Object.prototype member (e.g. `toString`, `valueOf`) resolved to the inherited method and was invoked as a refiner. Use the own-property getOwn helper so such names are treated as unconfigured. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gr2m
approved these changes
Jul 8, 2026
gr2m
left a comment
gr2m
left a comment
Collaborator
There was a problem hiding this comment.
Verified the fix against VULN-13170. Own-property lookups (Object.hasOwn / hasOwnProperty + null-prototype maps and the shared getOwn helper) correctly close the inherited-property bypass across both wrapMcpTools/shadow and the core approval/tool-call/streaming/UI paths. Tool names such as constructor, toString, and valueOf now resolve to "unconfigured" and receive the configured fallback. Tests cover the regression and CI is green.
5 tasks
gr2m
added a commit
that referenced
this pull request
Jul 8, 2026
…cpTools (#16959) ## Background Follow-up hardening to #16900 (VULN-13170), which closed the inherited-property approval bypass in `wrapMcpTools`. While reviewing that fix I found a separate, still-open gap in the same helper. `wrapMcpTools` documents that the resulting approval configuration is *total*: every discovered tool is gated by the supplied approval "when it has an opinion" or by the `default` otherwise (`user-approval` by default). The generic-function form already enforces this — it normalizes a "no opinion" result (`not-applicable`/`undefined`) to the fallback. The **per-tool map form did not**: it only substituted the fallback for missing entries (`configured ?? fallback`), so a per-tool approval *function* that returned `not-applicable`/`undefined` passed straight through. The tool then resolved to `not-applicable`, which the core treats as "no approval required", and it ran without an approval request — i.e. the per-tool function **failed open** instead of falling back to the default. ## Summary - `packages/policy-opa/src/wrap-mcp-tools.ts`: in the per-tool map form, wrap per-tool approval **functions** so a `not-applicable`/`undefined` result is forced through the configured fallback, mirroring the generic-function form. Functions that return an actual decision are passed through unchanged, and their `input`/`options` arguments are forwarded intact. Static per-tool statuses that the caller configured explicitly are left as-is. - `packages/policy-opa/src/wrap-mcp-tools.test.ts`: add tests covering a per-tool function that returns `not-applicable` (→ fallback), one that returns `undefined` (→ configured default), one that returns a real decision (unchanged), and argument forwarding. - Patch changeset for `@ai-sdk/policy-opa`. ## Manual Verification Confirmed the before/after behavior on the per-tool map form: - Before this change, `wrapMcpTools(tools, { search: () => 'not-applicable' })` produced an entry that resolved to `not-applicable`, so the tool executed with no approval request (fail open). - After this change, the same configuration resolves `search` to the fallback (`user-approval`, or the configured `default`), so the call pauses for a decision (fail closed). A per-tool function returning an explicit `approved`/`denied` decision is unaffected. Also ran the full package suite (node + edge, 76 passing), `pnpm type-check`, and `oxlint`/`oxfmt` — all clean. ## Checklist - [x] All commits are signed (PRs with unsigned commits cannot be merged) - [x] Tests have been added / updated (for bug fixes / features) - [ ] Documentation has been added / updated (for bug fixes / features) - [x] A _patch_ changeset for relevant packages has been added (for bug fixes / features - run `pnpm changeset` in the project root) - [x] I have reviewed this pull request (self-review) ## Future Work The per-tool map form still passes an **explicit static** `'not-applicable'` / `{ type: 'not-applicable' }` entry through unchanged. This PR deliberately does not alter that: unlike a function's runtime "no opinion", a static `not-applicable` is arguably an explicit caller decision ("this tool needs no approval"), and overriding it would contradict the "preserves explicit per-tool entries" behavior. Worth a follow-up discussion on whether a security-oriented default-deny wrapper should also refuse to honor a static `not-applicable`. ## Related Issues Follow-up to #16900 (VULN-13170). Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
🚀 Published in:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Tool sets, tool contexts, and per-tool approval maps are indexed by names that come from model output or client-supplied message history. Plain bracket access (
tools[name]) resolves names that match inherited object properties (constructor,toString,valueOf,__proto__) to values onObject.prototypeinstead ofundefined, which can slip past the "unknown tool" / "unconfigured approval" guards.This routes those lookups through a new
getOwnhelper (own-property check viaObject.hasOwn), builds the human-in-the-loop approval-matching maps with a null prototype, and adds the same own-property guards in policy-opawrapMcpToolsandshadow.Behavior is identical for every real tool set, since tool names are always own properties. The only change is that an inherited-property name now reads as absent.
Flow
Happy path (real tool named
weather, unchanged):weather.getOwn(tools, "weather")finds it as an own property and returns the tool.Unhappy path (name collides with a prototype member, e.g.
constructor):constructor, which is not a registered tool.tools["constructor"]returnedObject.prototype.constructor(a function), passed the!= nullguard, and could be treated as a tool or invoked as an approval callback.getOwn(tools, "constructor")returnsundefined, so the name routes to the safe "no such tool" / "unconfigured, apply fallback" branch (NoSuchToolError, denied/user-approval fallback, etc.).Notes
Covers the approval path (per-tool resolution and replay re-validation), tool-call parsing, execution, streaming callbacks, and UI message conversion/validation. Includes a
getOwnunit test plus regression tests inwrap-mcp-tools,shadow,resolve-tool-approval, andcollect-tool-approvals.