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..af95447 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,43 @@ 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 +}; + +// 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 +// 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 +66,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 +77,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 +101,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 +173,63 @@ 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; + ): { 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 }; + } - // Literal values are valid expressions 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 +237,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 +248,31 @@ 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; + let anyBlocking = false; + 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); + anyBlocking = anyBlocking || argResult.blocking; } }); - // Provide depth warnings for very deeply nested expressions if (depth > 10) { warnings.push({ severity: 'warning', @@ -339,10 +282,6 @@ export class ValidateExpressionTool extends BaseTool< }); } - return { - expressionType: operator, - returnType: operatorSpec.returnType, - depth: Math.max(currentDepth, depth) - }; + 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 99b4c1b..6a6d829 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,50 @@ 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 }; +} + +// 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 * - * 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 +94,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, @@ -106,15 +128,24 @@ 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[] = []; - // 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 +185,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 +232,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 +256,6 @@ export class ValidateStyleTool extends BaseTool< }); } - // Check glyphs if (!style.glyphs) { info.push({ severity: 'info', @@ -239,192 +265,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..ff6b922 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,79 @@ 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); + }); + }); + + 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); }); }); diff --git a/test/tools/validate-style-tool/ValidateStyleTool.test.ts b/test/tools/validate-style-tool/ValidateStyleTool.test.ts index bfa1a9b..501aa58 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,112 @@ 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); + }); + }); + + 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'); + }); + }); });