Deprecate duplicate keys in object

There are few legitimate use cases for duplicate keys, and can
in some case be exploited.

Rather to always silently accept them, we should emit a warning,
and in the future require to explictly allow them.

Author: byroot

ext/json/ext/parser/parser.c

@@ -35,7 +35,7 @@ static ID i_chr, i_aset, i_aref,
           i_leftshift, i_new, i_try_convert, i_uminus, i_encode;
 
 static VALUE sym_max_nesting, sym_allow_nan, sym_allow_trailing_comma, sym_symbolize_names, sym_freeze,
-             sym_decimal_class, sym_on_load;
+             sym_decimal_class, sym_on_load, sym_allow_duplicate_keys;
 
 static int binary_encindex;
 static int utf8_encindex;
@@ -363,10 +363,17 @@ static int convert_UTF32_to_UTF8(char *buf, uint32_t ch)
     return len;
 }
 
+enum duplicate_key_action {
+    DEPRECATED = 0,
+    IGNORE,
+    RAISE,
+};
+
 typedef struct JSON_ParserStruct {
     VALUE on_load_proc;
     VALUE decimal_class;
     ID decimal_method_id;
+    enum duplicate_key_action on_duplicate_key;
     int max_nesting;
     bool allow_nan;
     bool allow_trailing_comma;
@@ -386,15 +393,8 @@ typedef struct JSON_ParserStateStruct {
     int current_nesting;
 } JSON_ParserState;
 
-
-#define PARSE_ERROR_FRAGMENT_LEN 32
-#ifdef RBIMPL_ATTR_NORETURN
-RBIMPL_ATTR_NORETURN()
-#endif
-static void raise_parse_error(const char *format, JSON_ParserState *state)
+static void cursor_position(JSON_ParserState *state, long *line_out, long *column_out)
 {
-    unsigned char buffer[PARSE_ERROR_FRAGMENT_LEN + 3];
-
     const char *cursor = state->cursor;
     long column = 0;
     long line = 1;
@@ -411,6 +411,27 @@ static void raise_parse_error(const char *format, JSON_ParserState *state)
             line++;
         }
     }
