Conversation
musa-cf
marked this pull request as ready for review
September 25, 2026 04:44
When a YAML schema contains a property literally named "$ref" (e.g. the
SCIM Group member $ref field per RFC 7643), addQuotesToRefInString
corrupts the document by matching across newlines and quoting the next
line's key as if it were a $ref value.
Root cause: the regex /(\$ref:\s*)([^"'\\s>]+)/g uses \\s* which
includes \\n, so "$ref:" at the end of a line (a YAML mapping key)
matches into the following line.
Fix: replace \\s* with [ \\t]* so the regex only matches horizontal
whitespace and stays on the same line. When $ref is a property name its
value is a block mapping on the next line, so the regex correctly skips
it.
This bug was latent since the function was introduced but became fatal
in 1.33.6 when a doc.errors.length check was added to parseString,
turning the parse error into a SyntaxError return that propagates as
"{}" in the output file.
musa-cf
force-pushed
the
fix/dollar-ref-property-name
branch
from
September 25, 2026 04:56
240f75f to
454f491
Compare
Owner
|
Hi @musa-cf Thanks for taking the time to create this PR. The PR overall looks in great shape, with a logical improvement and test coverage to showcase the result. I m going to do one more extended investigation, since there might be a case with " or ' that might bypass the regex. |
Author
|
Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a YAML schema contains a property literally named
$ref(e.g. the SCIM Group member$reffield per RFC 7643),openapi-formatsilently produces{}as output (exit 0, no error on stderr).This is a data-loss bug: the entire OpenAPI document is replaced with an empty object.
Reproducer
openapi-format input.yaml --output output.yaml cat output.yaml # => {}Root cause
addQuotesToRefInStringuses the regex/(\$ref:\s*)([^"'\s>]+)/g. The\s*includes\n, so when$ref:is a YAML mapping key (property name) with its value on the next line, the regex reaches across the newline and wraps the following line's key in quotes:This produces invalid YAML. In >=1.33.6, the new
doc.errors.length > 0check inparseStringreturns aSyntaxErrorobject instead of the parsed document. The caller treats this error object as the formatted output, which serializes to{}.The bug was latent since
addQuotesToRefInStringwas introduced but became fatal in 1.33.6 when the error check was added.Fix
Replace
\s*with[ \t]*in the regex so it only matches horizontal whitespace and stays on the same line. When$refis a property name, its value is a block mapping on the next line, so the regex correctly skips it. When$refis a JSON Reference, the value is on the same line and still gets quoted as intended.Tests added
test/yaml-dollar-ref-property-name/): full input/output fixture with a SCIM-style schema containing a$refproperty nametest/util-file.test.js):addQuotesToRefInStringshould not modify$refused as a YAML property nametest/util-file.test.js):parseStringshould parse YAML with$refas a property name without errorAll 509 existing tests continue to pass.