From b00aecffba6ad902c2aa02a361689395d19faaa4 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 1 Apr 2026 14:16:50 -0400 Subject: [PATCH 1/3] chore: add CVE-2026-4926 entry to CHANGELOG Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69f74b9..8c815e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## Unreleased +### Security + +- **CVE-2026-4926**: Upgraded `@modelcontextprotocol/sdk` to `^1.29.0`, resolving `path-to-regexp` to `8.4.1` and fixing the ReDoS vulnerability [GHSA-j3q9-mxjg-w52f](https://github.com/advisories/GHSA-j3q9-mxjg-w52f); regenerated output-validation patch for the new version + ### Public API - **Add `getAllTools` and `getVersionInfo` to public exports** — `getAllTools` is now re-exported from `@mapbox/mcp-devkit-server/tools` and `getVersionInfo` (plus `VersionInfo` type) from `@mapbox/mcp-devkit-server/utils`. These are needed by `hosted-mcp-server` to import server functionality via npm packages instead of submodule filesystem paths. From ffdaf6ad3ab425008af13315a0577d4415ffd19b Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Mon, 13 Jul 2026 19:32:17 -0400 Subject: [PATCH 2/3] Delegate expression/style validation to @mapbox/mapbox-gl-style-spec 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 --- CHANGELOG.md | 6 + package-lock.json | 57 +++ package.json | 1 + .../ValidateExpressionTool.ts | 263 +++++--------- .../validate-style-tool/ValidateStyleTool.ts | 337 ++++-------------- .../ValidateExpressionTool.test.ts | 56 ++- .../ValidateStyleTool.test.ts | 93 ++++- 7 files changed, 365 insertions(+), 448 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c933ba..9334a82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ ## Unreleased +### Fixed + +- **`validate_expression_tool` / `validate_style_tool`**: Both tools now delegate expression and style validation to the official `@mapbox/mapbox-gl-style-spec` package (the same logic mapbox-gl-js runs at runtime) instead of a hand-rolled reimplementation. Fixes two disagreements with real mapbox-gl-js behavior around the `["zoom"]` expression placement rule (#123): + - `validate_expression_tool` no longer false-positives on ordinary zoom-based `interpolate`/`step` expressions (it previously misidentified the interpolation-type argument, e.g. `["linear"]`, as an unknown operator). + - `validate_style_tool` now correctly flags `["zoom"]` expressions nested inside e.g. a `case` (invalid per spec — zoom may only be used as the top-level input to `step`/`interpolate`), which it previously missed entirely. + ### New Features - **Style ID validation for `style_comparison_tool`**: `before` and `after` inputs are now validated to contain only alphanumeric characters, hyphens, and underscores (after stripping the optional `mapbox://styles/` prefix). Validation is enforced at both the Zod schema layer and inside `processStyleId()`. Malformed style IDs are rejected with a descriptive error before any URL is constructed. diff --git a/package-lock.json b/package-lock.json index 6dca895..128563c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,6 +10,7 @@ "hasInstallScript": true, "license": "MIT", "dependencies": { + "@mapbox/mapbox-gl-style-spec": "^14.26.0", "@mcp-ui/server": "^6.1.0", "@modelcontextprotocol/ext-apps": "^1.1.1", "@modelcontextprotocol/sdk": "^1.29.0", @@ -1357,6 +1358,38 @@ "url": "https://opencollective.com/js-sdsl" } }, + "node_modules/@mapbox/mapbox-gl-style-spec": { + "version": "14.26.0", + "resolved": "https://registry.npmjs.org/@mapbox/mapbox-gl-style-spec/-/mapbox-gl-style-spec-14.26.0.tgz", + "integrity": "sha512-Rh+RP9UHbjCRUkhr8OOHI30Ga92rWEF04NtNA6vX7RjTooL889tfMOWvlxIhNKT0q5V1Tor29sdFPkDe5SYbTg==", + "license": "SEE LICENSE IN LICENSE.txt", + "dependencies": { + "@mapbox/point-geometry": "^1.1.0", + "@mapbox/unitbezier": "^1.0.0", + "csscolorparser": "~1.0.3", + "json-stringify-pretty-compact": "^4.0.0", + "quickselect": "^3.0.0", + "tinyqueue": "^3.0.0" + }, + "bin": { + "gl-style-composite": "bin/gl-style-composite.js", + "gl-style-format": "bin/gl-style-format.js", + "gl-style-migrate": "bin/gl-style-migrate.js", + "gl-style-validate": "bin/gl-style-validate.js" + } + }, + "node_modules/@mapbox/point-geometry": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/@mapbox/point-geometry/-/point-geometry-1.1.0.tgz", + "integrity": "sha512-YGcBz1cg4ATXDCM/71L9xveh4dynfGmcLDqufR+nQQy3fKwsAZsWd/x4621/6uJaeB9mwOHE6hPeDgXz9uViUQ==", + "license": "ISC" + }, + "node_modules/@mapbox/unitbezier": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/@mapbox/unitbezier/-/unitbezier-1.0.0.tgz", + "integrity": "sha512-fqd515fjBmANKGGsQ286E2Wvj/XvDFpGzwJxq4CI6jMQue6Oy04uCKp+JWKF00xRTmk6cEu1jPJ9p3xqH8YWqQ==", + "license": "BSD-2-Clause" + }, "node_modules/@mcp-ui/server": { "version": "6.1.0", "resolved": "https://registry.npmjs.org/@mcp-ui/server/-/server-6.1.0.tgz", @@ -5344,6 +5377,12 @@ "url": "https://github.com/chalk/chalk?sponsor=1" } }, + "node_modules/csscolorparser": { + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/csscolorparser/-/csscolorparser-1.0.3.tgz", + "integrity": "sha512-umPSgYwZkdFoUrH5hIq5kf0wPSXiro51nPw0j2K/c83KflkPSTBGMz6NJvMB+07VlL0y7VPo6QJcDjcgKTTm3w==", + "license": "MIT" + }, "node_modules/data-uri-to-buffer": { "version": "4.0.1", "resolved": "https://registry.npmjs.org/data-uri-to-buffer/-/data-uri-to-buffer-4.0.1.tgz", @@ -7532,6 +7571,12 @@ "dev": true, "license": "MIT" }, + "node_modules/json-stringify-pretty-compact": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/json-stringify-pretty-compact/-/json-stringify-pretty-compact-4.0.0.tgz", + "integrity": "sha512-3CNZ2DnrpByG9Nqj6Xo8vqbjT4F6N+tb4Gb28ESAZjYZ5yqvmc56J+/kuIwkaAMOyblTQhUW7PxMkUb8Q36N3Q==", + "license": "MIT" + }, "node_modules/json5": { "version": "2.2.3", "resolved": "https://registry.npmjs.org/json5/-/json5-2.2.3.tgz", @@ -9375,6 +9420,12 @@ ], "license": "MIT" }, + "node_modules/quickselect": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/quickselect/-/quickselect-3.0.0.tgz", + "integrity": "sha512-XdjUArbK4Bm5fLLvlm5KpTFOiOThgfWWI4axAZDWg4E/0mKdZyI9tNEfds27qCi1ze/vwTR16kvmmGhRra3c2g==", + "license": "ISC" + }, "node_modules/range-parser": { "version": "1.2.1", "resolved": "https://registry.npmjs.org/range-parser/-/range-parser-1.2.1.tgz", @@ -10278,6 +10329,12 @@ "url": "https://github.com/sponsors/jonschlinkert" } }, + "node_modules/tinyqueue": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/tinyqueue/-/tinyqueue-3.0.0.tgz", + "integrity": "sha512-gRa9gwYU3ECmQYv3lslts5hxuIa90veaEcxDYuu3QGOIAEM2mOZkVHp48ANJuu1CURtRdHKUBY5Lm1tHV+sD4g==", + "license": "ISC" + }, "node_modules/tinyrainbow": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/tinyrainbow/-/tinyrainbow-3.1.0.tgz", diff --git a/package.json b/package.json index f2ea256..bbab8d9 100644 --- a/package.json +++ b/package.json @@ -49,6 +49,7 @@ "mcp" ], "dependencies": { + "@mapbox/mapbox-gl-style-spec": "^14.26.0", "@mcp-ui/server": "^6.1.0", "@modelcontextprotocol/ext-apps": "^1.1.1", "@modelcontextprotocol/sdk": "^1.29.0", diff --git a/src/tools/validate-expression-tool/ValidateExpressionTool.ts b/src/tools/validate-expression-tool/ValidateExpressionTool.ts index d87169d..f64b830 100644 --- a/src/tools/validate-expression-tool/ValidateExpressionTool.ts +++ b/src/tools/validate-expression-tool/ValidateExpressionTool.ts @@ -2,6 +2,8 @@ // Licensed under the MIT License. import { z } from 'zod'; +import { expression as expressionSpec } from '@mapbox/mapbox-gl-style-spec'; +import type { StylePropertySpecification } from '@mapbox/mapbox-gl-style-spec'; import { BaseTool } from '../BaseTool.js'; import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; import { ValidateExpressionInputSchema } from './ValidateExpressionTool.input.schema.js'; @@ -11,11 +13,36 @@ import { type ExpressionIssue } from './ValidateExpressionTool.output.schema.js'; +// Interpolate's interpolation-type argument (e.g. ["linear"], ["exponential", 1.5]) +// is a type descriptor, not a nested expression - it must be skipped when walking +// the tree for operator-name checks, or it gets misidentified as an unknown operator. +const INTERPOLATION_TYPE_ARG_INDEX: Record = { + interpolate: 0, + 'interpolate-hcl': 0, + 'interpolate-lab': 0 +}; + +// `StylePropertySpecification` only covers concrete typed properties (color, string, +// number, ...), so a literal like `true` or `"red"` would fail type-checking against +// any of them. Using an unrecognized `type` makes createPropertyExpression skip return +// type checking entirely while still enforcing zoom/interpolation placement rules - +// which is what a property-agnostic expression validator needs. Cast is unavoidable +// since the union has no generic/"any" variant. +const GENERIC_PROPERTY_SPEC = { + type: 'value', + expression: { + interpolated: true, + parameters: ['zoom', 'feature', 'feature-state'] + }, + 'property-type': 'data-driven' +} as unknown as StylePropertySpecification; + /** * ValidateExpressionTool - Validates Mapbox style expressions * - * Performs comprehensive validation of Mapbox style expressions including - * syntax validation, operator checking, and argument validation. + * Delegates correctness checks (operator names, argument counts/types, and + * zoom-expression placement) to the official `@mapbox/mapbox-gl-style-spec` + * package, the same validation logic mapbox-gl-js uses at runtime. */ export class ValidateExpressionTool extends BaseTool< typeof ValidateExpressionInputSchema, @@ -32,114 +59,6 @@ export class ValidateExpressionTool extends BaseTool< openWorldHint: false }; - // Mapbox expression operators with their expected argument counts - // Format: [minArgs, maxArgs, returnType] - private static readonly OPERATORS: Record< - string, - { min: number; max: number; returnType?: string } - > = { - // Decision - case: { min: 2, max: Infinity }, - match: { min: 3, max: Infinity }, - coalesce: { min: 1, max: Infinity }, - - // Lookup - get: { min: 1, max: 2, returnType: 'any' }, - has: { min: 1, max: 2, returnType: 'boolean' }, - in: { min: 2, max: 2, returnType: 'boolean' }, - 'index-of': { min: 2, max: 3, returnType: 'number' }, - length: { min: 1, max: 1, returnType: 'number' }, - slice: { min: 2, max: 3 }, - - // Math - '+': { min: 2, max: Infinity, returnType: 'number' }, - '-': { min: 2, max: 2, returnType: 'number' }, - '*': { min: 2, max: Infinity, returnType: 'number' }, - '/': { min: 2, max: 2, returnType: 'number' }, - '%': { min: 2, max: 2, returnType: 'number' }, - '^': { min: 2, max: 2, returnType: 'number' }, - min: { min: 1, max: Infinity, returnType: 'number' }, - max: { min: 1, max: Infinity, returnType: 'number' }, - round: { min: 1, max: 1, returnType: 'number' }, - floor: { min: 1, max: 1, returnType: 'number' }, - ceil: { min: 1, max: 1, returnType: 'number' }, - abs: { min: 1, max: 1, returnType: 'number' }, - sqrt: { min: 1, max: 1, returnType: 'number' }, - log10: { min: 1, max: 1, returnType: 'number' }, - log2: { min: 1, max: 1, returnType: 'number' }, - ln: { min: 1, max: 1, returnType: 'number' }, - e: { min: 0, max: 0, returnType: 'number' }, - pi: { min: 0, max: 0, returnType: 'number' }, - - // Comparison - '==': { min: 2, max: 3, returnType: 'boolean' }, - '!=': { min: 2, max: 3, returnType: 'boolean' }, - '>': { min: 2, max: 3, returnType: 'boolean' }, - '<': { min: 2, max: 3, returnType: 'boolean' }, - '>=': { min: 2, max: 3, returnType: 'boolean' }, - '<=': { min: 2, max: 3, returnType: 'boolean' }, - - // Logical - '!': { min: 1, max: 1, returnType: 'boolean' }, - all: { min: 1, max: Infinity, returnType: 'boolean' }, - any: { min: 1, max: Infinity, returnType: 'boolean' }, - - // String - concat: { min: 1, max: Infinity, returnType: 'string' }, - downcase: { min: 1, max: 1, returnType: 'string' }, - upcase: { min: 1, max: 1, returnType: 'string' }, - 'is-supported-script': { min: 1, max: 1, returnType: 'boolean' }, - 'resolved-locale': { min: 1, max: 1, returnType: 'string' }, - - // Color - rgb: { min: 3, max: 3, returnType: 'color' }, - rgba: { min: 4, max: 4, returnType: 'color' }, - 'to-rgba': { min: 1, max: 1, returnType: 'array' }, - - // Type conversion - array: { min: 1, max: 3 }, - boolean: { min: 1, max: 2, returnType: 'boolean' }, - collator: { min: 0, max: 1 }, - format: { min: 1, max: Infinity, returnType: 'formatted' }, - image: { min: 1, max: 1, returnType: 'image' }, - literal: { min: 1, max: 1 }, - number: { min: 1, max: 3, returnType: 'number' }, - object: { min: 1, max: 2, returnType: 'object' }, - string: { min: 1, max: 2, returnType: 'string' }, - 'to-boolean': { min: 1, max: 1, returnType: 'boolean' }, - 'to-color': { min: 1, max: 3, returnType: 'color' }, - 'to-number': { min: 1, max: 3, returnType: 'number' }, - 'to-string': { min: 1, max: 1, returnType: 'string' }, - typeof: { min: 1, max: 1, returnType: 'string' }, - - // Interpolation - interpolate: { min: 3, max: Infinity }, - 'interpolate-hcl': { min: 3, max: Infinity }, - 'interpolate-lab': { min: 3, max: Infinity }, - step: { min: 2, max: Infinity }, - - // Feature data - 'feature-state': { min: 1, max: 1 }, - 'geometry-type': { min: 0, max: 0, returnType: 'string' }, - id: { min: 0, max: 0 }, - properties: { min: 0, max: 0, returnType: 'object' }, - - // Camera - zoom: { min: 0, max: 0, returnType: 'number' }, - pitch: { min: 0, max: 0, returnType: 'number' }, - 'distance-from-center': { min: 0, max: 0, returnType: 'number' }, - - // Heatmap - 'heatmap-density': { min: 0, max: 0, returnType: 'number' }, - - // Variable binding - let: { min: 2, max: Infinity }, - var: { min: 1, max: 1 }, - - // Array/object - at: { min: 2, max: 2 } - }; - constructor() { super({ inputSchema: ValidateExpressionInputSchema, @@ -151,6 +70,7 @@ export class ValidateExpressionTool extends BaseTool< input: z.infer ): Promise { try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- expression shape is validated below, not known upfront let expression: any; if (typeof input.expression === 'string') { try { @@ -174,21 +94,52 @@ export class ValidateExpressionTool extends BaseTool< const warnings: ExpressionIssue[] = []; const info: ExpressionIssue[] = []; - // Validate the expression - const metadata = this.validateExpression( + const { expressionType, depth, blocking } = this.walkStructure( expression, errors, warnings, - info, '' ); + // Only delegate to the real parser when the top-level shape is sound; + // structural errors above (empty array, unknown operator, ...) already + // explain the problem and would otherwise be duplicated or obscured by + // the library's own (differently-worded) diagnostics. + let returnType: string | undefined; + if (!blocking) { + const result = expressionSpec.createPropertyExpression( + expression, + GENERIC_PROPERTY_SPEC + ); + if (result.result === 'error') { + for (const err of result.value) { + errors.push({ + severity: 'error', + message: err.message, + path: err.key || undefined + }); + } + } else { + // Re-parse via createExpression (which createPropertyExpression already + // succeeded through) purely to read the inferred return type - its result + // type (StyleExpression) is public, unlike createPropertyExpression's. + const parsed = expressionSpec.createExpression( + expression, + GENERIC_PROPERTY_SPEC + ); + if (parsed.result === 'success') { + const kind = parsed.value.expression.type.kind; + returnType = kind === 'value' ? 'any' : kind; + } + } + } + const result: ValidateExpressionOutput = { valid: errors.length === 0, errors, warnings, info, - metadata + metadata: { expressionType, returnType, depth } }; const validatedResult = ValidateExpressionOutputSchema.parse(result); @@ -215,61 +166,52 @@ export class ValidateExpressionTool extends BaseTool< } } - private validateExpression( + /** + * Walks the raw expression tree to check structural validity (is this an + * array with a recognized operator?) and compute display metadata + * (expressionType, depth). Argument counts, types, and zoom-placement + * rules are left to `createPropertyExpression`. + */ + private walkStructure( + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- raw, unvalidated JSON expression tree expression: any, errors: ExpressionIssue[], warnings: ExpressionIssue[], - info: ExpressionIssue[], path: string, depth = 0 - ): { expressionType?: string; returnType?: string; depth: number } { - const maxDepth = depth; - - // Literal values are valid expressions + ): { expressionType?: string; depth: number; blocking: boolean } { if ( typeof expression === 'string' || typeof expression === 'number' || typeof expression === 'boolean' || expression === null ) { - return { - expressionType: 'literal', - returnType: typeof expression === 'string' ? 'string' : 'number', - depth: maxDepth - }; + return { expressionType: 'literal', depth, blocking: false }; } - // Expressions must be arrays if (!Array.isArray(expression)) { if (typeof expression === 'object') { - // Objects are valid as literals (for filter expressions, etc.) - return { - expressionType: 'literal-object', - returnType: 'object', - depth: maxDepth - }; + return { expressionType: 'literal-object', depth, blocking: false }; } errors.push({ severity: 'error', message: 'Expression must be an array or literal value', path: path || 'root' }); - return { depth: maxDepth }; + return { depth, blocking: true }; } - // Empty arrays are invalid if (expression.length === 0) { errors.push({ severity: 'error', message: 'Expression array cannot be empty', path: path || 'root' }); - return { depth: maxDepth }; + return { depth, blocking: true }; } const operator = expression[0]; - // Operator must be a string if (typeof operator !== 'string') { errors.push({ severity: 'error', @@ -277,12 +219,10 @@ export class ValidateExpressionTool extends BaseTool< path: path ? `${path}[0]` : '[0]', suggestion: 'Use a valid Mapbox expression operator' }); - return { depth: maxDepth }; + return { depth, blocking: true }; } - // Check if operator is valid - const operatorSpec = ValidateExpressionTool.OPERATORS[operator]; - if (!operatorSpec) { + if (!expressionSpec.isExpression(expression)) { errors.push({ severity: 'error', message: `Unknown expression operator: "${operator}"`, @@ -290,46 +230,29 @@ export class ValidateExpressionTool extends BaseTool< suggestion: 'Use a valid Mapbox expression operator (e.g., "get", "case", "match")' }); - return { expressionType: operator, depth: maxDepth }; + return { expressionType: operator, depth, blocking: true }; } - // Validate argument count - const args = expression.slice(1); - if (args.length < operatorSpec.min) { - errors.push({ - severity: 'error', - message: `Operator "${operator}" requires at least ${operatorSpec.min} argument(s), got ${args.length}`, - path: path || 'root', - suggestion: `Add ${operatorSpec.min - args.length} more argument(s)` - }); - } - if (operatorSpec.max !== Infinity && args.length > operatorSpec.max) { - errors.push({ - severity: 'error', - message: `Operator "${operator}" accepts at most ${operatorSpec.max} argument(s), got ${args.length}`, - path: path || 'root', - suggestion: `Remove ${args.length - operatorSpec.max} argument(s)` - }); - } - - // Recursively validate nested expressions - let currentDepth = depth; - args.forEach((arg, index) => { + let maxDepth = depth; + const interpolationTypeArgIndex = INTERPOLATION_TYPE_ARG_INDEX[operator]; + expression.slice(1).forEach((arg: unknown, index: number) => { + if (index === interpolationTypeArgIndex) { + // e.g. ["linear"] / ["exponential", 1.5] - a type descriptor, not an expression + return; + } if (Array.isArray(arg)) { const argPath = path ? `${path}[${index + 1}]` : `[${index + 1}]`; - const argMetadata = this.validateExpression( + const argResult = this.walkStructure( arg, errors, warnings, - info, argPath, depth + 1 ); - currentDepth = Math.max(currentDepth, argMetadata.depth); + maxDepth = Math.max(maxDepth, argResult.depth); } }); - // Provide depth warnings for very deeply nested expressions if (depth > 10) { warnings.push({ severity: 'warning', @@ -339,10 +262,6 @@ export class ValidateExpressionTool extends BaseTool< }); } - return { - expressionType: operator, - returnType: operatorSpec.returnType, - depth: Math.max(currentDepth, depth) - }; + return { expressionType: operator, depth: maxDepth, blocking: false }; } } diff --git a/src/tools/validate-style-tool/ValidateStyleTool.ts b/src/tools/validate-style-tool/ValidateStyleTool.ts index 99b4c1b..0fb1c0f 100644 --- a/src/tools/validate-style-tool/ValidateStyleTool.ts +++ b/src/tools/validate-style-tool/ValidateStyleTool.ts @@ -2,6 +2,11 @@ // Licensed under the MIT License. import { z } from 'zod'; +import { validate as validateMapboxStyle } from '@mapbox/mapbox-gl-style-spec'; +import type { + StyleSpecification, + ValidationError as MapboxValidationError +} from '@mapbox/mapbox-gl-style-spec'; import { BaseTool } from '../BaseTool.js'; import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; import { ValidateStyleInputSchema } from './ValidateStyleTool.input.schema.js'; @@ -21,11 +26,27 @@ interface MapboxStyle { [key: string]: any; } +// style-spec ValidationError messages are formatted as ": " +// for anything scoped below the document root (e.g. "layers[0].paint.circle-opacity: ..."). +// Root-level errors (e.g. "missing required property \"version\"") have no prefix. +const PATH_PREFIXED_MESSAGE = /^([\w[\]-]+(?:\.[\w[\]-]+)*): ([\s\S]*)$/; + +function splitPathFromMessage(message: string): { + path?: string; + message: string; +} { + const match = message.match(PATH_PREFIXED_MESSAGE); + return match ? { path: match[1], message: match[2] } : { message }; +} + /** * ValidateStyleTool - Validates Mapbox GL JS style JSON * - * Performs comprehensive validation of Mapbox style JSON against the Mapbox Style Specification. - * Checks for required fields, valid layer types, source references, and common configuration issues. + * Delegates spec-conformance checks (required properties, layer/source types, + * source references, expression correctness, zoom-expression placement, etc.) + * to the official `@mapbox/mapbox-gl-style-spec` package - the same validation + * logic mapbox-gl-js runs at runtime - then layers on a few UX-oriented + * suggestions (missing sprite/glyphs, empty style) the spec itself doesn't flag. * * @example * ```typescript @@ -50,28 +71,6 @@ export class ValidateStyleTool extends BaseTool< openWorldHint: false }; - private static readonly VALID_LAYER_TYPES = [ - 'fill', - 'line', - 'symbol', - 'circle', - 'heatmap', - 'fill-extrusion', - 'raster', - 'hillshade', - 'background', - 'sky' - ]; - - private static readonly VALID_SOURCE_TYPES = [ - 'vector', - 'raster', - 'raster-dem', - 'geojson', - 'image', - 'video' - ]; - constructor() { super({ inputSchema: ValidateStyleInputSchema, @@ -110,11 +109,8 @@ export class ValidateStyleTool extends BaseTool< const warnings: ValidationIssue[] = []; const info: ValidationIssue[] = []; - // Validate structure - this.validateStructure(style, errors, warnings, info); - this.validateSources(style, errors, warnings, info); - this.validateLayers(style, errors, warnings, info); - this.validateReferences(style, errors, warnings, info); + this.validateAgainstSpec(style, errors, warnings); + this.provideSuggestions(style, warnings, info); const result: ValidateStyleOutput = { valid: errors.length === 0, @@ -154,44 +150,45 @@ export class ValidateStyleTool extends BaseTool< } } - private validateStructure( + /** + * Runs the official Mapbox style-spec validator, the ground truth mapbox-gl-js + * itself uses at runtime, and maps its findings onto our issue schema. + */ + private validateAgainstSpec( style: MapboxStyle, errors: ValidationIssue[], - warnings: ValidationIssue[], - info: ValidationIssue[] + warnings: ValidationIssue[] ): void { - // Check version - if (!style.version) { - errors.push({ - severity: 'error', - message: 'Missing required "version" property', - path: 'version', - suggestion: 'Add "version": 8 to your style' - }); - } else if (style.version !== 8) { - warnings.push({ - severity: 'warning', - message: `Style version is ${style.version}, but version 8 is the current standard`, - path: 'version', - suggestion: 'Consider updating to version 8' - }); + // The style is arbitrary/possibly-malformed user input - that's exactly what + // we're validating - so it won't generally satisfy StyleSpecification's type. + const specErrors = validateMapboxStyle( + style as unknown as StyleSpecification + ); + + for (const err of specErrors as MapboxValidationError[]) { + const { path, message } = splitPathFromMessage(err.message); + const issue: ValidationIssue = { severity: 'error', message, path }; + // ValidationWarning is a style-spec subclass of ValidationError that isn't + // part of the package's public API, so we distinguish it by name. + if (err.constructor.name === 'ValidationWarning') { + issue.severity = 'warning'; + warnings.push(issue); + } else { + errors.push(issue); + } } + } - // Check layers array - if (!style.layers) { - errors.push({ - severity: 'error', - message: 'Missing required "layers" array', - path: 'layers', - suggestion: 'Add "layers": [] to your style' - }); - } else if (!Array.isArray(style.layers)) { - errors.push({ - severity: 'error', - message: '"layers" must be an array', - path: 'layers' - }); - } else if (style.layers.length === 0) { + /** + * Non-spec suggestions: things that are technically spec-valid but are + * usually mistakes or worth calling out for a better-configured style. + */ + private provideSuggestions( + style: MapboxStyle, + warnings: ValidationIssue[], + info: ValidationIssue[] + ): void { + if (Array.isArray(style.layers) && style.layers.length === 0) { warnings.push({ severity: 'warning', message: 'Style has no layers', @@ -200,26 +197,21 @@ export class ValidateStyleTool extends BaseTool< }); } - // Check sources object - if (!style.sources) { - errors.push({ - severity: 'error', - message: 'Missing required "sources" object', - path: 'sources', - suggestion: 'Add "sources": {} to your style' - }); - } else if ( - typeof style.sources !== 'object' || - Array.isArray(style.sources) + if ( + style.sources && + typeof style.sources === 'object' && + !Array.isArray(style.sources) && + Object.keys(style.sources).length === 0 ) { - errors.push({ - severity: 'error', - message: '"sources" must be an object', - path: 'sources' + info.push({ + severity: 'info', + message: 'No sources defined', + path: 'sources', + suggestion: + 'Add sources to provide data for your layers (e.g., vector tiles, GeoJSON)' }); } - // Check sprite if (!style.sprite) { info.push({ severity: 'info', @@ -229,7 +221,6 @@ export class ValidateStyleTool extends BaseTool< }); } - // Check glyphs if (!style.glyphs) { info.push({ severity: 'info', @@ -239,192 +230,4 @@ export class ValidateStyleTool extends BaseTool< }); } } - - private validateSources( - style: MapboxStyle, - errors: ValidationIssue[], - _warnings: ValidationIssue[], - info: ValidationIssue[] - ): void { - if (!style.sources || typeof style.sources !== 'object') { - return; - } - - const sourceIds = Object.keys(style.sources); - - if (sourceIds.length === 0) { - info.push({ - severity: 'info', - message: 'No sources defined', - path: 'sources', - suggestion: - 'Add sources to provide data for your layers (e.g., vector tiles, GeoJSON)' - }); - } - - for (const [sourceId, source] of Object.entries(style.sources)) { - if (!source || typeof source !== 'object') { - errors.push({ - severity: 'error', - message: `Source "${sourceId}" is not a valid object`, - path: `sources.${sourceId}` - }); - continue; - } - - // Check source type - if (!source.type) { - errors.push({ - severity: 'error', - message: `Source "${sourceId}" is missing required "type" property`, - path: `sources.${sourceId}.type`, - suggestion: `Specify one of: ${ValidateStyleTool.VALID_SOURCE_TYPES.join(', ')}` - }); - } else if (!ValidateStyleTool.VALID_SOURCE_TYPES.includes(source.type)) { - errors.push({ - severity: 'error', - message: `Source "${sourceId}" has invalid type "${source.type}"`, - path: `sources.${sourceId}.type`, - suggestion: `Valid types are: ${ValidateStyleTool.VALID_SOURCE_TYPES.join(', ')}` - }); - } - - // Type-specific validation - if (source.type === 'vector' || source.type === 'raster') { - if (!source.url && !source.tiles) { - errors.push({ - severity: 'error', - message: `Source "${sourceId}" must have either "url" or "tiles" property`, - path: `sources.${sourceId}`, - suggestion: 'Add a "url" or "tiles" array to specify tile data' - }); - } - } - - if (source.type === 'geojson') { - if (!source.data) { - errors.push({ - severity: 'error', - message: `GeoJSON source "${sourceId}" is missing required "data" property`, - path: `sources.${sourceId}.data`, - suggestion: - 'Add "data" property with GeoJSON object or URL to GeoJSON file' - }); - } - } - } - } - - private validateLayers( - style: MapboxStyle, - errors: ValidationIssue[], - warnings: ValidationIssue[], - _info: ValidationIssue[] - ): void { - if (!style.layers || !Array.isArray(style.layers)) { - return; - } - - const layerIds = new Set(); - - for (let i = 0; i < style.layers.length; i++) { - const layer = style.layers[i]; - const layerPath = `layers[${i}]`; - - // Check layer ID - if (!layer.id) { - errors.push({ - severity: 'error', - message: `Layer at index ${i} is missing required "id" property`, - path: layerPath, - suggestion: 'Add a unique "id" string to identify this layer' - }); - } else { - // Check for duplicate IDs - if (layerIds.has(layer.id)) { - errors.push({ - severity: 'error', - message: `Duplicate layer ID "${layer.id}"`, - path: `${layerPath}.id`, - suggestion: 'Each layer must have a unique ID' - }); - } - layerIds.add(layer.id); - } - - // Check layer type - if (!layer.type) { - errors.push({ - severity: 'error', - message: `Layer "${layer.id || `at index ${i}`}" is missing required "type" property`, - path: `${layerPath}.type`, - suggestion: `Specify one of: ${ValidateStyleTool.VALID_LAYER_TYPES.join(', ')}` - }); - } else if (!ValidateStyleTool.VALID_LAYER_TYPES.includes(layer.type)) { - errors.push({ - severity: 'error', - message: `Layer "${layer.id}" has invalid type "${layer.type}"`, - path: `${layerPath}.type`, - suggestion: `Valid types are: ${ValidateStyleTool.VALID_LAYER_TYPES.join(', ')}` - }); - } - - // Check source requirement - if (layer.type !== 'background' && layer.type !== 'sky') { - if (!layer.source) { - errors.push({ - severity: 'error', - message: `Layer "${layer.id}" of type "${layer.type}" must have a "source" property`, - path: `${layerPath}.source`, - suggestion: 'Reference a source ID defined in the "sources" object' - }); - } - } - - // Check source-layer for vector sources - if ( - layer.type !== 'background' && - layer.type !== 'sky' && - layer.type !== 'raster' - ) { - const source = style.sources?.[layer.source]; - if (source?.type === 'vector' && !layer['source-layer']) { - warnings.push({ - severity: 'warning', - message: `Layer "${layer.id}" uses vector source but missing "source-layer"`, - path: `${layerPath}.source-layer`, - suggestion: - 'Specify which source layer from the vector tileset to use' - }); - } - } - } - } - - private validateReferences( - style: MapboxStyle, - errors: ValidationIssue[], - _warnings: ValidationIssue[], - _info: ValidationIssue[] - ): void { - if (!style.layers || !Array.isArray(style.layers)) { - return; - } - - const sourceIds = new Set(Object.keys(style.sources || {})); - - for (let i = 0; i < style.layers.length; i++) { - const layer = style.layers[i]; - - // Check if referenced source exists - if (layer.source && !sourceIds.has(layer.source)) { - errors.push({ - severity: 'error', - message: `Layer "${layer.id || `at index ${i}`}" references non-existent source "${layer.source}"`, - path: `layers[${i}].source`, - suggestion: `Source "${layer.source}" is not defined in the "sources" object` - }); - } - } - } } diff --git a/test/tools/validate-expression-tool/ValidateExpressionTool.test.ts b/test/tools/validate-expression-tool/ValidateExpressionTool.test.ts index ab21f13..3994484 100644 --- a/test/tools/validate-expression-tool/ValidateExpressionTool.test.ts +++ b/test/tools/validate-expression-tool/ValidateExpressionTool.test.ts @@ -252,9 +252,7 @@ describe('ValidateExpressionTool', () => { const parsed = JSON.parse(result.content[0].text); expect(parsed.valid).toBe(false); - expect(parsed.errors[0].message).toContain( - 'requires at least 1 argument' - ); + expect(parsed.errors[0].message).toContain('Expected arguments of type'); }); it('should detect too many arguments', async () => { @@ -267,7 +265,57 @@ describe('ValidateExpressionTool', () => { const parsed = JSON.parse(result.content[0].text); expect(parsed.valid).toBe(false); - expect(parsed.errors[0].message).toContain('accepts at most 0 argument'); + expect(parsed.errors[0].message).toContain('Expected arguments of type'); + }); + }); + + describe('regression: issue #123 - zoom expression placement', () => { + it('should accept a top-level zoom interpolation (previously a false positive)', async () => { + const input = { + expression: [ + 'interpolate', + ['linear'], + ['zoom'], + 4, + 0.4, + 6, + 0.28, + 8, + 0 + ], + context: 'paint' + }; + + const result = await tool.run(input); + expect(result.isError).toBe(false); + + const parsed = JSON.parse(result.content[0].text); + expect(parsed.valid).toBe(true); + expect(parsed.errors).toHaveLength(0); + }); + + it('should reject zoom interpolations nested inside a case (previously a false negative)', async () => { + const input = { + expression: [ + 'case', + ['==', ['get', 'inside'], true], + ['interpolate', ['linear'], ['zoom'], 4, 0.4, 6, 0.28, 8, 0], + ['interpolate', ['linear'], ['zoom'], 4, 0.2, 6, 0.14, 8, 0] + ] + }; + + const result = await tool.run(input); + expect(result.isError).toBe(false); + + const parsed = JSON.parse(result.content[0].text); + expect(parsed.valid).toBe(false); + expect( + parsed.errors.some((e: any) => + e.message.includes( + '"zoom" expression may only be used as input to a top-level' + ) + ) + ).toBe(true); }); }); diff --git a/test/tools/validate-style-tool/ValidateStyleTool.test.ts b/test/tools/validate-style-tool/ValidateStyleTool.test.ts index bfa1a9b..c736783 100644 --- a/test/tools/validate-style-tool/ValidateStyleTool.test.ts +++ b/test/tools/validate-style-tool/ValidateStyleTool.test.ts @@ -59,7 +59,7 @@ describe('ValidateStyleTool', () => { expect(parsedResponse.errors).toContainEqual( expect.objectContaining({ severity: 'error', - message: 'Missing required "version" property' + message: expect.stringContaining('missing required property "version"') }) ); }); @@ -78,7 +78,7 @@ describe('ValidateStyleTool', () => { expect(parsedResponse.errors).toContainEqual( expect.objectContaining({ severity: 'error', - message: 'Missing required "layers" array' + message: expect.stringContaining('missing required property "layers"') }) ); }); @@ -104,7 +104,7 @@ describe('ValidateStyleTool', () => { expect(parsedResponse.errors).toContainEqual( expect.objectContaining({ severity: 'error', - message: expect.stringContaining('invalid type') + message: expect.stringContaining('"invalid-type" found') }) ); }); @@ -140,7 +140,7 @@ describe('ValidateStyleTool', () => { expect(parsedResponse.errors).toContainEqual( expect.objectContaining({ severity: 'error', - message: 'Duplicate layer ID "duplicate-id"' + message: expect.stringContaining('duplicate layer id "duplicate-id"') }) ); }); @@ -166,7 +166,9 @@ describe('ValidateStyleTool', () => { expect(parsedResponse.errors).toContainEqual( expect.objectContaining({ severity: 'error', - message: expect.stringContaining('references non-existent source') + message: expect.stringContaining( + 'source "non-existent-source" not found' + ) }) ); }); @@ -248,4 +250,85 @@ describe('ValidateStyleTool', () => { hasGlyphs: true }); }); + + describe('regression: issue #123 - zoom expression placement', () => { + it('should reject a zoom interpolation nested inside a case (previously a false negative)', async () => { + const style = { + version: 8, + sources: { + s: { + type: 'geojson', + data: { type: 'FeatureCollection', features: [] } + } + }, + layers: [ + { + id: 'l', + type: 'circle', + source: 's', + paint: { + 'circle-opacity': [ + 'case', + ['==', ['get', 'inside'], true], + ['interpolate', ['linear'], ['zoom'], 4, 0.4, 6, 0.28, 8, 0], + ['interpolate', ['linear'], ['zoom'], 4, 0.2, 6, 0.14, 8, 0] + ] + } + } + ] + }; + + const result = await tool.run({ style }); + + expect(result.isError).toBe(false); + const parsedResponse = JSON.parse(result.content[0].text); + expect(parsedResponse.valid).toBe(false); + expect( + parsedResponse.errors.some((e: any) => + e.message.includes( + '"zoom" expression may only be used as input to a top-level' + ) + ) + ).toBe(true); + }); + + it('should accept the same style with a top-level (non-nested) zoom interpolation', async () => { + const style = { + version: 8, + sources: { + s: { + type: 'geojson', + data: { type: 'FeatureCollection', features: [] } + } + }, + layers: [ + { + id: 'l', + type: 'circle', + source: 's', + paint: { + 'circle-opacity': [ + 'interpolate', + ['linear'], + ['zoom'], + 4, + 0.4, + 6, + 0.28, + 8, + 0 + ] + } + } + ] + }; + + const result = await tool.run({ style }); + + expect(result.isError).toBe(false); + const parsedResponse = JSON.parse(result.content[0].text); + expect(parsedResponse.valid).toBe(true); + expect(parsedResponse.errors).toHaveLength(0); + }); + }); }); From 9e56477e0571f387bb04430f12c1c96ccadd4b4a Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 14 Jul 2026 11:56:06 -0400 Subject: [PATCH 3/3] Bound recursion depth to prevent stack-overflow on adversarial input 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 --- .../ValidateExpressionTool.ts | 22 +++++++++++- .../validate-style-tool/ValidateStyleTool.ts | 35 +++++++++++++++++++ .../ValidateExpressionTool.test.ts | 22 ++++++++++++ .../ValidateStyleTool.test.ts | 27 ++++++++++++++ 4 files changed, 105 insertions(+), 1 deletion(-) diff --git a/src/tools/validate-expression-tool/ValidateExpressionTool.ts b/src/tools/validate-expression-tool/ValidateExpressionTool.ts index f64b830..af95447 100644 --- a/src/tools/validate-expression-tool/ValidateExpressionTool.ts +++ b/src/tools/validate-expression-tool/ValidateExpressionTool.ts @@ -22,6 +22,13 @@ const INTERPOLATION_TYPE_ARG_INDEX: Record = { 'interpolate-lab': 0 }; +// Recursion in walkStructure (and in createPropertyExpression's own parser) is +// bounded by user-controlled nesting depth. A pathologically deep expression +// (cheap to construct - a few KB of brackets) would otherwise recurse tens of +// thousands of frames deep before failing with an unhelpful stack overflow. +// This cap is far beyond any realistic hand-written or generated expression. +const MAX_EXPRESSION_DEPTH = 64; + // `StylePropertySpecification` only covers concrete typed properties (color, string, // number, ...), so a literal like `true` or `"red"` would fail type-checking against // any of them. Using an unrecognized `type` makes createPropertyExpression skip return @@ -180,6 +187,17 @@ export class ValidateExpressionTool extends BaseTool< path: string, depth = 0 ): { expressionType?: string; depth: number; blocking: boolean } { + // Checked before recursing further, so the call stack never grows past this + // regardless of how deeply nested the (attacker-controlled) input actually is. + if (depth > MAX_EXPRESSION_DEPTH) { + errors.push({ + severity: 'error', + message: `Expression exceeds maximum nesting depth of ${MAX_EXPRESSION_DEPTH}`, + path: path || 'root' + }); + return { depth, blocking: true }; + } + if ( typeof expression === 'string' || typeof expression === 'number' || @@ -234,6 +252,7 @@ export class ValidateExpressionTool extends BaseTool< } let maxDepth = depth; + let anyBlocking = false; const interpolationTypeArgIndex = INTERPOLATION_TYPE_ARG_INDEX[operator]; expression.slice(1).forEach((arg: unknown, index: number) => { if (index === interpolationTypeArgIndex) { @@ -250,6 +269,7 @@ export class ValidateExpressionTool extends BaseTool< depth + 1 ); maxDepth = Math.max(maxDepth, argResult.depth); + anyBlocking = anyBlocking || argResult.blocking; } }); @@ -262,6 +282,6 @@ export class ValidateExpressionTool extends BaseTool< }); } - return { expressionType: operator, depth: maxDepth, blocking: false }; + return { expressionType: operator, depth: maxDepth, blocking: anyBlocking }; } } diff --git a/src/tools/validate-style-tool/ValidateStyleTool.ts b/src/tools/validate-style-tool/ValidateStyleTool.ts index 0fb1c0f..6a6d829 100644 --- a/src/tools/validate-style-tool/ValidateStyleTool.ts +++ b/src/tools/validate-style-tool/ValidateStyleTool.ts @@ -39,6 +39,29 @@ function splitPathFromMessage(message: string): { return match ? { path: match[1], message: match[2] } : { message }; } +// validateMapboxStyle recurses into nested paint/layout expressions with no +// depth limit of its own. A pathologically deep expression (cheap to construct +// - a few KB of brackets) would otherwise recurse tens of thousands of frames +// deep before failing with an unhelpful stack overflow. This check is itself +// depth-limited (bails before recursing further) so it can safely measure +// arbitrarily deep input without risking the same problem. +const MAX_STYLE_DEPTH = 64; + +function exceedsMaxDepth(value: unknown, depth = 0): boolean { + if (depth > MAX_STYLE_DEPTH) { + return true; + } + if (Array.isArray(value)) { + return value.some((item) => exceedsMaxDepth(item, depth + 1)); + } + if (value && typeof value === 'object') { + return Object.values(value).some((item) => + exceedsMaxDepth(item, depth + 1) + ); + } + return false; +} + /** * ValidateStyleTool - Validates Mapbox GL JS style JSON * @@ -105,6 +128,18 @@ export class ValidateStyleTool extends BaseTool< style = input.style as MapboxStyle; } + if (exceedsMaxDepth(style)) { + return { + content: [ + { + type: 'text', + text: `Error: style exceeds maximum nesting depth of ${MAX_STYLE_DEPTH}` + } + ], + isError: true + }; + } + const errors: ValidationIssue[] = []; const warnings: ValidationIssue[] = []; const info: ValidationIssue[] = []; diff --git a/test/tools/validate-expression-tool/ValidateExpressionTool.test.ts b/test/tools/validate-expression-tool/ValidateExpressionTool.test.ts index 3994484..ff6b922 100644 --- a/test/tools/validate-expression-tool/ValidateExpressionTool.test.ts +++ b/test/tools/validate-expression-tool/ValidateExpressionTool.test.ts @@ -319,6 +319,28 @@ describe('ValidateExpressionTool', () => { }); }); + describe('deeply nested (adversarial) expressions', () => { + it('should reject an expression exceeding the max nesting depth instead of recursing unbounded', async () => { + let expression: any = ['get', 'value']; + for (let i = 0; i < 5000; i++) { + expression = ['+', expression, 1]; + } + + const start = Date.now(); + const result = await tool.run({ expression }); + expect(Date.now() - start).toBeLessThan(1000); + + expect(result.isError).toBe(false); + const parsed = JSON.parse(result.content[0].text); + expect(parsed.valid).toBe(false); + expect( + parsed.errors.some((e: any) => + e.message.includes('exceeds maximum nesting depth') + ) + ).toBe(true); + }); + }); + describe('nested expressions', () => { it('should validate nested expressions', async () => { const input = { diff --git a/test/tools/validate-style-tool/ValidateStyleTool.test.ts b/test/tools/validate-style-tool/ValidateStyleTool.test.ts index c736783..501aa58 100644 --- a/test/tools/validate-style-tool/ValidateStyleTool.test.ts +++ b/test/tools/validate-style-tool/ValidateStyleTool.test.ts @@ -331,4 +331,31 @@ describe('ValidateStyleTool', () => { expect(parsedResponse.errors).toHaveLength(0); }); }); + + describe('deeply nested (adversarial) expressions', () => { + it('should reject a style with an expression exceeding the max nesting depth instead of recursing unbounded', async () => { + let expression: any = ['get', 'value']; + for (let i = 0; i < 5000; i++) { + expression = ['+', expression, 1]; + } + const style = { + version: 8, + sources: {}, + layers: [ + { + id: 'l', + type: 'background', + paint: { 'background-opacity': expression } + } + ] + }; + + const start = Date.now(); + const result = await tool.run({ style }); + expect(Date.now() - start).toBeLessThan(1000); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('exceeds maximum nesting depth'); + }); + }); });