From 2390a11d603b703c02ce51b717ab19bebb188d6d Mon Sep 17 00:00:00 2001
From: Alisdair McDiarmid <alisdair@users.noreply.github.com>
Date: Thu, 15 Apr 2021 07:30:50 -0400
Subject: [PATCH] core: Fix double-marked sensitive attributes

We need to unmark the decoded state and merge the marks with those from
the resource schema.

Co-authored-by: James Bardin <j.bardin@gmail.com>
---
 terraform/context_apply2_test.go | 78 ++++++++++++++++++++++++++++++++
 terraform/evaluate.go            | 16 +++----
 terraform/evaluate_test.go       |  4 +-
 3 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/terraform/context_apply2_test.go b/terraform/context_apply2_test.go
index 973a10396..dd028726c 100644
--- a/terraform/context_apply2_test.go
+++ b/terraform/context_apply2_test.go
@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/terraform/addrs"
+	"github.com/hashicorp/terraform/configs/configschema"
 	"github.com/hashicorp/terraform/providers"
 	"github.com/hashicorp/terraform/states"
 	"github.com/zclconf/go-cty/cty"
@@ -367,3 +368,80 @@ resource "aws_instance" "bin" {
 	}
 
 }
+
+func TestContext2Apply_additionalSensitiveFromState(t *testing.T) {
+	// Ensure we're not trying to double-mark values decoded from state
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+variable "secret" {
+  sensitive = true
+  default = ["secret"]
+}
+
+resource "test_resource" "a" {
+  sensitive_attr = var.secret
+}
+
+resource "test_resource" "b" {
+  value = test_resource.a.id
+}
+`,
+	})
+
+	p := new(MockProvider)
+	p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_resource": {
+				Attributes: map[string]*configschema.Attribute{
+					"id": {
+						Type:     cty.String,
+						Computed: true,
+					},
+					"value": {
+						Type:     cty.String,
+						Optional: true,
+					},
+					"sensitive_attr": {
+						Type:      cty.List(cty.String),
+						Optional:  true,
+						Sensitive: true,
+					},
+				},
+			},
+		},
+	})
+
+	state := states.BuildState(func(s *states.SyncState) {
+		s.SetResourceInstanceCurrent(
+			mustResourceInstanceAddr(`test_resource.a`),
+			&states.ResourceInstanceObjectSrc{
+				AttrsJSON: []byte(`{"id":"a","sensitive_attr":["secret"]}`),
+				AttrSensitivePaths: []cty.PathValueMarks{
+					{
+						Path:  cty.GetAttrPath("sensitive_attr"),
+						Marks: cty.NewValueMarks("sensitive"),
+					},
+				},
+				Status: states.ObjectReady,
+			}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+		)
+	})
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+		State: state,
+	})
+
+	_, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	_, diags = ctx.Apply()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+}
diff --git a/terraform/evaluate.go b/terraform/evaluate.go
index 0e5da5e00..92e50614d 100644
--- a/terraform/evaluate.go
+++ b/terraform/evaluate.go
@@ -782,9 +782,15 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc
 
 		val := ios.Value
 
-		// If our schema contains sensitive values, mark those as sensitive
+		// If our schema contains sensitive values, mark those as sensitive.
+		// Since decoding the instance object can also apply sensitivity marks,
+		// we must remove and combine those before remarking to avoid a double-
+		// mark error.
 		if schema.ContainsSensitive() {
-			val = markProviderSensitiveAttributes(schema, val)
+			var marks []cty.PathValueMarks
+			val, marks = val.UnmarkDeepWithPaths()
+			marks = append(marks, getValMarks(schema, val, nil)...)
+			val = val.MarkWithPaths(marks)
 		}
 		instances[key] = val
 	}
@@ -954,12 +960,6 @@ func moduleDisplayAddr(addr addrs.ModuleInstance) string {
 	}
 }
 
-// markProviderSensitiveAttributes returns an updated value
-// where attributes that are Sensitive are marked
-func markProviderSensitiveAttributes(schema *configschema.Block, val cty.Value) cty.Value {
-	return val.MarkWithPaths(getValMarks(schema, val, nil))
-}
-
 func getValMarks(schema *configschema.Block, val cty.Value, path cty.Path) []cty.PathValueMarks {
 	var pvm []cty.PathValueMarks
 	for name, attrS := range schema.Attributes {
diff --git a/terraform/evaluate_test.go b/terraform/evaluate_test.go
index 3061df5f7..c25429e93 100644
--- a/terraform/evaluate_test.go
+++ b/terraform/evaluate_test.go
@@ -563,7 +563,7 @@ func evaluatorForModule(stateSync *states.SyncState, changesSync *plans.ChangesS
 	}
 }
 
-func TestMarkProviderSensitive(t *testing.T) {
+func TestGetValMarks(t *testing.T) {
 	schema := &configschema.Block{
 		Attributes: map[string]*configschema.Attribute{
 			"unsensitive": {
@@ -647,7 +647,7 @@ func TestMarkProviderSensitive(t *testing.T) {
 		},
 	} {
 		t.Run(fmt.Sprintf("%#v", tc.given), func(t *testing.T) {
-			got := markProviderSensitiveAttributes(schema, tc.given)
+			got := tc.given.MarkWithPaths(getValMarks(schema, tc.given, nil))
 			if !got.RawEquals(tc.expect) {
 				t.Fatalf("\nexpected: %#v\ngot:      %#v\n", tc.expect, got)
 			}