Merge pull request #946 from dxbjavid/patch-null-field-fields

reject null op/path/from fields in json_patch_apply

json_patch.c

@@ -208,7 +208,11 @@ static int json_patch_apply_move_copy(struct json_object **res,
 		return -1;
 	}
 
-	from_s = json_object_get_string(jfrom);
+	from_s = json_object_get_type(jfrom) == json_type_string ? json_object_get_string(jfrom) : NULL;
+	if (from_s == NULL) {
+		_set_err(EINVAL, "Patch object 'from' field is not a string");
+		return -1;
+	}
 
 	from_s_len = strlen(from_s);
 	if (strncmp(from_s, path, from_s_len) == 0) {
@@ -319,12 +323,22 @@ int json_patch_apply(struct json_object *copy_from, struct json_object *patch,
 			_set_err(EINVAL, "Patch object does not contain 'op' field");
 			return -1;
 		}
-		op = json_object_get_string(jop);
+		op = json_object_get_type(jop) == json_type_string ? json_object_get_string(jop) : NULL;
+		if (op == NULL) {
+			_set_err(EINVAL, "Patch object 'op' field is not a string");
+			return -1;
+		}
 		if (!json_object_object_get_ex(patch_elem, "path", &jpath)) {
 			_set_err(EINVAL, "Patch object does not contain 'path' field");
 			return -1;
 		}
-		path = json_object_get_string(jpath); // Note: empty string is ok!
+		// Note: empty string is ok!
+		path = json_object_get_type(jpath) == json_type_string ? json_object_get_string(jpath)
+		                                                       : NULL;
+		if (path == NULL) {
+			_set_err(EINVAL, "Patch object 'path' field is not a string");
+			return -1;
+		}
 
 		if (!strcmp(op, "test"))
 			rc = json_patch_apply_test(base, patch_elem, path, patch_error);

tests/json_patch_tests.json

@@ -542,6 +542,24 @@
        "patch": [{"op": "copy", "from": "/a", "path": "/b"},
                  {"op": "copy", "from": "/b", "path": "/a/p/x"}],
        "expected": {"a": {"p": {"x": {"p": {}}}}, "b": {"p": {}}}
+    },
+
+    { "comment": "null op field must be rejected, not dereferenced",
+       "doc": {"foo": "bar"},
+       "patch": [{"op": null, "path": "/foo"}],
+       "error": "a null op field should fail rather than crash"
+    },
+
+    { "comment": "null from field must be rejected, not dereferenced",
+       "doc": {"foo": "bar"},
+       "patch": [{"op": "move", "from": null, "path": "/foo"}],
+       "error": "a null from field should fail rather than crash"
+    },
+
+    { "comment": "null path field must be rejected, not dereferenced",
+       "doc": {"foo": "bar"},
+       "patch": [{"op": "remove", "path": null}],
+       "error": "a null path field should fail rather than crash"
     }
 
 ]

tests/test_json_patch.expected

@@ -123,7 +123,7 @@ Testing 'test add with bad number should fail', doc '[ "foo", "sil" ]' patch '[
 Testing 'missing 'path' parameter', doc '{ }' patch '[ { "op": "add", "value": "bar" } ]' : OK
  => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 0: Patch object does not contain 'path' field
 Testing ''path' parameter with null value', doc '{ }' patch '[ { "op": "add", "path": null, "value": "bar" } ]' : OK
- => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 0: Failed to set value at path referenced by 'path' field
+ => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 0: Patch object 'path' field is not a string
 Testing 'invalid JSON Pointer token', doc '{ }' patch '[ { "op": "add", "path": "foo", "value": "bar" } ]' : OK
  => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 0: Failed to set value at path referenced by 'path' field
 Testing 'missing 'value' parameter to add', doc '[ 1 ]' patch '[ { "op": "add", "path": "\/-" } ]' : OK
@@ -157,3 +157,9 @@ Testing 'Removing nonexistent index', doc '[ "foo", "bar" ]' patch '[ { "op": "r
  => json_patch_apply failed as expected: ERRNO=ENOENT at patch idx 0: Did not find element referenced by path field
 Testing 'Patch with different capitalisation than doc', doc '{ "foo": "bar" }' patch '[ { "op": "add", "path": "\/FOO", "value": "BAR" } ]' : OK
 Testing 'copy must duplicate the value, not alias it, so no cycle can form', doc '{ "a": { "p": { } } }' patch '[ { "op": "copy", "from": "\/a", "path": "\/b" }, { "op": "copy", "from": "\/b", "path": "\/a\/p\/x" } ]' : OK
+Testing 'null op field must be rejected, not dereferenced', doc '{ "foo": "bar" }' patch '[ { "op": null, "path": "\/foo" } ]' : OK
+ => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 0: Patch object 'op' field is not a string
+Testing 'null from field must be rejected, not dereferenced', doc '{ "foo": "bar" }' patch '[ { "op": "move", "from": null, "path": "\/foo" } ]' : OK
+ => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 0: Patch object 'from' field is not a string
+Testing 'null path field must be rejected, not dereferenced', doc '{ "foo": "bar" }' patch '[ { "op": "remove", "path": null } ]' : OK
+ => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 0: Patch object 'path' field is not a string