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

CHANGELOG.md

@@ -16,6 +16,7 @@ changelog should follow [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
 
 #### Changed
 
+- **BREAKING**: `@json.parse` now rejects unpaired `\uXXXX` surrogate escapes in strings, raising `ParseError::InvalidChar` at the backslash that opens the offending escape; an escaped leading surrogate must be followed immediately by an escaped trailing surrogate, and the pair decodes to the character it denotes. Previously such escapes were decoded unchecked and produced a `String` that was not well-formed Unicode. `@json.valid` reports the same documents as invalid. JSON emitted by `JSON.stringify` in JavaScript can contain these escapes, so input that JavaScript and Python accept may now be rejected — as it is by Rust's serde_json
 - `@json.inspect` has been migrated to `json_inspect`
 - `String::sub` and `StringView::sub` now panic on invalid indices instead of raising `CreatingViewError`. The `CreatingViewError` type has been removed.
 

json/README.mbt.md

@@ -51,6 +51,35 @@ test "parse and validate jsons" {
 }
 ```
 
+#### What may appear inside a string
+
+Every string in a parsed document is well-formed Unicode, so each `\uXXXX`
+escape must denote a Unicode scalar value on its own or be one half of a
+correctly ordered surrogate pair. An escaped leading surrogate must be
+followed immediately by an escaped trailing surrogate, and the pair decodes
+to the single character it stands for; an escape that cannot pair up is a
+parse error, reported at the backslash that opens it.
+
+```mbt check
+///|
+test "surrogate escapes" {
+  // A surrogate pair decodes to the one character it denotes.
+  assert_true(@json.parse("\"\\uD83D\\uDE00\"") == Json::string("😀"))
+  // An escape that cannot pair up is rejected rather than producing a
+  // string that is not well-formed Unicode.
+  assert_false(@json.valid("\"\\uD800\""))
+}
+```
+
+RFC 8259 §9 leaves what a string may contain to the implementation, and this
+is where MoonBit draws that line: a `String` is required to be well-formed,
+so the alternatives would be to hand one back that is not, or to substitute
+U+FFFD and lose the difference between two distinct keys. Note that
+`JSON.stringify` in JavaScript does emit lone surrogates this way, so a
+document JavaScript and Python accept can be rejected here — as it is by
+Rust's serde_json when parsing into `String` or `Value`; Go's
+`encoding/json` substitutes U+FFFD instead.
+
 ### Object Navigation
 
 ```mbt check

json/lex_string.mbt

@@ -62,10 +62,44 @@ fn ParseContext::lex_string_slow(ctx : ParseContext) -> String raise ParseError
           Some('\\') => buf.write_char('\\')
           Some('/') => buf.write_char('/')
           Some('u') => {
+            // The backslash that opened this escape. `ctx.offset` is just
+            // past the `u`, and `\` and `u` are one code unit each.
+            let escape_start = ctx.offset - 2
             let c = ctx.lex_hex_digits(4)
-            buf.write_char(c.unsafe_to_char())
+            if c is (0xD800..=0xDBFF) {
+              // A leading-surrogate escape is only meaningful as the first
+              // half of an escaped surrogate pair; combine it with the
+              // immediately following trailing-surrogate escape into one
+              // Unicode scalar value. Anything else would manufacture a
+              // string containing an unpaired surrogate, which MoonBit
+              // strings disallow (RFC 8259 calls the behavior for such
+              // escapes unpredictable; I-JSON forbids them).
+              match ctx.read_char() {
+                Some('\\') => ()
+                Some(_) => ctx.unpaired_surrogate(escape_start)
+                None => raise InvalidEof
+              }
+              match ctx.read_char() {
+                Some('u') => ()
+                Some(_) => ctx.unpaired_surrogate(escape_start)
+                None => raise InvalidEof
+              }
+              let c2 = ctx.lex_hex_digits(4)
+              if c2 is (0xDC00..=0xDFFF) {
+                let combined = (c << 10) + c2 - 0x35fdc00
+                buf.write_char(combined.unsafe_to_char())
+              } else {
+                ctx.unpaired_surrogate(escape_start)
+              }
+            } else if c is (0xDC00..=0xDFFF) {
+              // A bare trailing-surrogate escape can never form a scalar
+              // value.
+              ctx.unpaired_surrogate(escape_start)
+            } else {
+              buf.write_char(c.unsafe_to_char())
+            }
           }
-          Some(_) => ctx.invalid_char(shift=-1)
+          Some(c) => ctx.invalid_char(shift=-c.utf16_len())
           None => raise InvalidEof
         }
         start = ctx.offset
