diff --git a/.changeset/humble-tunas-munch.md b/.changeset/humble-tunas-munch.md new file mode 100644 index 000000000..0c5430279 --- /dev/null +++ b/.changeset/humble-tunas-munch.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1807 +--- +reconstructFrontmatter now emits valid YAML for scalars and block-array items that were previously serialized unescaped. Values carrying a YAML indicator plus a literal quote/backslash, embedded control characters, the empty string, a leading YAML indicator, or leading/trailing whitespace are now routed through a properly escaped double-quoted form, so frontmatter round-trips through strict parsers (js-yaml, PyYAML) instead of corrupting the block on the next state sync. diff --git a/src/frontmatter.cts b/src/frontmatter.cts index 5d4028736..d9c06782b 100644 --- a/src/frontmatter.cts +++ b/src/frontmatter.cts @@ -133,6 +133,47 @@ function extractFrontmatter(content: string): Frontmatter { return frontmatter; } +/** + * Escape a string for emission inside a YAML double-quoted scalar (#1779). + * Backslash must be escaped first so the backslashes added for embedded quotes + * (and control chars) are not themselves doubled. Without this, a value + * carrying an indicator (`:`/`#`) that also contains a literal `"` serializes + * to invalid YAML, e.g. `upstream: "https://x (Tom; "Git. Ship. Done")"`. A + * literal newline/tab/control char inside the quotes likewise breaks (or + * silently alters) the scalar, so those are escaped to their YAML forms too. + */ +function escapeDoubleQuoted(s: string): string { + return s + .replace(/\\/g, '\\\\') + .replace(/"/g, '\\"') + .replace(/\n/g, '\\n') + .replace(/\t/g, '\\t') + .replace(/\r/g, '\\r') + // Remaining C0 controls + DEL → \xHH (a valid YAML double-quoted escape). + .replace(/[\u0000-\u001f\u007f]/g, (c) => `\\x${c.charCodeAt(0).toString(16).padStart(2, '0')}`); +} + +/** + * A plain (unquoted) scalar that would mis-parse or round-trip lossily when + * emitted bare must instead go through the double-quoted + escaped form + * (#1779): the empty string (bare `k:` reloads as null), an embedded `"`/`\` + * or control char, a leading YAML indicator (quote, `&`/`*`/`!` anchor/alias/ + * tag, `|`/`>` block scalar, flow `[]{},`, `#`, reserved `%`/`@`/backtick, or + * `-`/`?`/`:` before a space), or leading/trailing whitespace. This helper is + * the correctness complement of `escapeDoubleQuoted`: it broadens the *trigger* + * for quoting without broadening the lossy object-list handling deferred to + * #1572/#1660. + */ +function scalarNeedsDoubleQuoting(s: string): boolean { + if (s === '') return true; + if (/["\\\u0000-\u001f\u007f]/.test(s)) return true; + // Always-unsafe leading indicators, or leading/trailing whitespace. + if (/^[,[\]{}#&*!|>'"%@`]/.test(s) || /^\s|\s$/.test(s)) return true; + // `-` `?` `:` only start a plain scalar safely when NOT followed by a space. + if (/^[-?:](\s|$)/.test(s)) return true; + return false; +} + function reconstructFrontmatter(obj: Frontmatter): string { const lines: string[] = []; for (const [key, value] of Object.entries(obj)) { @@ -145,7 +186,7 @@ function reconstructFrontmatter(obj: Frontmatter): string { } else { lines.push(`${key}:`); for (const item of value) { - lines.push(` - ${typeof item === 'string' && (item.includes(':') || item.includes('#')) ? `"${item}"` : item}`); + lines.push(` - ${typeof item === 'string' && (item.includes(':') || item.includes('#') || scalarNeedsDoubleQuoting(item)) ? `"${escapeDoubleQuoted(item)}"` : item}`); } } } else if (typeof value === 'object') { @@ -160,7 +201,7 @@ function reconstructFrontmatter(obj: Frontmatter): string { } else { lines.push(` ${subkey}:`); for (const item of subval) { - lines.push(` - ${typeof item === 'string' && (item.includes(':') || item.includes('#')) ? `"${item}"` : item}`); + lines.push(` - ${typeof item === 'string' && (item.includes(':') || item.includes('#') || scalarNeedsDoubleQuoting(item)) ? `"${escapeDoubleQuoted(item)}"` : item}`); } } } else if (typeof subval === 'object') { @@ -184,13 +225,13 @@ function reconstructFrontmatter(obj: Frontmatter): string { } else { // eslint-disable-next-line @typescript-eslint/no-base-to-string const sv = String(subval); - lines.push(` ${subkey}: ${sv.includes(':') || sv.includes('#') ? `"${sv}"` : sv}`); + lines.push(` ${subkey}: ${sv.includes(':') || sv.includes('#') || scalarNeedsDoubleQuoting(sv) ? `"${escapeDoubleQuoted(sv)}"` : sv}`); } } } else { const sv = String(value); - if (sv.includes(':') || sv.includes('#') || sv.startsWith('[') || sv.startsWith('{')) { - lines.push(`${key}: "${sv}"`); + if (sv.includes(':') || sv.includes('#') || sv.startsWith('[') || sv.startsWith('{') || scalarNeedsDoubleQuoting(sv)) { + lines.push(`${key}: "${escapeDoubleQuoted(sv)}"`); } else { lines.push(`${key}: ${sv}`); } diff --git a/tests/frontmatter.property.test.cjs b/tests/frontmatter.property.test.cjs index 11a16be5a..01fafdee0 100644 --- a/tests/frontmatter.property.test.cjs +++ b/tests/frontmatter.property.test.cjs @@ -21,6 +21,7 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); const fc = require('./helpers/fast-check-setup.cjs'); +const yaml = require('js-yaml'); const { extractFrontmatter, @@ -263,3 +264,32 @@ describe('frontmatter: prohibitions parse ↔ splice bijection (#644)', () => { ); }); }); + +// #1779 — reconstructFrontmatter must emit YAML that a STRICT parser accepts and +// that preserves string values. The bijective contract is +// ∀ s: yaml.load(reconstructFrontmatter({ k: s })).k === s +// over the documented safe-input subset. Two classes are out of scope and +// excluded here, not silently passed: +// - lone UTF-16 surrogates (lossy through UTF-8 encoding) — filtered via +// fc.pre(s.isWellFormed()); +// - numeric/boolean/null-looking BARE strings (e.g. "42", "true", "-5") that a +// YAML loader resolves to a non-string type — a separate pre-existing bug +// class (valid YAML, wrong type), so we assert equality only when the value +// loads back AS a string. An escaping defect (invalid YAML) still fails +// loudly because yaml.load() throws. +describe('frontmatter: reconstructFrontmatter strict-YAML property (#1779)', () => { + test('property: every string value serializes to valid YAML and string-round-trips', () => { + fc.assert( + fc.property(fc.string({ maxLength: 200 }), (s) => { + fc.pre(s.isWellFormed()); + // Throws → reconstructFrontmatter emitted invalid YAML → property fails + // (fast-check shrinks + prints the replay seed automatically). + const loaded = yaml.load(reconstructFrontmatter({ k: s })); + if (typeof loaded.k === 'string') { + assert.equal(loaded.k, s, + `value did not round-trip through strict YAML: ${JSON.stringify(s)}`); + } + }) + ); + }); +}); diff --git a/tests/frontmatter.unit.test.cjs b/tests/frontmatter.unit.test.cjs index 14eb0505f..6ada479e6 100644 --- a/tests/frontmatter.unit.test.cjs +++ b/tests/frontmatter.unit.test.cjs @@ -15,6 +15,7 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); +const yaml = require('js-yaml'); const { extractFrontmatter, @@ -1125,6 +1126,81 @@ describe('reconstructFrontmatter: round-trip', () => { }); }); +// #1779 — reconstructFrontmatter must emit valid YAML for scalars and block- +// array items that carry an embedded `"`/`\`, a control char, an empty string, +// a leading YAML indicator, or surrounding whitespace. The project's own lossy +// `extractFrontmatter` tolerates the broken output, which is exactly why these +// assert round-trip through a STRICT parser (js-yaml) instead — the strict path +// fails on the unescaped/bare emission and passes only once every wrap site +// escapes and every unsafe-bare value is routed through the quoted form. +describe('reconstructFrontmatter: strict-YAML round-trip (#1779)', () => { + // Serialize via the production path, then load with a strict YAML parser. + const strictRoundTrip = (obj) => yaml.load(reconstructFrontmatter(obj)); + + test('top-level scalar with indicator + embedded quotes (the reported case)', () => { + const obj = { upstream: 'https://x (Tom; "Git. Ship. Done")' }; + assert.deepEqual(strictRoundTrip(obj), obj); + }); + + test('nested scalar with indicator + embedded quotes', () => { + const obj = { meta: { note: 'see: "the docs"' } }; + assert.deepEqual(strictRoundTrip(obj), obj); + }); + + test('top-level block-array item with indicator + embedded quotes', () => { + // 4 items forces block form (the inline `[a, b]` branch caps at 3). + const obj = { tags: ['a: "1"', 'b: "2"', 'c: "3"', 'd: "4"'] }; + assert.deepEqual(strictRoundTrip(obj), obj); + }); + + test('nested block-array item with indicator + embedded quotes', () => { + const obj = { meta: { tags: ['a: "1"', 'b: "2"', 'c: "3"', 'd: "4"'] } }; + assert.deepEqual(strictRoundTrip(obj), obj); + }); + + test('embedded backslash is escaped (not just quotes)', () => { + const obj = { path: 'a:\\b\\c "x"' }; + assert.deepEqual(strictRoundTrip(obj), obj); + }); + + test('control chars in a wrapped value are escaped (newline, tab, NUL)', () => { + const obj = { note: 'line1: a\nline2\twith\x00nul' }; + assert.deepEqual(strictRoundTrip(obj), obj); + }); + + test('CRLF round-trips (locks the \\r escape for Windows-authored values)', () => { + const obj = { note: 'line1: a\r\nline2' }; + assert.deepEqual(strictRoundTrip(obj), obj); + }); + + test('empty string round-trips as "" (bare `k:` would reload as null)', () => { + assert.deepEqual(strictRoundTrip({ k: '' }), { k: '' }); + }); + + test('embedded/leading quote with NO indicator still round-trips', () => { + assert.deepEqual(strictRoundTrip({ k: '"x' }), { k: '"x' }); + assert.deepEqual(strictRoundTrip({ k: "'leading single" }), { k: "'leading single" }); + assert.deepEqual(strictRoundTrip({ k: 'say "hi" there' }), { k: 'say "hi" there' }); + }); + + test('leading YAML indicators with no `:`/`#` still round-trip', () => { + for (const v of ['>x', '|x', '&anchor', '*alias', '!tag', '[flow', '{map', '%pct', '@reserved', '`tick']) { + assert.deepEqual(strictRoundTrip({ k: v }), { k: v }, `value ${JSON.stringify(v)}`); + } + }); + + test('leading/trailing whitespace is preserved (bare would be trimmed)', () => { + assert.deepEqual(strictRoundTrip({ k: ' leading' }), { k: ' leading' }); + assert.deepEqual(strictRoundTrip({ k: 'trailing ' }), { k: 'trailing ' }); + }); + + test('plain values without indicators are unaffected (no spurious quoting)', () => { + const obj = { name: 'simple-value', label: 'plain' }; + assert.equal(reconstructFrontmatter(obj), 'name: simple-value\nlabel: plain'); + assert.deepEqual(yaml.load(reconstructFrontmatter(obj)), obj); + }); +}); + describe('extractFrontmatter: boundary — dash at start of file', () => { test('--- at byte 0 is treated as frontmatter', () => { const result = extractFrontmatter('---\nkey: val\n---\n');