sec(api): dashboard commitment KPIs leak cross-tenant when the caller supplies an account filter #99

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Where

• internal/api/handler_dashboard.go:17-93 (getDashboardSummary)
• Scope retrofit: internal/api/handler_dashboard.go:39-44
• Unscoped consumption: internal/api/handler_dashboard.go:70 (calculateCommitmentMetrics)
• Client-controlled scope resolution: internal/api/handler_dashboard.go:111-126 (resolveDashboardAccountScope)
• Downstream: fetchCommitmentPurchases at :634 -> GetActivePurchaseHistory
• Route: AuthUser (internal/api/router.go), gated on view:recommendations only

What

resolveDashboardAccountScope builds accountUUIDs / accountExternalIDsByProvider purely from the client's account_ids / account_id query params. The allowed_accounts retrofit added for issue LeanerCloud/cloud-commitments-cli#956 sits behind this guard:

// Issue LeanerCloud/cloud-commitments-cli#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)
    ...
}

The comment records the intent precisely: the session's scope is applied "when the session is account-restricted and no explicit account filter was supplied". The parameterised case is simply unhandled. When the caller does supply a filter, the branch is skipped entirely and those client-controlled IDs flow straight into:

activeCommitments, committedMonthly, ytdSavings, currentSavingsByService :=
    h.calculateCommitmentMetrics(ctx, params["provider"], accountUUIDs, accountExternalIDsByProvider)

There is no intersection with the session's allowed_accounts and, unlike the inventory handlers, no downstream post-filter: filterPurchaseHistoryByAllowedAccounts is never called on the commitment half. Only the recommendations half is scoped, via filterDashboardRecommendations at :53.

Failure scenario

Read-Only user U has allowed_accounts = [acct-A] and view:recommendations.

