plugins/logs: Fixes unintended mutation of result

When mask rules targeted /result, it was modifying both the result
in the decision logs (intended) and the result in the API
response (unintended). Added a step to deep copy the result only once, if
there is at least one mask rule targeting the result.

Fixes open-policy-agent#2752
Signed-off-by: Grant Shively <gshively@godaddy.com>

internal/deepcopy/deepcopy.go

@@ -0,0 +1,27 @@
+// Copyright 2020 The OPA Authors.  All rights reserved.
+// Use of this source code is governed by an Apache2
+// license that can be found in the LICENSE file.
+
+package deepcopy
+
+// DeepCopy performs a recursive deep copy for nested slices/maps and
+// returns the copied object. Supports []interface{}
+// and map[string]interface{} only
+func DeepCopy(val interface{}) interface{} {
+	switch val := val.(type) {
+	case []interface{}:
+		cpy := make([]interface{}, len(val))
+		for i := range cpy {
+			cpy[i] = DeepCopy(val[i])
+		}
+		return cpy
+	case map[string]interface{}:
+		cpy := make(map[string]interface{}, len(val))
+		for k := range val {
+			cpy[k] = DeepCopy(val[k])
+		}
+		return cpy
+	default:
+		return val
+	}
+}

internal/deepcopy/deepcopy_test.go

@@ -0,0 +1,31 @@
+// Copyright 2020 The OPA Authors.  All rights reserved.
+// Use of this source code is governed by an Apache2
+// license that can be found in the LICENSE file.
+
+package deepcopy
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestDeepCopyMapRoot(t *testing.T) {
+	target := map[string]interface{}{
+		"a": map[string]interface{}{
+			"b": []interface{}{
+				"c",
+				"d",
+			},
+			"e": "f",
+		},
+		"x": "y",
+	}
+	result := DeepCopy(target).(map[string]interface{})
+	if !reflect.DeepEqual(target, result) {
+		t.Fatal("Expected result of DeepCopy to be DeepEqual with original.")
+	}
+	result["a"] = "mutated"
+	if target["a"] == "mutated" {
+		t.Fatal("Expected target to remain unmutated when the DeepCopy result was mutated")
+	}
+}

plugins/logs/mask.go

@@ -11,6 +11,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/open-policy-agent/opa/internal/deepcopy"
 	"github.com/open-policy-agent/opa/util"
 )
 
@@ -34,6 +35,12 @@ type maskRule struct {
 	failUndefinedPath bool
 }
 
+type maskRuleSet struct {
+	OnRuleError  func(*maskRule, error)
+	Rules        []*maskRule
+	resultCopied bool
+}
+
 func (r maskRule) String() string {
 	return "/" + strings.Join(r.escapedParts, "/")
 }
@@ -254,20 +261,20 @@ func (r maskRule) mkdirp(node map[string]interface{}, path []string, value inter
 	return nil
 }
 
-func resultValueToMaskRules(rv interface{}) ([]*maskRule, error) {
-
+func newMaskRuleSet(rv interface{}, onRuleError func(*maskRule, error)) (*maskRuleSet, error) {
 	bs, err := json.Marshal(rv)
 	if err != nil {
 		return nil, err
 	}
-
+	var mRuleSet = &maskRuleSet{
+		OnRuleError: onRuleError,
+	}
 	var rawRules []interface{}
 
 	if err := util.Unmarshal(bs, &rawRules); err != nil {
 		return nil, err
 	}
 
-	mRules := []*maskRule{}
 	for _, iface := range rawRules {
 
 		switch v := iface.(type) {
@@ -280,7 +287,7 @@ func resultValueToMaskRules(rv interface{}) ([]*maskRule, error) {
 				return nil, err
 			}
 
-			mRules = append(mRules, rule)
+			mRuleSet.Rules = append(mRuleSet.Rules, rule)
 
 		case map[string]interface{}:
 
@@ -307,12 +314,29 @@ func resultValueToMaskRules(rv interface{}) ([]*maskRule, error) {
 				return nil, err
 			}
 
-			mRules = append(mRules, rule)
+			mRuleSet.Rules = append(mRuleSet.Rules, rule)
 
 		default:
 			return nil, fmt.Errorf("invalid mask rule format encountered: %T", v)
 		}
 	}
 
-	return mRules, nil
+	return mRuleSet, nil
+}
+
+func (rs maskRuleSet) Mask(event *EventV1) {
+	for _, mRule := range rs.Rules {
+		// result must be deep copied if there are any mask rules
+		// targeting it, to avoid modifying the result sent
+		// to the consumer
+		if mRule.escapedParts[0] == "result" && event.Result != nil && !rs.resultCopied {
+			resultCopy := deepcopy.DeepCopy(*event.Result)
+			event.Result = &resultCopy
+			rs.resultCopied = true
+		}
+		err := mRule.Mask(event)
+		if err != nil {
+			rs.OnRuleError(mRule, err)
+		}
+	}
 }

plugins/logs/mask_test.go

@@ -586,21 +586,23 @@ func TestMaskRuleMask(t *testing.T) {
 	}
 }
 
