terraform test: remove marks before passing variables as inputs to a plan (#34190)

* remove marks before passing variables as inputs to a plan

* fix doc comments

internal/backend/local/test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/hashicorp/terraform/internal/backend"
 	"github.com/hashicorp/terraform/internal/command/views"
 	"github.com/hashicorp/terraform/internal/configs"
+	"github.com/hashicorp/terraform/internal/lang/marks"
 	"github.com/hashicorp/terraform/internal/logging"
 	"github.com/hashicorp/terraform/internal/moduletest"
 	configtest "github.com/hashicorp/terraform/internal/moduletest/config"
@@ -395,7 +396,12 @@ func (runner *TestFileRunner) run(run *moduletest.Run, file *moduletest.File, st
 		return state, false
 	}
 
-	planCtx, plan, planDiags := runner.plan(config, state, run, file, runner.FilterVariablesToConfig(config, variables), references, start)
+	// FilterVariablesToConfig only returns warnings, so we don't check the
+	// returned diags for errors.
+	setVariables, setVariableDiags := runner.FilterVariablesToConfig(config, variables)
+	run.Diagnostics = run.Diagnostics.Append(setVariableDiags)
+
+	planCtx, plan, planDiags := runner.plan(config, state, run, file, setVariables, references, start)
 	if run.Config.Command == configs.PlanTestCommand {
 		// Then we want to assess our conditions and diagnostics differently.
 		planDiags = run.ValidateExpectedFailures(planDiags)
@@ -586,9 +592,15 @@ func (runner *TestFileRunner) destroy(config *configs.Config, state *states.Stat
 		return state, diags
 	}
 
+	// During the destroy operation, we don't add warnings from this operation.
+	// Anything that would have been reported here was already reported during
+	// the original plan, and a successful destroy operation is the only thing
+	// we care about.
+	setVariables, _ := runner.FilterVariablesToConfig(config, variables)
+
 	planOpts := &terraform.PlanOpts{
 		Mode:         plans.DestroyMode,
-		SetVariables: runner.FilterVariablesToConfig(config, variables),
+		SetVariables: setVariables,
 	}
 
 	tfCtx, ctxDiags := terraform.NewContext(runner.Suite.Opts)
@@ -1140,17 +1152,46 @@ func (runner *TestFileRunner) GetVariables(config *configs.Config, run *modulete
 // This function is essentially the opposite of AddVariablesToConfig which
 // makes the config match the variables rather than the variables match the
 // config.
-func (runner *TestFileRunner) FilterVariablesToConfig(config *configs.Config, values terraform.InputValues) terraform.InputValues {
+//
+// This function can only return warnings, and the callers can rely on this so
+// please check the callers of this function if you add any error diagnostics.
+func (runner *TestFileRunner) FilterVariablesToConfig(config *configs.Config, values terraform.InputValues) (terraform.InputValues, tfdiags.Diagnostics) {
+	var diags tfdiags.Diagnostics
+
 	filtered := make(terraform.InputValues)
 	for name, value := range values {
-		if _, exists := config.Module.Variables[name]; !exists {
+		variableConfig, exists := config.Module.Variables[name]
+		if !exists {
 			// Only include values that are actually required by the config.
 			continue
 		}
 
+		if marks.Has(value.Value, marks.Sensitive) {
+			unmarkedValue, _ := value.Value.Unmark()
+			if !variableConfig.Sensitive {
+				// Then we are passing a sensitive value into a non-sensitive
+				// variable. Let's add a warning and tell the user they should
+				// mark the config as sensitive as well. If the config variable
+				// is sensitive, then we don't need to worry.
+				diags = diags.Append(&hcl.Diagnostic{
+					Severity: hcl.DiagWarning,
+					Summary:  "Sensitive metadata on variable lost",
+					Detail:   fmt.Sprintf("The input variable is marked as sensitive, while the receiving configuration is not. The underlying sensitive information may be exposed when var.%s is referenced. Mark the variable block in the configuration as sensitive to resolve this warning.", variableConfig.Name),
+					Subject:  value.SourceRange.ToHCL().Ptr(),
+				})
+			}
+
+			// Set the unmarked value into the input value.
+			value = &terraform.InputValue{
+				Value:       unmarkedValue,
+				SourceType:  value.SourceType,
+				SourceRange: value.SourceRange,
+			}
+		}
+
 		filtered[name] = value
 	}
-	return filtered
+	return filtered, diags
 }
 
 // AddVariablesToConfig extends the provided config to ensure it has definitions

internal/command/test_test.go

@@ -1573,6 +1573,80 @@ operation, and the specified output value is only known after apply.
 
 }
 
+func TestTest_SensitiveInputValues(t *testing.T) {
+	td := t.TempDir()
+	testCopyDir(t, testFixturePath(path.Join("test", "sensitive_input_values")), td)
+	defer testChdir(t, td)()
+
+	provider := testing_command.NewProvider(nil)
+
+	providerSource, close := newMockProviderSource(t, map[string][]string{
+		"test": {"1.0.0"},
+	})
+	defer close()
+
+	streams, done := terminal.StreamsForTesting(t)
+	view := views.NewView(streams)
+	ui := new(cli.MockUi)
+
+	meta := Meta{
+		testingOverrides: metaOverridesForProvider(provider.Provider),
+		Ui:               ui,
+		View:             view,
+		Streams:          streams,
+		ProviderSource:   providerSource,
+	}
+
+	init := &InitCommand{
+		Meta: meta,
+	}
+
+	if code := init.Run(nil); code != 0 {
+		t.Fatalf("expected status code 0 but got %d: %s", code, ui.ErrorWriter)
+	}
+
+	c := &TestCommand{
+		Meta: meta,
+	}
+
+	code := c.Run([]string{"-no-color"})
+	output := done(t)
+
+	if code != 0 {
+		t.Errorf("expected status code 0 but got %d", code)
+	}
+
+	expected := `main.tftest.hcl... in progress
+  run "setup"... pass
+  run "test"... pass
+
+Warning: Sensitive metadata on variable lost
+
+  on main.tftest.hcl line 13, in run "test":
+  13:     password = run.setup.password
+
+The input variable is marked as sensitive, while the receiving configuration
+is not. The underlying sensitive information may be exposed when var.password
+is referenced. Mark the variable block in the configuration as sensitive to
+resolve this warning.
+
+main.tftest.hcl... tearing down
+main.tftest.hcl... pass
+
+Success! 2 passed, 0 failed.
+`
+
+	actual := output.All()
+
+	if diff := cmp.Diff(actual, expected); len(diff) > 0 {
+		t.Errorf("output didn't match expected:\nexpected:\n%s\nactual:\n%s\ndiff:\n%s", expected, actual, diff)
+	}
+
+	if provider.ResourceCount() > 0 {
+		t.Errorf("should have deleted all resources on completion but left %v", provider.ResourceString())
+	}
+}
+
 // This test takes around 10 seconds to complete, as we're testing the progress
 // updates that are printed every 2 seconds. Sorry!
 func TestTest_LongRunningTest(t *testing.T) {

internal/command/testdata/test/sensitive_input_values/main.tf

@@ -0,0 +1,8 @@
+variable "password" {
+  type = string
+}
+
+output "password" {
+  value = var.password
+  sensitive = true
+}

internal/command/testdata/test/sensitive_input_values/main.tftest.hcl

@@ -0,0 +1,15 @@
+run "setup" {
+  variables {
+    password = "password"
+  }
+
+  module {
+    source = "./setup"
+  }
+}
+
+run "test" {
+  variables {
+    password = run.setup.password
+  }
+}

internal/command/testdata/test/sensitive_input_values/setup/main.tf

@@ -0,0 +1,9 @@
+variable "password" {
+  sensitive = true
+  type = string
+}
+
+output "password" {
+  value = var.password
+  sensitive = true
+}