[cristim]

Closes #1452.

Problem:

• parseAccountIDs accepted uppercase UUIDs but kept them as given, and the IDs were compared case-sensitively afterwards. A scoped user's own uppercase ID was refused, and an admin silently got an empty result. This affected /history and the analytics routes.
• The Savings History and Trends views still sent the account filter only when exactly one account was selected. Trends also sent the singular account_id.
• mergeAccountFilter duplicated the /history merge helpers.

Change:

• Backend (internal/api/validation.go, handler_analytics.go):
  • parseAccountIDs trims and lowercases IDs, so every caller gets canonical IDs. The 200-ID cap and the 400 on garbage are unchanged.
  • mergeAccountFilter reuses appendMissing and addExternalIDForProvider.
  • A stale comment is fixed.
• Frontend:
  • modules/savings-history.ts and trends.ts send the full selection as account_ids.
  • Trends does this through AnalyticsTrendsFilter.accountIDs in api/analytics.ts.
  • Both routes already read account_ids, since fix(analytics): honour account_ids on the savings analytics routes #1448.

Tests:

• Go:
  • TestParseAccountIDs_Canonical;
  • TestAnalytics_AccountIDs_UppercasePaddedCanonical, covering 3 routes for both admin and scoped users;
  • a new TestHandler_getHistory_FilterParams subtest.
• jest: savings-history, trends and api-analytics send every selected account.
• Mutation probes:
  • dropping the lowercasing fails the 3 new Go tests;
  • reverting savings-history to the singular param fails its test;
  • reverting trends fails the type check.

Local:

• go build ./... passes, and go test -race and the integration tests pass on api.
• golangci-lint is clean.
• Frontend: jest passes (4537), and tsc and eslint show 0 errors. pre-commit passes.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 11 billable files and costs up to $2.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 41 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 63 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/account-ids-canonical
• File: .coderabbit.yaml
• Commit: b9f8cf4
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: 26f1f287-e80f-48f2-bb9a-fabfbd9b807c

Commits

Reviewing files that changed from the base of the PR and between b0972e3 and b9f8cf4.

Files selected for processing: 11

• frontend/src/__tests__/api-analytics.test.ts
• frontend/src/__tests__/savings-history.test.ts
• frontend/src/__tests__/trends.test.ts
• frontend/src/api/analytics.ts
• frontend/src/modules/savings-history.ts
• frontend/src/trends.ts
• internal/api/coverage_gaps_test.go
• internal/api/handler_analytics.go
• internal/api/handler_analytics_account_ids_test.go
• internal/api/handler_history_test.go
• internal/api/validation.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review 1 (cold, fresh context):

VERDICT: APPROVE 3256bbe
Reviewer: independent adversarial review (non-author), PR #1497, closes #1452. No blocking findings.

Findings:

• nit frontend/src/modules/savings-history.ts:108 - the rewritten comment line runs to about 115 chars; reflow it. eslint does not enforce a limit, so it passes.
• nit frontend/src/trends.ts:92 + api/analytics.ts:71 - trends now sends account_ids, so a stale or hand-edited ?account= URL gets a 400 on /analytics/trends. Before, the singular param treated it as an external id. Same accepted trade-off as fix(analytics): honour account_ids on the savings analytics routes #1448 for the savings chart; dashboard/summary already 400s.

Attacked:

• Lowercase vs scope check: parseAccountIDs (validation.go:551) lowercases only after UUID validation. The regex (validation.go:30) is ASCII [0-9a-fA-F], so ToLower cannot fold non-ASCII. Every caller (handler_analytics.go:242, handler_history.go:748, handler_dashboard.go:122, handler_recommendations.go:56, handler_plans.go:29) parses before any scope or intersection step. I found no check on the raw form followed by a query on the canonical form.
• Bypass: the canonical value is itself a valid request (lowercase UUID), so canonicalising maps every input onto one the caller could already send. It cannot reach an account the lowercase form would not. AccountScope.Allows (auth/account_scope.go:145) is still exact-match. An allow-list entry stored in uppercase now stops matching the lowercased id, which fails closed (narrower access). An uppercase foreign id now returns the same 404 as its lowercase form (scoped analytics) or is dropped the same way at intersection (dashboard/history/plans). No leak, so the report has no PRIVATE section.
• External ids: not lowercased. They arrive only via the legacy singular account_id, which bypasses parseAccountIDs, and account_ids rejects non-UUIDs. AWS, Azure and GCP external ids are therefore unaffected, case-sensitive or not.
• Helper reuse is equivalent to the old mergeAccountFilter:
  • appendMissing (handler_history.go:1101) keeps order and dedups. Unlike the old code it also skips "", which can't happen because a.ID is never empty.
  • addExternalIDForProvider (scoping.go:371) dedups per provider, keeps order and allocates lazily.
  • One difference: the old code allocated an empty map for an addExt provider with an empty slice. Unreachable, since providerScopedExternalID returns nil, never an empty slice.
