Merge pull request #180 from radwaretaltr/master

Pre-flight DecodePatch validation: Issue #177

Author: evanphx

v5/patch.go

@@ -454,7 +454,11 @@ func (o Operation) value() *lazyNode {
 
 // ValueInterface decodes the operation value into an interface.
 func (o Operation) ValueInterface() (interface{}, error) {
-	if obj, ok := o["value"]; ok && obj != nil {
+	if obj, ok := o["value"]; ok {
+		if obj == nil {
+			return nil, nil
+		}
+
 		var v interface{}
 
 		err := json.Unmarshal(*obj, &v)
@@ -816,6 +820,43 @@ func ensurePathExists(pd *container, path string, options *ApplyOptions) error {
 	return nil
 }
 
+func validateOperation(op Operation) error {
+	switch op.Kind() {
+	case "add", "replace":
+		if _, err := op.ValueInterface(); err != nil {
+			return errors.Wrapf(err, "failed to decode 'value'")
+		}
+	case "move", "copy":
+		if _, err := op.From(); err != nil {
+			return errors.Wrapf(err, "failed to decode 'from'")
+		}
+	case "remove", "test":
+	default:
+		return fmt.Errorf("unsupported operation")
+	}
+
+	if _, err := op.Path(); err != nil {
+		return errors.Wrapf(err, "failed to decode 'path'")
+	}
+
+	return nil
+}
+
+func validatePatch(p Patch) error {
+	for _, op := range p {
+		if err := validateOperation(op); err != nil {
+			opData, infoErr := json.Marshal(op)
+			if infoErr != nil {
+				return errors.Wrapf(err, "invalid operation")
+			}
+
+			return errors.Wrapf(err, "invalid operation %s", opData)
+		}
+	}
+
+	return nil
+}
+
 func (p Patch) remove(doc *container, op Operation, options *ApplyOptions) error {
 	path, err := op.Path()
 	if err != nil {
@@ -1044,6 +1085,10 @@ func DecodePatch(buf []byte) (Patch, error) {
 		return nil, err
 	}
 
+	if err := validatePatch(p); err != nil {
+		return nil, err
+	}
+
 	return p, nil
 }
 

v5/patch_test.go

@@ -31,7 +31,7 @@ func applyPatch(doc, patch string) (string, error) {
 	obj, err := DecodePatch([]byte(patch))
 
 	if err != nil {
-		panic(err)
+		return "", err
 	}
 
 	out, err := obj.Apply([]byte(doc))
@@ -47,7 +47,7 @@ func applyPatchIndented(doc, patch string) (string, error) {
 	obj, err := DecodePatch([]byte(patch))
 
 	if err != nil {
-		panic(err)
+		return "", err
 	}
 
 	out, err := obj.ApplyIndent([]byte(doc), "  ")
@@ -63,7 +63,7 @@ func applyPatchWithOptions(doc, patch string, options *ApplyOptions) (string, er
 	obj, err := DecodePatch([]byte(patch))
 
 	if err != nil {
-		panic(err)
+		return "", err
 	}
 
 	out, err := obj.ApplyWithOptions([]byte(doc), options)
@@ -574,97 +574,125 @@ var Cases = []Case{
 }
 
 type BadCase struct {
-	doc, patch string
+	doc, patch   string
+	failOnDecode bool
 }
 
 var MutationTestCases = []BadCase{
 	{
 		`{ "foo": "bar", "qux": { "baz": 1, "bar": null } }`,
 		`[ { "op": "remove", "path": "/qux/bar" } ]`,
+		false,
 	},
 	{
 		`{ "foo": "bar", "qux": { "baz": 1, "bar": null } }`,
 		`[ { "op": "replace", "path": "/qux/baz", "value": null } ]`,
+		true,
+	},
+	// malformed value
+	{
+		`{ "foo": "bar" }`,
+		`[ { "op": "add", "path": "/", "value": "{qux" } ]`,
+		true,
 	},
 }
 
 var BadCases = []BadCase{
 	{
 		`{ "foo": "bar" }`,
 		`[ { "op": "add", "path": "/baz/bat", "value": "qux" } ]`,
+		false,
 	},
 	{
 		`{ "a": { "b": { "d": 1 } } }`,
 		`[ { "op": "remove", "path": "/a/b/c" } ]`,
+		false,
 	},
 	{
 		`{ "a": { "b": { "d": 1 } } }`,
 		`[ { "op": "move", "from": "/a/b/c", "path": "/a/b/e" } ]`,
+		false,
 	},
 	{
 		`{ "a": { "b": [1] } }`,
 		`[ { "op": "remove", "path": "/a/b/1" } ]`,
+		false,
 	},
 	{
 		`{ "a": { "b": [1] } }`,
 		`[ { "op": "move", "from": "/a/b/1", "path": "/a/b/2" } ]`,
+		false,
 	},
 	{
 		`{ "foo": "bar" }`,
 		`[ { "op": "add", "pathz": "/baz", "value": "qux" } ]`,
+		true,
 	},
 	{
 		`{ "foo": "bar" }`,
 		`[ { "op": "add", "path": "", "value": "qux" } ]`,
+		false,
 	},
 	{
 		`{ "foo": ["bar","baz"]}`,
 		`[ { "op": "replace", "path": "/foo/2", "value": "bum"}]`,
+		false,
 	},
 	{
 		`{ "foo": ["bar","baz"]}`,
 		`[ { "op": "add", "path": "/foo/-4", "value": "bum"}]`,
+		false,
 	},
 	{
 		`{ "name":{ "foo": "bat", "qux": "bum"}}`,
 		`[ { "op": "replace", "path": "/foo/bar", "value":"baz"}]`,
+		false,
 	},
 	{
 		`{ "foo": ["bar"]}`,
 		`[ {"op": "add", "path": "/foo/2", "value": "bum"}]`,
+		false,
 	},
 	{
 		`{ "foo": []}`,
 		`[ {"op": "remove", "path": "/foo/-"}]`,
+		false,
 	},
 	{
 		`{ "foo": []}`,
 		`[ {"op": "remove", "path": "/foo/-1"}]`,
+		false,
 	},
 	{
 		`{ "foo": ["bar"]}`,
 		`[ {"op": "remove", "path": "/foo/-2"}]`,
+		false,
 	},
 	{
 		`{}`,
 		`[ {"op":null,"path":""} ]`,
+		true,
 	},
 	{
 		`{}`,
 		`[ {"op":"add","path":null} ]`,
+		true,
 	},
 	{
 		`{}`,
 		`[ { "op": "copy", "from": null }]`,
+		true,
 	},
 	{
 		`{ "foo": ["bar"]}`,
 		`[{"op": "copy", "path": "/foo/6666666666", "from": "/"}]`,
+		false,
 	},
 	// Can't copy into an index greater than the size of the array
 	{
 		`{ "foo": ["bar"]}`,
 		`[{"op": "copy", "path": "/foo/2", "from": "/foo/0"}]`,
+		false,
 	},
 	// Accumulated copy size cannot exceed AccumulatedCopySizeLimit.
 	{
@@ -673,20 +701,24 @@ var BadCases = []BadCase{
 		// size, so each copy operation increases the size by 51 bytes.
 		`[ { "op": "copy", "path": "/foo/-", "from": "/foo/1" },
 		   { "op": "copy", "path": "/foo/-", "from": "/foo/1" }]`,
+		false,
 	},
 	// Can't move into an index greater than or equal to the size of the array
 	{
 		`{ "foo": [ "all", "grass", "cows", "eat" ] }`,
 		`[ { "op": "move", "from": "/foo/1", "path": "/foo/4" } ]`,
+		false,
 	},
 	{
 		`{ "baz": "qux" }`,
 		`[ { "op": "replace", "path": "/foo", "value": "bar" } ]`,
+		false,
 	},
 	// Can't copy from non-existent "from" key.
 	{
 		`{ "foo": "bar"}`,
 		`[{"op": "copy", "path": "/qux", "from": "/baz"}]`,
+		false,
 	},
 }
 
@@ -753,10 +785,22 @@ func TestAllCases(t *testing.T) {
 	}
 
 	for _, c := range BadCases {
-		_, err := applyPatch(c.doc, c.patch)
+		p, err := DecodePatch([]byte(c.patch))
+		if err == nil && c.failOnDecode {
+			t.Errorf("Patch %q should have failed decode but did not", c.patch)
+		}
+
+		if err != nil && !c.failOnDecode {
+			t.Errorf("Patch %q should have passed decode but failed with %v", c.patch, err)
+		}
+
+		if err == nil && !c.failOnDecode {
+			_, err = p.Apply([]byte(c.doc))
+
+			if err == nil {
+				t.Errorf("Patch %q should have failed to apply but it did not", c.patch)
+			}
 
-		if err == nil {
-			t.Errorf("Patch %q should have failed to apply but it did not", c.patch)
 		}
 	}
 }