fix: reject unpaired surrogates in strict JSON - #3116
Conversation
eamonnmcmanus
left a comment
There was a problem hiding this comment.
Thanks for putting this together! Just a couple of small things.
| } | ||
| for (int i = 0; i < value.length(); i++) { | ||
| char c = value.charAt(i); | ||
| if (Character.isHighSurrogate(c)) { |
There was a problem hiding this comment.
As a minor optimization, you could do this:
if (Character.isSurrogate(c)) {
if (Character.isHighSurrogate(c)
&& i + 1 < value.length()
&& Character.isLowSurrogate(value.charAt(++i))) {
continue;
}
throw syntaxError(...);
}That way, in the common case where there are no surrogate characters, there is only one if per iteration rather than two.
I thought there might be some clever way to use CharsetEncoder.canEncode(CharSequence) here, but it doesn't look as if that would be very efficient.
|
|
||
| @Test | ||
| public void testStrictModeRejectsUnpairedSurrogates() throws IOException { | ||
| for (String json : new String[] {"\"\\uD800\"", "\"\\uDC00\"", "{\"\\uD800\":1}"}) { |
There was a problem hiding this comment.
I believe the new code will also reject inputs that contain unpaired surrogates directly as characters, not expressed via \u escapes in the JSON. That makes sense to me, but can you test it too? I mean "\"\uD800\"" for example.
There was a problem hiding this comment.
I am wondering if raw unpaired surrogates are rather the responsibility of the user? The user is in control of how the underlying Reader is created, and could probably have prevented the unpaired surrogates if they wanted to. (But if the JSON is present as String already, then it would be cumbersome for the user to validate it; effectively duplicating the code of the PR here.)
I am mentioning this because I am wondering whether it would suffice to implement this validation only in the context of readEscapeCharacter() (maybe with an additional local boolean to store whether a low surrogate is expected).
The current implementation of this PR also has the side-effect that it allows creating surrogate pairs with mixed raw and escaped surrogates, as long as they form a valid pair in the end. Maybe a bit weird, but not necessarily bad?
But maybe this is not as important; the current implementation of this PR is fine too.
(Sorry that this comment is a bit late now.)
|
Addressed the review feedback in commit |
JsonReader has rejected unpaired UTF-16 surrogates in STRICT mode since PR google#3116, but JsonWriter still writes them. That produces a document which this class's own documentation promises conforms to RFC 8259 but which no conforming parser can read back, and which is silently replaced by '?' when the document is encoded as UTF-8. Gson's own default output is therefore unreadable by Gson in STRICT mode. Validate the value on the write path the same way the read path does, so the two sides of the API agree on what a strict JSON string is. The check is limited to Strictness.STRICT so callers of the legacy permissive modes see no behaviour change. Adds writer-side tests mirroring the existing JsonReaderTest coverage; the writer previously had none.
JsonReader has rejected unpaired UTF-16 surrogates in STRICT mode since PR google#3116, but JsonWriter still writes them. That produces a document which this class's own documentation promises conforms to RFC 8259 but which no conforming parser can read back, and which is silently replaced by '?' when the document is encoded as UTF-8. Gson's own default output is therefore unreadable by Gson in STRICT mode. Validate the value on the write path the same way the read path does, so the two sides of the API agree on what a strict JSON string is. The check is limited to Strictness.STRICT so callers of the legacy permissive modes see no behaviour change. Adds writer-side tests mirroring the existing JsonReaderTest coverage; the writer previously had none.
Fixes #3113
JsonReadercurrently accepts\uD800/\uDC00escapes as lone UTF-16 surrogate code units even whenStrictness.STRICTis selected. This can return malformed Java strings for both values and object member names. Validate the fully decoded quoted string at the boundary where it is returned, rejecting unpaired surrogates while allowing valid surrogate pairs and leaving legacy/lenient modes unchanged.Validation
mvn -pl gson -Denforcer.skip=true -Dtest=com.google.gson.stream.JsonReaderTest test(164 tests, 3 skipped; JDK 26 requires the repository enforcer override)git diff --checkThe regression test covers escaped lone high/low surrogates in values and names, plus a valid escaped pair.