Argo CD does not scrub secret values from patch errors

@@ -13,7 +13,7 @@ require (
 	github.com/TomOnTime/utfutil v0.0.0-20180511104225-09c41003ee1d
 	github.com/alicebob/miniredis/v2 v2.34.0
 	github.com/antonmedv/expr v1.15.1
-	github.com/argoproj/gitops-engine v0.7.1-0.20241216155226-54992bf42431
+	github.com/argoproj/gitops-engine v0.7.1-0.20250129155113-7e21b91e9d0f
 	github.com/argoproj/notifications-engine v0.4.1-0.20241007194503-2fef5c9049fd
 	github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1
 	github.com/aws/aws-sdk-go v1.55.6

@@ -90,8 +90,8 @@ github.com/antonmedv/expr v1.15.1/go.mod h1:0E/6TxnOlRNp81GMzX9QfDPAmHo2Phg00y4J
 github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
 github.com/apache/thrift v0.13.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
 github.com/appscode/go v0.0.0-20191119085241-0887d8ec2ecc/go.mod h1:OawnOmAL4ZX3YaPdN+8HTNwBveT1jMsqP74moa9XUbE=
-github.com/argoproj/gitops-engine v0.7.1-0.20241216155226-54992bf42431 h1:ku0Gzp1dHr7yn83B/xmMrmbB5sJbe32LXaYSDSBd6/c=
-github.com/argoproj/gitops-engine v0.7.1-0.20241216155226-54992bf42431/go.mod h1:WsnykM8idYRUnneeT31cM/Fq/ZsjkefCbjiD8ioCJkU=
+github.com/argoproj/gitops-engine v0.7.1-0.20250129155113-7e21b91e9d0f h1:6amQW2gmWyBr/3xz/YzpgrQ+91xKxtpaWiLBkgjjV8o=
+github.com/argoproj/gitops-engine v0.7.1-0.20250129155113-7e21b91e9d0f/go.mod h1:WsnykM8idYRUnneeT31cM/Fq/ZsjkefCbjiD8ioCJkU=
 github.com/argoproj/notifications-engine v0.4.1-0.20241007194503-2fef5c9049fd h1:lOVVoK89j9Nd4+JYJiKAaMNYC1402C0jICROOfUPWn0=
 github.com/argoproj/notifications-engine v0.4.1-0.20241007194503-2fef5c9049fd/go.mod h1:N0A4sEws2soZjEpY4hgZpQS8mRIEw6otzwfkgc3g9uQ=
 github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1 h1:qsHwwOJ21K2Ao0xPju1sNuqphyMnMYkyB3ZLoLtxWpo=

@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	"github.com/argoproj/gitops-engine/pkg/health"
+	"github.com/argoproj/gitops-engine/pkg/sync/common"
 
 	. "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
 	. "github.com/argoproj/argo-cd/v3/test/e2e/fixture"
@@ -56,3 +57,55 @@ data:
 			assert.False(t, sensitiveData.MatchString(diff))
 		})
 }
+
+// Secret values shouldn't be exposed in error messages and the diff view
+// when invalid secret is synced.
+func TestMaskValuesInInvalidSecret(t *testing.T) {
+	sensitiveData := regexp.MustCompile(`SECRETVAL|U0VDUkVUVkFM|12345`)
+
+	Given(t).
+		Path("empty-dir").
+		When().
+		// valid secret
+		AddFile("secrets.yaml", `apiVersion: v1
+kind: Secret
+metadata:
+  name: secret
+  annotations:
+    app: test
+stringData:
+  username: SECRETVAL
+data:
+  password: U0VDUkVUVkFM
+`).
+		CreateApp().
+		Sync().
+		Then().
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		Expect(HealthIs(health.HealthStatusHealthy)).
+		// secret data shouldn't be exposed in manifests output
+		And(func(app *Application) {
+			mnfs, _ := RunCli("app", "manifests", app.Name)
+			assert.False(t, sensitiveData.MatchString(mnfs))
+		}).
+		When().
+		// invalidate secret
+		PatchFile("secrets.yaml", `[{"op": "replace", "path": "/data/password", "value": 12345}]`).
+		Refresh(RefreshTypeHard).
+		IgnoreErrors().
+		Sync().
+		Then().
+		Expect(SyncStatusIs(SyncStatusCodeOutOfSync)).
+		Expect(OperationPhaseIs(common.OperationFailed)).
+		// secret data shouldn't be exposed in manifests, diff & error output for invalid secret
+		And(func(app *Application) {
+			mnfs, _ := RunCli("app", "manifests", app.Name)
+			assert.False(t, sensitiveData.MatchString(mnfs))
+
+			diff, _ := RunCli("app", "diff", app.Name)
+			assert.False(t, sensitiveData.MatchString(diff))
+
+			msg := app.Status.OperationState.Message
+			assert.False(t, sensitiveData.MatchString(msg))
+		})
+}