1. GET /api/dashboard/summary (no params) correctly reports acct-A only. The fix(api): filter purchases by account UUID AND external id (closes #701, #498, #866) cloud-commitments-cli#956 retrofit fires.
2. U calls GET /api/dashboard/summary?account_ids=<acct-B-uuid>.
3. The guard at :39 is skipped, and U receives acct-B's active_commitments, committed_monthly, ytd_savings, and the full by_service[].current_savings breakdown: another tenant's commitment spend and realized savings, from an AuthUser route, with no admin rights.

Account UUIDs are obtainable from GET /api/plans/{id}/accounts (unscoped, see the grouped medium/low issue from this same review) or GET /api/accounts/list.

Contrast that shows the gap is unintentional

• internal/api/handler_inventory.go:105-121 has the identical unintersected param path, but both callers (listActiveCommitments:47, getCoverageBreakdown:197) run filterPurchaseHistoryByAllowedAccounts afterwards.
• The analytics handlers gate on validateAnalyticsAccountScope (internal/api/handler_analytics.go:234-250), which does check the requested account_id against allowed_accounts for restricted sessions on all three analytics endpoints.

The dashboard is the one commitment reader with neither defence.

Fix direction

Intersect the resolved scope with resolveAllowedAccountScope unconditionally rather than only in the empty-filter branch: for a restricted session, the effective scope must be resolveDashboardAccountScope(params) INTERSECT resolveAllowedAccountScope(session), with an empty intersection returning empty KPIs (or 403) rather than falling through. Alternatively, post-filter calculateCommitmentMetrics' input the way handler_inventory.go does.

Regression test must use the explicit-filter shape (?account_ids=<out-of-scope-uuid>), not the no-filter shape. A test written against the no-filter path passes today and would have passed with this bug present.

Structural root cause (recorded here so it is fixed as a class, not a line)

This is the second recurring shape in internal/api, alongside "the read side of a feature is scoped by allowed_accounts, the write side of the same feature is not":

Where a scope check was retrofitted, it was applied to the no-explicit-filter branch only. A client that supplies a filter bypasses the retrofit.

The review found this shape in three places. Two are saved by a downstream post-filter and are therefore currently harmless:

• handler_inventory.go:105-121 -> saved by filterPurchaseHistoryByAllowedAccounts at listActiveCommitments:47 and getCoverageBreakdown:197.
• The analytics path -> saved by validateAnalyticsAccountScope (handler_analytics.go:234-250).
• handler_dashboard.go:39-44 -> not saved by anything. This issue.

Two consequences for whoever fixes this:

1. The two "safe" sites are safe by accident of a second, independently-added defence. Adding a third caller of the inventory helper without remembering the post-filter reintroduces the leak. The intersection belongs in the scope resolver, not in each caller.
2. Any future allowed_accounts retrofit in this package must be reviewed against the parameterised path, not just the default path. A retrofit whose guard is if len(clientSuppliedFilter) == 0 is by construction bypassable by supplying the filter.

Related

• bug(api/dashboard): calculateCommitmentMetrics does not intersect allowed_accounts for scoped sessions cloud-commitments-cli#959 (bug(api/dashboard): calculateCommitmentMetrics does not intersect allowed_accounts for scoped sessions) - related but not the same defect; please do not close this as already-done. bug(api/dashboard): calculateCommitmentMetrics does not intersect allowed_accounts for scoped sessions cloud-commitments-cli#959 covers the no-filter case, and the fix(api): filter purchases by account UUID AND external id (closes #701, #498, #866) cloud-commitments-cli#956 retrofit quoted above is what created the branch this issue is about. Note also that bug(api/dashboard): calculateCommitmentMetrics does not intersect allowed_accounts for scoped sessions cloud-commitments-cli#959's stated Expected Behaviour is "commitment metrics should be restricted to the same account set that governs the rest of the dashboard (the set resolved by resolveDashboardAccountScope)" - and that set is exactly the client-controlled one. An implementation that satisfies bug(api/dashboard): calculateCommitmentMetrics does not intersect allowed_accounts for scoped sessions cloud-commitments-cli#959 as literally written leaves this leak open. Whoever picks up either issue should fix both, and bug(api/dashboard): calculateCommitmentMetrics does not intersect allowed_accounts for scoped sessions cloud-commitments-cli#959's acceptance criteria should be amended to require the intersection rather than deference to resolveDashboardAccountScope.
• fix(api): filter purchases by account UUID AND external id (closes #701, #498, #866) cloud-commitments-cli#956 - the PR whose CR feedback added the guarded retrofit at :39-44.
• LeanerCloud/cloud-commitments-cli#1377 - a different defect in the same function family (fetchCommitmentPurchases zeroing KPIs on a store error).

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A02-026 (medium)

The reason this stays invisible to CI, for whoever writes the regression test. Every getDashboardSummary call in handler_dashboard_test.go (lines 79, 142, 186, 1212, 1257, 1302, 1352, 1372, 1396) passes either no account params or only provider, and the three scoped tests that do stub GetAllowedAccountsAPI (:636, :681, :1529) exercise filterDashboardRecommendations and upcoming purchases only. No test combines a scoped session with account_id/account_ids, which is exactly this bug's shape, so a fix needs a new test rather than an updated one -- TestGetDashboardSummary_ScopedUser_ExplicitOutOfScopeAccountIsIgnored, asserting zeroed commitment KPIs and no GetActivePurchaseHistory call carrying the foreign id. A related fixture flaw sits next door: handler_marketplace_test.go:164-165, 200-201 grant "acct-1"/"acct-other", i.e. the row's UUID, whereas production scopes hold account names, so the sell-own scope test passes against code that denies every real name-scoped user. (audit finding A02-026)

A02-006 (low)

The same unintersected-scope shape appears on the History read, with truncation instead of a leak as the consequence. getHistory (internal/api/handler_history.go:39-71) runs fetchPurchaseHistory with LIMIT filters.Limit (default 100) across every account's purchase_history rows and only then calls filterPurchaseHistoryByAllowedAccounts at :71. A scoped user whose account holds 20% of purchases gets roughly 20 rows for limit=100, cannot page because there is no offset, and the summary totals describe only that slice. The fix you prescribe here is the same one: fold the resolved scope into filters.AccountIDs / ExternalIDsByProvider before the SQL read. Separately, parseHistoryFilters (:699-706) rewrites limit=0 and limit=-5 to the default and limit=5000 to 1000 rather than returning 400. Finding A02-006.