+    *line_out = line;
+    *column_out = column;
+}
+
+static void emit_parse_warning(const char *message, JSON_ParserState *state)
+{
+    long line, column;
+    cursor_position(state, &line, &column);
+
+    rb_warn("%s at line %ld column %ld", message, line, column);
+}
+
+#define PARSE_ERROR_FRAGMENT_LEN 32
+#ifdef RBIMPL_ATTR_NORETURN
+RBIMPL_ATTR_NORETURN()
+#endif
+static void raise_parse_error(const char *format, JSON_ParserState *state)
+{
+    unsigned char buffer[PARSE_ERROR_FRAGMENT_LEN + 3];
+    long line, column;
+    cursor_position(state, &line, &column);
 
     const char *ptr = "EOF";
     if (state->cursor && state->cursor < state->end) {
@@ -807,11 +828,25 @@ static inline VALUE json_decode_array(JSON_ParserState *state, JSON_ParserConfig
     return array;
 }
 
-static inline VALUE json_decode_object(JSON_ParserState *state, JSON_ParserConfig *config, long count)
+static inline VALUE json_decode_object(JSON_ParserState *state, JSON_ParserConfig *config, size_t count)
 {
-    VALUE object = rb_hash_new_capa(count);
+    size_t entries_count = count / 2;
+    VALUE object = rb_hash_new_capa(entries_count);
     rb_hash_bulk_insert(count, rvalue_stack_peek(state->stack, count), object);
 
+    if (RB_UNLIKELY(RHASH_SIZE(object) < entries_count)) {
+        switch (config->on_duplicate_key) {
+            case IGNORE:
+                break;
+            case DEPRECATED:
+                emit_parse_warning("detected duplicate keys in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_keys: true`", state);
+                break;
+            case RAISE:
+                raise_parse_error("duplicate key", state);
+                break;
+        }
+    }
+
     rvalue_stack_pop(state->stack, count);
 
     if (config->freeze) {
@@ -1060,6 +1095,8 @@ static VALUE json_parse_any(JSON_ParserState *state, JSON_ParserConfig *config)
             break;
         }
         case '{': {
+            const char *object_start_cursor = state->cursor;
+
             state->cursor++;
             json_eat_whitespace(state);
             long stack_head = state->stack->head;
@@ -1094,8 +1131,15 @@ static VALUE json_parse_any(JSON_ParserState *state, JSON_ParserConfig *config)
                     if (*state->cursor == '}') {
                         state->cursor++;
                         state->current_nesting--;
-                        long count = state->stack->head - stack_head;
-                        return json_push_value(state, config, json_decode_object(state, config, count));
+                        size_t count = state->stack->head - stack_head;
+
+                        // Temporary rewind cursor in case an error is raised
+                        const char *final_cursor = state->cursor;
+                        state->cursor = object_start_cursor;
+                        VALUE object = json_decode_object(state, config, count);
+                        state->cursor = final_cursor;
+
+                        return json_push_value(state, config, object);
                     }
 
                     if (*state->cursor == ',') {
@@ -1184,6 +1228,7 @@ static int parser_config_init_i(VALUE key, VALUE val, VALUE data)
     else if (key == sym_symbolize_names)      { config->symbolize_names = RTEST(val); }
     else if (key == sym_freeze)               { config->freeze = RTEST(val); }
     else if (key == sym_on_load)              { config->on_load_proc = RTEST(val) ? val : Qfalse; }
+    else if (key == sym_allow_duplicate_keys) { config->on_duplicate_key = RTEST(val) ? IGNORE : RAISE; }
     else if (key == sym_decimal_class)        {
         if (RTEST(val)) {
             if (rb_respond_to(val, i_try_convert)) {
@@ -1400,6 +1445,7 @@ void Init_parser(void)
     sym_freeze = ID2SYM(rb_intern("freeze"));
     sym_on_load = ID2SYM(rb_intern("on_load"));
     sym_decimal_class = ID2SYM(rb_intern("decimal_class"));
+    sym_allow_duplicate_keys = ID2SYM(rb_intern("allow_duplicate_keys"));
 
     i_chr = rb_intern("chr");
     i_aset = rb_intern("[]=");

lib/json.rb

@@ -127,6 +127,24 @@
 #
 # ---
 #
+# Option +allow_duplicate_keys+ specifies whether duplicate keys in objects
+# should be ignored or cause an error to be raised:
+#
+# When not specified:
+#   # The last value is used and a deprecation warning emitted.
+#   JSON.parse('{"a": 1, "a":2}') => {"a" => 2}
+#   # waring: detected duplicate keys in JSON object.
+#   # This will raise an error in json 3.0 unless enabled via `allow_duplicate_keys: true`
+#
+# When set to `+true+`
+#   # The last value is used.
+#   JSON.parse('{"a": 1, "a":2}') => {"a" => 2}
+#
+# When set to `+false+`, the future default:
+#   JSON.parse('{"a": 1, "a":2}') => duplicate key at line 1 column 1 (JSON::ParserError)
+#
+# ---
+#
 # Option +allow_nan+ (boolean) specifies whether to allow
 # NaN, Infinity, and MinusInfinity in +source+;
 # defaults to +false+.

test/json/json_parser_test.rb

@@ -331,6 +331,15 @@ def test_parse_big_integers
     assert_equal orig, parse(json5)
   end
 
+  def test_parse_duplicate_keys
+    expected = {"a" => 2}
+    assert_equal expected, parse('{"a": 1, "a": 2}', allow_duplicate_keys: true)
+    assert_raise(ParserError) { parse('{"a": 1, "a": 2}', allow_duplicate_keys: false) }
+    assert_deprecated_warning(/duplicate keys/) do
+      assert_equal expected, parse('{"a": 1, "a": 2}')
+    end
+  end
+
   def test_some_wrong_inputs
     assert_raise(ParserError) { parse('[] bla') }
     assert_raise(ParserError) { parse('[] 1') }