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

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))

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