• Frontend: getAnalyticsTrends has one caller (trends.ts:91). getSavingsAnalytics has three: dashboard.ts:1176 and :1347, savings-history.ts:119. All now send account_ids when any account is selected. The only remaining singular account_id senders are inventory.ts:27/:43 and riexchange.ts:33, which hit other routes. The topbar chip is still single-select (topbar-filters.ts:185), so UI behaviour today is unchanged.
• git merge-tree --write-tree origin/main HEAD: clean (37cbba29).
• go test -race ./internal/api: 4776 passed. Integration suite (-tags integration) for ./internal/api: ok.
• Frontend: npm ci ok. jest on api-analytics, savings-history, trends and api-history tests: 141/141 passed. tsc clean. eslint on the touched sources clean.
• Mutation probes (scratch; each file restored with cp and confirmed byte-identical to HEAD):
  • drop ToLower -> TestParseAccountIDs_Canonical, getHistory_FilterParams FAIL.
  • uuid append without dedup -> DuplicatesCollapse FAIL.
  • drop the external-id merge -> AllReachFilter, ScopedAllowedSubset, DuplicatesCollapse, AbsentUnchanged FAIL.
  • savings-history back to length===1 -> 1 jest FAIL.
  • trends forwards [] -> 1 jest FAIL.
  • getAnalyticsTrends sends only the first id -> api-analytics FAIL. A first attempt with accountIDs[0] only failed the TS compile of the suite, so it was redone type-safely with slice(0,1).

[cristim]

Independent review 1, delta (same cold reviewer):

VERDICT: APPROVE b9f8cf4
Delta review of #1497 from 3256bbe (approved earlier) to b9f8cf4. No findings.

