diff --git a/internal/api/handler_ladder.go b/internal/api/handler_ladder.go index f8010e246..77f88697c 100644 --- a/internal/api/handler_ladder.go +++ b/internal/api/handler_ladder.go @@ -74,9 +74,24 @@ func (h *Handler) filterLadderConfigsByAllowedAccounts(ctx context.Context, sess // the auth component is nil (returns an error, never a session), so the // handler fails closed; the exact status is a 500-class error in that case // rather than a 403, but no unauthenticated write ever reaches the store. +// +// update:config alone is NOT sufficient: the target account must also be +// inside the session's allowed_accounts (issue #1539). Holding the verb said +// nothing about WHICH account it could be exercised on, so a caller scoped to +// one account could write the ladder config of any other -- including the +// deployment's own account, whose config the ladder engine acts on, and whose +// existing row this endpoint overwrites (the store upserts on the +// UNIQUE(cloud_account_id, provider) pair). getLadderConfigs has filtered on +// allowed_accounts since migration 000088 opened it to scoped users; the same +// reasoning was never applied to the write, so the row was also invisible to +// its author afterwards. func (h *Handler) upsertLadderConfig(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { - if _, err := h.requirePermission(ctx, req, "update", "config"); err != nil { - return nil, err + // Named permErr rather than err: the body-parsing and validation steps + // below each bind their own `err` in an if-statement, which would shadow a + // function-scoped `err` and trip govet's shadow check. + session, permErr := h.requirePermission(ctx, req, "update", "config") + if permErr != nil { + return nil, permErr } // DisallowUnknownFields so a typo'd key is rejected loudly instead of @@ -109,7 +124,7 @@ func (h *Handler) upsertLadderConfig(ctx context.Context, req *events.LambdaFunc return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err)) } - if err := h.validateLadderAccountProvider(ctx, &cfg); err != nil { + if err := h.requireLadderAccountAccess(ctx, session, &cfg); err != nil { return nil, err } @@ -121,18 +136,23 @@ func (h *Handler) upsertLadderConfig(ctx context.Context, req *events.LambdaFunc return result, nil } -// validateLadderAccountProvider confirms the (cloud_account_id, provider) pair -// refers to a real cloud account whose provider matches. This turns a -// nonexistent-account FK violation into a clean 400 (rather than a raw 500 from -// the store's FK constraint) and rejects a provider mismatch -- e.g. an inert -// gcp config attached to an AWS account, which would confuse PR-2 scoping. -func (h *Handler) validateLadderAccountProvider(ctx context.Context, cfg *config.LadderConfigDB) error { - account, err := h.config.GetCloudAccount(ctx, cfg.CloudAccountID) +// requireLadderAccountAccess confirms the caller may write ladder config for +// the (cloud_account_id, provider) pair in the request body: the account must +// exist, be within the session's allowed_accounts, and carry the claimed +// provider. It rejects a provider mismatch -- e.g. an inert gcp config attached +// to an AWS account, which would confuse PR-2 scoping. +// +// The account lookup lives in requireAccountAccess, which already fetches the +// account and nil-checks it, so this does not fetch it a second time. +// +// requireAccountAccess answers both "no such account" and "not yours" with the +// same errNotFound (404), deliberately: distinguishing them would let a scoped +// caller enumerate the account table by probing ids. That replaces the earlier +// 400 "cloud account %q does not exist", which leaked exactly that distinction. +func (h *Handler) requireLadderAccountAccess(ctx context.Context, session *Session, cfg *config.LadderConfigDB) error { + account, err := h.requireAccountAccess(ctx, session, cfg.CloudAccountID) if err != nil { - return fmt.Errorf("failed to look up cloud account: %w", err) - } - if account == nil { - return NewClientError(400, fmt.Sprintf("cloud account %q does not exist", cfg.CloudAccountID)) + return err } if account.Provider != cfg.Provider { return NewClientError(400, fmt.Sprintf("provider %q does not match cloud account provider %q", cfg.Provider, account.Provider)) diff --git a/internal/api/handler_ladder_test.go b/internal/api/handler_ladder_test.go index 83afd5d9e..cd2da9a7a 100644 --- a/internal/api/handler_ladder_test.go +++ b/internal/api/handler_ladder_test.go @@ -151,8 +151,14 @@ func TestUpsertLadderConfig_ProviderMismatchRejected(t *testing.T) { } // TestUpsertLadderConfig_NonexistentAccountRejected is F5: a cloud_account_id -// that does not resolve must 400 (clean rejection) rather than reaching the -// store and surfacing a raw 500 from the FK constraint. +// that does not resolve must be rejected cleanly rather than reaching the store +// and surfacing a raw 500 from the FK constraint. +// +// The refusal is errNotFound (404) rather than the earlier 400 "cloud account +// does not exist" (issue #1539). requireAccountAccess answers "no such account" +// and "not yours" identically on purpose: a distinguishable 400 would let a +// scoped caller enumerate the account table by probing ids and reading which +// error came back. func TestUpsertLadderConfig_NonexistentAccountRejected(t *testing.T) { ctx := context.Background() handler, mockStore, _ := newLadderHandler(t) @@ -162,8 +168,98 @@ func TestUpsertLadderConfig_NonexistentAccountRejected(t *testing.T) { result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) 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.ErrorIs(t, err, errNotFound) mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) } + +// --- issue #1539: allowed_accounts scoping on the ladder config WRITE path --- +// +// getLadderConfigs has filtered on allowed_accounts since migration 000088 +// opened view:config to scoped users; upsertLadderConfig never did. A caller +// scoped to one account could therefore write (and, because the store upserts +// on UNIQUE(cloud_account_id, provider), OVERWRITE) the ladder config of any +// other account -- and the GET then filtered the row back out, so the change +// was invisible to its author afterwards. +// +// Both directions are covered deliberately. A refusal-only test passes just as +// well against a handler that refuses everyone, which would hide the scope +// check having been wired to reject in-scope writes too. + +// ladderScopedReq builds an authenticated PUT carrying body, for the principal +// scopedHandler wires (restricted to scopedInAccount). +func ladderScopedReq(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer " + scopedToken}, + Body: body, + } +} + +// ladderScopedBody builds a valid ladder config body targeting accountID. +// mode=auto_approve with max_hourly_commit_per_run omitted (nil => no cap) is +// the shape from the issue's failure scenario. +func ladderScopedBody(accountID string) string { + return `{"cloud_account_id":"` + accountID + `","provider":"aws","enabled":true,` + + `"mode":"auto_approve","cadence":"daily",` + ladderValidRamp + `}` +} + +// ladderScopedHandler wires a handler whose session is restricted to +// scopedInAccount, with both cloud accounts resolvable so the refusal comes +// from the scope check rather than from a failed lookup. +func ladderScopedHandler(t *testing.T) (*Handler, *MockConfigStore) { + t.Helper() + h, mockStore := scopedHandler(t, scopedInAccount) + mockStore.On("GetCloudAccount", mock.Anything, scopedInAccount). + Return(&config.CloudAccount{ID: scopedInAccount, Name: "in-scope", Provider: "aws"}, nil).Maybe() + mockStore.On("GetCloudAccount", mock.Anything, scopedOutAccount). + Return(&config.CloudAccount{ID: scopedOutAccount, Name: "victim-account", Provider: "aws"}, nil).Maybe() + return h, mockStore +} + +// TestUpsertLadderConfig_OutOfScopeAccountRefused is the guard: this is the +// assertion that fails if the requireAccountAccess call is removed. +func TestUpsertLadderConfig_OutOfScopeAccountRefused(t *testing.T) { + ctx := context.Background() + handler, mockStore := ladderScopedHandler(t) + + result, err := handler.upsertLadderConfig(ctx, ladderScopedReq(ladderScopedBody(scopedOutAccount))) + + require.Error(t, err, "a caller scoped to another account must not write this config (#1539)") + assert.Nil(t, result) + assert.ErrorIs(t, err, errNotFound, + "must be the enumeration-safe not-found, not a 403 that confirms the account exists") + mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) +} + +// TestUpsertLadderConfig_InScopeAccountStillAllowed is the other direction: +// the same restricted principal, the account it IS entitled to. Without this, +// a handler that refused every write would satisfy the test above. +func TestUpsertLadderConfig_InScopeAccountStillAllowed(t *testing.T) { + ctx := context.Background() + handler, mockStore := ladderScopedHandler(t) + mockStore.On("UpsertLadderConfig", mock.Anything, mock.AnythingOfType("*config.LadderConfigDB")). + Return(&config.LadderConfigDB{ID: "written-row", CloudAccountID: scopedInAccount}, nil) + + result, err := handler.upsertLadderConfig(ctx, ladderScopedReq(ladderScopedBody(scopedInAccount))) + + require.NoError(t, err, "a caller scoped to this account must still be able to write its config") + require.NotNil(t, result) + mockStore.AssertCalled(t, "UpsertLadderConfig") +} + +// TestUpsertLadderConfig_UnrestrictedSessionUnaffected pins that the refusal +// above is driven by the caller's SCOPE and not by anything about the account +// itself: an unrestricted session writes that same account unchanged. +func TestUpsertLadderConfig_UnrestrictedSessionUnaffected(t *testing.T) { + ctx := context.Background() + handler, mockStore := scopedHandler(t) // no accounts => grantAdmin => unrestricted + mockStore.On("GetCloudAccount", mock.Anything, scopedOutAccount). + Return(&config.CloudAccount{ID: scopedOutAccount, Name: "victim-account", Provider: "aws"}, nil) + mockStore.On("UpsertLadderConfig", mock.Anything, mock.AnythingOfType("*config.LadderConfigDB")). + Return(&config.LadderConfigDB{ID: "written-row", CloudAccountID: scopedOutAccount}, nil) + + result, err := handler.upsertLadderConfig(ctx, ladderScopedReq(ladderScopedBody(scopedOutAccount))) + + require.NoError(t, err, "an unrestricted session must be unaffected by the #1539 scope check") + require.NotNil(t, result) + mockStore.AssertCalled(t, "UpsertLadderConfig") +}