validate utf-16 surrogate halves in decodeUnicodeCodePoint

[SABITHSAHEB]

1. after a high surrogate (D800-DBFF) the next \u escape is consumed and combined without checking it is a low surrogate, so "\uD801\u0041" or "\uD801\uD801" parse to a wrong astral code point and the second escape's real value is lost.
2. a low surrogate that appears on its own (e.g. "\uDC00") is passed straight through and gets written out as the invalid UTF-8 bytes ED B0 80.

OurReader (CharReader) now validates the low-surrogate range when completing a pair and rejects an unpaired low surrogate, gated behind a new rejectInvalidSurrogates CharReaderBuilder setting: on by default and in strictMode, off in ecma404Mode (lone surrogate escapes are conforming there). The legacy Json::Reader is intentionally left unchanged since its Features struct has no knob to gate this on. Added CharReader tests for the two rejection paths and the opt-out; existing valid-pair cases are unaffected.

[SABITHSAHEB]

any update?

[baylesj]

Thanks for the fix — the validation logic itself looks correct. One design concern before merging, though: this rejection is unconditional in both readers, with no way to opt out.

Unlike #1663 (unescaped control characters, which are grammar-invalid per RFC 8259), lone/mismatched surrogate escapes are syntactically valid JSON under both the RFC 8259 ABNF and ECMA-404 — the spec only calls the resulting value "unpredictable".

Real-world producers emit them legally: ES2019+ JSON.stringify('\uD83D') yields "\ud83d", and Python's json.dumps does the same. After this change, documents that parsed on every prior release hard-fail in every configuration — including CharReaderBuilder::ecma404Mode, which would now reject text that ECMA-404 defines as conforming, and the legacy Json::Reader, whose Features struct has no knob at all.

Comparable strictness decisions (failIfExtra, rejectDupKeys, allowSpecialFloats) are all routed through CharReaderBuilder settings. Could this be gated behind a setting (e.g. rejectInvalidSurrogates, arguably default-on) so downstream users with lenient-ingestion pipelines have an escape hatch? Alternatively, WHATWG-style U+FFFD replacement in the lenient path would avoid the hard break while still fixing the invalid-UTF-8 output.

[SABITHSAHEB]

Makes sense on the spec point: these are well-formed per RFC 8259 and ECMA-404, so an unconditional hard-fail in every config was too aggressive.

Routed it through a rejectInvalidSurrogates CharReaderBuilder setting, same as failIfExtra/rejectDupKeys/allowSpecialFloats. Default on (your "arguably default-on"), on in strictMode, off in ecma404Mode so conforming lone surrogates still parse there, and lenient-ingestion pipelines can flip it off. I also reverted the legacy Json::Reader change: it has no Features knob to gate this on, and leaving it as-is keeps input that parsed on prior releases parsing. Added a test for the opt-out path.

[SABITHSAHEB]

gentle ping

[baylesj]

@greptileai

[greptile-apps]

Greptile Summary

The PR adds configurable UTF-16 surrogate validation to the CharReader implementation and enables it by default while preserving an ECMA-404/explicitly lenient mode.

• Rejects mismatched second surrogate halves and standalone low surrogates in OurReader.
• Adds builder defaults, validation, strict-mode, and ECMA-404-mode handling for the new setting.
• Adds CharReader tests for both rejection paths and the lenient escape hatch.
• Does not apply the promised validation to the separate public Json::Reader implementation.

Confidence Score: 4/5

This should be fixed before merging because malformed surrogate escapes remain accepted through the public Json::Reader API despite the PR's intended coverage of both reader implementations.

The new checks correctly protect CharReader/OurReader, but Json::Reader uses a separate unchanged decoder with no equivalent setting or validation, leaving the reported incorrect decoding behavior reachable and making parser behavior inconsistent.

Files Needing Attention: src/lib_json/json_reader.cpp, src/test_lib_json/main.cpp

Important Files Changed

Filename	Overview
src/lib_json/json_reader.cpp	Adds correct configurable checks to OurReader and builder modes, but omits the equivalent validation from the separate public Reader decoder.
include/json/reader.h	Documents the new CharReaderBuilder setting, its validation behavior, and its ECMA-404 interaction.
src/test_lib_json/main.cpp	Covers both new rejection branches and lenient mode, but does not test the legacy Json::Reader path where the bug remains.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[JSON input with surrogate escape] --> B{Parser API}
  B -->|CharReader| C[OurReader decoder]
  C --> D{rejectInvalidSurrogates}
  D -->|true| E[Validate pair or reject lone low surrogate]
  D -->|false| F[Preserve lenient behavior]
  B -->|Json::Reader| G[Reader decoder]
  G --> H[No new range validation]
  H --> I[Malformed surrogate remains accepted]

Loading

_{Reviews (1): Last reviewed commit: "Merge branch 'master' into validate-surr..." | Re-trigger Greptile}

[greptile-apps]

Legacy Reader skips surrogate validation

When callers parse a mismatched pair such as "\uD801\u0041" or a lone low surrogate through the public Json::Reader API, its separate unchanged decoder still accepts the malformed escape, causing the second escape's value to be lost or invalid UTF-8 to be emitted. Apply equivalent validation to Reader::decodeUnicodeCodePoint so both public parser paths enforce the behavior described by this change.

[SABITHSAHEB]

That's intentional, see the thread above: the legacy Json::Reader change was dropped during review because its Features struct has no setting to gate this on, and an unconditional hard-fail there would break input that parsed on every prior release. The fix is scoped to CharReader behind rejectInvalidSurrogates. I've updated the PR description so it no longer says both readers.

[coveralls]

Coverage Report for CI Build 32424372791

Coverage increased (+0.01%) to 89.917%

Details

• Coverage increased (+0.01%) from the base build.
• Patch coverage: 1 uncovered change across 1 file (9 of 10 lines covered, 90.0%).
• 1 coverage regression across 1 file.

Uncovered Changes

File	Changed	Covered	%
src/lib_json/json_reader.cpp	10	9	90.0%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
src/lib_json/json_reader.cpp	1	89.91%

---

Coverage Stats

Relevant Lines:	2749
Covered Lines:	2624
Line Coverage:	95.45%
Relevant Branches:	2656
Covered Branches:	2236
Branch Coverage:	84.19%
Branches in Coverage %:	Yes
Coverage Strength:	23869.51 hits per line

---

💛 - Coveralls