From ec920678bd19f41c529594d012775a29067f0842 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 09:17:41 +0200 Subject: [PATCH 1/2] sec(api): scope ladder config writes to the caller's allowed_accounts upsertLadderConfig gated only on update:config and took cloud_account_id straight from the request body, so a caller scoped to one account could write the commitment-laddering config of any other. The read side of the same feature has filtered on allowed_accounts since migration 000088 opened view:config to scoped users; the write side never did, so the row was also filtered back out of the caller's own GET afterwards. Reachable by any principal holding update:config with a restricted allowed_accounts. No seeded group grants that pairing -- migration 000088 deliberately withheld update:config from the non-admin groups -- so it needs an operator-created custom group, which is why this is severity/high rather than critical. A plain admin is unaffected either way: admin:* carries allowed_accounts ARRAY['*'], so the new check passes for it. What it buys, concretely: the ladder engine only executes configs whose account matches the deployment's own AWS account (the single-account gate in internal/server/handler_ladder.go), and for that account mode auto_approve with a nil max_hourly_commit_per_run means uncapped auto-purchase. Because the store upserts on UNIQUE(cloud_account_id, provider), an out-of-scope caller could also overwrite an admin's existing row for that account -- raise the cap, flip the mode, or set enabled:false to silently stop a running ladder. validateLadderAccountProvider becomes requireLadderAccountAccess and delegates the lookup to requireAccountAccess, which already fetches and nil-checks the account, so the account is fetched once rather than twice. The rename is deliberate: the function now enforces authorization, not just validation. Behaviour change: a cloud_account_id that does not resolve now returns errNotFound (404) instead of 400 "cloud account %q does not exist". 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. Deliberately NOT added: the requirePermissionConstraints call the issue's fix direction suggests. requirePermissionConstraintsAction is hardcoded to "execute" (handler.go), with a comment saying a genuinely new action should add a real parameter back rather than resurrect an unused one. Calling it here would evaluate the caller's execute permission on config, which is not the verb being exercised and would fail closed for anyone holding only update:config. The allowed_accounts check is what closes the reported bypass. Tests cover both directions -- out-of-scope refused, in-scope still allowed -- plus an unrestricted session writing the same account the scoped one was refused, which pins that the refusal is driven by the caller's scope and not by anything about the account. Closes #1539 --- internal/api/handler_ladder.go | 43 +++++++---- internal/api/handler_ladder_test.go | 106 ++++++++++++++++++++++++++-- 2 files changed, 131 insertions(+), 18 deletions(-) diff --git a/internal/api/handler_ladder.go b/internal/api/handler_ladder.go index f8010e246..07770e9dd 100644 --- a/internal/api/handler_ladder.go +++ b/internal/api/handler_ladder.go @@ -74,8 +74,20 @@ 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 { + session, err := h.requirePermission(ctx, req, "update", "config") + if err != nil { return nil, err } @@ -109,7 +121,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 +133,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") +} From 606484940b198d2e18d622024818d471f4d98ec0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 10:47:50 +0200 Subject: [PATCH 2/2] fix(api): avoid shadowing err in upsertLadderConfig Capturing the session (session, err := h.requirePermission) introduced a function-scoped err, which the four subsequent `if err := ...` statements in the same function then shadowed. govet's shadow check flags all four, failing the Lint job. The findings are a side effect of this branch: the previous `if _, err := ...` bound err inside the if-statement, so no function-scoped err existed to shadow. Bind the permission error as permErr instead of err. That leaves the four body-parsing and validation statements untouched -- they are unrelated to this change and rewriting them to `err = ...` would enlarge the diff for no behavioural gain -- and matches the naming already used elsewhere in the package for exactly this situation (planErr, apiKeyErr, awsIDErr). golangci-lint v2.10.1 (the CI-pinned version) now exits 0 with a genuine "0 issues." line. Verified by re-running the linter, not inferred: the earlier run in this worktree reported exit 3 with no findings and the message "parallel golangci-lint is running", which is lock contention rather than a clean result and must not be read as one. No behaviour change: the rename does not touch the guard, and the ten ladder handler tests still pass. --- internal/api/handler_ladder.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/api/handler_ladder.go b/internal/api/handler_ladder.go index 07770e9dd..77f88697c 100644 --- a/internal/api/handler_ladder.go +++ b/internal/api/handler_ladder.go @@ -86,9 +86,12 @@ func (h *Handler) filterLadderConfigsByAllowedAccounts(ctx context.Context, sess // 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) { - session, err := h.requirePermission(ctx, req, "update", "config") - if 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