Add ability to disallow repeated keys in CBOR

Fixes Kotlin#2662 by adding a `visitKey` method to `CompositeDecoder`; map and set serializers should call this so that decoders have an opportunity to throw an error when a duplicate key is detected. Calls to this method are added to: - `MapLikeSerializer.readElement` to handle maps - `CborReader.decodeElementIndex` to handle data classes A new config option `forbidDuplicateKeys` can be set to true to enable this new behavior. This can form the basis of a Strict Mode in the future.
timmc · Nov 30, 2024 · d14f488 · d14f488
1 parent 1b0accd
commit d14f488
Show file tree

Hide file tree

Showing 12 changed files with 125 additions and 6 deletions.
diff --git a/core/api/kotlinx-serialization-core.api b/core/api/kotlinx-serialization-core.api
@@ -19,6 +19,10 @@ public abstract interface class kotlinx/serialization/DeserializationStrategy {
 	public abstract fun getDescriptor ()Lkotlinx/serialization/descriptors/SerialDescriptor;
 }
 
+public final class kotlinx/serialization/DuplicateKeyException : kotlinx/serialization/SerializationException {
+	public fun <init> (Ljava/lang/Object;)V
+}
+
 public abstract interface annotation class kotlinx/serialization/EncodeDefault : java/lang/annotation/Annotation {
 	public abstract fun mode ()Lkotlinx/serialization/EncodeDefault$Mode;
 }
@@ -445,6 +449,7 @@ public abstract interface class kotlinx/serialization/encoding/CompositeDecoder
 	public abstract fun decodeStringElement (Lkotlinx/serialization/descriptors/SerialDescriptor;I)Ljava/lang/String;
 	public abstract fun endStructure (Lkotlinx/serialization/descriptors/SerialDescriptor;)V
 	public abstract fun getSerializersModule ()Lkotlinx/serialization/modules/SerializersModule;
