fix(mcp): recursively coerce integer args nested in tool input schemas#11407
fix(mcp): recursively coerce integer args nested in tool input schemas#11407david-engelmann wants to merge 6 commits into
Conversation
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR replaces top-level MCP integer argument coercion with a recursive schema/value walker and adds tests for nested objects, arrays, composition keywords, refs, nullable type arrays, and conservative numeric cases.
Concerns
additionalPropertiescurrently treats keys covered by unsupportedpatternPropertiesas extras, which can coerce values that should be governed by a pattern schema instead.- This changes user-facing MCP behavior, and the shared helper can also affect displayed MCP call details, but the PR does not include screenshots or a screen recording demonstrating the repro working end to end. Please attach visual evidence per repo guidance.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| let Some(f) = n.as_f64() else { continue }; | ||
| if f.fract() != 0.0 { | ||
| continue; | ||
| if let Some(additional) = schema.get("additionalProperties") { |
There was a problem hiding this comment.
patternProperties are still treated as extras here, so a schema with patternProperties for numeric fields plus additionalProperties: {"type": "integer"} will coerce those pattern-matched values even though patternProperties is intentionally unsupported. Skip additionalProperties when patternProperties is present, or account for pattern matches before applying the additional schema.
…ercion (warpdotdev#10596 review) Per JSON Schema, a key matching any `patternProperties` regex is governed by that pattern's schema, not by `additionalProperties`. The recursive walker didn't (yet) coerce through `patternProperties`, but it was still iterating those keys when applying `additionalProperties`, which could coerce a value that should have been governed by the pattern schema instead. Oz flagged this on PR warpdotdev#11407: > additionalProperties currently treats keys covered by unsupported > patternProperties as extras, which can coerce values that should be > governed by a pattern schema instead. Compile each `patternProperties` key as a regex and skip matching args before falling back to `additionalProperties`. Patterns that fail to compile are ignored, matching the "unsupported shapes are skipped" policy elsewhere in this walker. Adds 2 regression tests: - `pattern_properties_keys_are_excluded_from_additional_properties` — a key matching `^_` is governed by its pattern (no coercion), while an unrelated key still gets coerced via `additionalProperties`. - `invalid_pattern_property_regex_is_ignored` — an uncompilable regex doesn't panic and doesn't block sibling coercion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks for the catch — you're right, a key matching a Addressed in
Added 2 regression tests in call_mcp_tool_tests.rs:
23/23 in Re: visual evidence — this PR is a behind-the-scenes serialization fix (whole-number f64 → i64 on the wire), so there's no UI surface to capture. The PANW-style end-to-end repro lives in /oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR expands MCP tool argument integer coercion from top-level properties to nested objects, arrays, composition keywords, internal refs, and additionalProperties, with broad unit coverage for the new traversal behavior.
Concerns
patternPropertiesmatching is not conservative for valid JSON Schema regexes that Rust's regex engine cannot compile; those keys can fall through toadditionalPropertiesand be coerced with the wrong schema.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| .and_then(|v| v.as_object()) | ||
| .map(|p| { | ||
| p.keys() | ||
| .filter_map(|pat| regex::Regex::new(pat).ok()) |
There was a problem hiding this comment.
patternProperties uses JSON Schema/ECMA regex semantics, but Rust regex rejects valid patterns such as look-around. Silently dropping those patterns lets matching keys fall through to additionalProperties and be coerced with the wrong schema; skip additionalProperties when pattern ownership cannot be evaluated with compatible semantics.
…ile (warpdotdev#10596 review) JSON Schema's pattern grammar is ECMA-262; Rust's `regex` crate is a strict subset and rejects valid patterns such as those with lookaheads or certain Unicode constructs. The previous carve-out filtered out uncompilable patterns with `.ok()`, which meant a valid JSON Schema regex that Rust couldn't compile silently failed to exclude its keys — those keys then fell through to `additionalProperties` and got coerced with the wrong schema. Oz flagged this on PR warpdotdev#11407: > patternProperties matching is not conservative for valid JSON Schema > regexes that Rust's regex engine cannot compile; those keys can fall > through to additionalProperties and be coerced with the wrong schema. If any `patternProperties` entry fails to compile, break out of the `additionalProperties` branch entirely (labeled break, scoped to that block — `properties` recursion at the same level is unaffected). A key that should have been governed by the pattern won't be coerced with the wrong schema. Replaces the old `invalid_pattern_property_regex_is_ignored` test with two new ones that pin the conservative behavior: - `uncompilable_pattern_skips_additional_properties_conservatively` — the would-be-coerced key keeps its float when the pattern is unparseable. - `uncompilable_pattern_still_allows_properties_recursion` — `properties` coercion at the same level still runs, because it doesn't depend on the pattern. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Good catch — Rust's regex crate is a strict ECMA-262 subset and silently rejecting unparseable patterns let a real pattern-governed key slip through to Addressed in
Replaced
24/24 in /oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR expands MCP tool argument integer coercion from top-level properties to a recursive JSON Schema walker covering nested objects, arrays, composition keywords, internal refs, additionalProperties, and related tests.
Concerns
- Recursive composition traversal does not carry a cycle/depth guard across
$ref+allOf/oneOf/anyOf, so a third-party MCP server can advertise a recursive composed schema that causes unbounded recursion while preparing a tool call.
Security
- The unbounded recursion is driven by MCP-provided schemas and can hang or crash the client during argument coercion.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| let schema = resolve_refs(schema, root); | ||
|
|
||
| // `allOf` — apply every branch; can stack with sibling keywords. | ||
| if let Some(branches) = schema.get("allOf").and_then(|v| v.as_array()) { |
There was a problem hiding this comment.
$ref visited set for every call, so a schema like A.allOf -> $ref B and B.allOf -> $ref A will recurse forever before consuming any data depth. MCP schemas are supplied by third-party servers; carry a visited ref/schema set, or a max schema-depth guard, across coerce_recursive when following composition branches.
…tion DoS (warpdotdev#10596 review) `resolve_refs` already breaks pure `$ref` chains, but the walker's recursive entries for `allOf` / `oneOf` / `anyOf` / `properties` / `items` / `additionalProperties` had no cross-cutting guard. A malicious or accidentally-recursive composed schema like $defs.Loop = { allOf: [{ "$ref": "#/$defs/Loop" }], ... } would loop indefinitely (allOf → resolve_refs → allOf → …) while preparing a tool call. Oz flagged this on PR warpdotdev#11407: > Recursive composition traversal does not carry a cycle/depth guard > across `$ref` + `allOf` / `oneOf` / `anyOf`, so a third-party MCP > server can advertise a recursive composed schema that causes > unbounded recursion while preparing a tool call. Thread a `depth: usize` counter through `coerce_recursive`, bail at `MAX_COERCE_DEPTH = 64`. Real MCP schemas top out well under this; the limit only kicks in for cycles. Termination is the guarantee — correctness on an intentionally-malicious schema is not a goal (the server would have to be cooperating to send valid args anyway). Adds 3 regression tests: - `all_of_self_reference_terminates` — `allOf` cycle via `$ref` self-link. - `one_of_self_reference_terminates` — same shape via `oneOf`. - `deep_finite_nesting_under_limit_still_coerces` — a 10-level nested `properties` chain still coerces correctly (proves the limit doesn't block reasonable usage). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Right — Addressed in
Added 3 regression tests:
27/27 in /oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR expands MCP tool integer argument coercion to nested objects, arrays, composition keywords, internal refs, and additional properties. No approved spec context was available, so this review focused on the annotated diff and security implications.
Concerns
- The recursive composition handling still allows untrusted MCP schemas to create exponential traversal despite the depth cap, which can hang the client while preparing a tool call.
Security
- The recursion fan-out issue is security-relevant resource exhaustion from a schema supplied by an MCP server.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // `allOf` — apply every branch; can stack with sibling keywords. | ||
| if let Some(branches) = schema.get("allOf").and_then(|v| v.as_array()) { | ||
| for b in branches { | ||
| coerce_recursive(value, b, root, depth + 1); |
There was a problem hiding this comment.
allOf/anyOf refs back to the same schema), so a malicious MCP server can hang the client before the cap helps; track composition refs already on the stack or otherwise short-circuit these cycles instead of relying only on depth.
…n-out (warpdotdev#10596 review) The previous depth-cap (64) bounded the longest single chain, but didn't bound TOTAL work. An adversarial schema like $defs.Loop = { allOf: [{ "$ref": "#/Loop" }, { "$ref": "#/Loop" }] } doubles work at each recursion level — total ops grow as `B^D` where B is the branching factor. With B=2 and D=64 that's ~1.8e19 calls before any single chain hits MAX_COERCE_DEPTH. Oz flagged this on PR warpdotdev#11407: > The recursive composition handling still allows untrusted MCP schemas > to create exponential traversal despite the depth cap, which can hang > the client while preparing a tool call. Thread a mutable `budget: &mut usize` through `coerce_recursive`. Each recursive call decrements the budget; the function bails when it hits 0 (in addition to the existing depth check). Budget set to `MAX_COERCE_OPS = 10_000`, which leaves ~20× headroom for real MCP schemas (which run a few hundred calls in practice) while bounding the worst case to a few milliseconds of work. Adds 1 regression test: - `fan_out_via_all_of_self_reference_terminates` — `allOf: [Self, Self]` schema cycle. Without the budget this generated 2^D calls before any single chain reached the depth cap. With the budget it bails at 10_000. Also reorders the const declarations so the docstring on `coerce_integer_args` attaches to the function rather than to `MAX_COERCE_DEPTH` (post-Edit fix). 28/28 in `call_mcp_tool` pass. clippy + fmt clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Right — the depth cap bounds the longest single chain, but Addressed in `2f784f01`:
Added 1 regression test:
(Also reordered the const declarations so the `coerce_integer_args` docstring attaches to the function rather than to `MAX_COERCE_DEPTH`.) 28/28 in `call_mcp_tool` pass. clippy + fmt clean. /oz-review |
|
This is your last |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR expands MCP tool argument integer coercion from top-level schema properties to recursive JSON Schema traversal, covering nested objects, arrays, composition keywords, additionalProperties, internal $refs, and regression tests for the reported nested PANW shape.
Concerns
- The PR changes user-facing MCP tool execution behavior, but the description does not include screenshots or a screen recording showing the nested integer MCP tool call succeeding end to end. For this user-facing change, please include screenshots or a screen recording demonstrating it working end to end.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Code-level visual evidence for warpdotdev/warp#11407. The fix lives in `coerce_integer_args` (a private helper that runs before MCP tool calls hit the wire), so a UI demonstration is indirect: the scenario drives a terminal inside warp-oss to: 1. `cat` the PANW-style test schema (object.object.array.oneOf.integer) so the recording shows the exact JSON Schema shape the walker has to handle. 2. Run `cargo test panw_audit_management_repro_coerces_all_integers -- --nocapture` so the recording shows the assertions pass — the wire-format guarantee that `0.0 / 100.0 / 1730419200000.0` → `0 / 100 / 1730419200000` at three nested positions. Uses the new `Scenario::caption()` API to overlay five narration cards explaining the schema, the would-be-wire payload, and the walker's recursion path. The captioned mp4 + gif drop out of the pipeline's 03b-captions stage automatically. Run with: RECIPE=11407-mcp-integer-coercion ./scripts/record-agent-evidence.sh or: warp-taper run-builtin 11407-mcp-integer-coercion \ --warp-source /tmp/warp-10596 --duration-ms 32000
|
Visual evidence for the nested integer coercion, recorded end-to-end against a
What the recording shows
The test the recording exercisesFrom call_mcp_tool_tests.rs:591: fn panw_audit_management_repro_coerces_all_integers() {
let mut args = obj(json!({
"request_data": {
"search_from": 0.0,
"search_to": 100.0,
"filters": [
{ "field": "timestamp", "operator": "gte", "value": 1730419200000.0 }
]
}
}));
let schema = obj(json!({
"type": "object",
"properties": {
"request_data": {
"type": "object",
"properties": {
"search_from": { "type": "integer" },
"search_to": { "type": "integer" },
"filters": {
"type": "array",
"items": {
"oneOf": [
{ "properties": { "value": { "type": "array", "items": { "type": "string" } } } },
{ "properties": { "value": { "type": "integer" } } }
]
}
}
}
}
}
}));
coerce_integer_args(&mut args, &schema);
assert_serialized_as(&args, "request_data.search_from", "0");
assert_serialized_as(&args, "request_data.search_to", "100");
assert_serialized_as(&args, "request_data.filters.0.value", "1730419200000");
}The three assertions pin the wire format: each integer field that the agent supplies as ReproducingThe scenario + recipe + the new
Full call-mcp-tool suite (all 28 tests covering the walker's recursion shapes — composition, |
|
/oz-review |
|
You've used all 5 |
david-engelmann
force-pushed
the
david/10596-mcp-integer-coercion
branch
from
2f784f0 to
c876721
Compare
…ercion (warpdotdev#10596 review) Per JSON Schema, a key matching any `patternProperties` regex is governed by that pattern's schema, not by `additionalProperties`. The recursive walker didn't (yet) coerce through `patternProperties`, but it was still iterating those keys when applying `additionalProperties`, which could coerce a value that should have been governed by the pattern schema instead. Oz flagged this on PR warpdotdev#11407: > additionalProperties currently treats keys covered by unsupported > patternProperties as extras, which can coerce values that should be > governed by a pattern schema instead. Compile each `patternProperties` key as a regex and skip matching args before falling back to `additionalProperties`. Patterns that fail to compile are ignored, matching the "unsupported shapes are skipped" policy elsewhere in this walker. Adds 2 regression tests: - `pattern_properties_keys_are_excluded_from_additional_properties` — a key matching `^_` is governed by its pattern (no coercion), while an unrelated key still gets coerced via `additionalProperties`. - `invalid_pattern_property_regex_is_ignored` — an uncompilable regex doesn't panic and doesn't block sibling coercion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ile (warpdotdev#10596 review) JSON Schema's pattern grammar is ECMA-262; Rust's `regex` crate is a strict subset and rejects valid patterns such as those with lookaheads or certain Unicode constructs. The previous carve-out filtered out uncompilable patterns with `.ok()`, which meant a valid JSON Schema regex that Rust couldn't compile silently failed to exclude its keys — those keys then fell through to `additionalProperties` and got coerced with the wrong schema. Oz flagged this on PR warpdotdev#11407: > patternProperties matching is not conservative for valid JSON Schema > regexes that Rust's regex engine cannot compile; those keys can fall > through to additionalProperties and be coerced with the wrong schema. If any `patternProperties` entry fails to compile, break out of the `additionalProperties` branch entirely (labeled break, scoped to that block — `properties` recursion at the same level is unaffected). A key that should have been governed by the pattern won't be coerced with the wrong schema. Replaces the old `invalid_pattern_property_regex_is_ignored` test with two new ones that pin the conservative behavior: - `uncompilable_pattern_skips_additional_properties_conservatively` — the would-be-coerced key keeps its float when the pattern is unparseable. - `uncompilable_pattern_still_allows_properties_recursion` — `properties` coercion at the same level still runs, because it doesn't depend on the pattern. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tion DoS (warpdotdev#10596 review) `resolve_refs` already breaks pure `$ref` chains, but the walker's recursive entries for `allOf` / `oneOf` / `anyOf` / `properties` / `items` / `additionalProperties` had no cross-cutting guard. A malicious or accidentally-recursive composed schema like $defs.Loop = { allOf: [{ "$ref": "#/$defs/Loop" }], ... } would loop indefinitely (allOf → resolve_refs → allOf → …) while preparing a tool call. Oz flagged this on PR warpdotdev#11407: > Recursive composition traversal does not carry a cycle/depth guard > across `$ref` + `allOf` / `oneOf` / `anyOf`, so a third-party MCP > server can advertise a recursive composed schema that causes > unbounded recursion while preparing a tool call. Thread a `depth: usize` counter through `coerce_recursive`, bail at `MAX_COERCE_DEPTH = 64`. Real MCP schemas top out well under this; the limit only kicks in for cycles. Termination is the guarantee — correctness on an intentionally-malicious schema is not a goal (the server would have to be cooperating to send valid args anyway). Adds 3 regression tests: - `all_of_self_reference_terminates` — `allOf` cycle via `$ref` self-link. - `one_of_self_reference_terminates` — same shape via `oneOf`. - `deep_finite_nesting_under_limit_still_coerces` — a 10-level nested `properties` chain still coerces correctly (proves the limit doesn't block reasonable usage). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…n-out (warpdotdev#10596 review) The previous depth-cap (64) bounded the longest single chain, but didn't bound TOTAL work. An adversarial schema like $defs.Loop = { allOf: [{ "$ref": "#/Loop" }, { "$ref": "#/Loop" }] } doubles work at each recursion level — total ops grow as `B^D` where B is the branching factor. With B=2 and D=64 that's ~1.8e19 calls before any single chain hits MAX_COERCE_DEPTH. Oz flagged this on PR warpdotdev#11407: > The recursive composition handling still allows untrusted MCP schemas > to create exponential traversal despite the depth cap, which can hang > the client while preparing a tool call. Thread a mutable `budget: &mut usize` through `coerce_recursive`. Each recursive call decrements the budget; the function bails when it hits 0 (in addition to the existing depth check). Budget set to `MAX_COERCE_OPS = 10_000`, which leaves ~20× headroom for real MCP schemas (which run a few hundred calls in practice) while bounding the worst case to a few milliseconds of work. Adds 1 regression test: - `fan_out_via_all_of_self_reference_terminates` — `allOf: [Self, Self]` schema cycle. Without the budget this generated 2^D calls before any single chain reached the depth cap. With the budget it bails at 10_000. Also reorders the const declarations so the docstring on `coerce_integer_args` attaches to the function rather than to `MAX_COERCE_DEPTH` (post-Edit fix). 28/28 in `call_mcp_tool` pass. clippy + fmt clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Rebased onto upstream/master ( /oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR expands MCP integer-argument coercion to recurse through nested objects, arrays, composition keywords, internal refs, and additional properties, with tests covering the issue repro and recursion guards. No approved spec context was available to validate against.
Concerns
- Applying every
oneOf/anyOfbranch can mutate values according to a branch that the instance does not actually satisfy, because branch-level constraints such asconst,enum, andrequiredare ignored by the coercion walker. - Untrusted MCP schemas can now trigger regex compilation from
patternPropertieswhile preparing tool calls, which leaves a resource-exhaustion path outside the new recursion and operation budgets.
Security
- Untrusted
patternPropertiespatterns are compiled without bounds; avoid compiling server-provided regexes on this path, or bound pattern count/size and use explicit regex builder limits.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // match the value is a no-op, so trying all branches sidesteps the | ||
| // ambiguity of picking the "right" branch when multiple top-level types | ||
| // would match. | ||
| for key in ["oneOf", "anyOf"] { |
There was a problem hiding this comment.
oneOf/anyOf branch ignores branch-level constraints like const, enum, and required, so an integer schema in a non-selected branch can coerce a field whose selected branch declares number; validate/pick matching branches before mutating, or limit this path to unambiguous schemas.
| .and_then(|v| v.as_object()) | ||
| .map(|p| { | ||
| p.keys() | ||
| .map(|pat| regex::Regex::new(pat)) |
There was a problem hiding this comment.
patternProperties regexes while preparing a tool call; a malicious server can send many or very large patterns to burn CPU/memory outside the recursion budget, so skip additionalProperties when patternProperties is present or bound pattern count/size and compile with explicit limits.
warpdotdev#10596) MCP tool args round-trip through `google.protobuf.Struct` on the wire, whose `NumberValue` stores everything as `f64`, erasing the integer/float distinction. The existing `coerce_integer_args` mitigation only walked depth-1 `properties` and matched the literal string `"integer"`, so nested integer fields (inside objects, array items, `oneOf`/`anyOf`/`allOf` branches, behind `$ref`, or declared as `["integer", "null"]`) were still serialized as `"5.0"` and rejected by strict MCP servers. Walks schema and args in parallel covering: nested objects, array `items` (single-schema and tuple), `additionalProperties`, `allOf` / `oneOf` / `anyOf`, internal `$ref` (with cycle guard), and nullable type-arrays. External `$ref` and uncommon keywords (`not`, `if`/`then`/`else`, `patternProperties`) are intentionally skipped — a skip preserves the existing wire behavior. Adds 19 new unit tests covering each shape plus the exact PANW-style repro from the issue. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ercion (warpdotdev#10596 review) Per JSON Schema, a key matching any `patternProperties` regex is governed by that pattern's schema, not by `additionalProperties`. The recursive walker didn't (yet) coerce through `patternProperties`, but it was still iterating those keys when applying `additionalProperties`, which could coerce a value that should have been governed by the pattern schema instead. Oz flagged this on PR warpdotdev#11407: > additionalProperties currently treats keys covered by unsupported > patternProperties as extras, which can coerce values that should be > governed by a pattern schema instead. Compile each `patternProperties` key as a regex and skip matching args before falling back to `additionalProperties`. Patterns that fail to compile are ignored, matching the "unsupported shapes are skipped" policy elsewhere in this walker. Adds 2 regression tests: - `pattern_properties_keys_are_excluded_from_additional_properties` — a key matching `^_` is governed by its pattern (no coercion), while an unrelated key still gets coerced via `additionalProperties`. - `invalid_pattern_property_regex_is_ignored` — an uncompilable regex doesn't panic and doesn't block sibling coercion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ile (warpdotdev#10596 review) JSON Schema's pattern grammar is ECMA-262; Rust's `regex` crate is a strict subset and rejects valid patterns such as those with lookaheads or certain Unicode constructs. The previous carve-out filtered out uncompilable patterns with `.ok()`, which meant a valid JSON Schema regex that Rust couldn't compile silently failed to exclude its keys — those keys then fell through to `additionalProperties` and got coerced with the wrong schema. Oz flagged this on PR warpdotdev#11407: > patternProperties matching is not conservative for valid JSON Schema > regexes that Rust's regex engine cannot compile; those keys can fall > through to additionalProperties and be coerced with the wrong schema. If any `patternProperties` entry fails to compile, break out of the `additionalProperties` branch entirely (labeled break, scoped to that block — `properties` recursion at the same level is unaffected). A key that should have been governed by the pattern won't be coerced with the wrong schema. Replaces the old `invalid_pattern_property_regex_is_ignored` test with two new ones that pin the conservative behavior: - `uncompilable_pattern_skips_additional_properties_conservatively` — the would-be-coerced key keeps its float when the pattern is unparseable. - `uncompilable_pattern_still_allows_properties_recursion` — `properties` coercion at the same level still runs, because it doesn't depend on the pattern. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tion DoS (warpdotdev#10596 review) `resolve_refs` already breaks pure `$ref` chains, but the walker's recursive entries for `allOf` / `oneOf` / `anyOf` / `properties` / `items` / `additionalProperties` had no cross-cutting guard. A malicious or accidentally-recursive composed schema like $defs.Loop = { allOf: [{ "$ref": "#/$defs/Loop" }], ... } would loop indefinitely (allOf → resolve_refs → allOf → …) while preparing a tool call. Oz flagged this on PR warpdotdev#11407: > Recursive composition traversal does not carry a cycle/depth guard > across `$ref` + `allOf` / `oneOf` / `anyOf`, so a third-party MCP > server can advertise a recursive composed schema that causes > unbounded recursion while preparing a tool call. Thread a `depth: usize` counter through `coerce_recursive`, bail at `MAX_COERCE_DEPTH = 64`. Real MCP schemas top out well under this; the limit only kicks in for cycles. Termination is the guarantee — correctness on an intentionally-malicious schema is not a goal (the server would have to be cooperating to send valid args anyway). Adds 3 regression tests: - `all_of_self_reference_terminates` — `allOf` cycle via `$ref` self-link. - `one_of_self_reference_terminates` — same shape via `oneOf`. - `deep_finite_nesting_under_limit_still_coerces` — a 10-level nested `properties` chain still coerces correctly (proves the limit doesn't block reasonable usage). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…n-out (warpdotdev#10596 review) The previous depth-cap (64) bounded the longest single chain, but didn't bound TOTAL work. An adversarial schema like $defs.Loop = { allOf: [{ "$ref": "#/Loop" }, { "$ref": "#/Loop" }] } doubles work at each recursion level — total ops grow as `B^D` where B is the branching factor. With B=2 and D=64 that's ~1.8e19 calls before any single chain hits MAX_COERCE_DEPTH. Oz flagged this on PR warpdotdev#11407: > The recursive composition handling still allows untrusted MCP schemas > to create exponential traversal despite the depth cap, which can hang > the client while preparing a tool call. Thread a mutable `budget: &mut usize` through `coerce_recursive`. Each recursive call decrements the budget; the function bails when it hits 0 (in addition to the existing depth check). Budget set to `MAX_COERCE_OPS = 10_000`, which leaves ~20× headroom for real MCP schemas (which run a few hundred calls in practice) while bounding the worst case to a few milliseconds of work. Adds 1 regression test: - `fan_out_via_all_of_self_reference_terminates` — `allOf: [Self, Self]` schema cycle. Without the budget this generated 2^D calls before any single chain reached the depth cap. With the budget it bails at 10_000. Also reorders the const declarations so the docstring on `coerce_integer_args` attaches to the function rather than to `MAX_COERCE_DEPTH` (post-Edit fix). 28/28 in `call_mcp_tool` pass. clippy + fmt clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ern regexes Addresses Oz's two review concerns on warpdotdev#10596: 1. oneOf/anyOf over-coercion. The walker applied every composition branch, so a value could be coerced per a branch it doesn't satisfy — e.g. a tagged union where one arm types a field `integer` and another `number` would coerce a legitimate float to an int. Added `branch_excluded`, which skips a branch when the value provably violates its `const`, `enum`, `type`, `required`, or a `const`/`enum` discriminator on a present property. It's a partial check that only ever *excludes* provable non-matches, so untagged unions behave exactly as before, and a whole-number float still counts as a valid `integer` candidate. 2. patternProperties ReDoS surface. Server-advertised patterns were compiled without bounds. Rust's regex is linear-time at match, so the risk is pathological compile cost; `compile_pattern_properties` now caps pattern count (64), per-pattern length (1024), and compiled program size (64 KiB via RegexBuilder size_limit/dfa_size_limit). Any rejection falls back to the existing conservative skip of additionalProperties coercion. 8 new tests: tagged-union (const/enum/required/type) branch selection both ways, plus pattern-count / pattern-length / well-formed-pattern bounds. All 36 coercion tests pass; clippy + fmt clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
david-engelmann
force-pushed
the
david/10596-mcp-integer-coercion
branch
from
c876721 to
f08cec7
Compare
|
Addressed both concerns in 1. { "oneOf": [
{ "properties": { "kind": {"const":"a"}, "val": {"type":"integer"} } },
{ "properties": { "kind": {"const":"b"}, "val": {"type":"number"} } }
]}with 2. 8 new tests covering tagged-union branch selection (const / enum / required / type, both matching and non-matching) and the three pattern-compile bounds. All 36 coercion tests pass; clippy + fmt clean. /oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
oz-for-oss
Bot
dismissed stale reviews from themself
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR expands MCP tool argument integer coercion from shallow top-level properties to recursive JSON Schema traversal, covering nested objects, arrays, composition keywords, internal refs, and additionalProperties with bounded recursion/resource guards and focused unit coverage.
Concerns
- No blocking correctness, spec-alignment, or security concerns were found in the annotated diff.
- No approved spec context was provided for this PR, so there are no spec commitments to compare beyond the issue-focused behavior described in the PR.
- No screenshot or screen recording is requested because this is non-visual MCP payload serialization behavior; the before/after wire payload and unit coverage are the meaningful evidence for this change.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
1 participant
Fixes #10596.
Why
MCP tool args round-trip through
google.protobuf.Structbetween the agent backend and the Warp client.structpb.NumberValuestores all numbers asf64, erasing the integer/float distinction. By the time args arrive client-side asserde_json::Value, an integer5has become5.0, and serde_json serializes it back as\"5.0\"— which strict MCP servers reject for\"type\": \"integer\"fields.The previous mitigation (
coerce_integer_args, originally from #6945) only walked depth-1propertiesand matched the literal string\"integer\", so every nested or composed integer field leaked the float through.What
Walks schema and args in parallel covering:
properties(objects)items(single-schema and positional/tuple form)additionalProperties(for keys outsideproperties)allOf/oneOf/anyOf(every branch applied — coercion is conservative and a non-matching branch is a no-op, so this sidesteps the ambiguity of "picking the right branch")$refpointers (#/$defs/Foo,#/definitions/Foo) with a cycle guard[\"integer\", \"null\"]and singleton[\"integer\"]Intentionally skipped (no-op, preserves existing wire behavior): external
$refURLs,not,if/then/else,patternProperties. These are vanishingly rare in real MCP tool schemas, and skipping them never makes a value worse — at worst the float survives, which is today's behavior.The public signature
coerce_integer_args(&mut Map, &Map)is unchanged, so the existing call site at app/src/ai/blocklist/action_model/execute/call_mcp_tool.rs is untouched.Before / after (the issue's PANW repro)
Schema (excerpt):
```yaml
AuditManagementLogRequest:
type: object
properties:
request_data:
type: object
properties:
search_from: { type: integer }
search_to: { type: integer }
filters:
type: array
items:
oneOf:
- { properties: { value: { type: array, items: { type: string } } } }
- { properties: { value: { type: integer } } }
```
Before — wire payload:
```json
{ "request_data": { "search_from": 0.0, "search_to": 100.0,
"filters": [{ "field": "timestamp", "operator": "gte", "value": 1730419200000.0 }] } }
```
After — wire payload:
```json
{ "request_data": { "search_from": 0, "search_to": 100,
"filters": [{ "field": "timestamp", "operator": "gte", "value": 1730419200000 }] } }
```
Test plan
New test coverage by shape:
[\"integer\", \"null\"]: `nullable_integer_is_coerced` + `null_value_for_nullable_integer_passes_through`[\"integer\"]: `singleton_type_array_integer_is_coerced`🤖 Generated with Claude Code