feat(json_family): Add json_nesting_depth_limit flag (#4444)

* feat: Add json_nesting_depth_limit flag Signed-off-by: Stepan Bagritsevich <[email protected]> * refactor: address comments Signed-off-by: Stepan Bagritsevich <[email protected]> --------- Signed-off-by: Stepan Bagritsevich <[email protected]>
dragonflydb · Jan 20, 2025 · 5d3e314 · 5d3e314
1 parent 99c4ab8
commit 5d3e314
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 5 deletions.
diff --git a/src/core/json/json_object.cc b/src/core/json/json_object.cc
@@ -18,8 +18,16 @@ optional<JsonType> JsonFromString(string_view input, PMR_NS::memory_resource* mr
     return false;
   };
 
+  // The maximum allowed JSON nesting depth is 256.
+  const uint32_t json_nesting_depth_limit = 256;
+
+  /* The maximum possible JSON nesting depth is either the specified json_nesting_depth_limit or
+     half of the input size. Since nesting a JSON object requires at least 2 characters. */
+  auto parser_options = jsoncons::json_options{}.max_nesting_depth(
+      std::min(json_nesting_depth_limit, uint32_t(input.size() / 2)));
+
   json_decoder<JsonType> decoder(std::pmr::polymorphic_allocator<char>{mr});
-  json_parser parser(basic_json_decode_options<char>{}, JsonErrorHandler);
+  json_parser parser(parser_options, JsonErrorHandler);
 
   parser.update(input);
   parser.finish_parse(decoder, ec);

diff --git a/src/facade/error.h b/src/facade/error.h
@@ -36,6 +36,7 @@ extern const char kLoadingErr[];
 extern const char kUndeclaredKeyErr[];
 extern const char kInvalidDumpValueErr[];
 extern const char kInvalidJsonPathErr[];
+extern const char kJsonParseError[];
 
 extern const char kSyntaxErrType[];
 extern const char kScriptErrType[];

diff --git a/src/facade/facade.cc b/src/facade/facade.cc
@@ -97,6 +97,7 @@ const char kLoadingErr[] = "-LOADING Dragonfly is loading the dataset in memory"
 const char kUndeclaredKeyErr[] = "script tried accessing undeclared key";
 const char kInvalidDumpValueErr[] = "DUMP payload version or checksum are wrong";
 const char kInvalidJsonPathErr[] = "invalid JSON path";
+const char kJsonParseError[] = "failed to parse JSON";
 
 const char kSyntaxErrType[] = "syntax_error";
 const char kScriptErrType[] = "script_error";

diff --git a/src/facade/op_status.cc b/src/facade/op_status.cc
@@ -36,6 +36,8 @@ std::string_view StatusToMsg(OpStatus status) {
       return kKeyNotFoundErr;
     case OpStatus::INVALID_JSON_PATH:
       return kInvalidJsonPathErr;
+    case OpStatus::INVALID_JSON:
+      return kJsonParseError;
     default:
       LOG(ERROR) << "Unsupported status " << status;
       return "Internal error";

diff --git a/src/facade/op_status.h b/src/facade/op_status.h
@@ -30,7 +30,8 @@ enum class OpStatus : uint16_t {
   CANCELLED,
   AT_LEAST_ONE_KEY,
   MEMBER_NOTFOUND,
-  INVALID_JSON_PATH
+  INVALID_JSON_PATH,
+  INVALID_JSON,
 };
 
 class OpResultBase {

diff --git a/src/server/json_family.cc b/src/server/json_family.cc
@@ -343,7 +343,7 @@ OpResult<DbSlice::AddOrFindResult> SetJson(const OpArgs& op_args, string_view ke
   std::optional<JsonType> parsed_json = ShardJsonFromString(json_str);
   if (!parsed_json) {
     VLOG(1) << "got invalid JSON string '" << json_str << "' cannot be saved";
-    return OpStatus::INVALID_VALUE;
+    return OpStatus::INVALID_JSON;
   }
 
   if (JsonEnconding() == kEncodingJsonFlat) {
@@ -1345,7 +1345,7 @@ OpResult<bool> OpSet(const OpArgs& op_args, string_view key, string_view path,
   optional<JsonType> parsed_json = ShardJsonFromString(json_str);
   if (!parsed_json) {
     VLOG(1) << "got invalid JSON string '" << json_str << "' cannot be saved";
-    return OpStatus::INVALID_VALUE;
+    return OpStatus::INVALID_JSON;
   }
   const JsonType& new_json = parsed_json.value();
 
@@ -1444,7 +1444,7 @@ OpStatus OpMerge(const OpArgs& op_args, string_view key, string_view path,
   std::optional<JsonType> parsed_json = ShardJsonFromString(json_str);
   if (!parsed_json) {
     VLOG(1) << "got invalid JSON string '" << json_str << "' cannot be saved";
-    return OpStatus::SYNTAX_ERR;
+    return OpStatus::INVALID_JSON;
   }
 
   auto cb = [&](std::optional<std::string_view> cur_path, JsonType* val) -> MutateCallbackResult<> {

diff --git a/src/server/json_family_test.cc b/src/server/json_family_test.cc
@@ -3017,4 +3017,33 @@ TEST_F(JsonFamilyTest, GetString) {
   EXPECT_THAT(resp, ErrArg("WRONGTYPE"));
 }
 
+TEST_F(JsonFamilyTest, MaxNestingJsonDepth) {
+  auto generate_nested_json = [](int depth) -> std::string {
+    std::string json = "{";
+    for (int i = 0; i < depth - 1; ++i) {
+      json += R"("key": {)";
+    }
+    json += R"("key": "value")";  // Innermost value
+    for (int i = 0; i < depth - 1; ++i) {
+      json += "}";
+    }
+    json += "}";
+    return json;
+  };
+
+  // Generate JSON with maximum allowed depth (256)
+  /* std::string valid_json = generate_nested_json(255);
+
+  // Test with valid JSON at depth 256
+  auto resp = Run({"JSON.SET", "valid_json",  ".", valid_json});
+  EXPECT_THAT(resp, "OK"); */
+
+  // Generate JSON exceeding maximum depth (257)
+  std::string invalid_json = generate_nested_json(257);
+
+  // Test with invalid JSON at depth 257
+  auto resp = Run({"JSON.SET", "invalid_json", ".", invalid_json});
+  EXPECT_THAT(resp, ErrArg("failed to parse JSON"));
+}
+
 }  // namespace dfly