Repository: jwtk/jjwt (java)

Issue:
Option to enable strict duplicate detection
<!--
PLEASE DO NOT USE GITHUB ISSUES TO ASK QUESTIONS! 

The JJWT team uses GitHub Issues only to track actionable work that requires changes to the JJWT codebase.

If you have a usability or general question about JJWT, please instead post your question to StackOverflow using the following JJWT-specific link:

https://stackoverflow.com/questions/ask?tags=jjwt&guided=false

To help ensure you receive a timely response, please follow the StackOverflow guidelines on how to ask a good question: https://stackoverflow.com/help/how-to-ask

Ok, now that that's out of the way - if you're still sure you want to create a feature request or enhancement, please fill in the sections below...
-->

**Is your feature request related to a problem? Please describe.**
For parsing, we are using the `JacksonDeserializer` via the constructor `public JacksonDeserializer(Map<String, Class<?>> claimTypeMap)`.
When parsing a JWT with duplicate properties, the last one will be used, and the rest are ignored. Although this behavior is allowed by the RFCs, we would prefer for the parser to throw an exception if there are duplicate keys.
In general, it can create vulnerabilities if e.g. different parsers are used in a stack, where some use the first item, and some use the last item, in cases of duplicates. This is of course not allowed in JWT context, but we would also like to fail fast, if the next parser fails.
This can be done by enabling `JsonParser.Feature.STRICT_DUPLICATE_DETECTION` on the underlying `ObjectMapper`.
The problem is that there is no way to access the `objectMapper` in `JacksonDeserializer`.

**Describe the solution you'd like**
An option to enable `JsonParser.Feature.STRICT_DUPLICATE_DETECTION` on the `objectMapper` in `JacksonDeserializer`. Whether it be by adding a getter for the `objectMapper` or an extra constructor that also takes `JsonParser.Feature`s?

**Describe alternatives you've considered**
We could write our own implementation of `AbstractSerializer`, but it would be a copy of the `JacksonDeserializer` class except for enabling the strict duplicate detection.

**Additional context**
The RFCs allows both behaviors for parsing duplicates in both header and payload:
https://www.rfc-editor.org/rfc/rfc7515.html#section-4
https://www.rfc-editor.org/rfc/rfc7519#section-4
Strict duplicate detection:
https://fasterxml.github.io/jackson-core/javadoc/2.9/com/fasterxml/jackson/core/JsonParser.Feature.html#STRICT_DUPLICATE_DETECTION

Pull request:
Jackson: disallow duplicate JSON properties
- Ensures that Jackson duplicate property detection/rejection is enabled by default.

Fixes #877

