diff --git a/.changeset/buy-the-validators.md b/.changeset/buy-the-validators.md new file mode 100644 index 0000000..a9bf39e --- /dev/null +++ b/.changeset/buy-the-validators.md @@ -0,0 +1,11 @@ +--- +'fiber': patch +--- + +Validate request bodies with Ajv, and read credential paths with real JSONPath. + +Body linting was a hand-written walker over "a deliberately useful subset" of JSON Schema, and the subset was the problem: no `$ref`, no `minimum`, `pattern`, `uniqueItems`, `minLength`, `patternProperties`, `if`/`then` or `dependentSchemas`. All of those passed silently, so a body could be reported clean and still be rejected by the API that published the schema. Ajv is the reference implementation of what that walker was approximating. + +What stays hand-written is the part that isn't JSON Schema: OpenAPI 3.0's `nullable`, folded into a union type, and `type` values that don't exist. One real 3.1 document arrives with `"type": "undefined"` 310 times alongside `emoji`, `icon`, `void` and `http` — Ajv rejects those at compile time, which would cost that schema all of its linting rather than one field's, so the invented names are dropped and every valid constraint beside them keeps working. Messages are unchanged, including the "must be number, not string" phrasing Ajv leaves out. + +Credential paths now go through `serde_json_path`, so a capture rule can use `$..id_token` to find a token whose nesting depth you don't know, or `$.keys[?(@.active == true)].secret` to pick the entry that is current rather than pinning an index that moves. The dotted form every saved rule already uses keeps working: it isn't valid JSONPath — `$.data.tokens.0.value` needs `[0]` under RFC 9535 — so the query is tried first and the original walk answers for anything it rejects. A query matching several nodes reports nothing rather than picking arbitrarily. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1cb1fae..c73ff00 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -104,6 +104,14 @@ jobs: - uses: actions/setup-node@v5 with: node-version-file: .nvmrc + # This job reads the lockfile and never installs, so there is no store + # to cache. Left on, setup-node infers pnpm from `packageManager` and + # enables caching anyway: a cache *hit* restores and skips the save, so + # this passed for as long as the key kept hitting — and the first + # lockfile change missed, tried to save a store that was never created, + # and failed the job on a post-step while both audits had passed. + # release.yml's draft job turns it off for the same reason. + package-manager-cache: false # --prod: a vulnerable dev tool can't reach a user's machine. High and # above, because failing CI over every low advisory teaches people to diff --git a/package.json b/package.json index 9f4d1ce..2fe4912 100644 --- a/package.json +++ b/package.json @@ -54,6 +54,7 @@ "@tauri-apps/plugin-opener": "2.5.4", "@tauri-apps/plugin-process": "2.3.1", "@tauri-apps/plugin-updater": "2.10.1", + "ajv": "^8.20.0", "bits-ui": "^2.19.0", "codemirror": "^6.0.2", "paneforge": "^1.0.2" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f93310f..fb53f10 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -41,6 +41,9 @@ importers: '@tauri-apps/plugin-updater': specifier: 2.10.1 version: 2.10.1 + ajv: + specifier: ^8.20.0 + version: 8.20.0 bits-ui: specifier: ^2.19.0 version: 2.19.0(@internationalized/date@3.12.3)(@sveltejs/kit@2.70.3(@sveltejs/vite-plugin-svelte@7.3.0(svelte@5.56.10)(vite@8.2.2(@types/node@26.2.0)(jiti@2.7.0)(yaml@2.9.0)))(svelte@5.56.10)(typescript@6.0.3)(vite@8.2.2(@types/node@26.2.0)(jiti@2.7.0)(yaml@2.9.0)))(svelte@5.56.10) @@ -777,6 +780,9 @@ packages: engines: {node: '>=0.4.0'} hasBin: true + ajv@8.20.0: + resolution: {integrity: sha512-Thbli+OlOj+iMPYFBVBfJ3OmCAnaSyNn4M1vz9T6Gka5Jt9ba/HIR56joy65tY6kx/FCF5VXNB819Y7/GUrBGA==} + aria-query@5.3.1: resolution: {integrity: sha512-Z/ZeOgVl7bcSYZ/u/rh0fOpvEpq//LZmdbkXyc7syVzjPAhfOa9ebsdTSjEBDU4vs5nC98Kfduj1uFo0qyET3g==} engines: {node: '>= 0.4'} @@ -876,12 +882,18 @@ packages: estree-walker@3.0.3: resolution: {integrity: sha512-7RUKfXgSMMkzt6ZuXmqapOurLGPPfgj6l9uRZ7lRGolvk0y2yocc35LdcxKC5PQZdn2DMqioAQ2NoWcrTKmm6g==} + fast-deep-equal@3.1.3: + resolution: {integrity: sha512-f3qQ9oQy9j2AhBe/H9VC91wLmKBCCU/gDOnKNAYG5hswO7BLKj09Hc5HYNz9cGI++xlpDCIgDaitVs03ATR84Q==} + fast-string-truncated-width@3.0.3: resolution: {integrity: sha512-0jjjIEL6+0jag3l2XWWizO64/aZVtpiGE3t0Zgqxv0DPuxiMjvB3M24fCyhZUO4KomJQPj3LTSUnDP3GpdwC0g==} fast-string-width@3.0.2: resolution: {integrity: sha512-gX8LrtNEI5hq8DVUfRQMbr5lpaS4nMIWV+7XEbXk2b8kiQIizgnlr12B4dA3ZEx3308ze0O4Q1R+cHts8kyUJg==} + fast-uri@3.1.6: + resolution: {integrity: sha512-7Ical1vFEMr0onbVzEDIreM22I4khW+fzyQPwvAFWBp1iwdshSZRsL4jjRvPG9JP1uiqMHRto+YU6R2/CzDz5Q==} + fast-wrap-ansi@0.2.2: resolution: {integrity: sha512-7F2Fl+TjRSenLqlU3UjSH0iyqopqoZIu7eZVpEirP2g1GtWa2G/ecEmBdgz31+Mxr+ELclgg6sokpSFIQiZ02Q==} @@ -928,6 +940,9 @@ packages: jju@1.4.0: resolution: {integrity: sha512-8wb9Yw966OSxApiCt0K3yNJL8pnNeIv+OEq2YMidz4FKP6nonSRoOXc80iXY4JaN2FC11B9qsNmDsm+ZOfMROA==} + json-schema-traverse@1.0.0: + resolution: {integrity: sha512-NM8/P9n3XjXhIZn1lLhkFaACTOURQXjWhV4BA/RnOv8xvgqtqpAX9IO4mRQxSx1Rlo4tqzeqb0sOlruaOy3dug==} + jsonc-parser@3.3.1: resolution: {integrity: sha512-HUgH65KyejrUFPvHFPbqOY0rsFip3Bo5wb4ngvdi1EpCYWUQDC5V+Y7mZws+DLkr4M//zQJoanu1SP+87Dv1oQ==} @@ -1122,6 +1137,10 @@ packages: resolution: {integrity: sha512-iETxpjK6YoRWJG5o6hXLwvjYAoW+FEZn9os0PD/b6AP6xQwsa/Y7lCVgIixBbUPMfhu+i2LtdeAqVTgGlQarfA==} hasBin: true + require-from-string@2.0.2: + resolution: {integrity: sha512-Xf0nWe6RseziFMu+Ap9biiUbmplq6S9/p+7w7YXP/JBHhrUDDUhwa+vANyubuqfZWTveU//DYVGsDG7RKL/vEw==} + engines: {node: '>=0.10.0'} + rolldown@1.2.5: resolution: {integrity: sha512-VD2IE5PUG4Oj8zz2VGykiYd5wbnjdIiSsNQb8Qu5B+noEp+A78mu2iVvpp27g8es14Tk9rofNs5Tku9iQCS4fA==} engines: {node: ^20.19.0 || >=22.12.0} @@ -2023,6 +2042,13 @@ snapshots: acorn@8.18.0: {} + ajv@8.20.0: + dependencies: + fast-deep-equal: 3.1.3 + fast-uri: 3.1.6 + json-schema-traverse: 1.0.0 + require-from-string: 2.0.2 + aria-query@5.3.1: {} axobject-query@4.1.0: {} @@ -2105,12 +2131,16 @@ snapshots: dependencies: '@types/estree': 1.0.9 + fast-deep-equal@3.1.3: {} + fast-string-truncated-width@3.0.3: {} fast-string-width@3.0.2: dependencies: fast-string-truncated-width: 3.0.3 + fast-uri@3.1.6: {} + fast-wrap-ansi@0.2.2: dependencies: fast-string-width: 3.0.2 @@ -2143,6 +2173,8 @@ snapshots: jju@1.4.0: {} + json-schema-traverse@1.0.0: {} + jsonc-parser@3.3.1: {} kleur@4.1.5: {} @@ -2324,6 +2356,8 @@ snapshots: regexp-tree@0.1.27: {} + require-from-string@2.0.2: {} + rolldown@1.2.5: dependencies: '@oxc-project/types': 0.146.0 diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 78d35d9..e0bb9ab 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -1302,6 +1302,7 @@ dependencies = [ "security-framework", "serde", "serde_json", + "serde_json_path", "serde_norway", "tauri", "tauri-build", @@ -2204,6 +2205,15 @@ dependencies = [ "generic-array", ] +[[package]] +name = "inventory" +version = "0.3.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a4f0c30c76f2f4ccee3fe55a2435f691ca00c0e4bd87abe4f4a851b1d4dac39b" +dependencies = [ + "rustversion", +] + [[package]] name = "ipnet" version = "2.12.1" @@ -2663,6 +2673,12 @@ dependencies = [ "unicase", ] +[[package]] +name = "minimal-lexical" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "68354c5c6bd36d73ff3feceb05efa59b6acb7626617f4962be322a825e61f79a" + [[package]] name = "minisign-verify" version = "0.2.5" @@ -2741,6 +2757,16 @@ version = "1.0.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "650eef8c711430f1a879fdd01d4745a7deea475becfb90269c06775983bbf086" +[[package]] +name = "nom" +version = "7.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d273983c5a657a70a3e8f2a01329822f3b8c8172b73826411a55751e404a0a4a" +dependencies = [ + "memchr", + "minimal-lexical", +] + [[package]] name = "num" version = "0.4.3" @@ -4095,6 +4121,56 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_json_path" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b992cea3194eea663ba99a042d61cea4bd1872da37021af56f6a37e0359b9d33" +dependencies = [ + "inventory", + "nom", + "regex", + "serde", + "serde_json", + "serde_json_path_core", + "serde_json_path_macros", + "thiserror 2.0.20", +] + +[[package]] +name = "serde_json_path_core" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dde67d8dfe7d4967b5a95e247d4148368ddd1e753e500adb34b3ffe40c6bc1bc" +dependencies = [ + "inventory", + "serde", + "serde_json", + "thiserror 2.0.20", +] + +[[package]] +name = "serde_json_path_macros" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "517acfa7f77ddaf5c43d5f119c44a683774e130b4247b7d3210f8924506cfac8" +dependencies = [ + "inventory", + "serde_json_path_core", + "serde_json_path_macros_internal", +] + +[[package]] +name = "serde_json_path_macros_internal" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "aafbefbe175fa9bf03ca83ef89beecff7d2a95aaacd5732325b90ac8c3bd7b90" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", +] + [[package]] name = "serde_norway" version = "0.9.42" diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index d88a117..f738e97 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -77,6 +77,7 @@ rmcp = { version = "3.1.4", features = ["server", "macros", "transport-io", "sch dirs = "6.0.0" schemars = "1.2.2" serde_norway = "0.9.42" +serde_json_path = "0.7.2" # Asking whether a keychain item exists, without reading it. keyring has no such # call — its `has` fetches the password — and on macOS that difference is a diff --git a/src-tauri/src/auth.rs b/src-tauri/src/auth.rs index b325d39..b0be425 100644 --- a/src-tauri/src/auth.rs +++ b/src-tauri/src/auth.rs @@ -310,15 +310,62 @@ async fn log_in( value_at(&parsed, token_path).ok_or_else(|| AuthError::NoToken(token_path.to_string())) } -/// Pulls a value out of a JSON document by dotted path. +/// Pulls a value out of a JSON document by path. /// /// Shared with `browser.rs`, which uses it to dig a token out of whatever JSON /// blob an SDK left in `localStorage`. /// -/// Accepts an optional `$.` prefix, and numeric segments index into arrays: -/// `$.data.tokens.0.value`. Deliberately not a full JSONPath — the extra syntax -/// buys nothing for reading one field out of a login response. +/// Two syntaxes, and both have to keep working: +/// +/// - **The dotted one this has always taken.** `$.data.tokens.0.value`, where a +/// numeric segment indexes an array. It is in every capture rule and every +/// login config already saved to disk, and it is not valid JSONPath — RFC +/// 9535 wants `$.data.tokens[0].value`, because `.0` is not a member name. +/// - **Real JSONPath**, via `serde_json_path`, which reaches what the dotted +/// form cannot: `$..token` for a blob whose depth you don't know, +/// `$.keys[?(@.active)].secret` for a list you have to pick out of. +/// +/// So the query is tried first, and the dotted walk answers for anything it +/// rejects or finds nothing for. That ordering matters: a dotted path is +/// *usually* also a valid JSONPath, but not always — an object keyed `"0"` is +/// reachable dotted and not by `[0]` — and the fallback is what makes adopting +/// the library free of migrations. pub(crate) fn value_at(value: &Value, path: &str) -> Option { + if let Some(found) = by_json_path(value, path) { + return Some(found); + } + value_at_dotted(value, path) +} + +/// The JSONPath reading, when the path is one and it matches exactly one node. +/// +/// A query yielding several nodes is ambiguous for "the token", and picking the +/// first would make a capture rule that silently changes meaning as the +/// document grows. Nothing is better than arbitrary here — the dotted walk gets +/// its turn either way. +fn by_json_path(value: &Value, path: &str) -> Option { + let trimmed = path.trim(); + if trimmed.is_empty() { + return None; + } + // The library requires the root identifier; the dotted form has always let + // it be left off. + let query = if trimmed.starts_with('$') { + std::borrow::Cow::Borrowed(trimmed) + } else { + std::borrow::Cow::Owned(format!("$.{trimmed}")) + }; + + let found = serde_json_path::JsonPath::parse(&query) + .ok()? + .query(value) + .exactly_one() + .ok()?; + scalar(found) +} + +/// The original walk: dotted segments, numbers indexing arrays. +fn value_at_dotted(value: &Value, path: &str) -> Option { let trimmed = path.trim().trim_start_matches('$').trim_start_matches('.'); if trimmed.is_empty() { return None; @@ -336,10 +383,16 @@ pub(crate) fn value_at(value: &Value, path: &str) -> Option { }; } - match current { + scalar(current) +} + +/// A credential has to be a single scalar; an object or array is a wrong turn +/// in the path rather than a token. +fn scalar(value: &Value) -> Option { + match value { Value::String(text) => Some(text.clone()), // A numeric or boolean token is unusual but not worth rejecting. - Value::Number(_) | Value::Bool(_) => Some(current.to_string()), + Value::Number(_) | Value::Bool(_) => Some(value.to_string()), _ => None, } } @@ -380,6 +433,47 @@ mod tests { } } + /// What the dotted form cannot reach, and the reason for a real query + /// engine rather than a wider hand-rolled one. + #[test] + fn a_jsonpath_query_reaches_what_a_dotted_path_cannot() { + let doc: Value = serde_json::from_str( + r#"{"a":{"b":{"c":{"id_token":"deep"}}}, + "keys":[{"active":false,"secret":"old"},{"active":true,"secret":"new"}]}"#, + ) + .unwrap(); + + // Descend to a key whose depth you don't know — the shape an SDK blob + // has when it nests by client id and audience. + assert_eq!(value_at(&doc, "$..id_token").as_deref(), Some("deep")); + // Pick the entry that is current, rather than pinning an index that + // moves the next time the list is written. + assert_eq!( + value_at(&doc, "$.keys[?(@.active == true)].secret").as_deref(), + Some("new") + ); + } + + /// Several matches is not "the token". Picking the first would make a + /// capture rule quietly change meaning as the document grows. + #[test] + fn an_ambiguous_query_reports_nothing() { + let doc: Value = serde_json::from_str(r#"{"a":{"token":"x"},"b":{"token":"y"}}"#).unwrap(); + assert_eq!(value_at(&doc, "$..token"), None); + } + + /// The one case where the dotted reading and JSONPath genuinely disagree: + /// an object keyed "0" is a member, not an index. The fallback is what + /// keeps every rule already on disk working. + #[test] + fn a_numeric_object_key_still_resolves() { + let doc: Value = serde_json::from_str(r#"{"sessions":{"0":{"token":"kept"}}}"#).unwrap(); + assert_eq!( + value_at(&doc, "$.sessions.0.token").as_deref(), + Some("kept") + ); + } + #[test] fn extracts_tokens_by_path() { let doc: Value = serde_json::from_str( @@ -400,6 +494,15 @@ mod tests { assert_eq!(value_at(&doc, "$.missing"), None); assert_eq!(value_at(&doc, "$.data.tokens.9.value"), None); assert_eq!(value_at(&doc, ""), None); + + // Bracket indexing — the spelling RFC 9535 actually wants, which the + // dotted walk never accepted. + assert_eq!( + value_at(&doc, "$.data.tokens[0].value").as_deref(), + Some("first") + ); + // A path to an object is not a credential, by either reading. + assert_eq!(value_at(&doc, "$.data"), None); } /// A stand-in for the keychain that records how often it was consulted — diff --git a/src/lib/json-schema.ts b/src/lib/json-schema.ts index 3d6c7da..2064d4a 100644 --- a/src/lib/json-schema.ts +++ b/src/lib/json-schema.ts @@ -1,103 +1,196 @@ -/** A small, deliberately useful subset of JSON Schema for request editing. */ -interface Schema { - type?: string | string[]; - nullable?: boolean; - enum?: unknown[]; - const?: unknown; - required?: string[]; - properties?: Record; - items?: unknown; - additionalProperties?: boolean | unknown; - allOf?: unknown[]; - anyOf?: unknown[]; - oneOf?: unknown[]; -} +// `.js` explicitly: Ajv 8 ships no `exports` map, so Node resolves this path +// literally and an extensionless import fails outside a bundler. +import Ajv2020, { type ErrorObject, type ValidateFunction } from 'ajv/dist/2020.js'; -function isSchema(value: unknown): value is Schema { - return value !== null && typeof value === 'object' && !Array.isArray(value); -} +/** + * Schema linting for the request and response panes, on top of Ajv. + * + * This was a hand-written walker over "a deliberately useful subset" of JSON + * Schema, and the subset was the problem: no `$ref`, no `minimum`, `pattern`, + * `uniqueItems`, `minLength`, `patternProperties`, `if`/`then`, or + * `dependentSchemas`. Every one of those silently passed, so a body could be + * reported clean and still be rejected by the API that published the schema. + * Ajv is the reference implementation of the thing we were approximating. + * + * What stays hand-written is everything that is *not* JSON Schema — the two + * ways a real OpenAPI document diverges from it, both handled in `normalize` + * below. Ajv is strict about its own spec, and correctly so; the documents we + * are handed are not. + */ -function valueKind(value: unknown): string { - if (value === null) return 'null'; - if (Array.isArray(value)) return 'array'; - return typeof value; -} +/** The only values `type` may hold. Anything else is a generator's invention. */ +const JSON_TYPES = new Set([ + 'null', + 'boolean', + 'object', + 'array', + 'number', + 'string', + 'integer' +]); -function acceptsType(value: unknown, type: string): boolean { - switch (type) { - case 'null': - return value === null; - case 'boolean': - return typeof value === 'boolean'; - case 'string': - return typeof value === 'string'; - case 'number': - return typeof value === 'number'; - case 'integer': - return typeof value === 'number' && Number.isInteger(value); - case 'array': - return Array.isArray(value); - case 'object': - return value !== null && typeof value === 'object' && !Array.isArray(value); - default: - return true; - } -} +const ajv = new Ajv2020({ + // Every error, not just the first: the pane lists them. + allErrors: true, + // OpenAPI schemas carry keywords Ajv doesn't know — `discriminator`, `xml`, + // `externalDocs`. They are annotations, not assertions, so ignoring them is + // right; strict mode would make each one fatal instead. + strict: false, + // `format` in OpenAPI is a documentation hint as often as a constraint + // (`format: "int64"`, `format: "uuid"`), and failing a body over one would + // be noise where the API itself does not care. + validateFormats: false +}); -function childPath(parent: string, key: string): string { - return /^[A-Za-z_$][\w$]*$/.test(key) ? `${parent}.${key}` : `${parent}[${JSON.stringify(key)}]`; -} - -function validate(value: unknown, schema: unknown, path: string): string[] { - if (!isSchema(schema)) return []; +/** + * An OpenAPI schema, made into one Ajv will accept. + * + * Two divergences, both real and both seen in specs in the wild: + * + * - **3.0's `nullable: true`.** 3.0 predates JSON Schema's union types and + * spells nullability with its own keyword. 3.1 writes `type: ["string", + * "null"]`. Ajv implements 3.1's reading, so the older spelling is folded + * into it here rather than silently ignored — ignoring it would report a + * legitimate `null` as the wrong type. + * - **Types that do not exist.** One real 3.1 document reaches us with + * `"type": "undefined"` 310 times, plus `emoji`, `icon`, `void` and `http`. + * Ajv throws on those at compile time, which would cost the whole document + * its linting over a field nobody was going to check anyway. Dropping the + * invalid names keeps every valid constraint in the same schema working. + */ +function normalize(value: unknown): unknown { + if (Array.isArray(value)) return value.map(normalize); + if (value === null || typeof value !== 'object') return value; - const errors: string[] = []; - const types = schema.type ? (Array.isArray(schema.type) ? schema.type : [schema.type]) : []; - if (schema.nullable && !types.includes('null')) types.push('null'); - if (types.length && !types.some((type) => acceptsType(value, type))) { - errors.push(`${path} must be ${types.join(' or ')}, not ${valueKind(value)}.`); - return errors; + const source = value as Record; + const out: Record = {}; + for (const [key, nested] of Object.entries(source)) { + // `properties` and `$defs` hold *names*, which may be anything at all — + // a property called "type" is a property, not a type. Recurse into their + // values without treating their keys as keywords. + out[key] = key === 'properties' || key === '$defs' || key === 'definitions' + ? Object.fromEntries( + Object.entries((nested ?? {}) as Record).map(([name, schema]) => [ + name, + normalize(schema) + ]) + ) + : normalize(nested); } - if (schema.const !== undefined && JSON.stringify(value) !== JSON.stringify(schema.const)) { - errors.push(`${path} must equal ${JSON.stringify(schema.const)}.`); - } - if (schema.enum && !schema.enum.some((option) => JSON.stringify(option) === JSON.stringify(value))) { - errors.push(`${path} must be one of: ${schema.enum.map((option) => JSON.stringify(option)).join(', ')}.`); - } + if ('type' in out) { + const declared = (Array.isArray(out.type) ? out.type : [out.type]).filter( + (name): name is string => typeof name === 'string' && JSON_TYPES.has(name) + ); + if (out.nullable === true && !declared.includes('null')) declared.push('null'); - for (const part of schema.allOf ?? []) errors.push(...validate(value, part, path)); - if (schema.anyOf && !schema.anyOf.some((part) => validate(value, part, path).length === 0)) { - errors.push(`${path} does not match any allowed schema.`); - } - if (schema.oneOf) { - const hits = schema.oneOf.filter((part) => validate(value, part, path).length === 0).length; - if (hits !== 1) { - errors.push(`${path} must match exactly one allowed schema.`); + if (declared.length === 0) { + // Every name was an invention. Saying nothing about the type still + // leaves `required`, `enum` and the rest of this schema enforceable. + delete out.type; + } else { + out.type = declared.length === 1 ? declared[0] : declared; } } - if (!isSchema(value) || Array.isArray(value)) { - if (Array.isArray(value) && schema.items) { - for (const [index, item] of value.entries()) errors.push(...validate(item, schema.items, `${path}[${index}]`)); - } - return errors; + return out; +} + +/** + * Compiled validators, keyed by the schema they came from. + * + * Linting runs on every keystroke and compiling is the expensive half, so the + * result is held for as long as the schema object is. A `WeakMap` because the + * key is the schema the loader cache handed us: when the section's cache is + * replaced, the old schemas and their validators go together. + * + * `null` marks a schema Ajv refused outright, so a bad one is diagnosed once + * rather than on every keystroke. + */ +const compiled = new WeakMap(); + +function validatorFor(schema: object): ValidateFunction | null { + const held = compiled.get(schema); + if (held !== undefined) return held; + + let built: ValidateFunction | null = null; + try { + built = ajv.compile(normalize(schema) as object); + } catch (error) { + // A schema this cannot read is not the user's problem to solve, and + // certainly not one to report against their body. Lint nothing instead. + console.warn('schema could not be compiled, skipping validation', error); } + compiled.set(schema, built); + return built; +} + +/** + * `$.items[0].name` — the shape the pane has always shown. + * + * Ajv reports JSON Pointer (`/items/0/name`), which is correct and not what + * anyone reading a JSON body is looking at. + */ +function pointerToPath(pointer: string): string { + if (!pointer) return '$'; + return pointer + .split('/') + .slice(1) + .reduce((path, raw) => { + const key = raw.replace(/~1/g, '/').replace(/~0/g, '~'); + if (/^\d+$/.test(key)) return `${path}[${key}]`; + return /^[A-Za-z_$][\w$]*$/.test(key) ? `${path}.${key}` : `${path}[${JSON.stringify(key)}]`; + }, '$'); +} - for (const key of schema.required ?? []) { - if (!Object.hasOwn(value, key)) errors.push(`${childPath(path, key)} is required.`); +/** What a value actually is, for the half of a type error Ajv leaves out. */ +function valueKind(value: unknown): string { + if (value === null) return 'null'; + if (Array.isArray(value)) return 'array'; + if (Number.isInteger(value)) return 'number'; + return typeof value; +} + +/** Walks a JSON Pointer into the parsed body, to reach the offending value. */ +function valueAtPointer(root: unknown, pointer: string): unknown { + if (!pointer) return root; + let current = root; + for (const raw of pointer.split('/').slice(1)) { + if (current === null || typeof current !== 'object') return undefined; + const key = raw.replace(/~1/g, '/').replace(/~0/g, '~'); + current = (current as Record)[key]; } - for (const [key, item] of Object.entries(value)) { - const property = schema.properties?.[key]; - if (property) { - errors.push(...validate(item, property, childPath(path, key))); - } else if (schema.additionalProperties === false) { - errors.push(`${childPath(path, key)} is not allowed.`); - } else if (isSchema(schema.additionalProperties)) { - errors.push(...validate(item, schema.additionalProperties, childPath(path, key))); + return current; +} + +/** One Ajv error as a sentence, in the voice the pane already used. */ +function describe(error: ErrorObject, root: unknown): string { + const path = pointerToPath(error.instancePath); + + switch (error.keyword) { + case 'required': + return `${pointerToPath(`${error.instancePath}/${error.params.missingProperty}`)} is required.`; + case 'additionalProperties': + return `${pointerToPath(`${error.instancePath}/${error.params.additionalProperty}`)} is not allowed.`; + case 'type': { + const expected = Array.isArray(error.params.type) + ? error.params.type.join(' or ') + : error.params.type; + // Ajv names what was wanted; naming what arrived is the other half of + // what makes the message actionable without going to look. + return `${path} must be ${expected}, not ${valueKind(valueAtPointer(root, error.instancePath))}.`; } + case 'enum': + return `${path} must be one of: ${(error.params.allowedValues as unknown[]).map((option) => JSON.stringify(option)).join(', ')}.`; + case 'const': + return `${path} must equal ${JSON.stringify(error.params.allowedValue)}.`; + case 'anyOf': + return `${path} does not match any allowed schema.`; + case 'oneOf': + return `${path} must match exactly one allowed schema.`; + default: + return `${path} ${error.message ?? 'is invalid'}.`; } - return errors; } /** @@ -106,14 +199,40 @@ function validate(value: unknown, schema: unknown, path: string): string[] { * would only flicker errors while someone is in the middle of typing. */ export function validateJsonBody(schema: unknown | null, text: string): string[] { - if (!schema || !text.trim()) return []; + if (!schema || typeof schema !== 'object' || !text.trim()) return []; // Same ceiling as `JSON_TOOLING_LIMIT` in api.ts: a synchronous JSON.parse // of a multi-megabyte body on a keystroke is a frozen window, and the // errors it would produce are not readable at that size anyway. if (text.length > 1.5 * 1024 * 1024) return []; + + let instance: unknown; try { - return validate(JSON.parse(text), schema, '$'); + instance = JSON.parse(text); } catch { return []; } + + const validate = validatorFor(schema); + if (!validate || validate(instance)) return []; + + // Ajv reports a failed branch *and* the composition above it. The branch + // errors are about a schema the value was never going to match, so they + // read as contradictions; the composition error is the one worth showing. + const errors = validate.errors ?? []; + const composed = new Set( + errors + .filter((error) => error.keyword === 'anyOf' || error.keyword === 'oneOf') + .map((error) => error.instancePath) + ); + + const seen = new Set(); + return errors + .filter( + (error) => + !composed.has(error.instancePath) || + error.keyword === 'anyOf' || + error.keyword === 'oneOf' + ) + .map((error) => describe(error, instance)) + .filter((message) => !seen.has(message) && seen.add(message)); } diff --git a/tests/e2e/json-schema.spec.ts b/tests/e2e/json-schema.spec.ts index 0757f48..11aecd7 100644 --- a/tests/e2e/json-schema.spec.ts +++ b/tests/e2e/json-schema.spec.ts @@ -73,6 +73,91 @@ test.describe('validateJsonBody', () => { ).toEqual(['$["content-type"] must be number, not boolean.']); }); + /** + * Pinned because it surprises everyone, including me: `additionalProperties` + * is scoped to the `properties` beside it and deliberately does not see what + * an `allOf` branch introduces. So this really is two disallowed fields, and + * a spec written this way rejects its own documents. Asserted so nobody + * later "fixes" it into a leniency the standard does not have. + */ + test('additionalProperties does not see an allOf branch, per the spec', () => { + const schema = { + allOf: [ + { type: 'object', properties: { id: { type: 'string' } } }, + { type: 'object', properties: { name: { type: 'string' } } } + ], + type: 'object', + additionalProperties: false + }; + expect(validateJsonBody(schema, '{ "id": "a", "name": "b" }')).toEqual([ + '$.id is not allowed.', + '$.name is not allowed.' + ]); + }); + + /** `$ref` inside the document, which the walker ignored outright. */ + test('a local $ref is followed', () => { + const schema = { + type: 'object', + properties: { child: { $ref: '#/$defs/leaf' } }, + $defs: { leaf: { type: 'number' } } + }; + expect(validateJsonBody(schema, '{ "child": "no" }')).toEqual([ + '$.child must be number, not string.' + ]); + }); + + /** Keywords the subset never covered at all. */ + test('constraints beyond the old subset are enforced', () => { + expect(validateJsonBody({ type: 'integer', minimum: 1 }, '0')).toEqual([ + '$ must be >= 1.' + ]); + expect(validateJsonBody({ type: 'array', items: { type: 'number' }, uniqueItems: true }, '[1, 1]')).not.toEqual([]); + expect(validateJsonBody({ type: 'string', pattern: '^a' }, '"b"')).not.toEqual([]); + }); + + /** + * A real 3.1 document reaches us with `"type": "undefined"` 310 times. Ajv + * throws on a type that does not exist, which would cost the schema all of + * its linting rather than just that field's. + */ + test('an invented type does not disable the rest of the schema', () => { + const schema = { + type: 'object', + required: ['id'], + properties: { ignored: { type: 'undefined' }, id: { type: 'string' } } + }; + expect(validateJsonBody(schema, '{ "ignored": 1, "id": 2 }')).toEqual([ + '$.id must be string, not number.' + ]); + expect(validateJsonBody(schema, '{ "ignored": "anything", "id": "ok" }')).toEqual([]); + }); + + /** OpenAPI 3.1's literal union, which is what `const` is for. */ + test('a choice of consts reports the composition, not each branch', () => { + const schema = { + anyOf: [ + { type: 'string', const: 'once' }, + { type: 'string', const: 'always' } + ] + }; + expect(validateJsonBody(schema, '"once"')).toEqual([]); + expect(validateJsonBody(schema, '"twice"')).toEqual([ + '$ does not match any allowed schema.' + ]); + }); + + /** A property literally called "type" is a name, not a keyword. */ + test('a property named type is not read as one', () => { + const schema = { + type: 'object', + properties: { type: { type: 'string' } } + }; + expect(validateJsonBody(schema, '{ "type": 1 }')).toEqual([ + '$.type must be string, not number.' + ]); + }); + test('invalid or empty JSON is not a schema error', () => { expect(validateJsonBody({ type: 'object' }, '')).toEqual([]); expect(validateJsonBody({ type: 'object' }, '{')).toEqual([]);