From 63586a9aac87b53519a8cfb66a75dac8c2d387b0 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 14 Aug 2026 16:09:08 +1000 Subject: [PATCH] fix: refuse to write a sensitive variable that would clear its value The server matches a variable without an ID by name, so writing a sensitive variable with an empty value and no ID replaced the stored secret with an empty string. Nothing errored and the loss was invisible on read, because a sensitive value is never returned. Both Update paths now reject that payload. Sending an ID, or an explicit value, is unaffected. Fixes #449 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/variables/sensitive_variable_test.go | 75 ++++++++++++++++++++++++ pkg/variables/variable.go | 8 ++- pkg/variables/variable_service.go | 34 +++++++++++ 3 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 pkg/variables/sensitive_variable_test.go diff --git a/pkg/variables/sensitive_variable_test.go b/pkg/variables/sensitive_variable_test.go new file mode 100644 index 00000000..f4bd143c --- /dev/null +++ b/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") + }) + } +} diff --git a/pkg/variables/variable.go b/pkg/variables/variable.go index 4a8c028b..33f76be4 100644 --- a/pkg/variables/variable.go +++ b/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 } diff --git a/pkg/variables/variable_service.go b/pkg/variables/variable_service.go index 26c078f7..080a1850 100644 --- a/pkg/variables/variable_service.go +++ b/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