@@ -7,6 +7,7 @@ package diff
 import (
 	"bytes"
 	"context"
+	"encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -843,6 +844,32 @@ func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) {
 	if gvk.Group != "" || gvk.Kind != "Secret" {
 		return
 	}
+
+	// move stringData to data section
+	if stringData, found, err := unstructured.NestedMap(un.Object, "stringData"); found && err == nil {
+		var data map[string]interface{}
+		data, found, _ = unstructured.NestedMap(un.Object, "data")
+		if !found {
+			data = make(map[string]interface{})
+		}
+
+		// base64 encode string values and add non-string values as is.
+		// This ensures that the apply fails if the secret is invalid.
+		for k, v := range stringData {
+			strVal, ok := v.(string)
+			if ok {
+				data[k] = base64.StdEncoding.EncodeToString([]byte(strVal))
+			} else {
+				data[k] = v
+			}
+		}
+
+		err := unstructured.SetNestedField(un.Object, data, "data")
+		if err == nil {
+			delete(un.Object, "stringData")
+		}
+	}
+
 	o := applyOptions(opts)
 	var secret corev1.Secret
 	err := runtime.DefaultUnstructuredConverter.FromUnstructured(un.Object, &secret)
@@ -856,15 +883,6 @@ func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) {
 			secret.Data[k] = []byte("")
 		}
 	}
