diff --git a/go.mod b/go.mod index 5fb3a2f6..abcf4fb6 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 06deac0b..83df52e6 100644 --- a/go.sum +++ b/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= diff --git a/internal/api/handler.go b/internal/api/handler.go index c0ee31a2..f5a63c8d 100644 --- a/internal/api/handler.go +++ b/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 { diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 83896a47..bda6b238 100644 --- a/internal/api/handler_config.go +++ b/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). 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 00000000..430e6beb --- /dev/null +++ b/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) +} diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 8a26ea6d..5d673405 100644 --- a/internal/api/handler_ri_exchange.go +++ b/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 { diff --git a/internal/api/ri_exchange_automode_gate_test.go b/internal/api/ri_exchange_automode_gate_test.go index e14517a9..92125a8c 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 3b65bd89..356a2891 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 62ae7a33..de5e2a82 100644 --- a/internal/config/validation_test.go +++ b/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) {