-func TestResultValueToMaskRules(t *testing.T) {
-
+func TestNewMaskRuleSet(t *testing.T) {
 	tests := []struct {
-		note  string
-		value interface{}
-		exp   []maskRule
-		err   error
+		onRuleError func(*maskRule, error)
+		note        string
+		value       interface{}
+		exp         *maskRuleSet
+		err         error
 	}{
 		{
-			note:  "invalid format: not []interface{}",
-			value: map[string]int{"invalid": 1},
-			err:   fmt.Errorf("json: cannot unmarshal object into Go value of type []interface {}"),
+			onRuleError: func(mRule *maskRule, err error) {},
+			note:        "invalid format: not []interface{}",
+			value:       map[string]int{"invalid": 1},
+			err:         fmt.Errorf("json: cannot unmarshal object into Go value of type []interface {}"),
 		},
 		{
-			note: "invalid format: nested type not string or map[string]interface{}",
+			onRuleError: func(mRule *maskRule, err error) {},
+			note:        "invalid format: nested type not string or map[string]interface{}",
 			value: []interface{}{
 				[]int{1, 2},
 			},
@@ -611,7 +613,7 @@ func TestResultValueToMaskRules(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.note, func(t *testing.T) {
 
-			_, err := resultValueToMaskRules(tc.value)
+			_, err := newMaskRuleSet(tc.value, func(mRule *maskRule, err error) {})
 
 			if err != nil {
 				if tc.err.Error() != err.Error() {
@@ -621,3 +623,128 @@ func TestResultValueToMaskRules(t *testing.T) {
 		})
 	}
 }
+
+func TestMaskRuleSetMask(t *testing.T) {
+
+	tests := []struct {
+		note   string
+		rules  []*maskRule
+		event  string
+		exp    string
+		expErr error
+	}{
+		{
+			note: "erase input",
+			rules: []*maskRule{
+				&maskRule{
+					OP:   maskOPRemove,
+					Path: "/input",
+				},
+			},
+			event: `{"input": {"a": 1}}`,
+			exp:   `{"erased": ["/input"]}`,
+		},
+		{
+			note: "erase result",
+			rules: []*maskRule{
+				&maskRule{
+					OP:   maskOPRemove,
+					Path: "/result",
+				},
+			},
+			event: `{"result": {"a": 1}}`,
+			exp:   `{"erased": ["/result"]}`,
+		},
+		{
+			note: "erase input and result nested",
+			rules: []*maskRule{
+				&maskRule{
+					OP:   maskOPRemove,
+					Path: "/input/a/b",
+				},
+				&maskRule{
+					OP:   maskOPRemove,
+					Path: "/result/c/d",
+				},
+			},
+			event: `{"input":{"a":{"b":"removeme","y":"stillhere"}},"result":{"c":{"d":"removeme","z":"stillhere"}}}`,
+			exp:   `{"input":{"a":{"y":"stillhere"}},"result":{"c":{"z":"stillhere"}},"erased":["/input/a/b", "/result/c/d"]}`,
+		},
+		{
+			note: "expected rule error",
+			rules: []*maskRule{
+				&maskRule{
+					OP:                maskOPRemove,
+					Path:              "/result",
+					failUndefinedPath: true,
+				},
+			},
+			event:  `{"input":"foo"}`,
+			exp:    `{"input":"foo"}`,
+			expErr: errMaskInvalidObject,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.note, func(t *testing.T) {
+			ptr := &maskRuleSet{}
+			var ruleErr error
+			if tc.expErr != nil {
+				ptr.OnRuleError = func(mRule *maskRule, err error) {
+					ruleErr = err
+				}
+			} else {
+				ptr.OnRuleError = func(mRule *maskRule, err error) {
+					t.Fatalf(fmt.Sprintf("unexpected rule error, rule: %s, error: %s", mRule.String(), err.Error()))
+				}
+			}
+			for _, rule := range tc.rules {
+				var mRule *maskRule
+				var err error
+				if rule.failUndefinedPath {
+					mRule, err = newMaskRule(rule.Path, withOP(rule.OP), withValue(rule.Value), withFailUndefinedPath())
+				} else {
+					mRule, err = newMaskRule(rule.Path, withOP(rule.OP), withValue(rule.Value))
+				}
+				if err != nil {
+					panic(err)
+				}
+				ptr.Rules = append(ptr.Rules, mRule)
+			}
+
+			var exp EventV1
+			if err := util.UnmarshalJSON([]byte(tc.exp), &exp); err != nil {
+				panic(err)
+			}
+
+			var event EventV1
+			var origEvent EventV1
+			if err := util.UnmarshalJSON([]byte(tc.event), &event); err != nil {
+				panic(err)
+			}
+			origEvent = event
+
+			ptr.Mask(&event)
+
+			// compare via json marshall to map tc input types
+			bs1, _ := json.MarshalIndent(exp, "", "  ")
+			bs2, _ := json.MarshalIndent(event, "", "  ")
+			if !bytes.Equal(bs1, bs2) {
+				t.Fatalf("Expected: %s\nGot: %s", string(bs1), string(bs2))
+			}
+
+			if origEvent.Result != nil && reflect.DeepEqual(origEvent.Result, event.Result) {
+				t.Fatal("Expected event.Result to be deep copied during masking, so that the event's original Result is not modified")
+			}
+
+			if tc.expErr != nil {
+				if ruleErr == nil {
+					t.Fatalf("Expected: %s\nGot:%s", tc.expErr.Error(), "nil")
+				}
+				if tc.expErr != ruleErr {
+					t.Fatalf("Expected: %s\nGot:%s", tc.expErr.Error(), ruleErr.Error())
+				}
+			}
+		})
+	}
+}

plugins/logs/plugin.go

@@ -706,17 +706,17 @@ func (p *Plugin) maskEvent(ctx context.Context, txn storage.Transaction, event *
 		return nil
 	}
 
-	mRules, err := resultValueToMaskRules(rs[0].Expressions[0].Value)
+	mRuleSet, err := newMaskRuleSet(
+		rs[0].Expressions[0].Value,
+		func(mRule *maskRule, err error) {
+			p.logError("mask rule skipped: %s: %s", mRule.String(), err.Error())
+		},
+	)
 	if err != nil {
 		return err
 	}
 
-	for _, mRule := range mRules {
-		err := mRule.Mask(event)
-		if err != nil {
-			p.logError("mask rule skipped: %s: %s", mRule.String(), err.Error())
-		}
-	}
+	mRuleSet.Mask(event)
 
 	return nil
 }

storage/inmem/txn.go

@@ -9,6 +9,7 @@ import (
 	"encoding/json"
 	"strconv"
 
+	"github.com/open-policy-agent/opa/internal/deepcopy"
 	"github.com/open-policy-agent/opa/storage"
 )
 
@@ -204,7 +205,7 @@ func (txn *transaction) Read(path storage.Path) (interface{}, error) {
 		return data, nil
 	}
 
-	cpy := deepCopy(data)
+	cpy := deepcopy.DeepCopy(data)
 
 	for _, update := range merge {
 		cpy = update.Relative(path).Apply(cpy)
@@ -388,25 +389,6 @@ func (u *update) Relative(path storage.Path) *update {
 	return &cpy
 }
 
-func deepCopy(val interface{}) interface{} {
-	switch val := val.(type) {
-	case []interface{}:
-		cpy := make([]interface{}, len(val))
-		for i := range cpy {
-			cpy[i] = deepCopy(val[i])
-		}
-		return cpy
-	case map[string]interface{}:
-		cpy := make(map[string]interface{}, len(val))
-		for k := range val {
-			cpy[k] = deepCopy(val[k])
-		}
-		return cpy
-	default:
-		return val
-	}
-}
-
 func ptr(data interface{}, path storage.Path) (interface{}, error) {
 	node := data
 	for i := range path {