@@ -82,6 +116,22 @@ fn ParseContext::lex_string_slow(ctx : ParseContext) -> String raise ParseError
   buf.to_string()
 }
 
+///|
+/// Reports the `\uXXXX` escape that begins at `escape_start` as invalid.
+///
+/// Every rejection of an unpaired surrogate points here — at the backslash
+/// opening the offending escape — rather than at whichever character the
+/// scan happened to stop on. Blaming the stopping point would name a
+/// perfectly valid hex digit for `"\uDC00"`, and for a leading surrogate
+/// followed by a non-BMP character it would name a position inside that
+/// character.
+fn[T] ParseContext::unpaired_surrogate(
+  ctx : ParseContext,
+  escape_start : Int,
+) -> T raise ParseError {
+  ctx.invalid_char(shift=escape_start - ctx.offset)
+}
+
 ///|
 fn ParseContext::lex_hex_digits(
   ctx : ParseContext,
@@ -92,7 +142,9 @@ fn ParseContext::lex_hex_digits(
       Some('0'..='9' as c) => c.to_int() - '0'
       Some('A'..='F' as c) => c.to_int() - 'A' + 10
       Some('a'..='f' as c) => c.to_int() - 'a' + 10
-      Some(_) => ctx.invalid_char(shift=-1)
+      // `-1` would land inside the character when it is not in the BMP,
+      // reporting a broken half at the wrong column.
+      Some(c) => ctx.invalid_char(shift=-c.utf16_len())
       None => raise InvalidEof
     }
     continue (r << 4) | d

json/lex_string_test.mbt

@@ -89,3 +89,153 @@ test "lex_hex_digits accepts all hex digit ranges" {
     ),
   )
 }