+	public fun visitKey (Ljava/lang/Object;)V
 }
 
 public final class kotlinx/serialization/encoding/CompositeDecoder$Companion {
@@ -457,6 +462,7 @@ public final class kotlinx/serialization/encoding/CompositeDecoder$DefaultImpls
 	public static synthetic fun decodeNullableSerializableElement$default (Lkotlinx/serialization/encoding/CompositeDecoder;Lkotlinx/serialization/descriptors/SerialDescriptor;ILkotlinx/serialization/DeserializationStrategy;Ljava/lang/Object;ILjava/lang/Object;)Ljava/lang/Object;
 	public static fun decodeSequentially (Lkotlinx/serialization/encoding/CompositeDecoder;)Z
 	public static synthetic fun decodeSerializableElement$default (Lkotlinx/serialization/encoding/CompositeDecoder;Lkotlinx/serialization/descriptors/SerialDescriptor;ILkotlinx/serialization/DeserializationStrategy;Ljava/lang/Object;ILjava/lang/Object;)Ljava/lang/Object;
+	public static fun visitKey (Lkotlinx/serialization/encoding/CompositeDecoder;Ljava/lang/Object;)V
 }
 
 public abstract interface class kotlinx/serialization/encoding/CompositeEncoder {

diff --git a/core/api/kotlinx-serialization-core.klib.api b/core/api/kotlinx-serialization-core.klib.api
@@ -184,6 +184,7 @@ abstract interface kotlinx.serialization.encoding/CompositeDecoder { // kotlinx.
     abstract fun endStructure(kotlinx.serialization.descriptors/SerialDescriptor) // kotlinx.serialization.encoding/CompositeDecoder.endStructure|endStructure(kotlinx.serialization.descriptors.SerialDescriptor){}[0]
     open fun decodeCollectionSize(kotlinx.serialization.descriptors/SerialDescriptor): kotlin/Int // kotlinx.serialization.encoding/CompositeDecoder.decodeCollectionSize|decodeCollectionSize(kotlinx.serialization.descriptors.SerialDescriptor){}[0]
     open fun decodeSequentially(): kotlin/Boolean // kotlinx.serialization.encoding/CompositeDecoder.decodeSequentially|decodeSequentially(){}[0]
+    open fun visitKey(kotlin/Any?) // kotlinx.serialization.encoding/CompositeDecoder.visitKey|visitKey(kotlin.Any?){}[0]
 
     final object Companion { // kotlinx.serialization.encoding/CompositeDecoder.Companion|null[0]
         final const val DECODE_DONE // kotlinx.serialization.encoding/CompositeDecoder.Companion.DECODE_DONE|{}DECODE_DONE[0]
@@ -735,6 +736,10 @@ final class kotlinx.serialization.modules/SerializersModuleBuilder : kotlinx.ser
     final fun include(kotlinx.serialization.modules/SerializersModule) // kotlinx.serialization.modules/SerializersModuleBuilder.include|include(kotlinx.serialization.modules.SerializersModule){}[0]
 }
 
+final class kotlinx.serialization/DuplicateKeyException : kotlinx.serialization/SerializationException { // kotlinx.serialization/DuplicateKeyException|null[0]
+    constructor <init>(kotlin/Any?) // kotlinx.serialization/DuplicateKeyException.<init>|<init>(kotlin.Any?){}[0]
+}
+
 final class kotlinx.serialization/MissingFieldException : kotlinx.serialization/SerializationException { // kotlinx.serialization/MissingFieldException|null[0]
     constructor <init>(kotlin.collections/List<kotlin/String>, kotlin/String) // kotlinx.serialization/MissingFieldException.<init>|<init>(kotlin.collections.List<kotlin.String>;kotlin.String){}[0]
     constructor <init>(kotlin.collections/List<kotlin/String>, kotlin/String?, kotlin/Throwable?) // kotlinx.serialization/MissingFieldException.<init>|<init>(kotlin.collections.List<kotlin.String>;kotlin.String?;kotlin.Throwable?){}[0]

diff --git a/core/commonMain/src/kotlinx/serialization/SerializationExceptions.kt b/core/commonMain/src/kotlinx/serialization/SerializationExceptions.kt
@@ -135,3 +135,10 @@ internal constructor(message: String?) : SerializationException(message) {
     // This constructor is used by the generated serializers
     constructor(index: Int) : this("An unknown field for index $index")
 }
+
+/**
+ * Thrown when a deserializer encounters a repeated key (and configuration disallows this.)
+ */
+@ExperimentalSerializationApi
+public class DuplicateKeyException(key: Any?) :
+    SerializationException("Duplicate keys not allowed. Key appeared twice: $key")
diff --git a/core/commonMain/src/kotlinx/serialization/encoding/Decoding.kt b/core/commonMain/src/kotlinx/serialization/encoding/Decoding.kt
@@ -559,6 +559,18 @@ public interface CompositeDecoder {
         deserializer: DeserializationStrategy<T?>,
         previousValue: T? = null
     ): T?
+
+    /**
+     * Called after a key has been read.
+     *
+     * This could be a map or set key, or anything otherwise intended to be
+     * distinct within the collection under normal circumstances.
+     *
+     * Implementations might use this as a hook for throwing an exception when
+     * duplicate keys are encountered.
+     */
+    @ExperimentalSerializationApi
+    public fun visitKey(key: Any?) { }
 }
 
 /**

diff --git a/core/commonMain/src/kotlinx/serialization/internal/CollectionSerializers.kt b/core/commonMain/src/kotlinx/serialization/internal/CollectionSerializers.kt
@@ -98,6 +98,7 @@ public sealed class MapLikeSerializer<Key, Value, Collection, Builder : MutableM
 
     final override fun readElement(decoder: CompositeDecoder, index: Int, builder: Builder, checkIndex: Boolean) {
         val key: Key = decoder.decodeSerializableElement(descriptor, index, keySerializer)
+        decoder.visitKey(key)
         val vIndex = if (checkIndex) {
             decoder.decodeElementIndex(descriptor).also {
                 require(it == index + 1) { "Value must follow key in a map, index for key: $index, returned index for value: $it" }

diff --git a/formats/cbor/api/kotlinx-serialization-cbor.api b/formats/cbor/api/kotlinx-serialization-cbor.api
@@ -31,6 +31,7 @@ public final class kotlinx/serialization/cbor/CborBuilder {
 	public final fun getEncodeKeyTags ()Z
 	public final fun getEncodeObjectTags ()Z
 	public final fun getEncodeValueTags ()Z
+	public final fun getForbidDuplicateKeys ()Z
 	public final fun getIgnoreUnknownKeys ()Z
 	public final fun getPreferCborLabelsOverNames ()Z
 	public final fun getSerializersModule ()Lkotlinx/serialization/modules/SerializersModule;
@@ -43,6 +44,7 @@ public final class kotlinx/serialization/cbor/CborBuilder {
 	public final fun setEncodeKeyTags (Z)V
 	public final fun setEncodeObjectTags (Z)V
 	public final fun setEncodeValueTags (Z)V
+	public final fun setForbidDuplicateKeys (Z)V
 	public final fun setIgnoreUnknownKeys (Z)V
 	public final fun setPreferCborLabelsOverNames (Z)V
 	public final fun setSerializersModule (Lkotlinx/serialization/modules/SerializersModule;)V
@@ -58,6 +60,7 @@ public final class kotlinx/serialization/cbor/CborConfiguration {
 	public final fun getEncodeKeyTags ()Z
 	public final fun getEncodeObjectTags ()Z
 	public final fun getEncodeValueTags ()Z
+	public final fun getForbidDuplicateKeys ()Z
 	public final fun getIgnoreUnknownKeys ()Z
 	public final fun getPreferCborLabelsOverNames ()Z
 	public final fun getUseDefiniteLengthEncoding ()Z

diff --git a/formats/cbor/api/kotlinx-serialization-cbor.klib.api b/formats/cbor/api/kotlinx-serialization-cbor.klib.api
@@ -68,6 +68,9 @@ final class kotlinx.serialization.cbor/CborBuilder { // kotlinx.serialization.cb
     final var encodeValueTags // kotlinx.serialization.cbor/CborBuilder.encodeValueTags|{}encodeValueTags[0]
         final fun <get-encodeValueTags>(): kotlin/Boolean // kotlinx.serialization.cbor/CborBuilder.encodeValueTags.<get-encodeValueTags>|<get-encodeValueTags>(){}[0]
         final fun <set-encodeValueTags>(kotlin/Boolean) // kotlinx.serialization.cbor/CborBuilder.encodeValueTags.<set-encodeValueTags>|<set-encodeValueTags>(kotlin.Boolean){}[0]
+    final var forbidDuplicateKeys // kotlinx.serialization.cbor/CborBuilder.forbidDuplicateKeys|{}forbidDuplicateKeys[0]
+        final fun <get-forbidDuplicateKeys>(): kotlin/Boolean // kotlinx.serialization.cbor/CborBuilder.forbidDuplicateKeys.<get-forbidDuplicateKeys>|<get-forbidDuplicateKeys>(){}[0]
+        final fun <set-forbidDuplicateKeys>(kotlin/Boolean) // kotlinx.serialization.cbor/CborBuilder.forbidDuplicateKeys.<set-forbidDuplicateKeys>|<set-forbidDuplicateKeys>(kotlin.Boolean){}[0]
     final var ignoreUnknownKeys // kotlinx.serialization.cbor/CborBuilder.ignoreUnknownKeys|{}ignoreUnknownKeys[0]
         final fun <get-ignoreUnknownKeys>(): kotlin/Boolean // kotlinx.serialization.cbor/CborBuilder.ignoreUnknownKeys.<get-ignoreUnknownKeys>|<get-ignoreUnknownKeys>(){}[0]
         final fun <set-ignoreUnknownKeys>(kotlin/Boolean) // kotlinx.serialization.cbor/CborBuilder.ignoreUnknownKeys.<set-ignoreUnknownKeys>|<set-ignoreUnknownKeys>(kotlin.Boolean){}[0]
@@ -102,6 +105,8 @@ final class kotlinx.serialization.cbor/CborConfiguration { // kotlinx.serializat
         final fun <get-encodeObjectTags>(): kotlin/Boolean // kotlinx.serialization.cbor/CborConfiguration.encodeObjectTags.<get-encodeObjectTags>|<get-encodeObjectTags>(){}[0]
     final val encodeValueTags // kotlinx.serialization.cbor/CborConfiguration.encodeValueTags|{}encodeValueTags[0]
         final fun <get-encodeValueTags>(): kotlin/Boolean // kotlinx.serialization.cbor/CborConfiguration.encodeValueTags.<get-encodeValueTags>|<get-encodeValueTags>(){}[0]
+    final val forbidDuplicateKeys // kotlinx.serialization.cbor/CborConfiguration.forbidDuplicateKeys|{}forbidDuplicateKeys[0]
+        final fun <get-forbidDuplicateKeys>(): kotlin/Boolean // kotlinx.serialization.cbor/CborConfiguration.forbidDuplicateKeys.<get-forbidDuplicateKeys>|<get-forbidDuplicateKeys>(){}[0]
     final val ignoreUnknownKeys // kotlinx.serialization.cbor/CborConfiguration.ignoreUnknownKeys|{}ignoreUnknownKeys[0]
         final fun <get-ignoreUnknownKeys>(): kotlin/Boolean // kotlinx.serialization.cbor/CborConfiguration.ignoreUnknownKeys.<get-ignoreUnknownKeys>|<get-ignoreUnknownKeys>(){}[0]
     final val preferCborLabelsOverNames // kotlinx.serialization.cbor/CborConfiguration.preferCborLabelsOverNames|{}preferCborLabelsOverNames[0]

diff --git a/formats/cbor/commonMain/src/kotlinx/serialization/cbor/Cbor.kt b/formats/cbor/commonMain/src/kotlinx/serialization/cbor/Cbor.kt
@@ -27,7 +27,9 @@ public sealed class Cbor(
 ) : BinaryFormat {
 
     /**
-     * The default instance of [Cbor]. Neither writes nor verifies tags. Uses indefinite length encoding by default.
+     * The default instance of [Cbor]. Neither writes nor verifies tags,
+     * and does not forbid reading duplicate map keys.
+     * Uses indefinite length encoding by default.
      */
     public companion object Default :
         Cbor(
@@ -42,13 +44,15 @@ public sealed class Cbor(
                 verifyObjectTags = false,
                 useDefiniteLengthEncoding = false,
                 preferCborLabelsOverNames = false,
-                alwaysUseByteString = false
+                alwaysUseByteString = false,
+                forbidDuplicateKeys = false,
             ), EmptySerializersModule()
         ) {
 
         /**
          * Preconfigured instance of [Cbor] for COSE compliance. Encodes and verifies all tags, uses definite length
-         * encoding and prefers labels to serial names. **DOES NOT** sort CBOR map keys; declare them in canonical order
+         * encoding, prefers labels to serial names, and forbids reading duplicate map keys.
+         * **DOES NOT** sort CBOR map keys; declare them in canonical order
          * for full cbor compliance!
          */
         public val CoseCompliant: Cbor =
@@ -64,6 +68,7 @@ public sealed class Cbor(
                 useDefiniteLengthEncoding = true
                 preferCborLabelsOverNames = true
                 alwaysUseByteString = false
+                forbidDuplicateKeys = true
                 serializersModule = EmptySerializersModule()
             }
     }
@@ -119,7 +124,8 @@ public fun Cbor(from: Cbor = Cbor, builderAction: CborBuilder.() -> Unit): Cbor
         builder.verifyObjectTags,
         builder.useDefiniteLengthEncoding,
         builder.preferCborLabelsOverNames,
-        builder.alwaysUseByteString),
+        builder.alwaysUseByteString,
+        builder.forbidDuplicateKeys),
         builder.serializersModule
     )
 }
