fix(json): reject unpaired \uXXXX surrogate escapes in strings

[bobzhang]

Fixes #4062

The bug

The parser decodes every \uXXXX escape by writing the hex value into the result unchecked (buf.write_char(c.unsafe_to_char()) in lex_string_slow), so "\uD800" manufactures an ill-formed lone-surrogate string out of perfectly valid ASCII JSON input — user code that never touches unsafe APIs ends up holding an ill-formed String, violating the well-formedness invariant ("we should maintain String unicode safe, so unsafe_to_char is indeed unsafe").

Spec position

Per RFC 8259 the \uXXXX escape syntax is grammatically valid for any hex value, but §8.2 explicitly flags unpaired surrogate escapes as unpredictable-behavior territory; RFC 7493 (I-JSON) forbids them outright. Ecosystem: JS JSON.parse and Python accept them (producing ill-formed strings), Go substitutes U+FFFD, serde_json (Rust) rejects — we align with serde_json.

The fix

An escaped leading surrogate (\uD800–\uDBFF) must be immediately followed by an escaped trailing surrogate (\uDC00–\uDFFF); the pair is combined into one Unicode scalar value. A bare trailing-surrogate escape, an unpaired leading-surrogate escape, and mixed escaped/raw halves raise the documented ParseError (InvalidChar). Valid escaped pairs (\uD83D\uDE00 → 😀), any hex digit case, and all non-surrogate escapes parse exactly as before.

⚠️ Behavior change

"\uD800" (and every other unpaired surrogate escape) previously parsed successfully and now raises a parse error. No test or snapshot in the repository relied on the old acceptance.

Scope and relation to #4056

This PR covers only the escaped spellings and is based on main, independent of #4056 (the diff regions in lex_string.mbt do not textually overlap: #4056 touches the fast path, flush, and the raw-character branch; this PR touches only the \u escape branch). Raw (unescaped) lone surrogates — including the process abort — are #4049, fixed in #4056.

Tests

Deterministic regressions in json/lex_string_test.mbt: every unpaired spelling (leading alone, bare trailing, double leading, reversed escaped pair, escaped-leading + raw-trailing), the exact ParseError shape, and still-accepted valid pairs.

Verified standalone on this branch: moon check clean; json suite green on wasm-gc, js, native (201/201 each); no .mbti changes. Found by the adversarial QuickCheck suite in #4045.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR fixes a JSON string-escape decoding bug where \uXXXX escapes in the surrogate range could previously construct ill-formed (lone-surrogate) String values, by validating and combining escaped surrogate pairs and rejecting unpaired surrogate escapes with the documented ParseError.

Changes:

• Update ParseContext::lex_string_slow to (1) require \uD800..\uDBFF leading surrogates to be immediately followed by a \uDC00..\uDFFF trailing surrogate escape, and (2) reject bare trailing-surrogate escapes.
• Combine valid escaped surrogate pairs into a single Unicode scalar value before writing into the output buffer.
• Add regression tests covering unpaired surrogate spellings, expected error shape, and still-accepted valid escaped pairs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
json/lex_string.mbt	Adds surrogate-pair validation/combination logic to \uXXXX handling in the slow string lexer path to prevent constructing ill-formed strings.
json/lex_string_test.mbt	Adds deterministic regression tests for rejecting unpaired surrogate escapes and accepting valid escaped pairs.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[coveralls]

Coverage Report for CI Build 6103

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.06%) to 90.73%

Details

• Coverage increased (+0.06%) from the base build.
• Patch coverage: 18 of 18 lines across 1 file are fully covered (100%).
• 1 coverage regression across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
bigint/bigint_nonjs.mbt	1	98.94%

---

Coverage Stats

Relevant Lines:	17702
Covered Lines:	16061
Line Coverage:	90.73%
Coverage Strength:	323746.34 hits per line

---

💛 - Coveralls

[bobzhang]

Codex CLI review

Run with codex exec (gpt-5.6-sol, reasoning effort ultra — maximum depth with task delegation), against the branch checked out locally rather than the diff text, and asked for both an adversarial correctness pass and an explicit position on the compatibility break. Posted verbatim.

