fix: refuse to write a sensitive variable that would clear its value

pkg/variables/sensitive_variable_test.go

@@ -0,0 +1,75 @@
+package variables
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func sensitiveVariable(name string, value string, id string) *Variable {
+	variable := NewVariable(name)
+	variable.IsSensitive = true
+	variable.Type = "Sensitive"
+	variable.Value = value
+	variable.ID = id
+	return variable
+}
+
+func TestValidateSensitiveVariables(t *testing.T) {
+	testCases := []struct {
+		name        string
+		variables   []*Variable
+		expectError bool
+	}{
+		{
+			name:        "sensitive with an ID and no value keeps the stored value",
+			variables:   []*Variable{sensitiveVariable("secret", "", "abc-123")},
+			expectError: false,
+		},
+		{
+			name:        "sensitive with a value and no ID sets it explicitly",
+			variables:   []*Variable{sensitiveVariable("secret", "s3cret", "")},
+			expectError: false,
+		},
+		{
+			name:        "sensitive with neither would clear the stored value",
+			variables:   []*Variable{sensitiveVariable("secret", "", "")},
+			expectError: true,
+		},
+		{
+			name:        "non-sensitive with neither is unaffected",
+			variables:   []*Variable{NewVariable("plain")},
+			expectError: false,
+		},
+		{
+			name:        "reported alongside valid variables",
+			variables:   []*Variable{NewVariable("plain"), sensitiveVariable("secret", "", "")},
+			expectError: true,
+		},
+		{
+			name:        "nil entries are skipped",
+			variables:   []*Variable{nil},
+			expectError: false,
+		},
+		{
+			name:        "empty set",
+			variables:   nil,
+			expectError: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validateSensitiveVariables(VariableSet{Variables: tc.variables})
+
+			if !tc.expectError {
+				require.NoError(t, err)
+				return
+			}
+
+			require.Error(t, err)
+			require.ErrorAs(t, err, &errSensitiveVariableWouldBeCleared{})
+			require.Contains(t, err.Error(), "secret")
+		})
+	}
+}

pkg/variables/variable.go

@@ -10,8 +10,12 @@ type Variable struct {
 	Prompt      *VariablePromptOptions `json:"Prompt,omitempty"`
 	Scope       VariableScope          `json:"Scope"`
 	Type        string                 `json:"Type"`
-	Value       string                 `json:"Value"`
-	SpaceID     string                 `json:"SpaceId,omitempty"`
+	// Value is never populated for a sensitive variable: the server returns null and it reads
+	// back as an empty string. Preserving an existing secret across an update therefore depends
+	// on sending the variable's ID, not on the value. Writing a sensitive variable with an empty
+	// Value and no ID replaces the stored secret with an empty string.
+	Value   string `json:"Value"`
+	SpaceID string `json:"SpaceId,omitempty"`
 
 	resources.Resource
 }

pkg/variables/variable_service.go

@@ -22,6 +22,32 @@ func (e errInvalidVariableServiceParameter) Error() string {
 	return fmt.Sprintf("VariableService: invalid parameter, %s", e.ParameterName)
 }
 
+// errSensitiveVariableWouldBeCleared reports a write that would replace a stored secret with an
+// empty string. The server matches a variable without an ID by name, so an empty value overwrites
+// whatever is already held under that name.
+type errSensitiveVariableWouldBeCleared struct {
+	VariableName string
+}
+
+func (e errSensitiveVariableWouldBeCleared) Error() string {
+	return fmt.Sprintf("VariableService: sensitive variable %s has no value and no ID; writing it would clear the stored value. Send the variable's ID to keep the existing value, or set a value explicitly", e.VariableName)
+}
+
+// validateSensitiveVariables rejects sensitive variables that carry neither a value nor an ID.
+func validateSensitiveVariables(variableSet VariableSet) error {
+	for _, variable := range variableSet.Variables {
+		if variable == nil {
+			continue
+		}
+
+		if variable.IsSensitive && internal.IsEmpty(variable.Value) && internal.IsEmpty(variable.GetID()) {
+			return errSensitiveVariableWouldBeCleared{VariableName: variable.Name}
+		}
+	}
+
+	return nil
+}
+
 type VariableService struct {
 	namesPath   string
 	previewPath string
@@ -235,6 +261,10 @@ func (s *VariableService) Update(ownerID string, variableSet VariableSet) (Varia
 		return VariableSet{}, errInvalidVariableServiceParameter{ParameterName: "ownerID"}
 	}
 
+	if err := validateSensitiveVariables(variableSet); err != nil {
+		return VariableSet{}, err
+	}
+
 	path := internal.TrimTemplate(s.GetPath())
 	path = fmt.Sprintf(path+"/variableset-%s", ownerID)
 
@@ -507,6 +537,10 @@ func Update(client newclient.Client, spaceID string, ownerID string, variableSet
 		return VariableSet{}, errInvalidVariableServiceParameter{ParameterName: "ownerID"}
 	}
 
+	if err := validateSensitiveVariables(variableSet); err != nil {
+		return VariableSet{}, err
+	}
+
 	spaceID, err := internal.GetSpaceID(spaceID, client.GetSpaceID())
 	if err != nil {
 		return VariableSet{}, err