Fix validate_expression_tool / validate_style_tool disagreement with mapbox-gl-js on zoom-expression placement#125
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes two disagreements with real mapbox-gl-js behavior around the ["zoom"] expression placement rule: validate_expression_tool false-positived on ordinary zoom-based interpolate/step expressions (misidentifying the interpolation-type argument, e.g. ["linear"], as an unknown operator), and validate_style_tool false-negatived on zoom expressions nested inside things like a case, which the spec disallows. Both tools now delegate to the official style-spec package instead of a hand-rolled reimplementation. Fixes #123 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| if (Array.isArray(arg)) { | ||
| const argPath = path ? `${path}[${index + 1}]` : `[${index + 1}]`; | ||
| const argMetadata = this.validateExpression( | ||
| const argResult = this.walkStructure( |
There was a problem hiding this comment.
is it safe to do a recursion here? I.e. can user generated input create stackoverflow for example?
There was a problem hiding this comment.
Good catch — confirmed this was a real gap. Repro'd it: a cheaply-constructed ~200k-deep nested expression drove both walkStructure and the delegated createPropertyExpression/validate() calls to a RangeError: Maximum call stack size exceeded. It didn't crash the process (the outer try/catch in execute() does catch it), but it burned an unbounded amount of stack/CPU on a trivial payload before failing with an unhelpful message, in both validate_expression_tool and validate_style_tool (the latter delegates straight to the style-spec's own validate(), which has the same unbounded recursion internally).\n\nFixed in 9e56477 by adding an explicit depth cap (64, well beyond any realistic expression) that's checked before recursing further in each function, so the actual call stack never grows past the cap regardless of how deep the adversarial input goes — rather than relying on catching a stack overflow. Also made sure a deep node anywhere in the tree (not just at the top level) correctly short-circuits the call into the style-spec library, and added an equivalent depth pre-check in validate_style_tool before it hands the style off to validate(). Added regression tests for both (asserting they return quickly, not just that they don't crash).
There was a problem hiding this comment.
Follow-up: while verifying the depth fix, I found the depth cap alone doesn't fully close this out — it only protects against deep input. A wide-but-shallow payload (e.g. a single match expression with millions of branches, depth ~1) sails right past the depth-64 cap and gets handed whole to createPropertyExpression/validate().
Reproduced under a constrained heap (node --max-old-space-size=150) and confirmed this is actually worse than the stack-overflow case: heap exhaustion is not a catchable JS error — V8 aborts the process directly (FATAL ERROR: Reached heap limit, SIGABRT), which would take down every concurrent MCP session, not just the offending request.
Fixed in a2150d4:
- Added a
.max()length cap on the JSON-string form ofexpression/stylein their Zod schemas (256 KB / 10 MB respectively — generous for any real style/expression), rejected beforeJSON.parseever runs. Forvalidate_expression_toolthis had to be a.refine()rather than.max()directly on the union member, since the siblingz.any()branch would otherwise swallow oversized strings right past the length check (caught this in testing before it shipped). - Added an O(1) array-length / object-key-count guard alongside the existing depth guard (checked via
.lengthbefore ever iterating into or copying the collection), so the already-parsed object-input path is covered too, not just the JSON-string path.
Added regression tests for both the wide-payload and oversized-string cases in both tools, asserting they return quickly rather than just that they don't crash.
Both the hand-rolled expression walk and the delegated style-spec validator recurse proportional to input nesting depth with no limit, so a cheaply constructed deeply-nested expression (a few KB of brackets) would recurse tens of thousands of stack frames deep before failing with an unhelpful "Maximum call stack size exceeded" RangeError. Add an explicit depth cap (64, well beyond any realistic expression) that's checked before recursing further, so the cap holds regardless of how deep the actual input goes, and short-circuit before delegating to the library so it never sees the oversized input either. Addresses review feedback from Valiunia on #125. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
validate_expression_toolandvalidate_style_toolnow delegate expression/style validation to@mapbox/mapbox-gl-style-spec(the same validation logic mapbox-gl-js itself runs at runtime) instead of a hand-rolled reimplementation.["zoom"]expression placement rule ("zoom" may only be used as the top-level input to astep/interpolateexpression):validate_expression_toolno longer false-positives on ordinary zoom-basedinterpolate/stepexpressions - it previously misidentified the interpolation-type argument (e.g.["linear"]) as an unknown top-level operator.validate_style_toolnow correctly flags["zoom"]expressions nested inside e.g. acase, which is invalid per spec and previously passed through silently asvalid: true.validate_expression_toolfor clearer error messages (unknown operator, non-string operator, empty array), now checked against the real operator registry viaisExpressioninstead of a hand-maintained list.validate_style_tool's non-spec UX extras (missing sprite/glyphs/sources info, empty-layers warning) since those aren't part of spec conformance."missing required property \"version\""instead of"Missing required \"version\" property"); existing tests were updated accordingly, and regression tests were added for both cases in the issue.Fixes #123
Test plan
npm test- all 590 tests pass, including new regression tests reproducing both cases from the issuenpm run lint- 0 errorsnpm run build- succeeds["interpolate", ["linear"], ["zoom"], 4, 0.4, 6, 0.28, 8, 0]→valid: truecasestyle from the issue →valid: falsewith the exact mapbox-gl-js error message🤖 Generated with Claude Code