Patch:
diff --git a/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonDeserializer.java b/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonDeserializer.java
index 8314f1e..afb1529 100644
--- a/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonDeserializer.java
+++ b/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonDeserializer.java
@@ -72,14 +72,8 @@ public class JacksonDeserializer<T> extends AbstractDeserializer<T> {
      * @param claimTypeMap The claim name-to-class map used to deserialize claims into the given type
      */
     public JacksonDeserializer(Map<String, Class<?>> claimTypeMap) {
-        // DO NOT reuse JacksonSerializer.DEFAULT_OBJECT_MAPPER as this could result in sharing the custom deserializer
-        // between instances
-        this(new ObjectMapper());
-        Assert.notNull(claimTypeMap, "Claim type map cannot be null.");
-        // register a new Deserializer
-        SimpleModule module = new SimpleModule();
-        module.addDeserializer(Object.class, new MappedTypeDeserializer(Collections.unmodifiableMap(claimTypeMap)));
-        objectMapper.registerModule(module);
+        // DO NOT specify JacksonSerializer.DEFAULT_OBJECT_MAPPER here as that would modify the shared instance
+        this(JacksonSerializer.newObjectMapper(), claimTypeMap);
     }
 
     /**
@@ -92,6 +86,46 @@ public class JacksonDeserializer<T> extends AbstractDeserializer<T> {
         this(objectMapper, (Class<T>) Object.class);
     }
 
+    /**
+     * Creates a new JacksonDeserializer where the values of the claims can be parsed into given types by registering
+     * a type-converting {@link  com.fasterxml.jackson.databind.Module Module} on the specified {@link ObjectMapper}.
+     * A common usage example is to parse custom User object out of a claim, for example the claims:
+     * <pre>{@code
+     * {
+     *     "issuer": "https://issuer.example.com",
+     *     "user": {
+     *         "firstName": "Jill",
+     *         "lastName": "Coder"
+     *     }
+     * }}</pre>
+     * Passing a map of {@code ["user": User.class]} to this constructor would result in the {@code user} claim being
+     * transformed to an instance of your custom {@code User} class, instead of the default of {@code Map}.
+     * <p>
+     * Because custom type parsing requires modifying the state of a Jackson {@code ObjectMapper}, this
+     * constructor modifies the specified {@code objectMapper} argument and customizes it to support the
+     * specified {@code claimTypeMap}.
+     * <p>
+     * If you do not want your {@code ObjectMapper} instance modified, but also want to support custom types for
+     * JWT {@code Claims}, you will need to first customize your {@code ObjectMapper} instance by registering
+     * your custom types separately and then use the {@link #JacksonDeserializer(ObjectMapper)} constructor instead
+     * (which does not modify the {@code objectMapper} argument).
+     *
+     * @param objectMapper the objectMapper to modify by registering a custom type-converting
+     *                     {@link com.fasterxml.jackson.databind.Module Module}
+     * @param claimTypeMap The claim name-to-class map used to deserialize claims into the given type
+     * @since 0.12.4
+     */
+    //TODO: Make this public on a minor release
+    //      (cannot do that on a point release as that would violate semver)
+    private JacksonDeserializer(ObjectMapper objectMapper, Map<String, Class<?>> claimTypeMap) {
+        this(objectMapper);
+        Assert.notNull(claimTypeMap, "Claim type map cannot be null.");
+        // register a new Deserializer on the ObjectMapper instance:
+        SimpleModule module = new SimpleModule();
+        module.addDeserializer(Object.class, new MappedTypeDeserializer(Collections.unmodifiableMap(claimTypeMap)));
+        objectMapper.registerModule(module);
+    }
+
     private JacksonDeserializer(ObjectMapper objectMapper, Class<T> returnType) {
         Assert.notNull(objectMapper, "ObjectMapper cannot be null.");
         Assert.notNull(returnType, "Return type cannot be null.");
diff --git a/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonSerializer.java b/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonSerializer.java
index 31c5ae0..2582da5 100644
--- a/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonSerializer.java
+++ b/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonSerializer.java
@@ -16,6 +16,7 @@
 package io.jsonwebtoken.jackson.io;
 
 import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.databind.Module;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.ObjectWriter;
@@ -41,7 +42,22 @@ public class JacksonSerializer<T> extends AbstractSerializer<T> {
         MODULE = module;
     }
 
-    static final ObjectMapper DEFAULT_OBJECT_MAPPER = new ObjectMapper().registerModule(MODULE);
+    static final ObjectMapper DEFAULT_OBJECT_MAPPER = newObjectMapper();
+
+    /**
+     * Creates and returns a new ObjectMapper with the {@code jjwt-jackson} module registered and
+     * {@code JsonParser.Feature.STRICT_DUPLICATE_DETECTION} enabled (set to true).
+     *
+     * @return and returns a new ObjectMapper with the {@code jjwt-jackson} module registered and
+     * {@code JsonParser.Feature.STRICT_DUPLICATE_DETECTION} enabled (set to true).
+     * @since 0.12.4
+     */
+    // package protected on purpose, do not expose to the public API
+    static ObjectMapper newObjectMapper() {
+        return new ObjectMapper()
+                .registerModule(MODULE)
+                .configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, true); // https://github.com/jwtk/jjwt/issues/877
+    }
 
     protected final ObjectMapper objectMapper;
 


Test patch:
diff --git a/extensions/jackson/src/test/groovy/io/jsonwebtoken/jackson/io/JacksonDeserializerTest.groovy b/extensions/jackson/src/test/groovy/io/jsonwebtoken/jackson/io/JacksonDeserializerTest.groovy
index 25e27a3..62f253e 100644
--- a/extensions/jackson/src/test/groovy/io/jsonwebtoken/jackson/io/JacksonDeserializerTest.groovy
+++ b/extensions/jackson/src/test/groovy/io/jsonwebtoken/jackson/io/JacksonDeserializerTest.groovy
@@ -16,6 +16,7 @@
 //file:noinspection GrDeprecatedAPIUsage
 package io.jsonwebtoken.jackson.io
 
+import com.fasterxml.jackson.core.JsonParseException
 import com.fasterxml.jackson.databind.ObjectMapper
 import io.jsonwebtoken.io.DeserializationException
 import io.jsonwebtoken.io.Deserializer
@@ -120,6 +121,31 @@ class JacksonDeserializerTest {
         assertEquals expected, result
     }
 
+    /**
+     * Asserts https://github.com/jwtk/jjwt/issues/877
+     */
+    @Test
+    void testStrictDuplicateDetection() {
+        // 'bKey' is repeated twice:
+        String json = """
+             {
+                "aKey":"oneValue", 
+                "bKey": 15,
+                "bKey": "hello"
+             }
+            """
+        try {
+            new JacksonDeserializer<>().deserialize(new StringReader(json))
+            fail()
+        } catch (DeserializationException expected) {
+            String causeMsg = "Duplicate field 'bKey'\n at [Source: (StringReader); line: 5, column: 23]"
+            String msg = "Unable to deserialize: $causeMsg"
+            assertEquals msg, expected.getMessage()
+            assertTrue expected.getCause() instanceof JsonParseException
+            assertEquals causeMsg, expected.getCause().getMessage()
+        }
+    }
+
     /**
      * For: https://github.com/jwtk/jjwt/issues/564
      */