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)
 }
 }
 }