fix(secretserver): validate store and map fields correctly (#6616)

* fix(secretserver): validate store and map fields correctly

Signed-off-by: David Binger <david.binger@delinea.com>

* fix(secretserver): avoid redundant Secret Server fetch in GetSecretMap property path

Signed-off-by: David Binger <david.binger@delinea.com>

* fix(secretserver): only treat valid JSON objects as JSON in GetSecretMap

Signed-off-by: David Binger <david.binger@delinea.com>

* fix(secretserver): lowercase Validate error and inline the search call

Signed-off-by: David Binger <david.binger@delinea.com>

* fix(secretserver): use esutils.GetByteValueFromMap to match sibling providers

Signed-off-by: David Binger <david.binger@delinea.com>

---------

Signed-off-by: David Binger <david.binger@delinea.com>

providers/v1/secretserver/client.go

@@ -48,6 +48,10 @@ const (
 	// folderPrefix is the prefix used to encode a folder ID in a remote key.
 	// Format: "folderId:<id>/<name>" (e.g. "folderId:73/my-secret").
 	folderPrefix = "folderId:"
+	// validateSearchText is intentionally unlikely to match a real secret. The
+	// Secret Server search call authenticates the client and returns an empty
+	// result set when no records match.
+	validateSearchText = "external-secrets-validation-check"
 )
 
 // isNotFoundError checks if an error indicates a secret was not found.
@@ -377,14 +381,33 @@ func (c *client) SecretExists(_ context.Context, ref esv1.PushSecretRemoteRef) (
 	return true, nil
 }
 
-// Validate not supported at this time.
 func (c *client) Validate() (esv1.ValidationResult, error) {
+	if c.api == nil {
+		return esv1.ValidationResultError, errors.New("secret server API client is not initialized")
+	}
+	if _, err := c.api.Secrets(validateSearchText, "Name"); err != nil {
+		return esv1.ValidationResultError, fmt.Errorf("failed to validate Secret Server credentials: %w", err)
+	}
 	return esv1.ValidationResultReady, nil
 }
 
 // GetSecretMap retrieves the secret referenced by ref from the Secret Server API
 // and returns it as a map of byte slices.
 func (c *client) GetSecretMap(ctx context.Context, ref esv1.ExternalSecretDataRemoteRef) (map[string][]byte, error) {
+	// When a property is requested, GetSecret already fetches the secret and
+	// applies its own empty-fields guard, so resolve it here and avoid the
+	// second round-trip a separate getSecret call would incur.
+	if ref.Property != "" {
+		value, err := c.GetSecret(ctx, ref)
+		if err != nil {
+			return nil, err
+		}
+		if data, ok, err := jsonObjectToByteMap(string(value)); ok || err != nil {
+			return data, err
+		}
+		return map[string][]byte{ref.Property: value}, nil
+	}
+
 	secret, err := c.getSecret(ctx, ref)
 	if err != nil {
 		return nil, err
@@ -394,21 +417,55 @@ func (c *client) GetSecretMap(ctx context.Context, ref esv1.ExternalSecretDataRe
 		return nil, errors.New("secret contains no fields")
 	}
 
-	secretData := make(map[string]any)
+	if data, ok, err := jsonObjectToByteMap(secret.Fields[0].ItemValue); ok || err != nil {
+		return data, err
+	}
+
+	return fieldsToByteMap(secret.Fields)
+}
+
+func jsonObjectToByteMap(value string) (map[string][]byte, bool, error) {
+	trimmed := strings.TrimSpace(value)
+	// Require a valid JSON object: gating on validity (not just a leading "{")
+	// means a value that merely starts with "{" but is not valid JSON falls
+	// through to the plain-text/field-map path instead of hard-failing, so the
+	// json.Unmarshal error below is effectively defensive only.
+	if !strings.HasPrefix(trimmed, "{") || !gjson.Valid(trimmed) {
+		return nil, false, nil
+	}
 
-	err = json.Unmarshal([]byte(secret.Fields[0].ItemValue), &secretData)
+	secretData := make(map[string]any)
+	err := json.Unmarshal([]byte(value), &secretData)
 	if err != nil {
 		// Do not return the raw error as json.Unmarshal errors may contain
 		// sensitive secret data in the error message
-		return nil, errors.New("failed to unmarshal secret: invalid JSON format")
+		return nil, true, errors.New("failed to unmarshal secret: invalid JSON format")
 	}
 
 	data := make(map[string][]byte)
-	for k, v := range secretData {
-		data[k], err = esutils.GetByteValue(v)
+	for k := range secretData {
+		data[k], err = esutils.GetByteValueFromMap(secretData, k)
 		if err != nil {
-			return nil, err
+			return nil, true, err
+		}
+	}
+	return data, true, nil
+}
+
+func fieldsToByteMap(fields []server.SecretField) (map[string][]byte, error) {
+	data := make(map[string][]byte, len(fields))
+	for _, field := range fields {
+		key := field.Slug
+		if key == "" {
+			key = field.FieldName
+		}
+		if key == "" && field.FieldID > 0 {
+			key = strconv.Itoa(field.FieldID)
+		}
+		if key == "" {
+			return nil, errors.New("secret field has no slug, field name, or field ID")
 		}
+		data[key] = []byte(field.ItemValue)
 	}
 	return data, nil
 }

providers/v1/secretserver/client_test.go

@@ -38,7 +38,8 @@ var (
 )
 
 type fakeAPI struct {
-	secrets []*server.Secret
+	secrets    []*server.Secret
+	secretsErr error
 }
 
 const (
@@ -56,6 +57,9 @@ func (f *fakeAPI) Secret(id int) (*server.Secret, error) {
 }
 
 func (f *fakeAPI) Secrets(searchText, _ string) ([]server.Secret, error) {
+	if f.secretsErr != nil {
+		return nil, f.secretsErr
+	}
 	// Match real SDK behavior: return ([]Secret{}, nil) for zero matches,
 	// NOT (nil, errNotFound). The real SDK's searchResources returns an empty
 	// SearchResult.Records slice and make([]Secret, 0).
@@ -844,13 +848,21 @@ func TestValidate(t *testing.T) {
 	assert.Equal(t, esv1.ValidationResultReady, result)
 }
 
+func TestValidateAPIError(t *testing.T) {
+	c := &client{api: &fakeAPI{secretsErr: errors.New("401 Unauthorized: invalid credentials")}}
+
+	result, err := c.Validate()
+	assert.Error(t, err)
+	assert.Equal(t, esv1.ValidationResultError, result)
+	assert.Contains(t, err.Error(), "failed to validate Secret Server credentials")
+}
+
 // TestValidateNilAPI tests the Validate functionality with nil API.
 func TestValidateNilAPI(t *testing.T) {
 	c := &client{api: nil}
 	result, err := c.Validate()
-	// Validate always succeeds and returns ValidationResultReady regardless of API state
-	assert.NoError(t, err)
-	assert.Equal(t, esv1.ValidationResultReady, result)
+	assert.Error(t, err)
+	assert.Equal(t, esv1.ValidationResultError, result)
 }
 
 // TestGetSecretMap tests the GetSecretMap functionality.
@@ -874,15 +886,14 @@ func TestGetSecretMap(t *testing.T) {
 			},
 			wantErr: false,
 		},
-		// The following test case expects an error because the secret with Key "9999"
-		// contains invalid JSON ("simulated error") which causes unmarshalling to fail
-		// in GetSecretMap, rather than because the secret is missing.
-		"error when secret not found": {
+		"successfully retrieve non-JSON data field map": {
 			ref: esv1.ExternalSecretDataRemoteRef{
 				Key: "9999",
 			},
-			want:    nil,
-			wantErr: true,
+			want: map[string][]byte{
+				"data": []byte("simulated error"),
+			},
+			wantErr: false,
 		},
 		"error when secret has nil fields": {
 			ref: esv1.ExternalSecretDataRemoteRef{
@@ -909,6 +920,25 @@ func TestGetSecretMap(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		"successfully retrieve multi-field template secret map": {
+			ref: esv1.ExternalSecretDataRemoteRef{
+				Key: "4000",
+			},
+			want: map[string][]byte{
+				"username": []byte("usernamevalue"),
+				"password": []byte("passwordvalue"),
+			},
+			wantErr: false,
+		},
+		"successfully retrieve plain text field map": {
+			ref: esv1.ExternalSecretDataRemoteRef{
+				Key: "5000",
+			},
+			want: map[string][]byte{
+				"content": []byte("non-json-secret-value"),
+			},
+			wantErr: false,
+		},
 	}
 
 	for name, tc := range testCases {
@@ -926,17 +956,70 @@ func TestGetSecretMap(t *testing.T) {
 	}
 }
 
-// TestGetSecretMapInvalidJSON tests GetSecretMap with invalid JSON in secret.
+func TestGetSecretMapWithProperty(t *testing.T) {
+	ctx := context.Background()
+	c := &client{api: &fakeAPI{secrets: []*server.Secret{
+		{
+			ID:   1000,
+			Name: "json-secret",
+			Fields: []server.SecretField{
+				{
+					FieldName: "Data",
+					Slug:      "data",
+					ItemValue: `{"credentials":{"username":"alice","password":"secret"},"server":"example.com"}`,
+				},
+			},
+		},
+		{
+			ID:   2000,
+			Name: "multi-field-secret",
+			Fields: []server.SecretField{
+				{FieldName: "Username", Slug: "username", ItemValue: "bob"},
+				{FieldName: "Password", Slug: "password", ItemValue: "secret"},
+			},
+		},
+	}}}
+
+	t.Run("extracts JSON object property with existing gjson behavior", func(t *testing.T) {
+		got, err := c.GetSecretMap(ctx, esv1.ExternalSecretDataRemoteRef{
+			Key:      "1000",
+			Property: "credentials",
+		})
+
+		assert.NoError(t, err)
+		assert.Equal(t, map[string][]byte{
+			"username": []byte("alice"),
+			"password": []byte("secret"),
+		}, got)
+	})
+
+	t.Run("returns a single field map for scalar properties", func(t *testing.T) {
+		got, err := c.GetSecretMap(ctx, esv1.ExternalSecretDataRemoteRef{
+			Key:      "2000",
+			Property: "username",
+		})
+
+		assert.NoError(t, err)
+		assert.Equal(t, map[string][]byte{
+			"username": []byte("bob"),
+		}, got)
+	})
+}
+
+// TestGetSecretMapInvalidJSON verifies that a first field whose value starts
+// with "{" but is not valid JSON is treated as a plain field value (mapped by
+// slug) rather than causing GetSecretMap to fail.
 func TestGetSecretMapInvalidJSON(t *testing.T) {
 	ctx := context.Background()
 	c := newTestClient(t)
 
-	// Overwrite one secret's value with invalid JSON
+	// Overwrite one secret's value with something that looks like JSON but isn't.
 	fake := c.(*client).api.(*fakeAPI)
 	fake.secrets[0].Fields[0].ItemValue = "{invalid-json"
 
-	_, err := c.GetSecretMap(ctx, esv1.ExternalSecretDataRemoteRef{Key: "1000"})
-	assert.Error(t, err)
+	got, err := c.GetSecretMap(ctx, esv1.ExternalSecretDataRemoteRef{Key: "1000"})
+	assert.NoError(t, err)
+	assert.Equal(t, []byte("{invalid-json"), got["data"])
 }
 
 // TestGetSecretMapValidJSON tests GetSecretMap with valid JSON data succeeds.