fix: reject lone surrogates on both the parse and generate sides - #261
Draft
DecimalTurn wants to merge 3 commits into
Draft
fix: reject lone surrogates on both the parse and generate sides#261DecimalTurn wants to merge 3 commits into
DecimalTurn wants to merge 3 commits into
Conversation
Surrogate code points are not Unicode scalar values, so they have no valid UTF-8 encoding and cannot appear in a TOML document. Two gaps: Parsing: only the 4-digit \uXXXX escape was checked. The 8-digit \UXXXXXXXX form let a surrogate through, because -- unlike an out-of-range code point -- String.fromCodePoint accepts one and simply yields an unpaired code unit. So \uD800 was rejected while \U0000D800 was accepted. Stringify/patch: an unpaired surrogate in a JS string was emitted verbatim, producing output that is not valid UTF-8. Now rejected at the two generation choke points both paths funnel through: generateString for values and generateKey for keys. Surrogate PAIRS are untouched -- an astral character is legitimately stored as a pair in a UTF-16 JS string, and those still round-trip. The check is hand-rolled rather than using String.prototype.isWellFormed(), which needs Node 20 while this package supports Node 16. Un-skips the existing stringify test this was tracked under.
The check runs on every string value and key, so the clean-string cost is what matters. Measured on the shipped function: size scan only isWellFormed speedup 40 B 117.9 ns 16.7 ns 7x 200 B 474.6 ns 15.6 ns 30x 4 KB 9,574 ns 15.5 ns 618x 64 KB 148,730 ns 15.4 ns 9,658x The flat timing is the point: V8 stores ASCII/Latin-1 as one-byte strings, which cannot hold a surrogate by construction, so isWellFormed answers in O(1) without scanning at all. Genuine two-byte strings still scan, but natively, ~5x faster than the JS loop. A regex fast path was also measured and is slower than both. End to end this takes ~7% off a stringify of a 200-key document. isWellFormed is Node 20+ and this package supports Node 16, so it is feature-detected once at module load and the scan remains the fallback. The scan is still needed either way to locate the offending unit for the error message, but now only runs on the failing path. Adds a test that deletes the native method and asserts both paths produce identical results, since CI on modern Node would never exercise the fallback otherwise.
There was a problem hiding this comment.
Pull request overview
This PR closes Unicode correctness gaps by rejecting surrogate code points both when parsing TOML unicode escapes and when generating TOML from JS strings (stringify/patch), preventing emission of documents that cannot be valid UTF-8.
Changes:
- Parsing: reject surrogate code points in
\UXXXXXXXXescapes (previously only\uXXXXwas checked). - Generation: add a shared
assertNoLoneSurrogateguard and enforce it at the two generation choke points for values and keys. - Tests/docs: add focused surrogate validation tests and unskip/extend existing coverage; document the fix in the changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils.ts | Adds assertNoLoneSurrogate (with optional native fast-path) to detect unpaired surrogates in JS strings. |
| src/parse-string.ts | Rejects surrogate code points in \UXXXXXXXX escapes during basic-string unescaping. |
| src/generate.ts | Enforces surrogate rejection when generating string values and key segments. |
| src/tests/surrogate-validation.test.ts | New tests covering surrogate pairs, lone surrogates, and fallback behavior without isWellFormed. |
| src/tests/patch.test.ts | Unskips/extends stringify+patch surrogate handling tests. |
| src/tests/parse-string.test.ts | Adds coverage ensuring both \u and \U surrogate escapes are rejected. |
| CHANGELOG.md | Documents the parsing and generation fixes under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { IS_BARE_KEY } from './tokenizer'; | ||
| import { escapeStringContent } from './escape-preference'; | ||
| import {isBasicString, isMultilineBasicString, isLiteralString, isMultilineLiteralString, temporalToTomlString} from './utils'; | ||
| import {isBasicString, isMultilineBasicString, isLiteralString, isMultilineLiteralString, temporalToTomlString, assertNoLoneSurrogate} from './utils'; |
Comment on lines
+141
to
+145
| return value.map(part => { | ||
| // Keys are encoded too, so a lone surrogate is just as invalid here as in a value. | ||
| assertNoLoneSurrogate(part, `Key "${part}"`); | ||
| return IS_BARE_KEY.test(part) ? part : quoteTomlString(part); | ||
| }).join('.'); |
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.
Surrogate code points are not Unicode scalar values, so they have no valid
UTF-8 encoding and cannot appear in a TOML document. Two gaps:
Parsing: only the 4-digit \uXXXX escape was checked. The 8-digit
\UXXXXXXXX form let a surrogate through, because -- unlike an out-of-range
code point -- String.fromCodePoint accepts one and simply yields an
unpaired code unit. So \uD800 was rejected while \U0000D800 was accepted.
Stringify/patch: an unpaired surrogate in a JS string was emitted verbatim,
producing output that is not valid UTF-8. Now rejected at the two
generation choke points both paths funnel through: generateString for
values and generateKey for keys.
Un-skips the existing stringify test this was tracked under.