From 86b896609426a512679ab52386eb8ecc6be50844 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 3 Nov 2026 18:55:39 +0100 Subject: [PATCH 1/2] fix(api): refuse an empty enabled_providers list on config writes An empty list means "show every provider" to the recommendation readers but "collect from none" to the scheduler. PUT /api/config now returns 400 when the body carries enabled_providers (any key casing, null included) and the merged value is empty. Presence-based, so partial saves that omit the key still work on a fresh install that stores []. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_config.go | 21 ++++++++++-- internal/api/handler_config_test.go | 50 +++++++++++++++++++++++++++++ internal/api/openapi.yaml | 1 + 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 7764f8581..96711d3d8 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -74,6 +74,7 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR } _, gracePresent := present["grace_period_days"] modeSent := riExchangeModeSent(present) + providersSent := bodyHasKey(present, "enabled_providers") if err := h.requireGlobalConfigScope(ctx, session); err != nil { return nil, err } @@ -114,7 +115,7 @@ 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 := validateGlobalConfigWrite(existing, modeSent || existing.RIExchangeMode != before.mode); vErr != nil { + if vErr := validateGlobalConfigWrite(existing, modeSent || existing.RIExchangeMode != before.mode, providersSent); vErr != nil { return vErr } if gErr := requireRIExchangeAutoModeGrant(before, riExchangeArmStateOf(existing), grants.riExchange); gErr != nil { @@ -145,6 +146,17 @@ func globalConfigWriteFields(body string) (map[string]json.RawMessage, error) { return present, nil } +// bodyHasKey reports whether the body carries the key under any casing, as +// json.Unmarshal matches keys case-insensitively. An explicit null counts. +func bodyHasKey(present map[string]json.RawMessage, name string) bool { + for key := range present { + if strings.EqualFold(key, name) { + return true + } + } + return false +} + // 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 { @@ -166,7 +178,12 @@ func riExchangeModeSent(present map[string]json.RawMessage) bool { // 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 { +func validateGlobalConfigWrite(cfg *config.GlobalConfig, modeSent, providersSent bool) error { + // Presence-based, not in Validate: a fresh install stores [] and partial + // saves that omit the key must keep working. + if providersSent && len(cfg.EnabledProviders) == 0 { + return NewClientError(400, "enabled_providers must list at least one provider") + } if modeSent && !config.IsValidRIExchangeMode(cfg.RIExchangeMode) { return NewClientError(400, "ri_exchange_mode must be 'manual' or 'auto'") } diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index 184449301..a27f0eb7f 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -1613,3 +1613,53 @@ func TestHandler_updateConfig_RequiresUpdateConfigPermission(t *testing.T) { mockStore.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) mockStore.AssertNotCalled(t, "SaveGlobalConfig", mock.Anything, mock.Anything) } + +func TestHandler_updateConfig_EnabledProvidersWrite(t *testing.T) { + cases := []struct { + name string + stored []string + body string + wantErr bool + wantSave bool + }{ + {"empty list refused", []string{"aws"}, `{"enabled_providers": []}`, true, false}, + {"null refused", []string{"aws"}, `{"enabled_providers": null}`, true, false}, + {"mixed-case key refused", []string{"aws"}, `{"Enabled_Providers": []}`, true, false}, + {"omitted key on stored empty row accepted", []string{}, `{"laddering_enabled": false}`, false, true}, + {"one provider accepted", []string{}, `{"enabled_providers": ["aws"]}`, false, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + EnabledProviders: tc.stored, DefaultTerm: 3, DefaultPayment: "all-upfront", + DefaultCoverage: 80, DefaultRampSchedule: "immediate", RIExchangeMode: "manual", + RIExchangeLookbackDays: 30, RecommendationsCacheStaleHours: 24, RecommendationsLookbackDays: 7, + }, nil) + if tc.wantSave { + mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")).Return(nil) + } + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: tc.body, + } + result, err := handler.updateConfig(ctx, req) + if tc.wantErr { + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "enabled_providers must list at least one provider") + mockStore.AssertNotCalled(t, "SaveGlobalConfig", mock.Anything, mock.Anything) + return + } + require.NoError(t, err) + assert.Equal(t, "updated", result.Status) + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index f8c18ae96..ef4e29b36 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -6631,6 +6631,7 @@ components: properties: enabled_providers: type: array + description: A write that carries this key must list at least one provider; an empty list or null returns 400. A GET on a fresh install can return an empty list. items: type: string enum: [aws, azure, gcp] From a64c9e6d6f2a890833ade84da7bee552050a3da2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 3 Nov 2026 19:27:14 +0100 Subject: [PATCH 2/2] test(api): assert the 400 code and pin duplicate-key and no-permission cases Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_config_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index a27f0eb7f..df1297ac5 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -1624,6 +1624,7 @@ func TestHandler_updateConfig_EnabledProvidersWrite(t *testing.T) { }{ {"empty list refused", []string{"aws"}, `{"enabled_providers": []}`, true, false}, {"null refused", []string{"aws"}, `{"enabled_providers": null}`, true, false}, + {"duplicate key differing in case refused", []string{"aws"}, `{"enabled_providers":["aws"],"ENABLED_PROVIDERS":[]}`, true, false}, {"mixed-case key refused", []string{"aws"}, `{"Enabled_Providers": []}`, true, false}, {"omitted key on stored empty row accepted", []string{}, `{"laddering_enabled": false}`, false, true}, {"one provider accepted", []string{}, `{"enabled_providers": ["aws"]}`, false, true}, @@ -1654,6 +1655,9 @@ func TestHandler_updateConfig_EnabledProvidersWrite(t *testing.T) { if tc.wantErr { require.Error(t, err) assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) assert.Contains(t, err.Error(), "enabled_providers must list at least one provider") mockStore.AssertNotCalled(t, "SaveGlobalConfig", mock.Anything, mock.Anything) return @@ -1663,3 +1667,28 @@ func TestHandler_updateConfig_EnabledProvidersWrite(t *testing.T) { }) } } + +func TestHandler_updateConfig_EmptyEnabledProvidersWithoutPermissionIs403(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + userSession := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", Email: "standard@example.com"} + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil) + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, mock.AnythingOfType("string"), mock.AnythingOfType("string")). + Return(false, nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer user-token"}, + Body: `{"enabled_providers": []}`, + } + result, err := handler.updateConfig(ctx, req) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 403, ce.code) + mockStore.AssertNotCalled(t, "SaveGlobalConfig", mock.Anything, mock.Anything) +}