---

Thanks @bobzhang. My bottom line for reviewers: the surrogate-pair decoding, scalar reconstruction, buffer writes, and cursor bookkeeping are correct, and all four targets pass. I agree that strict rejection is a reasonable default because returning an ill-formed String or silently replacing object keys is worse. I would not merge this as-is, though: this is a user-visible compatibility break that is absent from the public contract/changelog, and the new InvalidChar diagnostics can report a valid hex digit—or even the wrong UTF-16 position—as the offending character.

Correctness

The new branch handles the requested cases as follows:

Input condition	Outcome
Physical EOF after "\uD800	InvalidEof
Closing quote immediately after "\uD800"	InvalidChar on the quote at column 7
"\uD800x"	InvalidChar on x
"\uD800\n"	InvalidChar on n, since the second escape is not \u
Short second escape	InvalidEof at physical EOF; otherwise InvalidChar on the closing quote/invalid hex digit
Bare "\uDC00"	Rejected, currently as InvalidChar(... column: 6, '0')
"\uD800\uD800"	Rejected after decoding the second leading surrogate
"\uDE00\uD83D"	Rejected at the first, bare trailing surrogate
Escaped leading + raw trailing surrogate	Clean ParseError, rather than the previous abort
Upper/lowercase hex	Correctly accepted through the shared A–F/a–f branches

The reconstruction is algebraically correct:

(hi << 10) + lo - 0x35fdc00
= 0x10000 + ((hi - 0xD800) << 10) + (lo - 0xDC00)

Thus D800/DC00 maps to U+10000 and DBFF/DFFF maps to U+10FFFF. StringBuilder::write_char correctly emits the corresponding UTF-16 pair on non-JS targets, while JS uses the character-to-string intrinsic.

The fast/slow interaction is also sound. Every escaped spelling contains a backslash, so the fast path unconditionally falls back before materializing the string. After a valid pair, both six-code-unit escapes have been consumed and start = ctx.offset resumes at precisely the following source unit. The suffix/next-escape test exercises this successfully. Raw lone surrogates can still traverse the fast path in this branch, but that is the separately tracked #4049/#4056 issue, not an escaped-surrogate bypass.

All textual entry points converge here: values use lex_string, as do both first and subsequent object keys; valid delegates to parse. FromJson consumes an already-built Json, and ToJson/Json::string may preserve a pre-existing unsafely constructed string but do not decode JSON text.

One real error-offset defect remains: the new mismatch arms at lines 74–82 call read_char() and then use shift=-1. A non-BMP scalar consumes two UTF-16 units, so "\uD800😀" reports column 8 and U+FFFD instead of the emoji at column 7. Bind the returned character and use shift=-ch.utf16_len(), as lex_main.mbt already does.

Validation passed read-only:

• moon check --deny-warn --target all
• JSON tests: 201/201 on wasm, wasm-gc, js, and native
• Targeted lexer tests: 6/6 on native

Against HEAD^, all eight rejection assertions fail as intended: seven inputs are accepted, while escaped-high + raw-low aborts in the old slow-path flush. The exact expect_parse_error check also fails because the parent parses successfully. The four positive compatibility assertions already pass on the parent, appropriately—they are preservation checks, not regressions.

Should this be rejected at all?

Yes, I think strict rejection is the right default, but it must be presented as MoonBit’s policy rather than as required JSON behavior.

RFC 8259 §8.2 explicitly says its ABNF admits unpaired surrogate escapes, while §9 both requires grammar-conforming input to be accepted and permits implementations to limit string character contents. I-JSON forbids surrogates, but it is a separate profile that also forbids noncharacters and duplicate member names; @json.parse is not currently advertised as an I-JSON parser.

The compatibility cost is real: ECMAScript’s QuoteJSONString deliberately emits lone surrogates as \uXXXX, so valid output from JavaScript can become unparseable. Nevertheless:

• Preserving the old value lets a safe parser manufacture a Char through an operation documented as undefined for surrogate values.
• Unconditional U+FFFD replacement is lossy and can collapse distinct object keys such as \uD800 and \uD801.
• A checked ParseError is recoverable and makes the loss explicit.

