diff --git a/.changeset/solid-lands-repair.md b/.changeset/solid-lands-repair.md new file mode 100644 index 0000000000..d21972a27b --- /dev/null +++ b/.changeset/solid-lands-repair.md @@ -0,0 +1,6 @@ +--- +"@redocly/openapi-core": patch +--- + +Fixed an issue where JSON Pointers containing special characters (like `%`) were not properly URI-encoded. +When these pointers were used as URI identifiers, they caused validation errors with properties containing percent signs or other special characters. diff --git a/.github/workflows/performance.yaml b/.github/workflows/performance.yaml index c057bb1bd2..d17fd79f01 100644 --- a/.github/workflows/performance.yaml +++ b/.github/workflows/performance.yaml @@ -14,40 +14,7 @@ env: REDOCLY_TELEMETRY: off jobs: - latest-vs-next: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v3 - with: - node-version: 24 - cache: npm - - name: Install Dependencies - run: npm ci - - name: Install External - run: npm i -g hyperfine @redocly/cli@latest - - name: Prepare - run: | - jq '.bin = {"redocly-next":"bin/cli.js"} | .name = "@redocly/cli-next"' packages/cli/package.json > __tmp__.json - mv __tmp__.json packages/cli/package.json - cat packages/cli/package.json - npm run pack:prepare - npm i -g redocly-cli.tgz - - run: redocly-next --version - - run: redocly --version - - name: Run Benchmark - run: hyperfine -i --warmup 3 'redocly lint resources/rebilly.yaml' 'redocly-next lint resources/rebilly.yaml' --export-markdown benchmark_check.md --export-json benchmark_check.json - - - name: Comment PR - if: ${{ github.event.pull_request.head.repo.full_name == github.repository }} - uses: thollander/actions-comment-pull-request@v2 - with: - filePath: benchmark_check.md - comment_tag: latest-vs-next-comparison - historical-versions: - # Run only on release branch (changeset-release/main): - if: github.head_ref == 'changeset-release/main' runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 @@ -59,6 +26,15 @@ jobs: run: npm ci - name: Install External run: npm i -g hyperfine + + - name: Add more versions to test + # Run only on the release branch (changeset-release/main): + if: github.head_ref == 'changeset-release/main' + run: | + cd tests/performance/ + cat package.json | jq ".dependencies = $(cat package.json | jq ._enhancedDependencies)" > package.json + cat package.json + - name: Prepare run: | npm run compile @@ -66,10 +42,11 @@ jobs: cd tests/performance/ npm i npm run make-test + - name: Run Benchmark run: | cd tests/performance/ - npm test # This command is generated and injected into package.json in the previous step. + npm run test # This command is generated and injected into package.json in the previous step. cat benchmark_check.md npm run chart # Creates benchmark_chart.md with the performance bar chart. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 484d3cff9e..ed22b086c3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -286,10 +286,16 @@ For other commands you'd have to do something similar. ### Performance benchmark To run the performance tests locally, you should have `hyperfine` (v1.16.1+) installed on your machine. -Prepare the local build, go to the `tests/performance` folder, clean it up, do the preparations, and run the actual test: +Prepare the local build, go to the `tests/performance` folder, clean it up, do the preparations: ```sh -(npm run compile && npm run pack:prepare && cd tests/performance/ && git clean -dX -f . && git clean -dX -ff . && npm i && npm run make-test && npm test) +(npm run compile && npm run pack:prepare && cd tests/performance/ && git clean -dX -f . && git clean -dX -ff . && rm -rf node_modules && rm -f package-lock.json && npm i && npm run make-test) +``` + +and run the actual test: + +```sh +(cd tests/performance/ && npm run test && cat benchmark_check.md) ``` You might need to adjust the CLI versions that need to be tested in the `tests/performance/package.json` file. diff --git a/packages/core/src/__tests__/ref-utils.test.ts b/packages/core/src/__tests__/ref-utils.test.ts index 1b9a81c928..78450d8abb 100644 --- a/packages/core/src/__tests__/ref-utils.test.ts +++ b/packages/core/src/__tests__/ref-utils.test.ts @@ -1,6 +1,11 @@ import outdent from 'outdent'; import { parseYamlToDocument } from '../../__tests__/utils.js'; -import { parseRef, refBaseName } from '../ref-utils.js'; +import { + escapePointerFragment, + parseRef, + refBaseName, + unescapePointerFragment, +} from '../ref-utils.js'; import { lintDocument } from '../lint.js'; import { createConfig } from '../config/index.js'; import { BaseResolver } from '../resolve.js'; @@ -35,8 +40,8 @@ describe('ref-utils', () => { }); it(`should unescape complex urlencoded paths`, () => { - const referene = 'somefile.yaml#/components/schemas/scope%2Fcomplex~name'; - expect(parseRef(referene)).toMatchInlineSnapshot(` + const reference = 'somefile.yaml#/components/schemas/scope%2Fcomplex~name'; + expect(parseRef(reference)).toMatchInlineSnapshot(` { "pointer": [ "components", @@ -48,6 +53,20 @@ describe('ref-utils', () => { `); }); + it(`should unescape escaped paths`, () => { + const reference = 'somefile.yaml#/components/schemas/scope~1complex~0name with spaces'; + expect(parseRef(reference)).toMatchInlineSnapshot(` + { + "pointer": [ + "components", + "schemas", + "scope/complex~name with spaces", + ], + "uri": "somefile.yaml", + } + `); + }); + it(`should validate definition with urlencoded paths`, async () => { const document = parseYamlToDocument( outdent` @@ -139,4 +158,29 @@ describe('ref-utils', () => { expect(refBaseName('abcdefg')).toStrictEqual('abcdefg'); }); }); + + describe('escapePointerFragment', () => { + it('should escape a simple pointer fragment with ~ and / correctly', () => { + expect(escapePointerFragment('scope/complex~name')).toStrictEqual('scope~1complex~0name'); + }); + + it('should escape a pointer fragment with a number correctly', () => { + expect(escapePointerFragment(123)).toStrictEqual(123); + }); + + it('should not URI-encode other special characters when escaping pointer fragments per https://datatracker.ietf.org/doc/html/rfc6901#section-6', () => { + expect(escapePointerFragment('curly{braces}')).toStrictEqual('curly{braces}'); + expect(escapePointerFragment('plus+')).toStrictEqual('plus+'); + }); + }); + + describe('unescapePointerFragment', () => { + it('should unescape a pointer with a percent sign correctly', () => { + expect(unescapePointerFragment('activity_level_%25')).toStrictEqual('activity_level_%'); + }); + + it('should unescape a pointer correctly', () => { + expect(unescapePointerFragment('scope~1complex~0name')).toStrictEqual('scope/complex~name'); + }); + }); }); diff --git a/packages/core/src/format/codeframes.ts b/packages/core/src/format/codeframes.ts index 0708214436..a2f8c82f40 100644 --- a/packages/core/src/format/codeframes.ts +++ b/packages/core/src/format/codeframes.ts @@ -1,5 +1,5 @@ import * as yamlAst from 'yaml-ast-parser'; -import { unescapePointer } from '../ref-utils.js'; +import { parsePointer } from '../ref-utils.js'; import { colorize, colorOptions } from '../logger.js'; import type { LineColLocationObject, Loc, LocationObject } from '../walk.js'; @@ -14,11 +14,6 @@ type YAMLNode = YAMLMapping | YAMLMap | YAMLAnchorReference | YAMLSequence | YAM const MAX_LINE_LENGTH = 150; const MAX_CODEFRAME_LINES = 3; -// TODO: temporary -function parsePointer(pointer: string) { - return pointer.substr(2).split('/').map(unescapePointer); -} - export function getCodeframe(location: LineColLocationObject, color: boolean) { colorOptions.enabled = color; const { start, end = { line: start.line, col: start.col + 1 }, source } = location; @@ -181,7 +176,8 @@ function positionsToLoc( } export function getAstNodeByPointer(root: YAMLNode, pointer: string, reportOnKey: boolean) { - const pointerSegments = parsePointer(pointer); + const pointerSegments = parsePointer(pointer.substr(2)); + if (root === undefined) { return undefined; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 1e7ba15dea..3033accd55 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -52,10 +52,10 @@ export { export { YamlParseError } from './errors/yaml-parse-error.js'; export { parseYaml, stringifyYaml } from './js-yaml/index.js'; export { - unescapePointer, + unescapePointerFragment, isRef, isAbsoluteUrl, - escapePointer, + escapePointerFragment, type Location, } from './ref-utils.js'; export { detectSpec } from './detect-spec.js'; diff --git a/packages/core/src/ref-utils.ts b/packages/core/src/ref-utils.ts index e49db0ae4c..80956708f0 100644 --- a/packages/core/src/ref-utils.ts +++ b/packages/core/src/ref-utils.ts @@ -26,7 +26,7 @@ export class Location { this.source, joinPointer( this.pointer, - (Array.isArray(components) ? components : [components]).map(escapePointer).join('/') + (Array.isArray(components) ? components : [components]).map(escapePointerFragment).join('/') ) ); } @@ -40,13 +40,19 @@ export class Location { } } -export function unescapePointer(fragment: string): string { - return decodeURIComponent(fragment.replace(/~1/g, '/').replace(/~0/g, '~')); +export function unescapePointerFragment(fragment: string): string { + const unescaped = fragment.replace(/~1/g, '/').replace(/~0/g, '~'); + + try { + return decodeURIComponent(unescaped); + } catch (e) { + return unescaped; + } } -export function escapePointer(fragment: T): T { +export function escapePointerFragment(fragment: T): T { if (typeof fragment === 'number') return fragment; - return (fragment as string).replace(/~/g, '~0').replace(/\//g, '~1') as T; + return fragment.replaceAll('~', '~0').replaceAll('/', '~1') as T; } export function parseRef(ref: string): { uri: string | null; pointer: string[] } { @@ -58,7 +64,7 @@ export function parseRef(ref: string): { uri: string | null; pointer: string[] } } export function parsePointer(pointer: string) { - return pointer.split('/').map(unescapePointer).filter(isTruthy); + return pointer.split('/').map(unescapePointerFragment).filter(isTruthy); } export function pointerBaseName(pointer: string) { diff --git a/packages/core/src/resolve.ts b/packages/core/src/resolve.ts index 051bfe8ca3..b9d1be2adf 100644 --- a/packages/core/src/resolve.ts +++ b/packages/core/src/resolve.ts @@ -3,7 +3,7 @@ import * as path from 'node:path'; import { isRef, joinPointer, - escapePointer, + escapePointerFragment, parseRef, isAbsoluteUrl, isAnchor, @@ -315,7 +315,7 @@ export async function resolveDocument(opts: { continue; } - walk(propValue, propType, joinPointer(nodeAbsoluteRef, escapePointer(propName))); + walk(propValue, propType, joinPointer(nodeAbsoluteRef, escapePointerFragment(propName))); } if (isRef(node)) { @@ -426,7 +426,10 @@ export async function resolveDocument(opts: { break; } else if (target[segment] !== undefined) { target = target[segment]; - resolvedRef.nodePointer = joinPointer(resolvedRef.nodePointer!, escapePointer(segment)); + resolvedRef.nodePointer = joinPointer( + resolvedRef.nodePointer!, + escapePointerFragment(segment) + ); } else if (isRef(target)) { resolvedRef = await followRef(targetDoc, target, pushRef(refStack, target)); targetDoc = resolvedRef.document || targetDoc; @@ -437,7 +440,10 @@ export async function resolveDocument(opts: { } target = resolvedRef.node[segment]; - resolvedRef.nodePointer = joinPointer(resolvedRef.nodePointer!, escapePointer(segment)); + resolvedRef.nodePointer = joinPointer( + resolvedRef.nodePointer!, + escapePointerFragment(segment) + ); } else { target = undefined; break; diff --git a/packages/core/src/rules/ajv.ts b/packages/core/src/rules/ajv.ts index 45dc793539..12ca653f2b 100644 --- a/packages/core/src/rules/ajv.ts +++ b/packages/core/src/rules/ajv.ts @@ -1,6 +1,6 @@ import addFormats from 'ajv-formats'; import Ajv from '@redocly/ajv/dist/2020.js'; -import { escapePointer } from '../ref-utils.js'; +import { escapePointerFragment } from '../ref-utils.js'; import type { Location } from '../ref-utils.js'; import type { ValidateFunction, ErrorObject } from '@redocly/ajv/dist/2020.js'; @@ -26,9 +26,14 @@ function getAjv(resolve: ResolveFn, allowAdditionalProperties: boolean) { validateFormats: true, defaultUnevaluatedProperties: allowAdditionalProperties, loadSchemaSync(base: string, $ref: string, $id: string) { - const resolvedRef = resolve({ $ref }, base.split('#')[0]); + const decodedBase = decodeURI(base.split('#')[0]); + const resolvedRef = resolve({ $ref }, decodedBase); if (!resolvedRef || !resolvedRef.location) return false; - return { $id: resolvedRef.location.source.absoluteRef + '#' + $id, ...resolvedRef.node }; + + return { + $id: encodeURI(resolvedRef.location.source.absoluteRef) + '#' + $id, + ...resolvedRef.node, + }; }, logger: false, }); @@ -44,12 +49,13 @@ function getAjvValidator( allowAdditionalProperties: boolean ): ValidateFunction | undefined { const ajv = getAjv(resolve, allowAdditionalProperties); + const $id = encodeURI(loc.absolutePointer); - if (!ajv.getSchema(loc.absolutePointer)) { - ajv.addSchema({ $id: loc.absolutePointer, ...schema }, loc.absolutePointer); + if (!ajv.getSchema($id)) { + ajv.addSchema({ $id, ...schema }, $id); } - return ajv.getSchema(loc.absolutePointer); + return ajv.getSchema($id); } export function validateJsonSchema( @@ -95,7 +101,7 @@ export function validateJsonSchema( if (error.keyword === 'additionalProperties' || error.keyword === 'unevaluatedProperties') { const property = error.params.additionalProperty || error.params.unevaluatedProperty; message = `${message} \`${property}\``; - error.instancePath += '/' + escapePointer(property); + error.instancePath += '/' + escapePointerFragment(property); } return { diff --git a/tests/e2e/lint/no-invalid-schema-examples-oas3.1-error/openapi.yaml b/tests/e2e/lint/no-invalid-schema-examples-oas3.1-error/openapi.yaml index 777bf0209b..b611619854 100644 --- a/tests/e2e/lint/no-invalid-schema-examples-oas3.1-error/openapi.yaml +++ b/tests/e2e/lint/no-invalid-schema-examples-oas3.1-error/openapi.yaml @@ -39,6 +39,10 @@ paths: examples: - test - 25 + three_%: + type: number + example: wrong + responses: '200': description: My 200 response diff --git a/tests/e2e/lint/no-invalid-schema-examples-oas3.1-error/snapshot.txt b/tests/e2e/lint/no-invalid-schema-examples-oas3.1-error/snapshot.txt index 359f447e74..d94ac02fe8 100644 --- a/tests/e2e/lint/no-invalid-schema-examples-oas3.1-error/snapshot.txt +++ b/tests/e2e/lint/no-invalid-schema-examples-oas3.1-error/snapshot.txt @@ -20,22 +20,38 @@ Error was generated by the no-invalid-schema-examples rule. Example value must conform to the schema: type must be string. -39 | examples: -40 | - test -41 | - 25 - | ^^ -42 | responses: -43 | '200': +39 | examples: +40 | - test +41 | - 25 + | ^^ +42 | three_%: +43 | type: number referenced from openapi.yaml:36:19 at #/paths/~1my_post/post/requestBody/content/application~1json/schema/properties/two Error was generated by the no-invalid-schema-examples rule. +[3] openapi.yaml:44:28 at #/paths/~1my_post/post/requestBody/content/application~1json/schema/properties/three_%/example + +Example value must conform to the schema: type must be number. + +42 | three_%: +43 | type: number +44 | example: wrong + | ^^^^^ +45 | +46 | responses: + +referenced from openapi.yaml:43:19 at #/paths/~1my_post/post/requestBody/content/application~1json/schema/properties/three_% + +Error was generated by the no-invalid-schema-examples rule. + + validating openapi.yaml using lint rules for api 'main'... openapi.yaml: validated in ms -❌ Validation failed with 2 errors. +❌ Validation failed with 3 errors. run `redocly lint --generate-ignore-file` to add all problems to the ignore file. diff --git a/tests/performance/make-test-command.sh b/tests/performance/make-test-command.sh index eaa7e5122e..2d41240c80 100644 --- a/tests/performance/make-test-command.sh +++ b/tests/performance/make-test-command.sh @@ -7,7 +7,7 @@ git clone https://github.com/Rebilly/api-definitions.git cd api-definitions && npm install && cd .. # Store the command into a text file: -echo REDOCLY_SUPPRESS_UPDATE_NOTICE=true hyperfine --warmup 3 $(cat package.json | jq '.dependencies' | jq 'keys' | jq 'map("'\''node node_modules/" + . + "/bin/cli.js lint --config=api-definitions/redocly.yaml --generate-ignore-file'\''")' | jq 'join(" ")' | xargs) --export-markdown benchmark_check.md --export-json benchmark_check.json > test-command.txt +echo REDOCLY_SUPPRESS_UPDATE_NOTICE=true hyperfine --warmup 1 $(cat package.json | jq '.dependencies' | jq 'keys' | jq 'map("'\''node node_modules/" + . + "/bin/cli.js lint core@public --config=api-definitions/redocly.yaml --generate-ignore-file'\''")' | jq 'join(" ")' | xargs) --export-markdown benchmark_check.md --export-json benchmark_check.json > test-command.txt # Put the command in the test section of the package.json: cat package.json | jq ".scripts.test = \"$(cat test-command.txt)\"" > package.json diff --git a/tests/performance/package.json b/tests/performance/package.json index 1c5f37fb6a..b47df40d9c 100644 --- a/tests/performance/package.json +++ b/tests/performance/package.json @@ -5,14 +5,25 @@ "private": true, "scripts": { "chart": "node chart.js > benchmark_chart.md", - "make-test": "bash make-test-command.sh" + "make-test": "bash make-test-command.sh", + "test": "Placeholder..." }, - "dependencies": { + "_enhancedDependencies": { "cli-2.0.0": "npm:@redocly/cli@2.0.0", - "cli-2.3.1": "npm:@redocly/cli@2.3.1", - "cli-2.8.0": "npm:@redocly/cli@2.8.0", - "cli-2.9.0": "npm:@redocly/cli@2.9.0", + "cli-2.03.1": "npm:@redocly/cli@2.3.1", + "cli-2.08.0": "npm:@redocly/cli@2.8.0", + "cli-2.09.0": "npm:@redocly/cli@2.9.0", "cli-2.11.1": "npm:@redocly/cli@2.11.1", + "cli-2.12.0": "npm:@redocly/cli@2.12.0", + "cli-2.12.2": "npm:@redocly/cli@2.12.2", + "cli-2.12.5": "npm:@redocly/cli@2.12.5", + "cli-2.12.6": "npm:@redocly/cli@2.12.6", + "cli-2.13.0": "npm:@redocly/cli@2.13.0", + "cli-latest": "npm:@redocly/cli@latest", + "cli-next": "file:../../redocly-cli.tgz" + }, + "dependencies": { + "cli-latest": "npm:@redocly/cli@latest", "cli-next": "file:../../redocly-cli.tgz" } }