sec(api): scope ladder config writes to the caller's allowed_accounts (#1770)

* 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

* 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.

Author: cristim

internal/api/handler_ladder.go

@@ -73,9 +73,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
@@ -108,7 +123,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
 	}
 
@@ -120,18 +135,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))

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")
+}