fix(api): make credentials write-only and restrict sensitive settings to admins

Security fixes for credential leakage across three resources:

- NOTIFICATION setting: restrict GetInstanceSetting to admin-only
  (was publicly accessible, exposing SMTP credentials)
- SMTP password: never return SmtpPassword in API responses (write-only)
- S3 secret: never return AccessKeySecret in API responses (write-only)
- OAuth2 ClientSecret: never return in API responses for any role
  (was previously returned to admins); remove redactIdentityProviderResponse
  in favor of omitting the field at the conversion layer
- Preserve-on-empty: when updating settings with an empty credential
  field, preserve the existing stored value instead of overwriting
  (applies to SmtpPassword, AccessKeySecret, and ClientSecret)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

server/router/api/v1/idp_service.go

@@ -49,17 +49,8 @@ func (s *APIV1Service) ListIdentityProviders(ctx context.Context, _ *v1pb.ListId
 	response := &v1pb.ListIdentityProvidersResponse{
 		IdentityProviders: []*v1pb.IdentityProvider{},
 	}
-
-	// Default to lowest-privilege role, update later based on real role
-	currentUserRole := store.RoleUser
-	currentUser, err := s.fetchCurrentUser(ctx)
-	if err == nil && currentUser != nil {
-		currentUserRole = currentUser.Role
-	}
-
 	for _, identityProvider := range identityProviders {
-		identityProviderConverted := convertIdentityProviderFromStore(identityProvider)
-		response.IdentityProviders = append(response.IdentityProviders, redactIdentityProviderResponse(identityProviderConverted, currentUserRole))
+		response.IdentityProviders = append(response.IdentityProviders, convertIdentityProviderFromStore(identityProvider))
 	}
 	return response, nil
 }
@@ -79,15 +70,7 @@ func (s *APIV1Service) GetIdentityProvider(ctx context.Context, request *v1pb.Ge
 		return nil, status.Errorf(codes.NotFound, "identity provider not found")
 	}
 
-	// Default to lowest-privilege role, update later based on real role
-	currentUserRole := store.RoleUser
-	currentUser, err := s.fetchCurrentUser(ctx)
-	if err == nil && currentUser != nil {
-		currentUserRole = currentUser.Role
-	}
-
-	identityProviderConverted := convertIdentityProviderFromStore(identityProvider)
-	return redactIdentityProviderResponse(identityProviderConverted, currentUserRole), nil
+	return convertIdentityProviderFromStore(identityProvider), nil
 }
 
 func (s *APIV1Service) UpdateIdentityProvider(ctx context.Context, request *v1pb.UpdateIdentityProviderRequest) (*v1pb.IdentityProvider, error) {
@@ -137,6 +120,15 @@ func (s *APIV1Service) UpdateIdentityProvider(ctx context.Context, request *v1pb
 		}
 	}
 
+	// Preserve write-only credential when the caller sends an empty value.
+	if update.Config != nil {
+		if oauth2Config := update.Config.GetOauth2Config(); oauth2Config != nil && oauth2Config.ClientSecret == "" {
+			if existingOAuth := existing.Config.GetOauth2Config(); existingOAuth != nil {
+				oauth2Config.ClientSecret = existingOAuth.ClientSecret
+			}
+		}
+	}
+
 	identityProvider, err := s.Store.UpdateIdentityProvider(ctx, update)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "failed to update identity provider, error: %+v", err)
