Skip to content

fix: Make the default backend RFC 8259 compliant - #46

Merged
keelerm84 merged 4 commits into
v3from
mk/SDK-2771/rfc-8259-compliance
Jul 29, 2026
Merged

fix: Make the default backend RFC 8259 compliant#46
keelerm84 merged 4 commits into
v3from
mk/SDK-2771/rfc-8259-compliance

Conversation

@keelerm84

@keelerm84 keelerm84 commented Jul 28, 2026

Copy link
Copy Markdown
Member

The default (non-easyjson) tokenizer accepted several inputs that RFC 8259
forbids and mangled one valid input. Since the default implementation is the
source of truth for this library, align its accept/reject decisions with
encoding/json's json.Valid.

Reader (jreader/token_reader_default.go):

  • Whitespace: allow only space, tab, LF, and CR between tokens instead of the
    broader unicode.IsSpace (which accepted vertical tab, form feed, NEL, NBSP).
  • Numbers: reject leading zeros, a bare minus sign, and a trailing decimal
    point by walking the RFC number grammar.
  • Strings: reject unescaped control characters (below 0x20).
  • Strings: combine UTF-16 surrogate pairs into a single code point; lone or
    invalid surrogates decode to the replacement character, matching
    encoding/json.

Writer (jwriter/token_writer_default.go):

  • Float64 returns an error for NaN and infinities rather than emitting invalid
    JSON.

Tests are default-only because the easyjson backend does not make all of the
same decisions.


Note

Medium Risk
Stricter parsing and different edge-case decoding may reject or change values for inputs the old tokenizer accepted; scope is core JSON I/O but behavior is intentionally aligned with encoding/json.

Overview
Aligns the default (non-easyjson) JSON tokenizer and writer with RFC 8259 and encoding/json accept/reject and decode behavior.

Reader: Token whitespace is limited to space, tab, LF, and CR (no longer all unicode.IsSpace). Number parsing follows the RFC grammar (e.g. rejects leading zeros, bare -, trailing .) and int64 overflow falls back to float parsing instead of wrapping. Strings reject unescaped controls; invalid UTF-8 and lone/malformed \u surrogates match stdlib (replacement char); valid surrogate pairs combine into one code point.

Writer: Float64 returns an error for NaN and ±Inf instead of emitting invalid JSON.

Adds default-only RFC compliance tests (easyjson backend unchanged).

Reviewed by Cursor Bugbot for commit c14f916. Bugbot is set up for automated code reviews on this repo. Configure here.

The default (non-easyjson) tokenizer accepted several inputs that RFC 8259
forbids and mangled one valid input. Since the default implementation is the
source of truth for this library, align its accept/reject decisions with
encoding/json's json.Valid.

Reader (jreader/token_reader_default.go):
- Whitespace: allow only space, tab, LF, and CR between tokens instead of the
  broader unicode.IsSpace (which accepted vertical tab, form feed, NEL, NBSP).
- Numbers: reject leading zeros, a bare minus sign, and a trailing decimal
  point by walking the RFC number grammar.
- Strings: reject unescaped control characters (below 0x20).
- Strings: combine UTF-16 surrogate pairs into a single code point; lone or
  invalid surrogates decode to the replacement character, matching
  encoding/json.

Writer (jwriter/token_writer_default.go):
- Float64 returns an error for NaN and infinities rather than emitting invalid
  JSON.

Tests are default-only because the easyjson backend does not make all of the
same decisions.
Follow-up to the RFC 8259 compliance pass, addressing pre-existing divergences
surfaced by review of the default reader:

- Large integer literals that overflow int64 no longer return a wrapped garbage
  value (wrong sign, or zero) with no error. readNumber now falls back to float
  parsing when parseIntFromBytes reports overflow, matching the value
  encoding/json produces for magnitudes within float64 range and rejecting
  those beyond it (consistent with out-of-range float literals).

- Raw invalid UTF-8 bytes inside a string now decode to the Unicode replacement
  character, matching encoding/json, instead of being passed through verbatim
  in the zero-copy fast path (which also made the result depend on where an
  escape appeared in the string).

Extracted readUnicodeEscape and beginEscapedCopy helpers from readString. Added
regression tests for both fixes plus coverage for the escaped surrogate-pair
success path, control characters in property names, and additional number
grammar cases.
@keelerm84
keelerm84 marked this pull request as ready for review July 28, 2026 16:54
@keelerm84
keelerm84 requested a review from a team as a code owner July 28, 2026 16:54

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1551a97. Configure here.

Comment thread jreader/token_reader_rfc8259_default_test.go
Cursor Bugbot correctly flagged that TestReaderCombinesSurrogatePairs fed
literal UTF-8 (e.g. the decoded clef glyph) rather than JSON \u escape bytes,
so readUnicodeEscape's surrogate-combining branch had no coverage despite the
comment claiming it. Switch the combine cases to raw string literals carrying
the actual backslash-u escapes (\uD834\uDD1E etc.), add the maximum code point
\uDBFF\uDFFF, and keep one literal-UTF-8 case to cover the plain ReadRune
path. The invalid-surrogate test already used real escape bytes.
got := r.String()
require.NoError(t, r.Error())
require.NoError(t, r.RequireEOF())
assert.Equal(t, "a\U0001D11Eb", got)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I think these 5 lines are duplicate code - it looks like the exact same thing as the 2nd test case.

@aaron-zeisler aaron-zeisler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a minor recommendation 👍 To me, this code feels finicky, but I understand its purpose, and I'm not sure how to improve it generally.

@kinyoklion kinyoklion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential concern about non-breaking spaces, but it is JSON compliant.

@kinyoklion

Copy link
Copy Markdown
Member

Potential concern about non-breaking spaces, but it is JSON compliant.

Looks like at least some other SDKs also reject, so we should be good.

@keelerm84
keelerm84 merged commit a74e968 into v3 Jul 29, 2026
13 checks passed
@keelerm84
keelerm84 deleted the mk/SDK-2771/rfc-8259-compliance branch July 29, 2026 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants