From 9616ae72c9d5a7b8104d2725e5a317887a9c3db8 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 09:45:21 +0100 Subject: [PATCH] fix(config): validate ri_exchange_mode on PUT /api/config PUT /api/config unmarshals the body onto the stored GlobalConfig and never checked ri_exchange_mode, so "", "Auto" or a typo was stored as-is, while PUT /api/ri-exchange/config already accepted only "manual" or "auto". Reject any ri_exchange_mode the caller sends that is not exactly "manual" or "auto", with a 400 that runs before the arming gates. The rule lives in config.IsValidRIExchangeMode and both routes use it. The check applies to the value the body sets, not to the merged row, so a row stored before this check keeps working for unrelated partial saves (e.g. the laddering kill-switch toggle) and can be repaired by sending a valid mode. armed() stays fail-safe for those rows; its comment now says why. Closes #1196 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler.go | 14 +- internal/api/handler_config.go | 31 +++- .../handler_config_ri_exchange_mode_test.go | 149 ++++++++++++++++++ internal/api/handler_ri_exchange.go | 2 +- .../api/ri_exchange_automode_gate_test.go | 18 ++- internal/config/validation.go | 15 ++ internal/config/validation_test.go | 15 ++ 7 files changed, 228 insertions(+), 16 deletions(-) create mode 100644 internal/api/handler_config_ri_exchange_mode_test.go diff --git a/internal/api/handler.go b/internal/api/handler.go index a5a1df9e6..8eaa969c3 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -575,11 +575,15 @@ func riExchangeArmStateOf(cfg *config.GlobalConfig) riExchangeArmState { // The predicate mirrors the CONSUMER exactly. pkg/exchange/auto.go's // processRecommendation routes to processManualExchange only on the literal // "manual" and sends every other value -- including "", "Auto" and any typo -- -// to processAutoExchange. Defining "armed" as mode == "auto" would therefore -// leave the gate below open on precisely the route that matters: PUT /api/config -// unmarshals onto GlobalConfig wholesale and GlobalConfig.Validate never -// constrains RIExchangeMode, so any non-"manual" string reaches the scheduler. -// Both axes of the guard must see what the scheduler sees. +// to processAutoExchange. +// +// Both write routes now accept only "manual" or "auto" when a caller sets the +// mode (issue #1196), but rows stored before that check may still hold "", +// "Auto" or a typo, and a partial PUT /api/config that omits ri_exchange_mode +// preserves them. Defining "armed" as mode == "auto" would leave the gate below +// open for such a row: flipping ri_exchange_enabled on would arm it unseen. So +// armed() stays fail-safe and treats every non-"manual" value as auto. Both +// axes of the guard must see what the scheduler sees. func (s riExchangeArmState) armed() bool { return s.enabled && s.mode != "manual" } diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 74dcdb067..c7f36f687 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -79,6 +79,7 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR return nil, NewClientError(400, "invalid request body") } _, gracePresent := present["grace_period_days"] + modeSent := riExchangeModeSent(present) if err := h.requireGlobalConfigScope(ctx, session); err != nil { return nil, err } @@ -116,8 +117,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); vErr != nil { + return vErr } if gErr := h.requireRIExchangeAutoModeGrant(ctx, req, before, riExchangeArmStateOf(existing)); gErr != nil { return gErr @@ -133,6 +134,32 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR return &StatusResponse{Status: "updated"}, 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 { + raw, ok := present["ri_exchange_mode"] + return ok && string(raw) != "null" +} + +// validateGlobalConfigWrite validates the merged config as a 400. +// +// ri_exchange_mode is checked only when the body sets it (issue #1196), 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. func (h *Handler) requireSwitchGrants(ctx context.Context, req *events.LambdaFunctionURLRequest, session *Session, autoBefore, ladderBefore bool, cfg *config.GlobalConfig) error { diff --git a/internal/api/handler_config_ri_exchange_mode_test.go b/internal/api/handler_config_ri_exchange_mode_test.go new file mode 100644 index 000000000..6d96a965f --- /dev/null +++ b/internal/api/handler_config_ri_exchange_mode_test.go @@ -0,0 +1,149 @@ +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) +} diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 4304a8932..4e455b466 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2854,7 +2854,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 { diff --git a/internal/api/ri_exchange_automode_gate_test.go b/internal/api/ri_exchange_automode_gate_test.go index e14517a9a..92125a8cf 100644 --- a/internal/api/ri_exchange_automode_gate_test.go +++ b/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) diff --git a/internal/config/validation.go b/internal/config/validation.go index e8f3ab455..20831ee7a 100644 --- a/internal/config/validation.go +++ b/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"} diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 2b9301b13..90ce2dfe6 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -1145,3 +1145,18 @@ 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) + } +}