diff --git a/CHANGELOG.md b/CHANGELOG.md index 290ec431f..64d8420c0 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/json/README.mbt.md b/json/README.mbt.md index e33bf48ba..b34fc8b30 100644 --- a/json/README.mbt.md +++ b/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 diff --git a/json/lex_string.mbt b/json/lex_string.mbt index fbaa8d387..3db187efe 100644 --- a/json/lex_string.mbt +++ b/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 diff --git a/json/lex_string_test.mbt b/json/lex_string_test.mbt index 31303069c..57cd6b9af 100644 --- a/json/lex_string_test.mbt +++ b/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 + "\"")) +} diff --git a/json/parse.mbt b/json/parse.mbt index 174e361dd..a6acc6d16 100644 --- a/json/parse.mbt +++ b/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,