+
+// Regression for #4062. The parser used to decode every `\uXXXX` escape by
+// writing the hex value into the result unchecked, manufacturing an
+// ill-formed lone-surrogate string out of valid ASCII JSON input. MoonBit
+// strings stay well-formed Unicode, so an escaped leading surrogate must be
+// followed immediately by an escaped trailing surrogate — the pair decodes
+// to the one character it denotes — and every unpaired spelling is a
+// `ParseError`.
+//
+// The cases are split by shape so that one failing assertion cannot mask the
+// rest. The old parser accepted the *unpaired-surrogate* spellings below —
+// it already rejected the ones that also run out of input or misspell the
+// second escape — and aborted the process outright on the escaped-leading +
+// raw-trailing one, which is why that case is kept last.
+
+///|
+test "unpaired leading-surrogate escape is rejected" {
+  // A closing quote after it, and nothing after it at all. (The second was
+  // already an EOF error before the surrogate rule.)
+  assert_false(@json.valid("\"\\uD800\""))
+  assert_false(@json.valid("\"\\uD800"))
+  // A character that is not the start of an escape.
+  assert_false(@json.valid("\"\\uD800x\""))
+  // An escape that is not `\u`.
+  assert_false(@json.valid("\"\\uD800\\n\""))
+  // Two leading-surrogate escapes in a row.
+  assert_false(@json.valid("\"\\uD800\\uD800\""))
+  // A second escape that is present but malformed or cut short.
+  assert_false(@json.valid("\"\\uD800\\uZZZZ\""))
+  assert_false(@json.valid("\"\\uD800\\u00\""))
+  assert_false(@json.valid("\"\\uD800\\u\""))
+  assert_false(@json.valid("\"\\uD800\\u"))
+}
+
+///|
+test "bare trailing-surrogate escape is rejected" {
+  assert_false(@json.valid("\"\\uDC00\""))
+  assert_false(@json.valid("\"a\\uDFFF b\""))
+  // An escaped pair in reverse order: the trailing half is bare.
+  assert_false(@json.valid("\"\\uDE00\\uD83D\""))
+}
+
+///|
+test "unpaired surrogate escapes are rejected in object keys too" {
+  assert_false(@json.valid("{\"\\uD800\": 1}"))
+  assert_false(@json.valid("{\"a\": 1, \"\\uDC00\": 2}"))
+  // ...and a valid pair in a key still works.
+  assert_true(@json.valid("{\"\\uD83D\\uDE00\": 1}"))
+}
+
+///|
+test "the surrogate error names the escape that could not pair up" {
+  // The position is the backslash opening the offending escape, not
+  // whichever character the scan stopped on — that would blame the last,
+  // perfectly valid, hex digit. Columns count code units from zero, so the
+  // backslash after the opening quote is column 1.
+  debug_inspect(
+    expect_parse_error("\"\\uDC00\"", "expected InvalidChar"),
+    content=(
+      #|InvalidChar({ line: 1, column: 1 }, '\\')
+    ),
+  )
+  debug_inspect(
+    expect_parse_error("\"ab\\uD800x\"", "expected InvalidChar"),
+    content=(
+      #|InvalidChar({ line: 1, column: 3 }, '\\')
+    ),
+  )
+  // A non-BMP character after the escape occupies two code units; the
+  // reported column must not land inside it.
+  debug_inspect(
+    expect_parse_error("\"\\uD800\u{1F600}\"", "expected InvalidChar"),
+    content=(
+      #|InvalidChar({ line: 1, column: 1 }, '\\')
+    ),
+  )
+  // The same position whichever way the pairing fails: a follower that is
+  // not a backslash, a backslash not followed by `u`, and a second escape
+  // that is well formed but is not a trailing surrogate.
+  debug_inspect(
+    expect_parse_error("\"\\uD800\\n\"", "expected InvalidChar"),
+    content=(
+      #|InvalidChar({ line: 1, column: 1 }, '\\')
+    ),
+  )
+  debug_inspect(
+    expect_parse_error("\"\\uD800\\u0041\"", "expected InvalidChar"),
+    content=(
+      #|InvalidChar({ line: 1, column: 1 }, '\\')
+    ),
+  )
+  // Running out of input is still an EOF error rather than a character one,
+  // including on a trailing backslash where the second escape should start.
+  debug_inspect(
+    expect_parse_error("\"\\uD800", "expected InvalidEof"),
+    content="InvalidEof",
+  )
+  debug_inspect(
+    expect_parse_error("\"\\uD800\\", "expected InvalidEof"),
+    content="InvalidEof",
+  )
+}
+
+///|
+test "an escape naming a non-BMP character reports it whole" {
+  // These two arms read a character and then step back by its width. Using
+  // a fixed step of one code unit landed inside a non-BMP character and
+  // reported a broken half one column further on.
+  debug_inspect(
+    expect_parse_error("\"\\\u{1F600}\"", "expected InvalidChar"),
+    content=(
+      #|InvalidChar({ line: 1, column: 2 }, '😀')
+    ),
+  )
+  debug_inspect(
+    expect_parse_error("\"\\u1\u{1F600}23\"", "expected InvalidChar"),
+    content=(
+      #|InvalidChar({ line: 1, column: 4 }, '😀')
+    ),
+  )
+}
+
+///|
+test "valid surrogate pairs decode to one scalar value" {
+  // Both ends of the supplementary range.
+  assert_true(@json.parse("\"\\uD800\\uDC00\"") == Json::string("\u{10000}"))
+  assert_true(@json.parse("\"\\udbff\\udfff\"") == Json::string("\u{10FFFF}"))
+  // Any hex digit case, and in any position within the string.
+  assert_true(@json.parse("\"\\uD83D\\uDE00\"") == Json::string("\u{1F600}"))
+  assert_true(
+    @json.parse("\"a\\uD83D\\uDE00b\\n\"") == Json::string("a\u{1F600}b\n"),
+  )
+  // Two pairs running together are decoded independently.
+  assert_true(
+    @json.parse("\"\\uD83D\\uDE00\\uD83D\\uDE01\"") ==
+    Json::string("\u{1F600}\u{1F601}"),
+  )
+  // Non-surrogate escapes are unaffected, including the replacement
+  // character, which is a scalar value like any other.
+  assert_true(@json.parse("\"\\u0041\\uFFFD\"") == Json::string("A\u{FFFD}"))
+}
+
+///|
+test "escaped and raw surrogate halves do not pair up" {
+  // Kept last: before the surrogate rule this input aborted the process
+  // rather than raising, so a regression here would take the whole test
+  // binary with it.
+  let lone_low = String::from_array([(0xDC00).unsafe_to_char()])
+  assert_false(@json.valid("\"\\uD800" + lone_low + "\""))
+}

