fix(config): validate ri_exchange_mode on PUT /api/config

go.mod

@@ -59,7 +59,7 @@ require (
 	go.opentelemetry.io/otel/metric v1.45.0 // indirect
 	go.opentelemetry.io/otel/trace v1.45.0 // indirect
 	golang.org/x/crypto v0.58.0
-	golang.org/x/net v0.61.0 // indirect
+	golang.org/x/net v0.62.0 // indirect
 	golang.org/x/oauth2 v0.37.0
 	golang.org/x/sync v0.24.0
 	golang.org/x/sys v0.49.0 // indirect

go.sum

@@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54=
 golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w=
 golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg=
 golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs=
-golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw=
-golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI=
+golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY=
+golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI=
 golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98=
 golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58=
 golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA=

internal/api/handler.go

@@ -583,8 +583,8 @@ func riExchangeArmStateOf(cfg *config.GlobalConfig) riExchangeArmState {
 // RunAutoExchange executes without approval only for the literal "auto" and
 // refuses any other unknown mode (including "") with an error, so a typo
 // fails the scheduled run loudly instead of executing. PUT /api/config
-// unmarshals onto GlobalConfig wholesale and GlobalConfig.Validate never
-// constrains RIExchangeMode, so any non-"manual" string can be stored; treating
+// validates newly assigned modes, but preserves legacy unknown modes during
+// unrelated partial writes. Treating
 // every such value as armed keeps the grant check closed if the consumer's
 // mode handling ever changes.
 func (s riExchangeArmState) armed() bool {

internal/api/handler_config.go

@@ -68,19 +68,12 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR
 		return nil, permErr
 	}
 
-	// Reject a malformed body before any DB work so a bad request fails fast
-	// (and never opens a transaction). Keeps the nil-store fast-fail contract.
-	if !json.Valid([]byte(req.Body)) {
-		return nil, NewClientError(400, "invalid request body")
-	}
-
-	// Presence map of the top-level keys the caller actually sent, used to
-	// replace (not merge) grace_period_days when present.
-	var present map[string]json.RawMessage
-	if err := json.Unmarshal([]byte(req.Body), &present); err != nil {
-		return nil, NewClientError(400, "invalid request body")
+	present, parseErr := globalConfigWriteFields(req.Body)
+	if parseErr != nil {
+		return nil, parseErr
 	}
 	_, gracePresent := present["grace_period_days"]
+	modeSent := riExchangeModeSent(present)
 	if err := h.requireGlobalConfigScope(ctx, session); err != nil {
 		return nil, err
 	}
@@ -121,8 +114,8 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR
 		if uErr := json.Unmarshal([]byte(req.Body), existing); uErr != nil {
 			return NewClientError(400, "invalid request body")
 		}
-		if vErr := existing.Validate(); vErr != nil {
-			return NewClientError(400, fmt.Sprintf("validation error: %s", vErr))
+		if vErr := validateGlobalConfigWrite(existing, modeSent || existing.RIExchangeMode != before.mode); vErr != nil {
+			return vErr
 		}
 		if gErr := requireRIExchangeAutoModeGrant(before, riExchangeArmStateOf(existing), grants.riExchange); gErr != nil {
 			return gErr
@@ -138,6 +131,51 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR
 	return &StatusResponse{Status: "updated"}, nil
 }
 
+// globalConfigWriteFields validates the body before any DB work and returns
+// the supplied top-level fields, including explicit nulls. The write uses this
+// presence map to replace grace_period_days and validate ri_exchange_mode.
+func globalConfigWriteFields(body string) (map[string]json.RawMessage, error) {
+	if !json.Valid([]byte(body)) {
+		return nil, NewClientError(400, "invalid request body")
+	}
+	var present map[string]json.RawMessage
+	if err := json.Unmarshal([]byte(body), &present); err != nil {
+		return nil, NewClientError(400, "invalid request body")
+	}
+	return present, nil
+}
+
+// riExchangeModeSent reports whether the body writes ri_exchange_mode. A JSON
+// null does not: json.Unmarshal leaves the stored value in place for it.
+func riExchangeModeSent(present map[string]json.RawMessage) bool {
+	for key, raw := range present {
+		if strings.EqualFold(key, "ri_exchange_mode") && string(raw) != "null" {
+			return true
+		}
+	}
+	return false
+}
+
+// validateGlobalConfigWrite validates the merged config as a 400.
+//
+// ri_exchange_mode is checked when explicitly set or changed by JSON decoding
+// (including duplicate keys followed by null), with
+// the same manual|auto rule PUT /api/ri-exchange/config applies. Checking the
+// merged value instead would refuse every partial save, including the
+// laddering kill-switch toggle, on a row stored before this check with a mode
+// such as "Auto". Such a row keeps its value until a caller sends a valid one,
+// and armed() keeps treating it as auto. Runs before the arming gates so a
+// malformed write never reveals grant state.
+func validateGlobalConfigWrite(cfg *config.GlobalConfig, modeSent bool) error {
+	if modeSent && !config.IsValidRIExchangeMode(cfg.RIExchangeMode) {
+		return NewClientError(400, "ri_exchange_mode must be 'manual' or 'auto'")
+	}
+	if err := cfg.Validate(); err != nil {
+		return NewClientError(400, fmt.Sprintf("validation error: %s", err))
+	}
+	return nil
+}
+
 // requireSwitchGrants chains the arming gates of the two money-spending
 // switches: auto-purchase, then ladder execution. purchasesGrant is the
 // pre-resolved execute:purchases outcome (armingGrants.purchases).

internal/api/handler_config_ri_exchange_mode_test.go

@@ -0,0 +1,171 @@
+package api
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/mocks"
+)
+
+// Coverage for issue #1196: PUT /api/config must validate ri_exchange_mode the
+// same way PUT /api/ri-exchange/config does (exactly "manual" or "auto").
+//
+// The principal below holds execute:ri-exchange, so the #1765 auto-mode gate
+// never refuses these writes. Without the check, every invalid mode is
+// therefore stored as-is, which is what makes each refusal case fail before the
+// fix rather than passing on a 403 from the gate.
+
+const riExchangeModeErr = "ri_exchange_mode must be 'manual' or 'auto'"
+
+// savedGlobalConfig returns the config passed to the last SaveGlobalConfig call.
+func savedGlobalConfig(t *testing.T, m *mocks.MockConfigStore) config.GlobalConfig {
+	t.Helper()
+	for i := len(m.Calls) - 1; i >= 0; i-- {
+		if m.Calls[i].Method == "SaveGlobalConfig" {
+			cfg, ok := m.Calls[i].Arguments.Get(1).(*config.GlobalConfig)
+			require.True(t, ok, "SaveGlobalConfig argument must be a *config.GlobalConfig")
+			return *cfg
+		}
+	}
+	t.Fatal("SaveGlobalConfig was never called")
+	return config.GlobalConfig{}
+}
+
+// mustJSONString encodes s as a JSON string literal.
+func mustJSONString(t *testing.T, s string) string {
+	t.Helper()
+	b, err := json.Marshal(s)
+	require.NoError(t, err)
+	return string(b)
+}
+
+func assertModeRejected(t *testing.T, err error, store *mocks.MockConfigStore) {
+	t.Helper()
+	require.Error(t, err, "an invalid ri_exchange_mode must be refused")
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a client error, got %v", err)
+	assert.Equal(t, 400, ce.code)
+	assert.Contains(t, err.Error(), riExchangeModeErr)
+	assert.False(t, wrote(store), "a rejected write must not reach the store")
+}
+
+func TestUpdateConfig_RejectsInvalidRIExchangeMode(t *testing.T) {
+	// Typos, the empty string, wrong case and padded values. The consumer
+	// matches the two valid values exactly, so none of these is "close enough".
+	for _, mode := range []string{"Auto", "AUTO", "Manual", "", "atuo", "automatic", "enabled", " auto", "manual "} {
+		t.Run("mode="+mode, func(t *testing.T) {
+			r, store := autoGateSessionRouter(t, autoGateAdminPlusExchange, disarmedConfig())
+
+			_, err := r.Route(context.Background(), "PUT", genericPath,
+				autoGateTokenReq(`{"ri_exchange_mode":`+mustJSONString(t, mode)+`}`))
+
+			assertModeRejected(t, err, store)
+		})
+	}
+}
+
+// TestUpdateConfig_InvalidRIExchangeModeRejectedBeforeGate pins the order: a
+// malformed write is a 400, never a 403 that reveals the caller's grant state.
+func TestUpdateConfig_InvalidRIExchangeModeRejectedBeforeGate(t *testing.T) {
+	r, store := autoGateSessionRouter(t, autoGateAdminOnly, disarmedConfig())
+
+	_, err := r.Route(context.Background(), "PUT", genericPath,
+		autoGateTokenReq(genericBody("Auto", true, 100, 500)))
+
+	assertModeRejected(t, err, store)
+}
+
+func TestUpdateConfig_AcceptsValidRIExchangeMode(t *testing.T) {
+	for _, mode := range []string{"manual", "auto"} {
+		t.Run("mode="+mode, func(t *testing.T) {
+			r, store := autoGateSessionRouter(t, autoGateAdminPlusExchange, disarmedConfig())
+
+			_, err := r.Route(context.Background(), "PUT", genericPath,
+				autoGateTokenReq(`{"ri_exchange_mode":`+mustJSONString(t, mode)+`}`))
+
+			require.NoError(t, err)
+			assert.Equal(t, mode, savedGlobalConfig(t, store).RIExchangeMode)
+		})
+	}
+}
+
+// legacyModeConfig is a row stored before this validation existed, carrying a
+// mode the API no longer accepts.
+func legacyModeConfig(mode string) config.GlobalConfig {
+	cfg := disarmedConfig()
+	cfg.RIExchangeMode = mode
+	return cfg
+}
+
+// TestUpdateConfig_LegacyStoredModeDoesNotBlockUnrelatedWrites pins that the
+// check applies to the value the caller sends, not to whatever is already
+// stored. Otherwise a single legacy row would 400 every partial settings save,
+// including the laddering kill-switch toggle.
+func TestUpdateConfig_LegacyStoredModeDoesNotBlockUnrelatedWrites(t *testing.T) {
+	for name, body := range map[string]string{
+		"unrelated field":      `{"laddering_enabled":true}`,
+		"explicit JSON null":   `{"ri_exchange_mode":null,"laddering_enabled":true}`,
+		"null with whitespace": `{"ri_exchange_mode" :  null ,"laddering_enabled":true}`,
+	} {
+		t.Run(name, func(t *testing.T) {
+			r, store := autoGateSessionRouter(t, autoGateAdminPlusExchange, legacyModeConfig("automatic"))
+
+			_, err := r.Route(context.Background(), "PUT", genericPath, autoGateTokenReq(body))
+
+			require.NoError(t, err, "a write that does not set ri_exchange_mode must not be blocked by a legacy row")
+			saved := savedGlobalConfig(t, store)
+			assert.True(t, saved.LadderingEnabled, "the field the body carried must be applied")
+			assert.Equal(t, "automatic", saved.RIExchangeMode, "the legacy mode is preserved, not silently rewritten")
+		})
+	}
+}
+
+// TestUpdateConfig_LegacyStoredModeCanBeRepaired pins the repair path for a
+// legacy row: sending a valid mode replaces it.
+func TestUpdateConfig_LegacyStoredModeCanBeRepaired(t *testing.T) {
+	r, store := autoGateSessionRouter(t, autoGateAdminOnly, legacyModeConfig("Auto"))
+
+	_, err := r.Route(context.Background(), "PUT", genericPath,
+		autoGateTokenReq(`{"ri_exchange_mode":"manual"}`))
+
+	require.NoError(t, err)
+	assert.Equal(t, "manual", savedGlobalConfig(t, store).RIExchangeMode)
+}
+
+// TestUpdateConfig_LegacyStoredModeResentIsRejected pins that a caller cannot
+// re-assert a legacy value just because it is already stored.
+func TestUpdateConfig_LegacyStoredModeResentIsRejected(t *testing.T) {
+	r, store := autoGateSessionRouter(t, autoGateAdminPlusExchange, legacyModeConfig("Auto"))
+
+	_, err := r.Route(context.Background(), "PUT", genericPath,
+		autoGateTokenReq(`{"ri_exchange_mode":"Auto"}`))
+
+	assertModeRejected(t, err, store)
+}
+
+func TestUpdateConfig_RejectsCaseFoldedAndDuplicateInvalidRIExchangeMode(t *testing.T) {
+	for name, body := range map[string]string{
+		"upper key":              `{"RI_EXCHANGE_MODE":"Auto"}`,
+		"mixed key":              `{"Ri_Exchange_Mode":"automatic"}`,
+		"duplicate null":         `{"ri_exchange_mode":"Auto","ri_exchange_mode":null}`,
+		"case folded then null":  `{"RI_EXCHANGE_MODE":"bogus","ri_exchange_mode":null}`,
+		"duplicate invalid last": `{"ri_exchange_mode":"manual","ri_exchange_mode":"Auto"}`,
+	} {
+		t.Run(name, func(t *testing.T) {
+			r, store := autoGateSessionRouter(t, autoGateAdminPlusExchange, disarmedConfig())
+			_, err := r.Route(context.Background(), "PUT", genericPath, autoGateTokenReq(body))
+			assertModeRejected(t, err, store)
+		})
+	}
+}
+
+func TestUpdateConfig_LegacyStoredModeResentWithCaseFoldedKeyIsRejected(t *testing.T) {
+	r, store := autoGateSessionRouter(t, autoGateAdminPlusExchange, legacyModeConfig("Auto"))
+	_, err := r.Route(context.Background(), "PUT", genericPath, autoGateTokenReq(`{"RI_EXCHANGE_MODE":"Auto"}`))
+	assertModeRejected(t, err, store)
+}

internal/api/handler_ri_exchange.go

@@ -3011,7 +3011,7 @@ type RIExchangeConfigUpdateRequest struct {
 }
 
 func (r *RIExchangeConfigUpdateRequest) validate() error {
-	if r.Mode != "manual" && r.Mode != "auto" {
+	if !config.IsValidRIExchangeMode(r.Mode) {
 		return NewClientError(400, "mode must be 'manual' or 'auto'")
 	}
 	if r.UtilizationThreshold < 0 || r.UtilizationThreshold > 100 {

internal/api/ri_exchange_automode_gate_test.go

@@ -369,21 +369,23 @@ func TestRIExchangeAutoModeGate_UserAPIKeyUsesKeyPermissions(t *testing.T) {
 //
 // pkg/exchange/auto.go's processRecommendation routes to processManualExchange
 // only on the literal "manual" and sends EVERY other value to
-// processAutoExchange. GlobalConfig.Validate never constrains RIExchangeMode,
-// and PUT /api/config unmarshals onto the struct wholesale -- so a mode of
-// "Auto", "automatic" or "" arms unattended execution just as effectively as
+// processAutoExchange. Both routes now refuse to WRITE such a mode (issue
+// #1196), but a row stored before that check can still hold "Auto",
+// "automatic" or "", and a partial PUT /api/config preserves it. Enabling
+// exchange on such a row arms unattended execution just as effectively as
 // "auto", while a gate written as `mode == "auto"` would wave it through.
 //
-// Only the generic route is exercised: the dedicated one's validate() restricts
-// mode to manual|auto, so this shape cannot reach it. That asymmetry is exactly
-// why the gate reads the field the scheduler reads.
+// The body therefore flips only ri_exchange_enabled and leaves the legacy mode
+// in place. That is exactly why the gate reads the field the scheduler reads.
 func TestRIExchangeAutoModeGate_NonManualModeIsArmed(t *testing.T) {
 	for _, mode := range []string{"Auto", "AUTO", "automatic", "", "enabled"} {
 		t.Run("mode="+mode, func(t *testing.T) {
-			r, store := autoGateSessionRouter(t, autoGateAdminOnly, disarmedConfig())
+			stored := disarmedConfig()
+			stored.RIExchangeMode = mode
+			r, store := autoGateSessionRouter(t, autoGateAdminOnly, stored)
 
 			_, err := r.Route(context.Background(), "PUT", genericPath,
-				autoGateTokenReq(genericBody(mode, true, 100, 500)))
+				autoGateTokenReq(`{"ri_exchange_enabled":true}`))
 
 			require.Error(t, err,
 				"mode %q is not \"manual\", so the scheduler runs it as auto; the gate must see the same", mode)

internal/config/validation.go

@@ -273,6 +273,21 @@ func PaymentCoercionChangesSchedule(raw, canonical string) bool {
 // purchase time to preserve pre-694 behavior).
 var ValidOfferingClasses = []string{"convertible", "standard"}
 
+// RIExchangeModeManual and RIExchangeModeAuto are the only accepted
+// ri_exchange_mode values.
+const (
+	RIExchangeModeManual = "manual"
+	RIExchangeModeAuto   = "auto"
+)
+
+// IsValidRIExchangeMode reports whether mode is exactly "manual" or "auto".
+// The match is case-sensitive and does not trim, because the RI exchange
+// consumer compares the literal values: "Auto", "" or " auto" are not
+// equivalent to "auto" there.
+func IsValidRIExchangeMode(mode string) bool {
+	return mode == RIExchangeModeManual || mode == RIExchangeModeAuto
+}
+
 // ValidRampScheduleTypes lists all supported ramp schedule types.
 var ValidRampScheduleTypes = []string{"immediate", "weekly", "monthly", "custom"}
 

internal/config/validation_test.go

@@ -1184,6 +1184,21 @@ func TestPurchasePlan_CheckNoMultiStepSavingsPlans(t *testing.T) {
 	}
 }
 
+func TestIsValidRIExchangeMode(t *testing.T) {
+	for mode, want := range map[string]bool{
+		"manual":    true,
+		"auto":      true,
+		"":          false,
+		"Auto":      false,
+		"MANUAL":    false,
+		"automatic": false,
+		" auto":     false,
+		"manual ":   false,
+	} {
+		assert.Equal(t, want, IsValidRIExchangeMode(mode), "mode %q", mode)
+	}
+}
+
 // Only purchase plans refuse a 0% target (platform#523); service and global
 // coverage keep accepting 0.
 func TestCoverageZeroStillValidOutsidePlans(t *testing.T) {