-	if len(secret.StringData) > 0 {
-		if secret.Data == nil {
-			secret.Data = make(map[string][]byte)
-		}
-		for k, v := range secret.StringData {
-			secret.Data[k] = []byte(v)
-		}
-		delete(un.Object, "stringData")
-	}
 	newObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&secret)
 	if err != nil {
 		o.log.Error(err, "object unable to convert from secret")

@@ -1044,6 +1044,126 @@ func TestHideSecretDataDifferentKeysDifferentValues(t *testing.T) {
 	assert.Equal(t, map[string]interface{}{"key2": replacement2, "key3": replacement1}, secretData(live))
 }
 
+func TestHideStringDataInInvalidSecret(t *testing.T) {
+	liveUn := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name": "test-secret",
+			},
+			"type": "Opaque",
+			"data": map[string]interface{}{
+				"key1": "a2V5MQ==",
+				"key2": "a2V5MQ==",
+			},
+		},
+	}
+	targetUn := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name": "test-secret",
+			},
+			"type": "Opaque",
+			"data": map[string]interface{}{
+				"key1": "a2V5MQ==",
+				"key2": "a2V5Mg==",
+				"key3": false,
+			},
+			"stringData": map[string]interface{}{
+				"key4": "key4",
+				"key5": 5,
+			},
+		},
+	}
+
+	liveUn = remarshal(liveUn, applyOptions(diffOptionsForTest()))
+	targetUn = remarshal(targetUn, applyOptions(diffOptionsForTest()))
+
+	target, live, err := HideSecretData(targetUn, liveUn, nil)
+	require.NoError(t, err)
+
+	assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement2}, secretData(live))
+	assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement1, "key3": replacement1, "key4": replacement1, "key5": replacement1}, secretData(target))
+}
+
+// stringData in secrets should be normalized even if it is invalid
+func TestNormalizeSecret(t *testing.T) {
+	var tests = []struct {
+		testname   string
+		data       map[string]interface{}
+		stringData map[string]interface{}
+	}{
+		{
+			testname: "Valid secret",
+			data: map[string]interface{}{
+				"key1": "key1",
+			},
+			stringData: map[string]interface{}{
+				"key2": "a2V5Mg==",
+			},
+		},
+		{
+			testname: "Invalid secret",
+			data: map[string]interface{}{
+				"key1": "key1",
+				"key2": 2,
+			},
+			stringData: map[string]interface{}{
+				"key3": "key3",
+				"key4": nil,
+			},
+		},
+		{
+			testname: "Invalid secret with stringData only",
+			data:     nil,
+			stringData: map[string]interface{}{
+				"key3": "key3",
+				"key4": true,
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.testname, func(t *testing.T) {
+			un := &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "Secret",
+					"metadata": map[string]interface{}{
+						"name": "test-secret",
+					},
+					"type":       "Opaque",
+					"data":       tt.data,
+					"stringData": tt.stringData,
+				},
+			}
+			un = remarshal(un, applyOptions(diffOptionsForTest()))
+
+			NormalizeSecret(un)
+
+			_, found, _ := unstructured.NestedMap(un.Object, "stringData")
+			assert.False(t, found)
+
+			data, found, _ := unstructured.NestedMap(un.Object, "data")
+			assert.True(t, found)
+
+			// check all secret keys are found under data in normalized secret
+			for _, obj := range []map[string]interface{}{tt.data, tt.stringData} {
+				if obj == nil {
+					continue
+				}
+				for k := range obj {
+					_, ok := data[k]
+					assert.True(t, ok)
+				}
+			}
+		})
+	}
+}
+
 func TestHideSecretAnnotations(t *testing.T) {
 	tests := []struct {
 		name           string

@@ -205,12 +205,15 @@ var (
 	// See ApplyOpts::Run()
 	// cmdutil.AddSourceToErr(fmt.Sprintf("applying patch:\n%s\nto:\n%v\nfor:", patchBytes, info), info.Source, err)
 	kubectlApplyPatchErrOutRegexp = regexp.MustCompile(`(?s)^error when applying patch:.*\nfor: "\S+": `)
+
+	kubectlErrOutMapRegexp = regexp.MustCompile(`map\[.*\]`)
 )
 
 // cleanKubectlOutput makes the error output of kubectl a little better to read
 func cleanKubectlOutput(s string) string {
 	s = strings.TrimSpace(s)
 	s = kubectlErrOutRegexp.ReplaceAllString(s, "")
+	s = kubectlErrOutMapRegexp.ReplaceAllString(s, "")
 	s = kubectlApplyPatchErrOutRegexp.ReplaceAllString(s, "")
 	s = strings.Replace(s, "; if you choose to ignore these errors, turn validation off with --validate=false", "", -1)
 	return s

@@ -74,6 +74,18 @@ Object: &{map["apiVersion":"v1" "kind":"Service" "metadata":map["annotations":ma
 for: "/var/folders/_m/991sn1ds7g39lnbhp6wvqp9d_j5476/T/224503547": Service "my-service" is invalid: spec.clusterIP: Invalid value: "10.96.0.44": field is immutable`
 		assert.Equal(t, cleanKubectlOutput(s), `Service "my-service" is invalid: spec.clusterIP: Invalid value: "10.96.0.44": field is immutable`)
 	}
+	{
+		s := `error when patching "/var/folders/mj/c96jcs7j2cq7xcjfhqjq3m2w0000gn/T/745145319": "" is invalid: patch: Invalid value: "map[data:map[email:aaaaa password:<nil> username:<nil>] metadata:map[annotations:map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"data\":{\"email\":\"aaaaa\"},\"kind\":\"Secret\",\"metadata\":{\"annotations\":{},\"labels\":{\"app.kubernetes.io/instance\":\"test\"},\"name\":\"my-secret\",\"namespace\":\"default\"},\"stringData\":{\"id\":1,\"password\":0,\"username\":\"username\"},\"type\":\"Opaque\"}\n]] stringData:map[id:1 password:0 username:username]]": error decoding from json: illegal base64 data at input byte 4`
+		assert.Equal(t, cleanKubectlOutput(s), `error when patching "/var/folders/mj/c96jcs7j2cq7xcjfhqjq3m2w0000gn/T/745145319": "" is invalid: patch: Invalid value: "": error decoding from json: illegal base64 data at input byte 4`)
+	}
+	{
+		s := `error when patching "/var/folders/mj/c96jcs7j2cq7xcjfhqjq3m2w0000gn/T/2250018703": "" is invalid: patch: Invalid value: "map[data:<nil> metadata:map[annotations:map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"kind\":\"Secret\",\"metadata\":{\"annotations\":{},\"labels\":{\"app.kubernetes.io/instance\":\"test\"},\"name\":\"my-secret\",\"namespace\":\"default\"},\"stringData\":{\"id\":1,\"password\":0,\"username\":\"username\"},\"type\":\"Opaque\"}\n]]]": cannot convert int64 to string`
+		assert.Equal(t, cleanKubectlOutput(s), `error when patching "/var/folders/mj/c96jcs7j2cq7xcjfhqjq3m2w0000gn/T/2250018703": "" is invalid: patch: Invalid value: "": cannot convert int64 to string`)
+	}
+	{
+		s := `Secret in version "v1" cannot be handled as a Secret: json: cannot unmarshal bool into Go struct field Secret.data of type []uint8`
+		assert.Equal(t, cleanKubectlOutput(s), `Secret in version "v1" cannot be handled as a Secret: json: cannot unmarshal bool into Go struct field Secret.data of type []uint8`)
+	}
 }
 
 func TestInClusterKubeConfig(t *testing.T) {