Repository navigation
Align the Gson extension with Jackson's duplicate member name rejection - #1073
masiljangajji wants to merge 4 commits into
Conversation
RFC 7515 Section 4 and RFC 7519 Section 4 require JOSE header and JWT claims member names to be unique, and permit parsers to either reject such input or use only the lexically last value. Gson silently does the latter, so a token whose header or payload repeats a member name is accepted with the last occurrence winning. The Jackson extension already rejects this input: the ObjectMapper that JacksonSerializer creates enables STRICT_DUPLICATE_DETECTION. This applies the same behavior to the Gson extension's default Gson instance by wrapping the reader in a JsonReader that tracks member names per object depth and throws when one repeats. Gson forbids overriding its built-in Object type adapter, so detection is implemented at the token reader layer, which is the same layer where Jackson's STRICT_DUPLICATE_DETECTION operates. A caller-supplied Gson instance retains Gson's default behavior, mirroring how the Jackson extension only configures the ObjectMapper it creates itself. No public API changes.
Gson only asserts that nothing follows the parsed value when it creates the JsonReader itself; Gson#fromJson(JsonReader, Type) skips that check. Wrapping the reader therefore made the deserializer accept trailing content that it previously rejected. Perform the same assertion after parsing, and cover it with tests.
|
I measured the overhead this adds, since the change allocates one name set per JSON object on a hot path. Harness, raw JMH output and environment records: https://github.com/masiljangajji/jjwt-duplicate-name-bench Both arms are this branch's End to end,
Temurin 17.0.14, 8 warmup × 8 measurement iterations × 6 forks, confidence intervals separated (7,633 against 7,657). Corretto 21.0.5 was run as well; its JSON-level deltas sum to +152 ns, which agrees. The allocation figures are identical to the byte across both JDKs and all twelve forks. They scale with the number of JSON objects in a document rather than its length — +328 B for the header, +824 B for the claims set. I also measured a |
|
@masiljangajji great work, thank you so much for the contribution! |
|
@masiljangajji looks like some minor code coverage needs to be addressed with tests |
A strict JsonReader, the only kind this deserializer creates, throws MalformedJsonException for anything following the top-level value, so peek() never returns a token other than END_DOCUMENT and the JsonSyntaxException that branch guarded could not be reached. Dropping it leaves the reader's own error as the single path for trailing content, which the existing tests cover. Also adds a test for the JsonIOException path, where the stream fails as the check reads past the parsed value.
Removing the guard along with the unreachable token comparison changed behavior: a top-level null literal followed by trailing content, and an empty or blank document, all returned null before and started throwing afterwards. Gson skips the check for a null value for the same reason, so the guard stays and only the comparison is gone. Adds a test for the null literal, which is also the input that exercises the guard's false branch.
|
@lhazlewood Coverage is addressed. Two tests were added, and one comparison was removed because no input can reach it.
Trailing content is still rejected. The reader's own The new tests cover the two branches that were reachable but untested: a top level Locally |
Summary
The Gson extension silently accepts JSON objects with duplicate member names and keeps the last occurrence. A signed token whose header or payload repeats a name is therefore parsed without complaint:
This makes the two JSON extensions disagree about whether the same signed token is valid.
Why reject
RFC 7515, Section 4 and RFC 7519, Section 4 require member names to be unique and let recipients either reject the input or use only the lexically last value. Both are conformant, so this is a policy choice rather than a bug fix.
JJWT already made that choice for Jackson in #877, which concluded:
Nothing in that reasoning is specific to Jackson. The Gson extension wraps the same kind of input for the same purpose, and today the two extensions disagree about whether a given signed token is acceptable. This change makes the Gson extension follow the decision already taken in #877.
Implementation
Gson has no equivalent switch, and
GsonBuilderrefuses to register an adapter forObject.class, which is the only typeGsonDeserializerever requests:Detection is therefore done at the token reader layer, which is where Jackson's
STRICT_DUPLICATE_DETECTIONalso operates (JsonParser.Feature).doDeserializewraps the suppliedReaderin aJsonReadersubclass that keeps one name set per object depth:beginObject()pushes a set,endObject()pops itnextName()throwsJsonParseExceptionwhenSet#addreports the name was already presentGson still performs all value parsing, so number handling, escaping and every other behavior is unchanged. No extra pass and no buffering are introduced.
One subtlety: Gson asserts that nothing follows the parsed value only when it creates the
JsonReaderitself —Gson#fromJson(JsonReader, Type)skips that check. Supplying our own reader would therefore have started accepting trailing content, sodoDeserializeperforms the same assertion after parsing. Two tests cover it.Scope
Rejection applies to the
Gsoninstance JJWT creates for itself. An instance supplied throughGsonDeserializer(Gson)keeps Gson's behavior, which is the same approach the Jackson extension already takes: it configures only theObjectMapperit creates and leaves an application-supplied one untouched.Tests
Eight tests added to
GsonDeserializerTest— three that must reject duplicates, three that must not, and two that pin the trailing-content behavior:{"sub":"alice","sub":"attacker"}{"user":{"id":1,"id":2}}{"list":[{"k":"a","k":"b"}]}{"x":{"k":1},"y":{"k":2}}{"l":[{"k":1},{"k":2}]}Gson{"sub":"alice"}trailing{"a":1} {"b":2}The caller-supplied case pins the scope decision above so it cannot be widened by accident.
Compatibility
No public API changes — the new flag flows only through private constructors, and
javapreports an identical public member list before and after../mvnw -pl extensions/gson -am clean verifypasses, and theimplsuite (1,511 tests, which loads this extension throughServiceLoader) passes unchanged.