That makes fail-fast rejection preferable as the strict default. MoonBit already follows this pattern for UTF-8 with strict @utf8.decode and separate decode_lossy; an explicit JSON replacement mode would be a useful interoperability counterpart. I would not retain an exact-code-unit mode unless MoonBit formally supports WTF-16 strings and audits APIs that currently assume well-formed UTF-16.

Findings

1. [blocking] Document the new accepted-language contract. parse still only says “Parse a JSON input,” valid does not define validity, the README only gives generic ParseError examples, and the changelog is unchanged. Document the Unicode-scalar restriction, pairing rule, valid semantics, JavaScript/Python incompatibility, and this breaking behavior. Describe it as a deliberate character-content limit, not blanket RFC 8259 or I-JSON conformance.

2. [should] Fix the error semantics and position before pinning them. InvalidChar(..., '0') for "\uDC00" blames the final valid hex digit; invalid second code units do the same. Prefer a dedicated InvalidUnicodeEscape(Position)/surrogate error at the escape start. At minimum, fix the two-unit offset bug described above and choose a documented position convention.

3. [should] Expand the regression matrix. Add U+10000 (\uD800\uDC00), physical EOF and malformed/short hex in the second escape, and representative exact error assertions. One object-key success/failure case would pin the other parser route. The existing U+10FFFF and suffix tests already cover the upper arithmetic boundary and resume index.

4. [nice-to-have] Split the monolithic regression test. An early assertion currently masks later cases, including the old escaped-high/raw-low process abort.

5. [nice-to-have] Consider an explicit lossy interoperability mode. A parse_lossy or surrogate_policy=Reject|Replace API would let callers intentionally consume JavaScript-produced payloads while keeping strict parsing safe and lossless-by-default.

VERDICT: REQUEST CHANGES

---

I reproduced the error-offset defect from Finding 2 independently before posting, since it is the one concrete code bug here:

"\uD800😀"  ->  Invalid character '�' at line 1, column 8
"\uD800x"   ->  Invalid character 'x' at line 1, column 7
"\uD800中"   ->  Invalid character '中' at line 1, column 7

The ASCII and BMP cases report the real character at the right column; the non-BMP one reports a broken half at column 8. lex_main.mbt:75 already does let shift = -c.utf16_len() for exactly this reason, while the new arms in lex_string.mbt discard the character and hardcode shift=-1. Binding it and using -ch.utf16_len() fixes both the column and the reported character.

[bobzhang]

Codex CLI re-review

Second pass with codex exec (gpt-5.6-sol, reasoning effort ultra), after cea6d3a0 addressed the first round. REQUEST CHANGES → APPROVE WITH NITS. Posted verbatim.

---

Thanks for the thorough follow-up. I re-reviewed both commits and found no remaining merge blocker.

1. Finding 1 — resolved. parse, valid, the package README, and the changelog now document the accepted-language change and interoperability cost. The standards framing is accurate: RFC 8259 §9 permits limits on string character content, while §8.2 explicitly notes that the grammar allows unpaired surrogates. Calling this something narrower than I-JSON is also correct; RFC 7493 additionally excludes noncharacters and duplicate names.

   Documentation nits:

   • “Each \uXXXX escape has to denote a Unicode scalar value” is technically imprecise because each half of an accepted surrogate pair is not independently a scalar. Say that an escape must denote a scalar directly or participate in a correctly ordered pair.
   • parse currently promises that an escape unable to pair always raises InvalidChar at its backslash. Physical EOF remains InvalidEof, while malformed/short second escapes retain their normal hex/EOF diagnostics. The documentation should qualify that promise.
   • The interoperability summary is right for ordinary serde_json String/Value parsing and Go’s encoding/json v1, but serde_json’s byte mode accepts WTF-8 and Go’s experimental v2 parser is stricter. Qualifying those names would be more exact.

