From 07689aabe5f0fd058bc0b64a1883aeaf09a487b4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 04:26:12 +0200 Subject: [PATCH 1/2] sec(api): intersect explicit account filters with allowed_accounts on dashboard KPIs resolveDashboardAccountScope applied the session's allowed_accounts scope only when the caller supplied no account_ids/account_id filter at all. A restricted session that DID supply an explicit filter got it back unintersected, and calculateCommitmentMetrics had no downstream post-filter (unlike the recommendations half or the inventory handlers), so GET /api/dashboard/summary?account_ids= leaked that account's ActiveCommitments/CommittedMonthly/YTDSavings/CurrentSavings to a scoped view:recommendations user. Move the allowed_accounts intersection into a new intersectAccountFilterScope helper (internal/api/scoping.go) and run every caller-supplied filter through it unconditionally: an unrestricted session's filter passes through unchanged, a restricted session with no filter still gets the full allowed_accounts scope, and a restricted session with an explicit filter now gets the intersection of the two, dropping any out-of-scope account down to the existing "matches nothing" sentinel. Regression test: TestGetDashboardSummary_ScopedUser_ExplicitOutOfScopeAccountIsIgnored (internal/api/handler_dashboard_test.go) asserts a session scoped to acct-A requesting ?account_ids= gets zeroed commitment KPIs and never calls GetActivePurchaseHistory. Verified it fails pre-fix (git stash on the two source files reintroduces the bug; the mock panics with "I don't know what to return" because the pre-fix code calls GetActivePurchaseHistory carrying acct-B's id, which the test deliberately leaves unstubbed) and passes post-fix. A companion test, TestGetDashboardSummary_ScopedUser_ExplicitInScopeAccountStillWorks, guards against overcorrecting the intersection for an account the session does have access to. The audit's A02-006 finding (getHistory applies its LIMIT before the allowed_accounts post-filter, truncating a scoped user's page) is the same unintersected-scope shape but on a different, more invasive code path (the SQL LIMIT/pagination contract in fetchPurchaseHistory) and is left as a residual finding per the issue, noted in the PR body rather than folded into this fix. Closes #99 Co-Authored-By: claude-flow --- internal/api/handler_dashboard.go | 53 +++++----- internal/api/handler_dashboard_test.go | 130 +++++++++++++++++++++++++ internal/api/scoping.go | 87 +++++++++++++++++ 3 files changed, 242 insertions(+), 28 deletions(-) diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 5b5275f3a..20294e64d 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -22,26 +22,11 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun return nil, err } - accountUUIDs, accountExternalIDsByProvider, err := h.resolveDashboardAccountScope(ctx, params) + accountUUIDs, accountExternalIDsByProvider, err := h.resolveDashboardAccountScope(ctx, session, params) if err != nil { return nil, err } - // Issue #956 (CR): when the session is account-restricted and no explicit - // account filter was supplied, scope the commitment metrics to the session's - // allowed_accounts instead of falling through to all-accounts history. The - // recommendations half is already gated by filterDashboardRecommendations; - // without this the commitment KPIs (ActiveCommitments / CommittedMonthly / - // CurrentCoverage / YTDSavings) would leak other accounts' data to a scoped - // user. Unrestricted / admin sessions resolve to an empty scope and keep the - // all-accounts behavior. - if len(accountUUIDs) == 0 && len(accountExternalIDsByProvider) == 0 { - accountUUIDs, accountExternalIDsByProvider, err = h.resolveAllowedAccountScope(ctx, session) - if err != nil { - return nil, err - } - } - recommendations, err := h.scheduler.ListRecommendations(ctx, config.RecommendationFilter{ Provider: params["provider"], }) @@ -91,9 +76,12 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun }, nil } -// resolveDashboardAccountScope parses the account filter params and resolves -// them to the dual-column purchase-history filter inputs: the cloud_accounts -// UUIDs and their cloud-provider external account numbers. +// resolveDashboardAccountScope parses the account filter params, resolves +// them to the dual-column purchase-history filter inputs (cloud_accounts +// UUIDs and their cloud-provider external account numbers), and then +// intersects that caller-supplied scope with the session's allowed_accounts +// via intersectAccountFilterScope. +// // Both are needed because purchase_history rows carry either identifier // independently and the top-bar chip emits the UUID, so matching only // cloud_account_id dropped every NULL-cloud_account_id row (issue #701/#498). @@ -104,24 +92,33 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun // raw external number for pre-UUID callers; resolved via // resolveSingleAccountFilterIDs. // -// Both return values are nil when neither param is supplied, so the caller -// fetches across all accounts (or, for a restricted session, scopes to the -// session's allowed_accounts — see resolveAllowedAccountScope). -func (h *Handler) resolveDashboardAccountScope(ctx context.Context, params map[string]string) (uuids []string, externalIDsByProvider map[string][]string, err error) { +// Issue #99: an EARLIER version of this function returned the caller-supplied +// filter as-is and left the allowed_accounts fallback to a guard in the +// caller that only fired when NO filter was supplied — so a restricted +// session that supplied an explicit (possibly out-of-scope) account_ids/ +// account_id filter bypassed the session's allowed_accounts entirely, with no +// downstream post-filter to catch it (unlike handler_inventory.go's +// analogous no-filter-only retrofit, which happens to be saved by its own +// separate filterPurchaseHistoryByAllowedAccounts post-filter). The +// intersection now lives here, in the resolver, so every caller gets it for +// free rather than having to remember a guard or a post-filter. +func (h *Handler) resolveDashboardAccountScope(ctx context.Context, session *Session, params map[string]string) (uuids []string, externalIDsByProvider map[string][]string, err error) { parsedUUIDs, parseErr := parseAccountIDs(params["account_ids"]) if parseErr != nil { return nil, nil, NewClientError(400, parseErr.Error()) } + var filterUUIDs []string + var filterExternalIDsByProvider map[string][]string if len(parsedUUIDs) > 0 { // UUID-based multi-account filter — takes precedence over legacy param. - uuids, externalIDsByProvider = h.resolveAccountFilterIDs(ctx, parsedUUIDs) - return uuids, externalIDsByProvider, nil + filterUUIDs, filterExternalIDsByProvider = h.resolveAccountFilterIDs(ctx, parsedUUIDs) + } else { + // No plural UUID filter: fall back to the legacy singular param. + filterUUIDs, filterExternalIDsByProvider = h.resolveSingleAccountFilterIDs(ctx, params["account_id"]) } - // No plural UUID filter: fall back to the legacy singular param. - uuids, externalIDsByProvider = h.resolveSingleAccountFilterIDs(ctx, params["account_id"]) - return uuids, externalIDsByProvider, nil + return h.intersectAccountFilterScope(ctx, session, filterUUIDs, filterExternalIDsByProvider) } // resolveAllowedAccountScope resolves a restricted session's allowed_accounts diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 85ab0e46a..dd241a017 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -1399,6 +1399,136 @@ func TestHandler_getDashboardSummary_Errors(t *testing.T) { }) } +// TestGetDashboardSummary_ScopedUser_ExplicitOutOfScopeAccountIsIgnored is the +// issue #99 regression test. Read-Only user "viewer-1" is restricted to +// acct-A only (GetAllowedAccountsAPI). Before the fix, resolveDashboardAccountScope +// applied the allowed_accounts fallback ONLY when the caller supplied no +// account_ids/account_id filter at all; a caller who DID supply an explicit +// filter got it back unintersected, so requesting acct-B's UUID leaked +// acct-B's commitment KPIs (ActiveCommitments/CommittedMonthly/YTDSavings) to +// a session with no access to acct-B. +// +// This must fail pre-fix: the old resolveDashboardAccountScope returned +// acct-B's UUID unchanged, fetchCommitmentPurchases would NOT short-circuit, +// and GetActivePurchaseHistory would be called carrying "acct-B-uuid" -- which +// this test does not stub, so the pre-fix code panics the mock ("I don't know +// what to return") instead of silently passing. Post-fix, the intersection of +// {acct-B} (requested) and {acct-A} (allowed) is empty, so +// fetchCommitmentPurchases short-circuits on the non-nil-but-empty sentinel +// and GetActivePurchaseHistory is never called. +func TestGetDashboardSummary_ScopedUser_ExplicitOutOfScopeAccountIsIgnored(t *testing.T) { + ctx := context.Background() + const scopedUserID = "viewer-1" + const acctAUUID = "aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa" + const acctBUUID = "bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbbbb" + + mockScheduler := new(MockScheduler) + mockStore := new(MockConfigStore) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockScheduler.AssertExpectations(t) + }) + + mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) + // Deliberately NOT stubbing GetActivePurchaseHistory: the fix must never + // reach it for this request, so any call (with any arguments, including + // acct-B's) fails the test via testify's "I don't know what to return". + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{ + {ID: acctAUUID, Name: "Account A"}, + {ID: acctBUUID, Name: "Account B"}, + }, nil + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "viewer-token").Return(&Session{ + UserID: scopedUserID, + Email: "viewer@example.com", + }, nil) + mockAuth.On("HasPermissionAPI", ctx, scopedUserID, "view", "recommendations").Return(true, nil) + // Restricted to acct-A only. + mockAuth.On("GetAllowedAccountsAPI", ctx, scopedUserID).Return([]string{acctAUUID}, nil) + + handler := &Handler{ + auth: mockAuth, + scheduler: mockScheduler, + config: mockStore, + } + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer viewer-token"}, + } + + result, err := handler.getDashboardSummary(ctx, req, map[string]string{"account_ids": acctBUUID}) + require.NoError(t, err) + + assert.Equal(t, 0, result.ActiveCommitments, "acct-B is outside allowed_accounts; must not contribute commitments") + assert.Equal(t, 0.0, result.CommittedMonthly, "acct-B's committed spend must not leak to a session scoped to acct-A") + assert.Equal(t, 0.0, result.YTDSavings, "acct-B's realized savings must not leak to a session scoped to acct-A") +} + +// TestGetDashboardSummary_ScopedUser_ExplicitInScopeAccountStillWorks guards +// against an overcorrection of the issue #99 fix: a restricted session that +// explicitly requests an account it DOES have access to must still see that +// account's commitment KPIs, not get zeroed out by the new intersection. +func TestGetDashboardSummary_ScopedUser_ExplicitInScopeAccountStillWorks(t *testing.T) { + ctx := context.Background() + const scopedUserID = "viewer-1" + const acctAUUID = "aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa" + const acctBUUID = "bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbbbb" + + now := time.Now() + purchases := []config.PurchaseHistoryRecord{ + { + AccountID: acctAUUID, + Service: "ec2", + Timestamp: now.AddDate(0, -3, 0), + Term: 1, + EstimatedSavings: 42.0, + }, + } + + mockScheduler := new(MockScheduler) + mockStore := new(MockConfigStore) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockScheduler.AssertExpectations(t) + }) + + mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string{acctAUUID}, map[string][]string(nil)).Return(purchases, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{ + {ID: acctAUUID, Name: "Account A"}, + {ID: acctBUUID, Name: "Account B"}, + }, nil + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "viewer-token").Return(&Session{ + UserID: scopedUserID, + Email: "viewer@example.com", + }, nil) + mockAuth.On("HasPermissionAPI", ctx, scopedUserID, "view", "recommendations").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, scopedUserID).Return([]string{acctAUUID}, nil) + + handler := &Handler{ + auth: mockAuth, + scheduler: mockScheduler, + config: mockStore, + } + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer viewer-token"}, + } + + result, err := handler.getDashboardSummary(ctx, req, map[string]string{"account_ids": acctAUUID}) + require.NoError(t, err) + + assert.Equal(t, 1, result.ActiveCommitments) + assert.InDelta(t, 42.0, result.CommittedMonthly, 0.001) +} + func TestHandler_getUpcomingPurchases_Errors(t *testing.T) { ctx := context.Background() diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 053565b03..3dd42c01a 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -279,6 +279,93 @@ func (h *Handler) resolveSingleAccountFilterIDs(ctx context.Context, accountID s return nil, map[string][]string{"": {accountID}} } +// intersectAccountFilterScope narrows a caller-supplied dual-column account +// filter (filterUUIDs, filterExternalIDsByProvider — e.g. the resolved +// account_ids/account_id query params) down to the accounts the session's +// allowed_accounts scope permits. +// +// This is the single choke point for the "retrofit only guards the +// no-explicit-filter branch" defect (issue #99): resolveDashboardAccountScope +// previously fell back to resolveAllowedAccountScope only when the caller +// supplied no filter at all, so a restricted session that DID supply an +// account_ids/account_id filter got that filter unintersected — any account +// the client named came straight through, in or out of allowed_accounts. Every +// resolver that turns a client-controlled account filter into a query-ready +// scope must run it through here rather than re-deriving the guard locally. +// +// - Unrestricted session (resolveAllowedAccountScope's AllowsAll() case, +// signaled by a nil uuid slice): the caller-supplied filter passes through +// unchanged, including nil/nil for "no filter -> all accounts". +// - Restricted session, no caller-supplied filter: narrows to the full +// allowed_accounts scope (the pre-existing no-filter behavior). +// - Restricted session, explicit caller-supplied filter: narrows to the +// INTERSECTION of the two sets. An account named in the filter but +// outside allowed_accounts is dropped rather than honored. An empty +// intersection returns the non-nil-but-empty uuid sentinel that +// fetchCommitmentPurchases / GetActivePurchaseHistory callers already +// treat as "match nothing" (issue #956). +func (h *Handler) intersectAccountFilterScope( + ctx context.Context, + session *Session, + filterUUIDs []string, + filterExternalIDsByProvider map[string][]string, +) (uuids []string, externalIDsByProvider map[string][]string, err error) { + allowedUUIDs, allowedExternalIDsByProvider, err := h.resolveAllowedAccountScope(ctx, session) + if err != nil { + return nil, nil, err + } + if allowedUUIDs == nil { + // resolveAllowedAccountScope's (nil, nil, nil) sentinel for an + // unrestricted/admin session: keep the caller-supplied filter as-is. + return filterUUIDs, filterExternalIDsByProvider, nil + } + + if len(filterUUIDs) == 0 && len(filterExternalIDsByProvider) == 0 { + // No explicit filter from the caller: the effective scope is the full + // allowed_accounts set. + return allowedUUIDs, allowedExternalIDsByProvider, nil + } + + uuids, externalIDsByProvider = intersectDualColumnScope( + filterUUIDs, filterExternalIDsByProvider, + allowedUUIDs, allowedExternalIDsByProvider, + ) + return uuids, externalIDsByProvider, nil +} + +// intersectDualColumnScope computes the AND of two dual-column account +// filters: an id (UUID, or external id under its provider bucket) survives +// only when present on both sides. Pure set intersection is correct here +// because both sides are always produced by resolveAccountFilterIDs (or its +// callers) from the same cloud_accounts source, so a given account resolves +// to the same UUID/external-id representation on both sides. +// +// The returned uuid slice is always non-nil (even when empty) so callers can +// rely on the "non-nil-but-empty means scoped to zero accounts" sentinel +// documented on resolveAllowedAccountScope. The returned map is nil when no +// external id survives the intersection. +func intersectDualColumnScope( + filterUUIDs []string, filterExternalIDsByProvider map[string][]string, + allowedUUIDs []string, allowedExternalIDsByProvider map[string][]string, +) (uuids []string, externalIDsByProvider map[string][]string) { + uuids = make([]string, 0, len(filterUUIDs)) + for _, id := range filterUUIDs { + if stringInSlice(id, allowedUUIDs) { + uuids = append(uuids, id) + } + } + + for provider, ids := range filterExternalIDsByProvider { + allowedIDs := allowedExternalIDsByProvider[provider] + for _, id := range ids { + if stringInSlice(id, allowedIDs) { + externalIDsByProvider = addExternalIDForProvider(externalIDsByProvider, provider, id) + } + } + } + return uuids, externalIDsByProvider +} + // a map from account identifier → display name. The map is keyed by BOTH // the internal UUID (CloudAccount.ID) and the cloud-provider external ID // (CloudAccount.ExternalID, e.g. an AWS account number or Azure subscription From c11a240ef0f5815d440ba30c66327cac312a12fe Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 05:12:21 +0200 Subject: [PATCH 2/2] fix(api): match the "" external-id bucket against every allowed provider CodeRabbit review on #374: intersectDualColumnScope compared filterExternalIDsByProvider's "" (unknown-provider) bucket only against allowedExternalIDsByProvider[""], which resolveAllowedAccountScope never populates -- it always groups by the account's real provider via resolveAccountFilterIDs. The "" bucket comes from resolveSingleAccountFilterIDs's fallback for a legacy account_id that resolves to a raw external number rather than a known UUID, so a scoped user filtering by an external number belonging to one of their own allowed accounts got zeroed commitment KPIs instead of that account's real data. Fails closed (no leak), but a functional regression from pre-#99-fix behavior. Fix: match a "" entry against every allowed provider's ids and, on a hit, re-group it under that real provider so the downstream dual-column predicate still scopes it correctly. Regression test: TestGetDashboardSummary_ScopedUser_LegacyExternalAccountIDStillMatchesAllowedAccount (internal/api/handler_dashboard_test.go). Verified it fails against the prior intersectDualColumnScope (git stash on scoping.go alone, keeping the new test) -- ActiveCommitments comes back 0 instead of 1 -- and passes with this fix. Co-Authored-By: claude-flow --- internal/api/handler_dashboard_test.go | 79 ++++++++++++++++++++++++++ internal/api/scoping.go | 37 +++++++++--- 2 files changed, 109 insertions(+), 7 deletions(-) diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index dd241a017..72eeed752 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -1529,6 +1529,85 @@ func TestGetDashboardSummary_ScopedUser_ExplicitInScopeAccountStillWorks(t *test assert.InDelta(t, 42.0, result.CommittedMonthly, 0.001) } +// TestGetDashboardSummary_ScopedUser_LegacyExternalAccountIDStillMatchesAllowedAccount +// is the CodeRabbit #374 follow-up on the issue #99 fix: the legacy singular +// `account_id` param can resolve to a raw external account number grouped +// under the "" (unknown-provider) bucket (resolveSingleAccountFilterIDs, when +// the value isn't a known cloud_accounts UUID). intersectDualColumnScope's +// first version compared that "" bucket only against +// allowedExternalIDsByProvider[""], which is never populated -- +// resolveAllowedAccountScope always groups by the account's REAL provider -- +// so a scoped user filtering by an external number that legitimately belongs +// to one of their allowed accounts got zeroed KPIs instead of that account's +// real data (fails closed, but a functional regression from pre-fix +// behavior). +// +// acct-A resolves to external id "111122223333" under provider "aws"; the +// session is scoped to acct-A; the request filters by +// account_id=111122223333 (the external number, not the UUID). The fix must +// match the "" bucket against every allowed provider's ids and re-group the +// hit under its real provider, so GetActivePurchaseHistory is still called +// with that id and the KPIs are NOT zeroed. +func TestGetDashboardSummary_ScopedUser_LegacyExternalAccountIDStillMatchesAllowedAccount(t *testing.T) { + ctx := context.Background() + const scopedUserID = "viewer-1" + const acctAUUID = "aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa" + const acctAExternalID = "111122223333" + + now := time.Now() + purchases := []config.PurchaseHistoryRecord{ + { + AccountID: acctAExternalID, + Service: "ec2", + Provider: "aws", + Timestamp: now.AddDate(0, -3, 0), + Term: 1, + EstimatedSavings: 77.0, + }, + } + + mockScheduler := new(MockScheduler) + mockStore := new(MockConfigStore) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockScheduler.AssertExpectations(t) + }) + + mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) + // filterUUIDs is empty (the raw external number isn't a known UUID), so + // only the external-id half of the dual-column predicate carries the match. + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string{}, map[string][]string{"aws": {acctAExternalID}}).Return(purchases, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{ + {ID: acctAUUID, Name: "Account A", Provider: "aws", ExternalID: acctAExternalID}, + }, nil + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "viewer-token").Return(&Session{ + UserID: scopedUserID, + Email: "viewer@example.com", + }, nil) + mockAuth.On("HasPermissionAPI", ctx, scopedUserID, "view", "recommendations").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, scopedUserID).Return([]string{acctAUUID}, nil) + + handler := &Handler{ + auth: mockAuth, + scheduler: mockScheduler, + config: mockStore, + } + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer viewer-token"}, + } + + result, err := handler.getDashboardSummary(ctx, req, map[string]string{"account_id": acctAExternalID}) + require.NoError(t, err) + + assert.Equal(t, 1, result.ActiveCommitments, "the external-number filter matches acct-A, which IS in allowed_accounts") + assert.InDelta(t, 77.0, result.CommittedMonthly, 0.001) +} + func TestHandler_getUpcomingPurchases_Errors(t *testing.T) { ctx := context.Background() diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 3dd42c01a..6ab6c671b 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -335,10 +335,24 @@ func (h *Handler) intersectAccountFilterScope( // intersectDualColumnScope computes the AND of two dual-column account // filters: an id (UUID, or external id under its provider bucket) survives -// only when present on both sides. Pure set intersection is correct here -// because both sides are always produced by resolveAccountFilterIDs (or its -// callers) from the same cloud_accounts source, so a given account resolves -// to the same UUID/external-id representation on both sides. +// only when present on both sides. The uuid half is a straightforward set +// intersection because both sides are always produced by +// resolveAccountFilterIDs (or resolveAllowedAccountScope, its caller) from +// the same cloud_accounts source, so a given account resolves to the same +// UUID on both sides. +// +// The external-id half needs one extra step for the "" (unknown-provider) +// bucket. filterExternalIDsByProvider can carry a "" entry -- +// resolveSingleAccountFilterIDs groups a raw external number under "" when +// the value isn't a known UUID (or the cloud_accounts load failed) -- but +// allowedExternalIDsByProvider (built from resolveAccountFilterIDs, which +// only ever groups by a *resolved* account's real provider) never has a "" +// key. A plain per-provider lookup would therefore always miss the "" +// bucket and zero out an otherwise-in-scope legacy external-number filter +// (CodeRabbit #374). Since "" means "provider not known", a "" entry is +// matched against every allowed provider's ids and, on a hit, re-grouped +// under that real provider so the downstream dual-column predicate still +// scopes it correctly. // // The returned uuid slice is always non-nil (even when empty) so callers can // rely on the "non-nil-but-empty means scoped to zero accounts" sentinel @@ -356,10 +370,19 @@ func intersectDualColumnScope( } for provider, ids := range filterExternalIDsByProvider { - allowedIDs := allowedExternalIDsByProvider[provider] for _, id := range ids { - if stringInSlice(id, allowedIDs) { - externalIDsByProvider = addExternalIDForProvider(externalIDsByProvider, provider, id) + if provider != "" { + if stringInSlice(id, allowedExternalIDsByProvider[provider]) { + externalIDsByProvider = addExternalIDForProvider(externalIDsByProvider, provider, id) + } + continue + } + // Unknown-provider bucket: narrow to whichever allowed provider(s) + // actually own this id. + for allowedProvider, allowedIDs := range allowedExternalIDsByProvider { + if stringInSlice(id, allowedIDs) { + externalIDsByProvider = addExternalIDForProvider(externalIDsByProvider, allowedProvider, id) + } } } }