json/parse.mbt

@@ -13,7 +13,10 @@
 // limitations under the License.
 
 ///|
-/// Validate input and return whether it is valid.
+/// Returns whether `input` parses. "Valid" here means exactly what `parse`
+/// accepts, so it includes the restriction on string contents described
+/// there: a document whose only defect is an unpaired surrogate escape is
+/// reported as invalid.
 pub fn valid(input : StringView) -> Bool {
   try {
     parse(input) |> ignore
@@ -25,6 +28,37 @@ pub fn valid(input : StringView) -> Bool {
 
 ///|
 /// Parse a JSON input string into a Json value, with an optional maximum nesting depth (default is 1024)
+///
+/// ## What strings may contain
+///
+/// Every string in the result is well-formed Unicode: each `\uXXXX` escape
+/// must denote a Unicode scalar value on its own, or be one half of a
+/// correctly ordered surrogate pair. An escaped leading surrogate
+/// (`\uD800`–`\uDBFF`) must therefore be followed immediately by an escaped
+/// trailing surrogate (`\uDC00`–`\uDFFF`), and the pair is decoded as the
+/// one character it stands for.
+///
+/// An escape that cannot pair up raises `InvalidChar` positioned at the
+/// backslash that opens it — that one position, whatever the scan actually
+/// stopped on. Input that simply runs out still raises `InvalidEof`, and a
+/// second escape that is itself malformed is reported as the hex-digit
+/// error it is, at the offending digit.
+///
+/// This is a limit on what a string may *contain*, which RFC 8259 §9 leaves
+/// to the implementation — not a claim about which documents are
+/// grammatically well formed, since §8.2 admits unpaired surrogate escapes,
+/// nor a claim of I-JSON (RFC 7493) conformance, which restricts more than
+/// this. It is chosen because MoonBit's `String` is required to be
+/// well-formed, so the alternatives are to hand back a string that violates
+/// that invariant, or to substitute U+FFFD and silently lose the
+/// distinction between two different keys. A parse error is the only one of
+/// the three a caller can see and act on.
+///
+/// The cost is real: `JSON.stringify` in JavaScript emits lone surrogates as
+/// `\uXXXX`, so some JSON that JavaScript and Python accept is rejected
+/// here. Rust's serde_json rejects it too when parsing into `String` or
+/// `Value`, though its byte-oriented mode admits WTF-8; Go's `encoding/json`
+/// substitutes U+FFFD, while its experimental v2 parser is stricter.
 #label_migration(max_nesting_depth, fill=false)
 pub fn parse(
   input : StringView,