From da4ddd01605cd49b3016db21fd614813c0f3a74c Mon Sep 17 00:00:00 2001
From: Pam Selle <204372+pselle@users.noreply.github.com>
Date: Mon, 12 Oct 2020 15:37:44 -0400
Subject: [PATCH] Avoid disclosing values in errors on marked vals

AssertObjectCompatible is a special case that will
expose Go string values of values unless otherwise
stopped. This adds that check.
---
 plans/objchange/compatible.go      | 26 +++++++---------
 plans/objchange/compatible_test.go | 50 ++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go
index 5536ad41c..6dc622dfa 100644
--- a/plans/objchange/compatible.go
+++ b/plans/objchange/compatible.go
@@ -51,18 +51,17 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu
 		actualV := actual.GetAttr(name)
 
 		path := append(path, cty.GetAttrStep{Name: name})
-		// If our value is marked, unmark it here before
-		// checking value assertions
-		unmarkedActualV := actualV
-		if actualV.ContainsMarked() {
-			unmarkedActualV, _ = actualV.UnmarkDeep()
-		}
-		unmarkedPlannedV := plannedV
-		if plannedV.ContainsMarked() {
-			unmarkedPlannedV, _ = actualV.UnmarkDeep()
-		}
+
+		// Unmark values here before checking value assertions,
+		// but save the marks so we can see if we should supress
+		// exposing a value through errors
+		unmarkedActualV, marksA := actualV.UnmarkDeep()
+		unmarkedPlannedV, marksP := plannedV.UnmarkDeep()
+		_, isMarkedActual := marksA["sensitive"]
+		_, isMarkedPlanned := marksP["sensitive"]
+
 		moreErrs := assertValueCompatible(unmarkedPlannedV, unmarkedActualV, path)
-		if attrS.Sensitive {
+		if attrS.Sensitive || isMarkedActual || isMarkedPlanned {
 			if len(moreErrs) > 0 {
 				// Use a vague placeholder message instead, to avoid disclosing
 				// sensitive information.
@@ -73,9 +72,8 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu
 		}
 	}
 	for name, blockS := range schema.BlockTypes {
-		// Unmark values before testing compatibility
-		plannedV, _ := planned.GetAttr(name).UnmarkDeep()
-		actualV, _ := actual.GetAttr(name).UnmarkDeep()
+		plannedV, _ := planned.GetAttr(name).Unmark()
+		actualV, _ := actual.GetAttr(name).Unmark()
 
 		// As a special case, if there were any blocks whose leaf attributes
 		// are all unknown then we assume (possibly incorrectly) that the
diff --git a/plans/objchange/compatible_test.go b/plans/objchange/compatible_test.go
index 4c67e610d..1eacdd8b1 100644
--- a/plans/objchange/compatible_test.go
+++ b/plans/objchange/compatible_test.go
@@ -144,6 +144,56 @@ func TestAssertObjectCompatible(t *testing.T) {
 				`.name: inconsistent values for sensitive attribute`,
 			},
 		},
+		{
+			&configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"id": {
+						Type:     cty.String,
+						Computed: true,
+					},
+					"name": {
+						Type:     cty.String,
+						Required: true,
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"id":   cty.UnknownVal(cty.String),
+				"name": cty.StringVal("wotsit").Mark("sensitive"),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"id":   cty.UnknownVal(cty.String),
+				"name": cty.StringVal("thingy"),
+			}),
+			[]string{
+				`.name: inconsistent values for sensitive attribute`,
+			},
+		},
+		{
+			&configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"id": {
+						Type:     cty.String,
+						Computed: true,
+					},
+					"name": {
+						Type:     cty.String,
+						Required: true,
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"id":   cty.UnknownVal(cty.String),
+				"name": cty.StringVal("wotsit"),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"id":   cty.UnknownVal(cty.String),
+				"name": cty.StringVal("thingy").Mark("sensitive"),
+			}),
+			[]string{
+				`.name: inconsistent values for sensitive attribute`,
+			},
+		},
 		{
 			&configschema.Block{
 				Attributes: map[string]*configschema.Attribute{