From 0b5903d4589410fa2256ffc57a518d03e7510285 Mon Sep 17 00:00:00 2001 From: visa2 <1927401+visa2@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:19:05 +0800 Subject: [PATCH] fix(settings): keep fields a settings PUT never sent at their stored value PUT /api/v1/admin/settings is a whole-document write. The admin UI always sends the complete document, so saving from the settings page is unaffected both before and after this change. The bug is only reachable when an API client calls the endpoint directly and sends just the fields it wants to change, which is the natural assumption for a PUT on a settings resource. Value-typed fields of UpdateSettingsRequest bind to their zero value when the payload omits them, and buildSystemSettingsUpdates writes every key unconditionally, so such a caller has no way to say "leave this one alone". Omitting a field and explicitly clearing it are indistinguishable on the wire. A caller that sends only the field it wants to change, e.g. {"risk_control_enabled": true} sets that flag and clears every other unguarded field in the same request. Measured against a fully configured store, one such call empties site_name, site_subtitle, api_base_url, contact_info and doc_url, and turns registration_enabled, email_verify_enabled, invitation_code_enabled and turnstile_enabled off. turnstile_enabled alone gates the captcha on login, register, forgot-password and both verify-code endpoints, and email_verify_enabled is a precondition of IsPasswordResetEnabled. The damage is easy to miss. site_name has a built-in fallback, so getStringOrDefault renders the cleared value as the default product name and the login page visibly changes, while the toggles just go quiet. Reopening the settings page reads the already-cleared state back into the form, so correcting the one visible field and saving persists the rest of the damage. Fields that grew their own guard already survive this: the SMTP block falls back to the previous values when smtp_host arrives empty, secret fields are written only when non-empty, and 132 request fields are pointers whose handler merges an omitted field with the stored value. This generalizes that pattern rather than adding a fourth ad-hoc guard. The handler now decodes the payload a second time as a raw field map, resolves the setting key each absent field would have written, and hands that set to the service, which drops those keys before SetMultiple, so the stored value is never touched. Fields the payload does carry are written as before, giving the caller the partial-update semantics it was already assuming. The mapping is reflected off the request's json tags so new fields are covered without maintaining a list; smtp_from_email is the only field whose json name differs from its setting key and is aliased explicitly. Only value-typed fields are filtered. Pointer fields keep whole-document behaviour on purpose: forwarded_client_ip_headers and api_key_acl_trust_forwarded_ip depend on being rewritten on every save to re-normalize fail-closed state, which the malformed forwarded-client-IP header test pins down. An explicitly sent empty value is still a deliberate clear; only absent fields are preserved. A partial write refreshes the in-process caches from storage instead of from the request struct, which holds zero values for whatever the caller omitted. Co-Authored-By: Claude Opus 5 --- .../setting_handler_partial_payload_test.go | 67 +++++++++++++++++++ .../handler/admin/setting_handler_update.go | 66 +++++++++++++++++- backend/internal/service/setting_update.go | 63 ++++++++++++++--- 3 files changed, 185 insertions(+), 11 deletions(-) create mode 100644 backend/internal/handler/admin/setting_handler_partial_payload_test.go diff --git a/backend/internal/handler/admin/setting_handler_partial_payload_test.go b/backend/internal/handler/admin/setting_handler_partial_payload_test.go new file mode 100644 index 0000000000..e39e664a3e --- /dev/null +++ b/backend/internal/handler/admin/setting_handler_partial_payload_test.go @@ -0,0 +1,67 @@ +//go:build unit + +package admin + +import ( + "net/http" + "testing" + + "github.com/Wei-Shaw/sub2api/internal/service" + + "github.com/stretchr/testify/require" +) + +// Saving settings is a whole-document PUT. A client that sends only the field it +// cares about must not reset everything else: a payload as small as +// `{"risk_control_enabled":true}` used to clear site_name, after which +// getStringOrDefault rendered the empty value as the built-in default and the +// login page silently changed name. + +func TestUpdateSettingsPartialPayloadKeepsUnsentKeys(t *testing.T) { + h, repo := newStepUpSwitchTestHandler(t, map[string]string{ + service.SettingKeySiteName: "Example Gateway", + service.SettingKeySiteSubtitle: "Example Gateway Platform", + service.SettingKeySMTPHost: "smtp.example.com", + service.SettingKeySMTPFrom: "noreply@example.com", + service.SettingKeyTurnstileEnabled: "true", + }) + + rec := doUpdateSettings(t, h, map[string]any{"risk_control_enabled": true}, nil) + require.Equal(t, http.StatusOK, rec.Code) + + require.Equal(t, "true", repo.values[service.SettingKeyRiskControlEnabled], + "the field the caller actually sent must be written") + + require.Equal(t, "Example Gateway", repo.values[service.SettingKeySiteName]) + require.Equal(t, "Example Gateway Platform", repo.values[service.SettingKeySiteSubtitle]) + require.Equal(t, "smtp.example.com", repo.values[service.SettingKeySMTPHost]) + require.Equal(t, "noreply@example.com", repo.values[service.SettingKeySMTPFrom]) + require.Equal(t, "true", repo.values[service.SettingKeyTurnstileEnabled]) +} + +// A full payload keeps whole-document semantics: fields explicitly set to their +// zero value are still cleared. +func TestUpdateSettingsFullPayloadStillClearsSentEmptyFields(t *testing.T) { + h, repo := newStepUpSwitchTestHandler(t, map[string]string{ + service.SettingKeySiteName: "Example Gateway", + }) + + rec := doUpdateSettings(t, h, map[string]any{"site_name": ""}, nil) + require.Equal(t, http.StatusOK, rec.Code) + + require.Equal(t, "", repo.values[service.SettingKeySiteName], + "an explicitly sent empty value is a deliberate clear, not an omission") +} + +// smtp_from_email is the one request field whose JSON name differs from its +// setting key; the alias keeps it from being treated as always-omitted. +func TestUpdateSettingsSMTPFromAliasIsWritable(t *testing.T) { + h, repo := newStepUpSwitchTestHandler(t, map[string]string{ + service.SettingKeySMTPFrom: "old@example.com", + }) + + rec := doUpdateSettings(t, h, map[string]any{"smtp_from_email": "new@example.com"}, nil) + require.Equal(t, http.StatusOK, rec.Code) + + require.Equal(t, "new@example.com", repo.values[service.SettingKeySMTPFrom]) +} diff --git a/backend/internal/handler/admin/setting_handler_update.go b/backend/internal/handler/admin/setting_handler_update.go index f852bb7e6f..ec910b5256 100644 --- a/backend/internal/handler/admin/setting_handler_update.go +++ b/backend/internal/handler/admin/setting_handler_update.go @@ -6,6 +6,7 @@ import ( "errors" "log/slog" "net/http" + "reflect" "strings" "github.com/Wei-Shaw/sub2api/internal/config" @@ -15,6 +16,7 @@ import ( "github.com/Wei-Shaw/sub2api/internal/service" "github.com/gin-gonic/gin" + "github.com/gin-gonic/gin/binding" ) // UpdateSettingsRequest 更新设置请求 @@ -375,12 +377,70 @@ func (h *SettingHandler) ensureActorTotpForStepUp(c *gin.Context) bool { return true } +// settingKeyJSONAliases covers the request fields whose JSON name differs from +// the setting key they persist to. Every other field of UpdateSettingsRequest +// is named after its setting key. +var settingKeyJSONAliases = map[string]string{ + "smtp_from_email": service.SettingKeySMTPFrom, +} + +// settingKeyByJSONName maps the value-typed top-level JSON fields of +// UpdateSettingsRequest to the setting key each one writes. Resolved once from +// the struct tags so new fields are covered without touching this file. +// +// Pointer-typed fields are deliberately excluded: they already carry their own +// "omitted = keep the stored value" merge in UpdateSettings, and some of them +// rely on being rewritten on every save to re-normalize fail-closed security +// state (see TestUpdateSettingsMalformedForwardedClientIPHeadersRemainFailClosedWhenOmitted). +// Only the value-typed fields are indistinguishable from a deliberate clear. +var settingKeyByJSONName = buildSettingKeyByJSONName() + +func buildSettingKeyByJSONName() map[string]string { + t := reflect.TypeOf(UpdateSettingsRequest{}) + out := make(map[string]string, t.NumField()) + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + if field.Type.Kind() == reflect.Ptr { + continue + } + name, _, _ := strings.Cut(field.Tag.Get("json"), ",") + if name == "" || name == "-" { + continue + } + if alias, ok := settingKeyJSONAliases[name]; ok { + out[name] = alias + continue + } + out[name] = name + } + return out +} + +// omittedSettingKeys reports the setting keys this payload never mentioned. +// Saving settings is a whole-document PUT, so without this a client that sends +// only the one field it cares about resets every other field to a zero value. +func omittedSettingKeys(sentFields map[string]json.RawMessage) service.OmittedSettingKeys { + omitted := make(service.OmittedSettingKeys, len(settingKeyByJSONName)) + for jsonName, settingKey := range settingKeyByJSONName { + if _, sent := sentFields[jsonName]; !sent { + omitted[settingKey] = struct{}{} + } + } + return omitted +} + func (h *SettingHandler) UpdateSettings(c *gin.Context) { - var req UpdateSettingsRequest - if err := c.ShouldBindJSON(&req); err != nil { + var sentFields map[string]json.RawMessage + if err := c.ShouldBindBodyWith(&sentFields, binding.JSON); err != nil { response.BadRequest(c, "Invalid request: "+err.Error()) return } + var req UpdateSettingsRequest + if err := c.ShouldBindBodyWith(&req, binding.JSON); err != nil { + response.BadRequest(c, "Invalid request: "+err.Error()) + return + } + omitted := omittedSettingKeys(sentFields) previousSettings, err := h.settingService.GetAllSettings(c.Request.Context()) if err != nil { @@ -1692,7 +1752,7 @@ func (h *SettingHandler) UpdateSettings(c *gin.Context) { }, ForceEmailOnThirdPartySignup: boolValueOrDefault(req.ForceEmailOnThirdPartySignup, previousAuthSourceDefaults.ForceEmailOnThirdPartySignup), } - if err := h.settingService.UpdateSettingsWithAuthSourceDefaults(c.Request.Context(), settings, authSourceDefaults); err != nil { + if err := h.settingService.UpdateSettingsWithAuthSourceDefaultsOmitting(c.Request.Context(), settings, authSourceDefaults, omitted); err != nil { response.ErrorFrom(c, err) return } diff --git a/backend/internal/service/setting_update.go b/backend/internal/service/setting_update.go index 7449a3de69..2364b89484 100644 --- a/backend/internal/service/setting_update.go +++ b/backend/internal/service/setting_update.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "math" "strconv" "strings" @@ -15,22 +16,50 @@ import ( infraerrors "github.com/Wei-Shaw/sub2api/internal/pkg/errors" ) +// OmittedSettingKeys marks setting keys the caller's payload never carried. +// SystemSettings is a plain struct, so a field the caller omitted arrives as a +// zero value and is indistinguishable from a deliberate clear. Listing the key +// here drops it from the write, leaving the stored value in place. +// +// A nil or empty set keeps whole-document semantics: every key is written. +type OmittedSettingKeys map[string]struct{} + +func (o OmittedSettingKeys) dropFrom(updates map[string]string) { + for key := range o { + delete(updates, key) + } +} + // UpdateSettings 更新系统设置 func (s *SettingService) UpdateSettings(ctx context.Context, settings *SystemSettings) error { + return s.UpdateSettingsOmitting(ctx, settings, nil) +} + +// UpdateSettingsOmitting persists system settings, leaving the keys in omitted +// at their stored value. +func (s *SettingService) UpdateSettingsOmitting(ctx context.Context, settings *SystemSettings, omitted OmittedSettingKeys) error { updates, err := s.buildSystemSettingsUpdates(ctx, settings) if err != nil { return err } + omitted.dropFrom(updates) - err = s.settingRepo.SetMultiple(ctx, updates) - if err == nil { - s.refreshCachedSettings(settings) + if err := s.settingRepo.SetMultiple(ctx, updates); err != nil { + return err } - return err + s.refreshCachedSettingsAfterWrite(ctx, settings, omitted) + return nil } // UpdateSettingsWithAuthSourceDefaults persists system settings and auth-source defaults in a single write. func (s *SettingService) UpdateSettingsWithAuthSourceDefaults(ctx context.Context, settings *SystemSettings, authDefaults *AuthSourceDefaultSettings) error { + return s.UpdateSettingsWithAuthSourceDefaultsOmitting(ctx, settings, authDefaults, nil) +} + +// UpdateSettingsWithAuthSourceDefaultsOmitting persists system settings and +// auth-source defaults in a single write, leaving the keys in omitted at their +// stored value. +func (s *SettingService) UpdateSettingsWithAuthSourceDefaultsOmitting(ctx context.Context, settings *SystemSettings, authDefaults *AuthSourceDefaultSettings, omitted OmittedSettingKeys) error { updates, err := s.buildSystemSettingsUpdates(ctx, settings) if err != nil { return err @@ -43,12 +72,30 @@ func (s *SettingService) UpdateSettingsWithAuthSourceDefaults(ctx context.Contex for key, value := range authSourceUpdates { updates[key] = value } + omitted.dropFrom(updates) - err = s.settingRepo.SetMultiple(ctx, updates) - if err == nil { - s.refreshCachedSettings(settings) + if err := s.settingRepo.SetMultiple(ctx, updates); err != nil { + return err } - return err + s.refreshCachedSettingsAfterWrite(ctx, settings, omitted) + return nil +} + +// refreshCachedSettingsAfterWrite keeps the in-process caches in step with the +// write that just landed. A partial payload carries zero values for the fields +// it omitted, so in that case the caches are rebuilt from storage rather than +// from the request struct. +func (s *SettingService) refreshCachedSettingsAfterWrite(ctx context.Context, settings *SystemSettings, omitted OmittedSettingKeys) { + if len(omitted) == 0 { + s.refreshCachedSettings(settings) + return + } + stored, err := s.GetAllSettings(ctx) + if err != nil { + slog.Warn("refresh cached settings after partial update failed", "error", err) + return + } + s.refreshCachedSettings(stored) } func (s *SettingService) buildSystemSettingsUpdates(ctx context.Context, settings *SystemSettings) (map[string]string, error) {