@@ -188,12 +180,12 @@ func convertIdentityProviderFromStore(identityProvider *storepb.IdentityProvider
 		temp.Config = &v1pb.IdentityProviderConfig{
 			Config: &v1pb.IdentityProviderConfig_Oauth2Config{
 				Oauth2Config: &v1pb.OAuth2Config{
-					ClientId:     oauth2Config.ClientId,
-					ClientSecret: oauth2Config.ClientSecret,
-					AuthUrl:      oauth2Config.AuthUrl,
-					TokenUrl:     oauth2Config.TokenUrl,
-					UserInfoUrl:  oauth2Config.UserInfoUrl,
-					Scopes:       oauth2Config.Scopes,
+					ClientId: oauth2Config.ClientId,
+					// ClientSecret is write-only: never returned in responses.
+					AuthUrl:     oauth2Config.AuthUrl,
+					TokenUrl:    oauth2Config.TokenUrl,
+					UserInfoUrl: oauth2Config.UserInfoUrl,
+					Scopes:     oauth2Config.Scopes,
 					FieldMapping: &v1pb.FieldMapping{
 						Identifier:  oauth2Config.FieldMapping.Identifier,
 						DisplayName: oauth2Config.FieldMapping.DisplayName,
@@ -242,12 +234,3 @@ func convertIdentityProviderConfigToStore(identityProviderType v1pb.IdentityProv
 	return nil
 }
 
-func redactIdentityProviderResponse(identityProvider *v1pb.IdentityProvider, userRole store.Role) *v1pb.IdentityProvider {
-	if userRole != store.RoleAdmin {
-		if identityProvider.Type == v1pb.IdentityProvider_OAUTH2 {
-			identityProvider.Config.GetOauth2Config().ClientSecret = ""
-		}
-	}
-
-	return identityProvider
-}

server/router/api/v1/instance_service.go

@@ -71,8 +71,9 @@ func (s *APIV1Service) GetInstanceSetting(ctx context.Context, request *v1pb.Get
 		return nil, status.Errorf(codes.NotFound, "instance setting not found")
 	}
 
-	// For storage setting, only admin can get it.
-	if instanceSetting.Key == storepb.InstanceSettingKey_STORAGE {
+	// Storage and notification settings contain credentials; restrict to admins only.
+	if instanceSetting.Key == storepb.InstanceSettingKey_STORAGE ||
+		instanceSetting.Key == storepb.InstanceSettingKey_NOTIFICATION {
 		user, err := s.fetchCurrentUser(ctx)
 		if err != nil {
 			return nil, status.Errorf(codes.Internal, "failed to get current user: %v", err)
@@ -108,6 +109,26 @@ func (s *APIV1Service) UpdateInstanceSetting(ctx context.Context, request *v1pb.
 	}
 
 	updateSetting := convertInstanceSettingToStore(request.Setting)
+
+	// Preserve write-only credential fields when the caller sends an empty value.
+	// An empty string means "no change", not "clear the credential".
+	switch updateSetting.Key {
+	case storepb.InstanceSettingKey_NOTIFICATION:
+		if notif := updateSetting.GetNotificationSetting(); notif != nil && notif.Email != nil && notif.Email.SmtpPassword == "" {
+			existing, err := s.Store.GetInstanceNotificationSetting(ctx)
+			if err == nil && existing != nil && existing.Email != nil {
+				notif.Email.SmtpPassword = existing.Email.SmtpPassword
+			}
+		}
+	case storepb.InstanceSettingKey_STORAGE:
+		if storage := updateSetting.GetStorageSetting(); storage != nil && storage.S3Config != nil && storage.S3Config.AccessKeySecret == "" {
+			existing, err := s.Store.GetInstanceStorageSetting(ctx)
+			if err == nil && existing != nil && existing.S3Config != nil {
+				storage.S3Config.AccessKeySecret = existing.S3Config.AccessKeySecret
+			}
+		}
+	}
+
 	instanceSetting, err := s.Store.UpsertInstanceSetting(ctx, updateSetting)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "failed to upsert instance setting: %v", err)
@@ -240,12 +261,12 @@ func convertInstanceStorageSettingFromStore(settingpb *storepb.InstanceStorageSe
 	}
 	if settingpb.S3Config != nil {
 		setting.S3Config = &v1pb.InstanceSetting_StorageSetting_S3Config{
-			AccessKeyId:     settingpb.S3Config.AccessKeyId,
-			AccessKeySecret: settingpb.S3Config.AccessKeySecret,
-			Endpoint:        settingpb.S3Config.Endpoint,
-			Region:          settingpb.S3Config.Region,
-			Bucket:          settingpb.S3Config.Bucket,
-			UsePathStyle:    settingpb.S3Config.UsePathStyle,
+			AccessKeyId:  settingpb.S3Config.AccessKeyId,
+			// AccessKeySecret is write-only: never returned in responses.
+			Endpoint:    settingpb.S3Config.Endpoint,
+			Region:      settingpb.S3Config.Region,
+			Bucket:      settingpb.S3Config.Bucket,
+			UsePathStyle: settingpb.S3Config.UsePathStyle,
 		}
 	}
 	return setting
@@ -341,12 +362,12 @@ func convertInstanceNotificationSettingFromStore(setting *storepb.InstanceNotifi
 			SmtpHost:     setting.Email.SmtpHost,
 			SmtpPort:     setting.Email.SmtpPort,
 			SmtpUsername: setting.Email.SmtpUsername,
-			SmtpPassword: setting.Email.SmtpPassword,
-			FromEmail:    setting.Email.FromEmail,
-			FromName:     setting.Email.FromName,
-			ReplyTo:      setting.Email.ReplyTo,
-			UseTls:       setting.Email.UseTls,
-			UseSsl:       setting.Email.UseSsl,
+			// SmtpPassword is write-only: never returned in responses.
+			FromEmail: setting.Email.FromEmail,
+			FromName:  setting.Email.FromName,
+			ReplyTo:   setting.Email.ReplyTo,
+			UseTls:    setting.Email.UseTls,
+			UseSsl:    setting.Email.UseSsl,
 		}
 	}
 	return notificationSetting

server/router/api/v1/test/idp_service_test.go

@@ -8,6 +8,8 @@ import (
 	"google.golang.org/protobuf/types/known/fieldmaskpb"
 
 	v1pb "github.com/usememos/memos/proto/gen/api/v1"
+	apiv1 "github.com/usememos/memos/server/router/api/v1"
+	"github.com/usememos/memos/store"
 )
 
 func TestCreateIdentityProvider(t *testing.T) {
@@ -233,7 +235,7 @@ func TestGetIdentityProvider(t *testing.T) {
 			Name: created.Name,
 		}
 
-		// Test unauthenticated, should not contain client secret
+		// ClientSecret is write-only: never returned in responses, even to admins.
 		resp, err := ts.Service.GetIdentityProvider(ctx, getReq)
 		require.NoError(t, err)
 		require.NotNil(t, resp)
@@ -242,18 +244,16 @@ func TestGetIdentityProvider(t *testing.T) {
 		require.Equal(t, v1pb.IdentityProvider_OAUTH2, resp.Type)
 		require.NotNil(t, resp.Config.GetOauth2Config())
 		require.Equal(t, "test-client", resp.Config.GetOauth2Config().ClientId)
-		require.Equal(t, "", resp.Config.GetOauth2Config().ClientSecret)
+		require.Empty(t, resp.Config.GetOauth2Config().ClientSecret,
+			"ClientSecret must never be returned in responses")
 
-		// Test as host user, should contain client secret
-		respHostUser, err := ts.Service.GetIdentityProvider(userCtx, getReq)
+		// Same for admin: secret is still write-only.
+		respAdmin, err := ts.Service.GetIdentityProvider(userCtx, getReq)
 		require.NoError(t, err)
-		require.NotNil(t, respHostUser)
-		require.Equal(t, created.Name, respHostUser.Name)
-		require.Equal(t, "Test Provider", respHostUser.Title)
-		require.Equal(t, v1pb.IdentityProvider_OAUTH2, respHostUser.Type)
-		require.NotNil(t, respHostUser.Config.GetOauth2Config())
-		require.Equal(t, "test-client", respHostUser.Config.GetOauth2Config().ClientId)
-		require.Equal(t, "test-secret", respHostUser.Config.GetOauth2Config().ClientSecret)
+		require.NotNil(t, respAdmin)
+		require.Equal(t, "test-client", respAdmin.Config.GetOauth2Config().ClientId)
+		require.Empty(t, respAdmin.Config.GetOauth2Config().ClientSecret,
+			"ClientSecret must never be returned in responses, even to admins")
 	})
 
 	t.Run("GetIdentityProvider not found", func(t *testing.T) {
@@ -361,6 +361,67 @@ func TestUpdateIdentityProvider(t *testing.T) {
 		require.Equal(t, "updated-client", updated.Config.GetOauth2Config().ClientId)
 	})
 
+	t.Run("UpdateIdentityProvider empty ClientSecret preserves existing credential", func(t *testing.T) {
+		ts := NewTestService(t)
+		defer ts.Cleanup()
+
+		hostUser, err := ts.CreateHostUser(ctx, "admin")
+		require.NoError(t, err)
+		userCtx := ts.CreateUserContext(ctx, hostUser.ID)
+
+		// Create IDP with a real secret.
+		created, err := ts.Service.CreateIdentityProvider(userCtx, &v1pb.CreateIdentityProviderRequest{
+			IdentityProvider: &v1pb.IdentityProvider{
+				Title: "Preserve Secret Test",
+				Type:  v1pb.IdentityProvider_OAUTH2,
+				Config: &v1pb.IdentityProviderConfig{
+					Config: &v1pb.IdentityProviderConfig_Oauth2Config{
+						Oauth2Config: &v1pb.OAuth2Config{
+							ClientId:     "cid",
+							ClientSecret: "original-secret",
+							AuthUrl:      "https://ex.com/auth",
+							TokenUrl:     "https://ex.com/token",
+							UserInfoUrl:  "https://ex.com/user",
+							FieldMapping: &v1pb.FieldMapping{Identifier: "id"},
+						},
+					},
+				},
+			},
+		})
+		require.NoError(t, err)
+
+		// Update with empty ClientSecret (simulating UI that doesn't resend the secret).
+		_, err = ts.Service.UpdateIdentityProvider(userCtx, &v1pb.UpdateIdentityProviderRequest{
+			IdentityProvider: &v1pb.IdentityProvider{
+				Name:  created.Name,
+				Title: "Updated Title",
+				Type:  v1pb.IdentityProvider_OAUTH2,
+				Config: &v1pb.IdentityProviderConfig{
+					Config: &v1pb.IdentityProviderConfig_Oauth2Config{
+						Oauth2Config: &v1pb.OAuth2Config{
+							ClientId:     "cid",
+							ClientSecret: "", // empty = preserve existing
+							AuthUrl:      "https://ex.com/auth",
+							TokenUrl:     "https://ex.com/token",
+							UserInfoUrl:  "https://ex.com/user",
+							FieldMapping: &v1pb.FieldMapping{Identifier: "id"},
+						},
+					},
+				},
+			},
+			UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"title", "config"}},
+		})
+		require.NoError(t, err)
+
+		// Verify the stored secret was preserved by reading from store directly.
+		uid, _ := apiv1.ExtractIdentityProviderUIDFromName(created.Name)
+		stored, err := ts.Store.GetIdentityProvider(ctx, &store.FindIdentityProvider{UID: &uid})
+		require.NoError(t, err)
+		require.Equal(t, "original-secret", stored.Config.GetOauth2Config().ClientSecret,
+			"existing ClientSecret must be preserved when an empty value is sent")
+		require.Equal(t, "Updated Title", stored.Name)
+	})
+
 	t.Run("UpdateIdentityProvider missing update mask", func(t *testing.T) {
 		ts := NewTestService(t)
 		defer ts.Cleanup()

server/router/api/v1/test/instance_service_test.go

@@ -204,21 +204,38 @@ func TestGetInstanceSetting(t *testing.T) {
 		require.Empty(t, resp.GetTagsSetting().GetTags())
 	})
 
-	t.Run("GetInstanceSetting - notification setting", func(t *testing.T) {
+	t.Run("GetInstanceSetting - notification setting requires admin", func(t *testing.T) {
 		ts := NewTestService(t)
 		defer ts.Cleanup()
 
-		req := &v1pb.GetInstanceSettingRequest{
-			Name: "instance/settings/NOTIFICATION",
-		}
-		resp, err := ts.Service.GetInstanceSetting(ctx, req)
+		admin, err := ts.CreateHostUser(ctx, "admin")
+		require.NoError(t, err)
+		adminCtx := ts.CreateUserContext(ctx, admin.ID)
+
+		regularUser, err := ts.CreateRegularUser(ctx, "user")
+		require.NoError(t, err)
+		userCtx := ts.CreateUserContext(ctx, regularUser.ID)
+
+		req := &v1pb.GetInstanceSettingRequest{Name: "instance/settings/NOTIFICATION"}
 
+		// Unauthenticated request must be rejected.
+		_, err = ts.Service.GetInstanceSetting(ctx, req)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "not authenticated")
+
+		// Non-admin request must be rejected.
+		_, err = ts.Service.GetInstanceSetting(userCtx, req)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "permission denied")
+
+		// Admin request succeeds and does NOT expose SmtpPassword.
+		resp, err := ts.Service.GetInstanceSetting(adminCtx, req)
 		require.NoError(t, err)
 		require.NotNil(t, resp)
 		require.Equal(t, "instance/settings/NOTIFICATION", resp.Name)
 		require.NotNil(t, resp.GetNotificationSetting())
-		require.NotNil(t, resp.GetNotificationSetting().GetEmail())
-		require.False(t, resp.GetNotificationSetting().GetEmail().GetEnabled())
+		require.Empty(t, resp.GetNotificationSetting().GetEmail().GetSmtpPassword(),
+			"SmtpPassword must never be returned in responses")
 	})
 
 	t.Run("GetInstanceSetting - invalid setting name", func(t *testing.T) {
@@ -301,14 +318,16 @@ func TestUpdateInstanceSetting(t *testing.T) {
 		require.Contains(t, err.Error(), "invalid instance setting")
 	})
 
-	t.Run("UpdateInstanceSetting - notification setting", func(t *testing.T) {
+	t.Run("UpdateInstanceSetting - notification setting password is write-only", func(t *testing.T) {
 		ts := NewTestService(t)
 		defer ts.Cleanup()
 
 		hostUser, err := ts.CreateHostUser(ctx, "admin")
 		require.NoError(t, err)
+		adminCtx := ts.CreateUserContext(ctx, hostUser.ID)
 
-		resp, err := ts.Service.UpdateInstanceSetting(ts.CreateUserContext(ctx, hostUser.ID), &v1pb.UpdateInstanceSettingRequest{
+		// Save notification setting with a password.
+		resp, err := ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{
 			Setting: &v1pb.InstanceSetting{
 				Name: "instance/settings/NOTIFICATION",
 				Value: &v1pb.InstanceSetting_NotificationSetting_{
@@ -330,9 +349,117 @@ func TestUpdateInstanceSetting(t *testing.T) {
 			UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"notification_setting"}},
 		})
 		require.NoError(t, err)
-		require.NotNil(t, resp.GetNotificationSetting())
-		require.NotNil(t, resp.GetNotificationSetting().GetEmail())
 		require.True(t, resp.GetNotificationSetting().GetEmail().GetEnabled())
 		require.Equal(t, "smtp.example.com", resp.GetNotificationSetting().GetEmail().GetSmtpHost())
+		// Password must not be returned even in the update response.
+		require.Empty(t, resp.GetNotificationSetting().GetEmail().GetSmtpPassword(),
+			"SmtpPassword must never be returned in responses")
+	})
+
+	t.Run("UpdateInstanceSetting - empty password preserves existing credential", func(t *testing.T) {
+		ts := NewTestService(t)
+		defer ts.Cleanup()
+
+		hostUser, err := ts.CreateHostUser(ctx, "admin")
+		require.NoError(t, err)
+		adminCtx := ts.CreateUserContext(ctx, hostUser.ID)
+
+		notificationSetting := &v1pb.InstanceSetting{
+			Name: "instance/settings/NOTIFICATION",
+			Value: &v1pb.InstanceSetting_NotificationSetting_{
+				NotificationSetting: &v1pb.InstanceSetting_NotificationSetting{
+					Email: &v1pb.InstanceSetting_NotificationSetting_EmailSetting{
+						Enabled:      true,
+						SmtpHost:     "smtp.example.com",
+						SmtpPort:     587,
+						SmtpUsername: "bot@example.com",
+						SmtpPassword: "original-password",
+						FromEmail:    "bot@example.com",
+					},
+				},
+			},
+		}
+
+		// First save with a real password.
+		_, err = ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{
+			Setting: notificationSetting,
+		})
+		require.NoError(t, err)
+
+		// Second update with an empty password (simulating a UI that doesn't re-send the secret).
+		notificationSetting.GetNotificationSetting().GetEmail().SmtpPassword = ""
+		notificationSetting.GetNotificationSetting().GetEmail().SmtpHost = "smtp2.example.com"
+		_, err = ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{
+			Setting: notificationSetting,
+		})
+		require.NoError(t, err)
+
+		// The stored setting should have preserved the original password.
+		stored, err := ts.Store.GetInstanceNotificationSetting(ctx)
+		require.NoError(t, err)
+		require.Equal(t, "original-password", stored.GetEmail().GetSmtpPassword(),
+			"existing SmtpPassword must be preserved when an empty value is sent")
+		require.Equal(t, "smtp2.example.com", stored.GetEmail().GetSmtpHost())
+	})
+
+	t.Run("UpdateInstanceSetting - S3 secret is write-only and preserved on empty", func(t *testing.T) {
+		ts := NewTestService(t)
+		defer ts.Cleanup()
+
+		hostUser, err := ts.CreateHostUser(ctx, "admin")
+		require.NoError(t, err)
+		adminCtx := ts.CreateUserContext(ctx, hostUser.ID)
+
+		// Save storage setting with a real secret.
+		_, err = ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{
+			Setting: &v1pb.InstanceSetting{
+				Name: "instance/settings/STORAGE",
+				Value: &v1pb.InstanceSetting_StorageSetting_{
+					StorageSetting: &v1pb.InstanceSetting_StorageSetting{
+						S3Config: &v1pb.InstanceSetting_StorageSetting_S3Config{
+							AccessKeyId:     "AKID",
+							AccessKeySecret: "super-secret",
+							Endpoint:        "s3.example.com",
+							Region:          "us-east-1",
+							Bucket:          "memos",
+						},
+					},
+				},
+			},
+		})
+		require.NoError(t, err)
+
+		// Read back: secret must not be returned.
+		resp, err := ts.Service.GetInstanceSetting(adminCtx, &v1pb.GetInstanceSettingRequest{
+			Name: "instance/settings/STORAGE",
+		})
+		require.NoError(t, err)
+		require.Empty(t, resp.GetStorageSetting().GetS3Config().GetAccessKeySecret(),
+			"AccessKeySecret must never be returned in responses")
+
+		// Update with empty secret; original must be preserved in the store.
+		_, err = ts.Service.UpdateInstanceSetting(adminCtx, &v1pb.UpdateInstanceSettingRequest{
+			Setting: &v1pb.InstanceSetting{
+				Name: "instance/settings/STORAGE",
+				Value: &v1pb.InstanceSetting_StorageSetting_{
+					StorageSetting: &v1pb.InstanceSetting_StorageSetting{
+						S3Config: &v1pb.InstanceSetting_StorageSetting_S3Config{
+							AccessKeyId:     "AKID",
+							AccessKeySecret: "", // omitted / not changed
+							Endpoint:        "s3-v2.example.com",
+							Region:          "us-east-1",
+							Bucket:          "memos",
+						},
+					},
+				},
+			},
+		})
+		require.NoError(t, err)
+
+		stored, err := ts.Store.GetInstanceStorageSetting(ctx)
+		require.NoError(t, err)
+		require.Equal(t, "super-secret", stored.GetS3Config().GetAccessKeySecret(),
+			"existing AccessKeySecret must be preserved when an empty value is sent")
+		require.Equal(t, "s3-v2.example.com", stored.GetS3Config().GetEndpoint())
 	})
 }