• Merge f67564b (parents 3256bbe and b0972e3 = origin/main incl. d39a375 fix(auth): bind group account scope entries to account IDs #1116): git show --remerge-diff is empty. Its diff against main is the same 11 files (+96/-40) as the approved PR. merge-tree of origin/main and b9f8cf4 is clean.
• b9f8cf4: one line, handler_analytics_account_ids_test.go:145, where the scoped fixture changes from {"a","b"} to {idA, idB}. After fix(auth): bind group account scope entries to account IDs #1116, AccountScope.Allows (auth/account_scope.go:190-201) matches an entry only by account ID, plus a unique name that also identifies the id. The other scoped fixtures in the file already use ids (:32, :50).
• Canonicalisation still holds: contains() (:217) is an exact, case-sensitive match. parseAccountIDs lowercases (validation.go:551) before resolveAnalyticsAccountFilter calls Allows, so an uppercase own id passes the scope check.
• Uppercase foreign id: I added a scratch-only test, scope {A} with account_ids=UPPER(unregistered),UPPER(C) and " UPPER(C)". All three routes return 404 with no query filter.
• go test -race -count=1 ./internal/api/ on an archive of b9f8cf4: ok.
• Probe, drop ToLower at validation.go:551: all six UppercasePaddedCanonical subtests fail (3 routes x scoped and unrestricted), plus ParseAccountIDs_Canonical and getHistory uppercase. Restored, byte-identical to HEAD confirmed.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE b9f8cf4

Scope: first reviewed at 3256bbe, then extended to b9f8cf4 (merge of main f67564b plus a one-line fixture fix). The production diff is the same 11 files, +96/-40, at both heads.

Findings:

• internal/api/handler_analytics.go:332-357 (scopedAnalyticsAccountFilter external-id branch) - account_ids values that are not registered UUIDs are still read as raw external ids, and they are now lowercased before externalIDOwner and Allows compare them exactly. cloud_accounts.external_id is free-text VARCHAR(255) (internal/database/postgres/migrations/000011_cloud_accounts.up.sql:15), and validateCloudAccountRequest does not normalise it (handler_accounts.go:386+). An API client that sends a mixed-case stored Azure subscription GUID through account_ids now gets a 404 (scoped) or no rows (admin) where it used to match. The frontend only sends cloud_accounts UUIDs, so only direct API callers are affected, and the change fails closed. [non-blocking]
• internal/api/handler_analytics.go:240-252 - the legacy singular account_id is not canonicalised, so an uppercase UUID sent there still misses on analytics while the plural param now matches. The PR body only claims the plural param, and the frontend no longer sends account_id on these routes. [non-blocking]
• internal/api/handler_history.go:1084-1092 - resolveHistoryAccountFilter still hand-rolls the same appendMissing plus addExternalIDForProvider loop that mergeAccountFilter now wraps. It could call mergeAccountFilter, so a near-duplicate remains in the opposite direction. [non-blocking]
• frontend/src/modules/savings-history.ts:108 - the reflowed comment line runs past the block's wrap width. Cosmetic. [non-blocking]

Attacked:

• Canonicalisation order: parseAccountIDs (validation.go:525-552) trims, validates and lowercases. The result flows unchanged into requestedAnalyticsAccounts, then resolveAnalyticsAccountFilter, then the query, so the scope decision and the query see the same value. For a registered UUID, Allows receives the stored a.ID (not the request), and cloud_accounts.id is a Postgres UUID, read back lowercase. No SQL path uses LOWER, ILIKE or other case-folding on account columns (grep of internal/analytics and internal/database). Empty items, ",,," and whitespace-only input behave as before (trim already existed in base). The cap counts entries before allocating. Duplicates collapse (TestAnalytics_AccountIDs_DuplicatesCollapse).
• After d39a375: Allows/contains (auth/account_scope.go:190-227) does an exact, case-sensitive comparison against Accounts. bindGroupAccounts (handler_groups.go:187-270) stores entries only in stored form (an exact id, an exact external id, or a unique name rewritten to the stored ID). The lowercase request therefore matches what grants hold. Legacy uppercase UUID entries never matched a.ID anyway, so this PR does not change them.
• The fixture change at b9f8cf4 swaps the scope from names {"a","b"} to IDs {idA,idB}. After fix(auth): bind group account scope entries to account IDs #1116 a name entry grants nothing, so the old lowercase baseline returned 404 and failed assert.Equal(200, wantStatus). Scoping by ID is the production shape now. It also makes the scoped subtest prove attack 1 directly: the uppercase ID passes only if it is canonicalised before contains().
• Mutation probes (head copies, each restored with cp and checked with cmp):
  • M1, drop ToLower. Killed: TestParseAccountIDs_Canonical, the getHistory canonical subtest, and all 6 TestAnalytics_AccountIDs_UppercasePaddedCanonical subtests, at both heads.
  • M2, drop TrimSpace. Killed: TestParseAccountIDs, the Canonical test, and the getHistory subtest.
  • M3, lowercase only after resolveAnalyticsAccountFilter. Killed: all 6 analytics subtests at both heads. In a full-package run, the getHistory mock panic stops the binary before the parallel tests run, so their names do not show there; run on their own, they fail.
  • M4, restore the base mergeAccountFilter. Passes, as expected for a behaviour-equivalent refactor.
  • M5, drop the external-id merge. Killed: TestHandler_getHistoryAnalytics_ExternalIDOnlyAccount.
  • M6, no uuid dedup. Killed: DuplicatesCollapse.
  • Frontend, forward only the first id (trends.ts, savings-history.ts, api/analytics.ts). Each one kills its suite.
• Fail-before: the new Go tests fail on base (Canonical, the getHistory subtest, and all 6 analytics subtests). The three jest suites fail on base (2 tests fail, plus a TS compile failure in api-analytics).

Verified locally (GOTOOLCHAIN=go1.26.10, no creds):

• At 3256bbe and b9f8cf4: go build ./..., go vet and go vet -tags integration on ./internal/api (and ./internal/auth at b9f8cf4) are OK. gofmt -l is clean.
• go test -count=1 ./internal/api/ is ok at 3256bbe. At b9f8cf4, ./internal/api/ ./internal/auth/ are ok.
• golangci-lint run ./internal/api/...: 0 issues at b9f8cf4.
• jest on api-analytics, savings-history and trends: 114/114 at both heads. tsc --noEmit exits 0 and eslint on the 6 touched TS files is clean at both heads. node_modules were cloned after a byte-identical package-lock check.
• git merge-tree --write-tree origin/main b9f8cf4f, with origin/main at b0972e3 (no fetch): clean. origin/main is an ancestor of the head.
• No migration in this PR.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification at b9f8cf4ffc045d7fcebd219158366831885c5120 merged onto current main (e2d04494; merge-tree fce5c84c; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration passes on real Postgres (testcontainers) for the touched packages: ./internal/api
• Frontend: npm ci, tsc --noEmit and npm run build pass on the merged tree, and the full jest suite has no failures beyond those that fail identically on main in this local environment (CI runs the authoritative jest suite).
• Nothing was applied to any cloud.