@@ -243,6 +249,14 @@ public class CborBuilder internal constructor(cbor: Cbor) {
      */
     public var alwaysUseByteString: Boolean = cbor.configuration.alwaysUseByteString
 
+    /**
+     * Specifies whether it is an error to read a map with duplicate keys.
+     *
+     * If this is set to true, decoding a map with two keys that compare as equal
+     * will cause a [DuplicateKeyException] error to be thrown.
+     */
+    public var forbidDuplicateKeys: Boolean = cbor.configuration.forbidDuplicateKeys
+
     /**
      * Module with contextual and polymorphic serializers to be used in the resulting [Cbor] instance.
      */

diff --git a/formats/cbor/commonMain/src/kotlinx/serialization/cbor/CborConfiguration.kt b/formats/cbor/commonMain/src/kotlinx/serialization/cbor/CborConfiguration.kt
@@ -89,6 +89,9 @@ import kotlinx.serialization.*
  * basis. The [alwaysUseByteString] configuration switch allows for globally preferring **major type 2** without needing
  * to annotate every `ByteArray` in a class hierarchy.
  *
+ * @param forbidDuplicateKeys Specifies whether it is an error to read a map with duplicate keys.
+ * If this is set to true, decoding a map with two keys that compare as equal
+ * will cause a [DuplicateKeyException] error to be thrown.
  */
 @ExperimentalSerializationApi
 public class CborConfiguration internal constructor(
@@ -103,12 +106,14 @@ public class CborConfiguration internal constructor(
     public val useDefiniteLengthEncoding: Boolean,
     public val preferCborLabelsOverNames: Boolean,
     public val alwaysUseByteString: Boolean,
+    public val forbidDuplicateKeys: Boolean,
 ) {
     override fun toString(): String {
         return "CborConfiguration(encodeDefaults=$encodeDefaults, ignoreUnknownKeys=$ignoreUnknownKeys, " +
             "encodeKeyTags=$encodeKeyTags, encodeValueTags=$encodeValueTags, encodeObjectTags=$encodeObjectTags, " +
             "verifyKeyTags=$verifyKeyTags, verifyValueTags=$verifyValueTags, verifyObjectTags=$verifyObjectTags, " +
             "useDefiniteLengthEncoding=$useDefiniteLengthEncoding, " +
-            "preferCborLabelsOverNames=$preferCborLabelsOverNames, alwaysUseByteString=$alwaysUseByteString)"
+            "preferCborLabelsOverNames=$preferCborLabelsOverNames, alwaysUseByteString=$alwaysUseByteString, " +
+            "forbidDuplicateKeys=$forbidDuplicateKeys)"
     }
-}
+}
diff --git a/formats/cbor/commonMain/src/kotlinx/serialization/cbor/internal/Decoder.kt b/formats/cbor/commonMain/src/kotlinx/serialization/cbor/internal/Decoder.kt
@@ -24,6 +24,13 @@ internal open class CborReader(override val cbor: Cbor, protected val parser: Cb
     protected var decodeByteArrayAsByteString = false
     protected var tags: ULongArray? = null
 
+    /**
+     * Keys that have been seen so far while reading this map.
+     *
+     * Only used if [Cbor.configuration.forbidDuplicateKeys] is in effect.
+     */
+    private val seenKeys = mutableSetOf<Any?>()
+
     protected fun setSize(size: Int) {
         if (size >= 0) {
             finiteMode = true
@@ -54,12 +61,19 @@ internal open class CborReader(override val cbor: Cbor, protected val parser: Cb
         if (!finiteMode) parser.end()
     }
 
+    override fun visitKey(key: Any?) {
+        if (cbor.configuration.forbidDuplicateKeys) {
+            seenKeys.add(key) || throw DuplicateKeyException(key)
+        }
+    }
+
     override fun decodeElementIndex(descriptor: SerialDescriptor): Int {
         val index = if (cbor.configuration.ignoreUnknownKeys) {
             val knownIndex: Int
             while (true) {
                 if (isDone()) return CompositeDecoder.DECODE_DONE
                 val (elemName, tags) = decodeElementNameWithTagsLenient(descriptor)
+                visitKey(elemName)
                 readProperties++
 
                 val index = elemName?.let { descriptor.getElementIndex(it) } ?: CompositeDecoder.UNKNOWN_NAME
@@ -75,6 +89,7 @@ internal open class CborReader(override val cbor: Cbor, protected val parser: Cb
         } else {
             if (isDone()) return CompositeDecoder.DECODE_DONE
             val (elemName, tags) = decodeElementNameWithTags(descriptor)
+            visitKey(elemName)
             readProperties++
             descriptor.getElementIndexOrThrow(elemName).also { index ->
                 verifyKeyTags(descriptor, index, tags)

diff --git a/formats/cbor/commonTest/src/kotlinx/serialization/cbor/CborStrictModeTest.kt b/formats/cbor/commonTest/src/kotlinx/serialization/cbor/CborStrictModeTest.kt
@@ -0,0 +1,45 @@
+package kotlinx.serialization.cbor
+
+import kotlinx.serialization.assertFailsWithMessage
+import kotlinx.serialization.decodeFromByteArray
+import kotlinx.serialization.HexConverter
+import kotlinx.serialization.DuplicateKeyException
+import kotlinx.serialization.Serializable
+import kotlin.test.Test
+
+class CborStrictModeTest {
+    private val strict = Cbor { forbidDuplicateKeys = true }
+
+    /** Duplicate keys are rejected in generic maps. */
+    @Test
+    fun testDuplicateKeysInMap() {
+        val duplicateKeys = HexConverter.parseHexBinary("A2617805617806")
+        assertFailsWithMessage<DuplicateKeyException>("Duplicate keys not allowed. Key appeared twice: x") {
+            strict.decodeFromByteArray<Map<String, Long>>(duplicateKeys)
+        }
+    }
+
+    @Serializable
+    data class ExampleClass(val x: Long)
+
+    /** Duplicate keys are rejected in classes. */
+    @Test
+    fun testDuplicateKeysInDataClass() {
+        // {"x": 5, "x", 6}
+        val duplicateKeys = HexConverter.parseHexBinary("A2617805617806")
+        assertFailsWithMessage<DuplicateKeyException>("Duplicate keys not allowed. Key appeared twice: x") {
+            strict.decodeFromByteArray<ExampleClass>(duplicateKeys)
+        }
+    }
+
+    /** Duplicate unknown keys are rejected as well. */
+    @Test
+    fun testDuplicateUnknownKeys() {
+        // {"a": 1, "a", 2, "x", 6}
+        val duplicateKeys = HexConverter.parseHexBinary("A3616101616102617806")
+        val cbor = Cbor(strict) { ignoreUnknownKeys = true }
+        assertFailsWithMessage<DuplicateKeyException>("Duplicate keys not allowed. Key appeared twice: a") {
+            cbor.decodeFromByteArray<ExampleClass>(duplicateKeys)
+        }
+    }
+}
diff --git a/formats/json/api/kotlinx-serialization-json.api b/formats/json/api/kotlinx-serialization-json.api
@@ -182,6 +182,7 @@ public final class kotlinx/serialization/json/JsonDecoder$DefaultImpls {
 	public static fun decodeNullableSerializableValue (Lkotlinx/serialization/json/JsonDecoder;Lkotlinx/serialization/DeserializationStrategy;)Ljava/lang/Object;
 	public static fun decodeSequentially (Lkotlinx/serialization/json/JsonDecoder;)Z
 	public static fun decodeSerializableValue (Lkotlinx/serialization/json/JsonDecoder;Lkotlinx/serialization/DeserializationStrategy;)Ljava/lang/Object;
+	public static fun visitKey (Lkotlinx/serialization/json/JsonDecoder;Ljava/lang/Object;)V
 }
 
 public abstract class kotlinx/serialization/json/JsonElement {