2. Finding 2 — resolved in the implementation. At escape_start, ctx.offset - 2 is correct: the matched ASCII backslash and u are exactly one UTF-16 code unit each. shift = escape_start - ctx.offset algebraically recovers that absolute offset regardless of how far scanning advanced, including both read_char() mismatch arms, a non-BMP mismatch, and a completed but non-trailing second escape.

   The two -c.utf16_len() changes are also correct. They affect only pre-existing invalid inputs whose offending character is non-BMP; BMP positions are unchanged. The old result landed inside a surrogate pair, so keeping these localized fixes in this PR is appropriate. They should ideally gain exact tests for an unknown non-BMP escape and a non-BMP character in lex_hex_digits.

   The exact snapshots are not over-pinned because parse now promises the opening-backslash convention. Positions are 1-based by line and 0-based by UTF-16 code-unit column. That follows from offset_to_position, although the code-unit basis should preferably be exposed in the public Position documentation.

3. Finding 3 — resolved, with minor coverage gaps. The requested boundary, EOF, malformed/short second escape, exact-error, adjacent-pair, and object-key cases are present. I ran the exact current test file against exported copies of all three revisions:

   Test block	HEAD	HEAD~1	origin/main
   Leading-surrogate rejection	Pass	Pass	Fail
   Bare trailing-surrogate rejection	Pass	Pass	Fail
   Escaped/raw halves	Pass	Pass	Process abort
   Object keys	Pass	Pass	Fail
   Exact surrogate error	Pass	Fail	Fail
   Valid pairs	Pass	Pass	Pass

   Thus, not all six fail on either baseline. That is expected: the first commit already implements the rejection behavior, and the valid-pair block is preservation coverage that should pass on origin/main. Exact assertions are still missing for the second mismatch arm, completed non-trailing second escape, and terminal-backslash EOF, but the centralized helper makes this non-blocking.

4. Finding 4 — resolved in substance, with two nits. Splitting the test improves isolation. However, the escaped/raw test remains third and aborts the old process, so a regression to the original behavior would still mask the final three blocks; moving it last would complete the isolation. Also, the comment saying the old parser “accepted all of the rejections below” is false because physical EOF and malformed/short escapes were already rejected.

5. Finding 5 — not implemented, but remains non-blocking. With the strict default and incompatibility now documented, a lossy mode is an additive API/design decision that can be considered separately.

Validation passed:

• moon check --deny-warn --target all
• moon test -p moonbitlang/core/json on wasm, wasm-gc, js, and native: 207/207 on each target
• git diff --check

The tracked worktree remained clean.

VERDICT: APPROVE WITH NITS

---

The remaining nits are applied in 701b9eff, since three of them were prose that was simply wrong:

• 1a — "each escape must denote a scalar value" is not true of either half of an accepted pair. Now: must denote a scalar on its own or be one half of a correctly ordered pair. Same wording fixed in the README.
• 1b — the promise that a failed pairing always raises InvalidChar at the backslash overstated it. Running out of input still raises InvalidEof, and a malformed second escape is reported as the hex-digit error it is. Both qualified.
• 1c — narrowed the comparisons: serde_json rejects when parsing into String/Value while its byte mode admits WTF-8, and it is Go's encoding/json that substitutes U+FFFD, its v2 parser being stricter.
• 2 / 3 — added the missing exact assertions: the second mismatch arm, a well-formed second escape that is not a trailing surrogate, a trailing backslash at EOF, plus the two arms whose shift was corrected (an unknown escape naming a non-BMP character, and a non-BMP character among hex digits) — the latter reported at columns 2 and 4 rather than as a broken half one column further on.
• 4 — the preamble's claim that the old parser accepted every rejection below it was false; the EOF and malformed-escape spellings were already errors. Corrected, and the escaped-leading + raw-trailing case moved last so a regression to the old aborting behaviour cannot mask the blocks after it.

Not taken: exposing the code-unit basis in Position's own docs (finding 2's "preferably") — that documents a pre-existing public type shared by every parser error, so it seemed better as its own change than buried in a surrogate fix. Say the word if you'd rather have it here.

Full matrix green after the follow-up: moon check --deny-warn --target all, moon test --target all (7241/7242/7186/7160), moon fmt, moon info.