From d20cf01ed2531b3b53718fa54e76661eab180809 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:11:46 +0000 Subject: [PATCH 1/2] fix(api): keep enabled and aws_is_org_root when an account update omits them PUT /api/accounts/{id} built the stored row from the request alone, so a body without an `enabled` or `aws_is_org_root` key silently disabled the account and cleared its org-root flag (audit finding A02-013). An API client or Terraform provider renaming an account or rotating its role would stop the scheduler from scanning it, with no error. AWSIsOrgRoot on CloudAccountRequest is now a *bool like Enabled, and updateAccount falls back to the stored value of each flag when the request omits it or sends null. An explicit false still clears it. Create keeps its defaults (both false when absent). openapi.yaml documents the new update semantics. Refs #151 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_accounts.go | 24 +++- .../api/handler_accounts_update_keep_test.go | 136 ++++++++++++++++++ internal/api/openapi.yaml | 18 ++- 3 files changed, 172 insertions(+), 6 deletions(-) create mode 100644 internal/api/handler_accounts_update_keep_test.go diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index bec9a02c..8686bcf1 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -26,8 +26,12 @@ import ( ) // CloudAccountRequest is the request body for create/update account endpoints. +// Enabled and AWSIsOrgRoot are pointers so an omitted (or null) key can be +// told apart from an explicit false: create treats absent as false, update +// keeps the stored value (issue #151, A02-013). type CloudAccountRequest struct { Enabled *bool `json:"enabled"` + AWSIsOrgRoot *bool `json:"aws_is_org_root"` AWSWebIdentityTokenFile string `json:"aws_web_identity_token_file"` AzureClientID string `json:"azure_client_id"` Provider string `json:"provider"` @@ -46,7 +50,6 @@ type CloudAccountRequest struct { GCPProjectID string `json:"gcp_project_id"` GCPClientEmail string `json:"gcp_client_email"` GCPAuthMode string `json:"gcp_auth_mode"` - AWSIsOrgRoot bool `json:"aws_is_org_root"` } // CredentialsRequest is the request body for the save-credentials endpoint. @@ -544,7 +547,6 @@ func cloudAccountFromRequest(req CloudAccountRequest) *config.CloudAccount { AWSExternalID: req.AWSExternalID, AWSBastionID: req.AWSBastionID, AWSWebIdentityTokenFile: req.AWSWebIdentityTokenFile, - AWSIsOrgRoot: req.AWSIsOrgRoot, AzureSubscriptionID: req.AzureSubscriptionID, AzureTenantID: req.AzureTenantID, AzureClientID: req.AzureClientID, @@ -558,10 +560,27 @@ func cloudAccountFromRequest(req CloudAccountRequest) *config.CloudAccount { if req.Enabled != nil { a.Enabled = *req.Enabled } + if req.AWSIsOrgRoot != nil { + a.AWSIsOrgRoot = *req.AWSIsOrgRoot + } return a } +// keepOmittedAccountFlags copies the stored enabled and aws_is_org_root +// values onto an update built by cloudAccountFromRequest when the request +// omitted them (or sent null), so a PUT that only renames an account or +// rotates its role does not silently disable it or clear its org-root flag. +// An explicit false in the request is still applied. +func keepOmittedAccountFlags(account, existing *config.CloudAccount, req CloudAccountRequest) { + if req.Enabled == nil { + account.Enabled = existing.Enabled + } + if req.AWSIsOrgRoot == nil { + account.AWSIsOrgRoot = existing.AWSIsOrgRoot + } +} + // getAccount handles GET /api/accounts/:id. func (h *Handler) getAccount(ctx context.Context, req *events.LambdaFunctionURLRequest, id string) (any, error) { if err := validateUUID(id); err != nil { @@ -615,6 +634,7 @@ func (h *Handler) updateAccount(ctx context.Context, httpReq *events.LambdaFunct } account := cloudAccountFromRequest(req) + keepOmittedAccountFlags(account, existing, req) account.ID = id account.CreatedAt = existing.CreatedAt account.CreatedBy = existing.CreatedBy diff --git a/internal/api/handler_accounts_update_keep_test.go b/internal/api/handler_accounts_update_keep_test.go new file mode 100644 index 00000000..65c230e3 --- /dev/null +++ b/internal/api/handler_accounts_update_keep_test.go @@ -0,0 +1,136 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #151 (audit finding A02-013): PUT /api/accounts/{id} built the stored +// row from the request alone, so a body without an `enabled` or +// `aws_is_org_root` key silently disabled the account and cleared its +// org-root flag. An omitted (or null) key now keeps the stored value, while an +// explicit false still clears it. Create keeps its defaults (both false). + +// accountWriteCapture records the account handed to the store so the tests +// can assert what would be persisted, not just what the handler returns. +type accountWriteCapture struct { + *mockConfigStoreAccounts + updated *config.CloudAccount + created *config.CloudAccount +} + +func (m *accountWriteCapture) UpdateCloudAccount(_ context.Context, a *config.CloudAccount) error { + cp := *a + m.updated = &cp + return nil +} + +func (m *accountWriteCapture) CreateCloudAccount(_ context.Context, a *config.CloudAccount) error { + cp := *a + m.created = &cp + return nil +} + +func newAccountWriteCapture(ctx context.Context, existing *config.CloudAccount) *accountWriteCapture { + return &accountWriteCapture{ + mockConfigStoreAccounts: &mockConfigStoreAccounts{ + MockConfigStore: setupAdminMock(ctx), + getResult: existing, + }, + } +} + +// putAccountFlags sends PUT /api/accounts/{id} with the given trailing JSON +// members (empty for none) against an existing row with the given flags and +// returns the row the handler wrote to the store. +func putAccountFlags(t *testing.T, existingEnabled, existingOrgRoot bool, extra string) *config.CloudAccount { + t.Helper() + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + existing := sampleAccount() + existing.Enabled = existingEnabled + existing.AWSIsOrgRoot = existingOrgRoot + store := newAccountWriteCapture(ctx, &existing) + handler := &Handler{auth: mockAuth, config: store} + + body := `{"name":"Renamed","provider":"aws","external_id":"123456789012"` + extra + `}` + result, err := handler.updateAccount(ctx, adminRequest(body), existing.ID) + require.NoError(t, err) + require.NotNil(t, store.updated, "updateAccount must write the account") + + returned := result.(*config.CloudAccount) + assert.Equal(t, store.updated.Enabled, returned.Enabled, "response must match the stored enabled") + assert.Equal(t, store.updated.AWSIsOrgRoot, returned.AWSIsOrgRoot, "response must match the stored aws_is_org_root") + assert.Equal(t, "Renamed", store.updated.Name, "the rest of the body is still applied") + return store.updated +} + +func TestUpdateAccount_OmittedFlagsKeepStoredValues(t *testing.T) { + cases := []struct { + name string + extra string + existingEnabled bool + existingOrgRoot bool + wantEnabled bool + wantOrgRoot bool + }{ + {name: "omitted keeps true", existingEnabled: true, existingOrgRoot: true, wantEnabled: true, wantOrgRoot: true}, + {name: "omitted keeps false", existingEnabled: false, existingOrgRoot: false, wantEnabled: false, wantOrgRoot: false}, + {name: "null keeps true", extra: `,"enabled":null,"aws_is_org_root":null`, existingEnabled: true, existingOrgRoot: true, wantEnabled: true, wantOrgRoot: true}, + {name: "explicit false clears", extra: `,"enabled":false,"aws_is_org_root":false`, existingEnabled: true, existingOrgRoot: true, wantEnabled: false, wantOrgRoot: false}, + {name: "explicit true sets", extra: `,"enabled":true,"aws_is_org_root":true`, existingEnabled: false, existingOrgRoot: false, wantEnabled: true, wantOrgRoot: true}, + {name: "enabled omitted, org root explicit false", extra: `,"aws_is_org_root":false`, existingEnabled: true, existingOrgRoot: true, wantEnabled: true, wantOrgRoot: false}, + {name: "enabled explicit false, org root omitted", extra: `,"enabled":false`, existingEnabled: true, existingOrgRoot: true, wantEnabled: false, wantOrgRoot: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + stored := putAccountFlags(t, tc.existingEnabled, tc.existingOrgRoot, tc.extra) + assert.Equal(t, tc.wantEnabled, stored.Enabled, "enabled") + assert.Equal(t, tc.wantOrgRoot, stored.AWSIsOrgRoot, "aws_is_org_root") + }) + } +} + +// The issue's verification rule: a real false and a genuine absence go through +// the same path and must produce different outcomes. +func TestUpdateAccount_ExplicitFalseAndOmittedDiverge(t *testing.T) { + omitted := putAccountFlags(t, true, true, "") + explicit := putAccountFlags(t, true, true, `,"enabled":false,"aws_is_org_root":false`) + assert.NotEqual(t, omitted.Enabled, explicit.Enabled, "enabled: omitted and explicit false must differ") + assert.NotEqual(t, omitted.AWSIsOrgRoot, explicit.AWSIsOrgRoot, "aws_is_org_root: omitted and explicit false must differ") +} + +func TestCreateAccount_FlagDefaultsUnchanged(t *testing.T) { + cases := []struct { + name string + extra string + wantEnabled bool + wantOrgRoot bool + }{ + {name: "omitted defaults to false"}, + {name: "null defaults to false", extra: `,"enabled":null,"aws_is_org_root":null`}, + {name: "explicit true", extra: `,"enabled":true,"aws_is_org_root":true`, wantEnabled: true, wantOrgRoot: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + store := newAccountWriteCapture(ctx, nil) + handler := &Handler{auth: mockAuth, config: store} + + body := `{"name":"Acme","provider":"aws","external_id":"123456789012"` + tc.extra + `}` + _, err := handler.createAccount(ctx, adminRequest(body)) + require.NoError(t, err) + require.NotNil(t, store.created) + assert.Equal(t, tc.wantEnabled, store.created.Enabled, "enabled") + assert.Equal(t, tc.wantOrgRoot, store.created.AWSIsOrgRoot, "aws_is_org_root") + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index a7782101..e3850221 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -4450,9 +4450,10 @@ paths: summary: Replace a cloud account (admin only) description: > Replaces the account's configuration with the request body; fields - omitted from the body are cleared, and enabled defaults to false. - The provider is validated but not changed. Stored credentials are - not affected. + omitted from the body are cleared, except enabled and + aws_is_org_root, which keep their stored values when omitted or + null (send false to clear them). The provider is validated but not + changed. Stored credentials are not affected. parameters: - $ref: '#/components/parameters/CSRFToken' requestBody: @@ -7731,7 +7732,11 @@ components: format: email enabled: type: boolean - description: Defaults to false when omitted. + nullable: true + description: > + On create, defaults to false when omitted or null. On update, + omitted or null keeps the stored value; an explicit false + disables the account. aws_auth_mode: type: string enum: [access_keys, role_arn, bastion, workload_identity_federation] @@ -7750,6 +7755,11 @@ components: contain "..". aws_is_org_root: type: boolean + nullable: true + description: > + On create, defaults to false when omitted or null. On update, + omitted or null keeps the stored value; an explicit false clears + the flag. azure_subscription_id: type: string azure_tenant_id: From 74cc208a005f98484deff3b31a96230701827731 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 28 Oct 2026 00:19:39 +0000 Subject: [PATCH 2/2] fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) govulncheck in the Docker image build now fails on these two advisories, both fixed in x/net v0.62.0. Co-Authored-By: Claude Opus 5.5 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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=