[IAmJSD]

Root cause

getHistoryAnalytics, getHistoryBreakdown and getAnalyticsTrends did the scope check (validateAnalyticsAccountScope) and built the query filter (resolveSingleAccountFilterIDs) in two separate steps. The second step does not know about external ids: any value that is not a registered UUID becomes {"": {id}}. accountFilterClause (in both internal/api/analytics_postgres.go and internal/analytics/postgres_analytics.go) turns that into account_id = ANY(...) with no provider condition. So when a scoped user asked for the raw external id of their one registered account, the scope check passed, and the query also picked up rows of another provider's unregistered account with the same number (for example an AWS account id that equals a GCP project number).

Fix

validateAnalyticsAccountScope + analyticsAccountAllowed are replaced by resolveAnalyticsAccountFilter + scopedAnalyticsAccountFilter. For a scoped session these make the scope decision and build the filter from one ListCloudAccounts snapshot:

Request (scoped session)	Before	After
Registered account UUID	[uuid], {provider: {ext}}	unchanged
Raw id, exactly one registered owner	{"": {id}} (any provider)	{owner.Provider: {id}}
Raw id shared by 2+ registered accounts	404 (#520)	unchanged
Raw id no registered account has, literal allow-list entry	{"": {id}}	unchanged (see decisions)
Registered account with an empty provider (not possible on Postgres)	{"": {...}}	raw id is refused (404); UUID request matches by UUID only

Unrestricted/admin sessions still go through resolveSingleAccountFilterIDs, so their filters are unchanged.

Because the decision and the filter come from the same snapshot, there is no check-then-use gap. If a GCP account with the same id is registered between the scope check and the query, the request either answers with the AWS rows only or is refused as ambiguous. The concurrency test below runs that race on real Postgres.

Decisions

• Legacy rows with an empty provider are no longer returned to a scoped user who asks by raw id. provider = 'aws' AND account_id = ANY(...) does not match them. Nothing links such a row to the registered account rather than to the other provider's account with the same number, and the dashboard/inventory scope intersection (intersectDualColumnScope re-grouping the "" bucket under the allowed provider) already leaves them out. Unrestricted sessions still see them.
• A raw id that no registered account has keeps the unconstrained "" bucket when the allow-list names that literal id. There is no provider to pin it to, and the grant itself names only the bare id. Refusing these requests outright would remove access that fix(api): resolve analytics and inventory accounts by provider and external id #622 kept on purpose (TestHandleRequest_Analytics_UnregisteredRawID). If you want that path closed too, tell me and I'll do it as a follow-up.
• I did not add the matched account's UUID to the filter for raw-id requests. The issue asked for {match.Provider: {id}}, and adding the UUID would also match rows that carry only cloud_account_id, which is a separate behavior change.

Overlap with #1154

#1154 (branch fix/issue-32, still open) adds real-Postgres tests for the store-level predicate in internal/analytics. Its gcp-same-number fixture shows the store-level behavior this fix depends on: the "" key matches the GCP row with the same number and the "aws" key does not. This PR doesn't change either predicate. It changes which key the handler passes, so the two PRs don't conflict and don't depend on each other. The new handler-level integration test here seeds its own copy of the same rows (aws / gcp-same-number / legacy-no-provider) in both purchase_history and savings_snapshots, because #1154's fixture lives in package analytics_test and isn't merged.

Verification

All of this ran locally on macOS. The handler tests use mocks or fixtures; the integration tests use a real Postgres testcontainer. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is broken. No real cloud API was called.

Fail before / pass after, real Postgres through HandleRequest (internal/api/handler_analytics_scope_provider_integration_test.go):

go test -tags integration ./internal/api/ -run 'TestAnalyticsScoped.*_DB' -count=1 -v

Before (on main 2d822f39 with only the test added):

--- FAIL: TestAnalyticsScopedRawExternalIDIsProviderScoped_DB
    --- FAIL: .../scope_dev,_raw_id_request         expected [aws], actual ["", aws, gcp]  (history/analytics, history/breakdown, trends by_provider/monthly/by_service)
    --- FAIL: .../scope_424242,_raw_id_request      same
    --- PASS: .../scoped_UUID_request_is_unchanged
    --- PASS: .../unrestricted_raw_id_request_is_unchanged
--- FAIL: TestAnalyticsScopedRawExternalIDConcurrentRegistration_DB   request N aggregated a "gcp" row / a "" row

After:

--- PASS: TestAnalyticsScopedRawExternalIDIsProviderScoped_DB (all 4 subtests)
--- PASS: TestAnalyticsScopedRawExternalIDConcurrentRegistration_DB   (22 answered with aws rows only, 18 refused as ambiguous)

Also passes with -race (CI runs integration tests with -race).

Unit, through HandleRequest with mocks (internal/api/handler_analytics_scope_provider_test.go). I put the old handler_analytics.go back temporarily and these fail: TestHandleRequest_Analytics_RawExternalIDProviderScopedOnEveryEndpoint (all three endpoints), TestHandleRequest_Analytics_UniqueRawExternalID (it used to assert the leaky {"": {"111"}} shape and now asserts {"aws": {"111"}}), and TestHandleRequest_Analytics_ProviderlessAccountFailsClosed. All of them pass with the fix. The existing #520/#622 cases (ambiguous ids, unrestricted, unregistered, account-list failure) pass both before and after.

Other checks:

go build ./...
go vet ./internal/api/ && go vet -tags integration ./internal/api/
go test ./internal/api/ -count=1                       # ok
go test -short -race ./internal/api/ -count=1          # ok
go test ./internal/mocks/ -count=1                     # ok (repo-wide test guards)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/...                         # 0 issues
golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/api/... # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_analytics.go                        # silent
scripts/gosec-hook.sh internal/api/handler_analytics.go                                       # exit 0
git-secrets --scan (fresh temp repo, isolated HOME, --register-aws + scripts/setup-git-secrets.sh patterns) # clean
scripts/test-git-secrets-allowlist.sh                                                         # 42 PASS, 0 FAIL

Not covered: real-account analytics data. There are no real AWS/GCP accounts with colliding ids behind this; the colliding rows come from fixtures.

Out of scope

• sec(auth): AccountScope.Allows grants every account that shares an allowed display name #433 (display-name matching) is unchanged.
• resolveSingleAccountFilterIDs itself still produces the "" bucket for raw ids. The dashboard and inventory paths intersect its output with the allowed scope, and unrestricted analytics uses it unchanged.

Maintainer: please mirror the issue's triage labels on this PR: triaged, priority/p3, severity/low, urgency/this-quarter, impact/few, effort/s, type/bug.

Closes #624

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 44f64e5c-f707-4cf1-9214-c6342fb900ae

📥 Commits

Reviewing files that changed from the base of the PR and between ba38120 and eb8c974.

📒 Files selected for processing: 4

• internal/api/handler_analytics.go
• internal/api/handler_analytics_scope_provider_integration_test.go
• internal/api/handler_analytics_scope_provider_test.go
• internal/api/handler_per_account_perms_test.go

+-------------------------------------------------------+
| I find bugs faster than you can say 'merge conflict.' |
+-------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

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

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

[cristim]

VERDICT: APPROVE eb8c974

Reviewed head eb8c974 (1 commit) against origin/main f4387e4. git merge-tree against origin/main: clean, no conflicts. CI on the head: every check passes.

Findings (none blocking):

• nit (process) - the single commit is +468/-67. That is over the 400-line per-commit cap if the cap applies here. The 273-line integration test could be its own commit.
• note internal/api/handler_analytics.go:291-296 - a raw id that no account has registered keeps the unconstrained "" bucket when the allow-list names it literally. The PR body documents this choice. The issue's acceptance only covers a unique registered raw id, so it is in scope as written.

Issue #624 acceptance:

• A unique raw id now resolves to {match.Provider: {id}}.
• Legacy rows with an empty provider are excluded deliberately, and the doc comment says so.
• HandleRequest tests cover unit and real Postgres, with an unregistered other-provider row that has the same id.
• UUID and unrestricted requests are unchanged.
• Fixed.

Attacked:

• Widening to "all" for a scoped session. Every scoped branch returns a non-empty uuid or external half:
  • The UUID path always returns [uuid].
  • A raw id that resolves to nil (empty provider) is refused at :288 before the map is built.
  • An empty account_id is refused with a 400.
• Parity with the old analyticsAccountAllowed: the same allow decisions on every branch.
• Unrestricted sessions still use resolveSingleAccountFilterIDs unchanged.
• The decision and the filter come from one ListCloudAccounts snapshot, so there is no fallback to "" on a second list failure.
• Mutation probes in a scratch export, all KILLED, each reverted afterwards:
  • raw id back to the "" bucket (killed by the unit tests and by both integration _DB tests)
  • providerless guard dropped
  • UUID path made unconstrained
  • ambiguous id resolved to the first match
  • Allows check dropped on the unregistered path
  • Allows check dropped on the UUID path
  • empty scoped account_id allowed
• Local:
  • go build ./... ok
  • go test ./internal/api/ -count=1 ok
  • go vet -tags integration ./internal/api/ ok
  • go test -tags integration -race ./internal/api/ -run 'TestAnalyticsScoped.*_DB' PASS on a Docker Postgres testcontainer (23 answered aws-only, 17 refused as ambiguous)
• The colliding rows are fixtures. No real accounts were used.

[cristim]

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

Findings (none blocking). A related non-blocking note was shared privately with the maintainers, per SECURITY.md.

• [non-blocking] internal/api/handler_analytics.go:228-230 - The doc comment and the PR body overstate the "one cloud_accounts snapshot" guarantee slightly. It holds for id and external-id ownership. Suggest narrowing the wording to exactly what the snapshot covers.
• [non-blocking, cosmetic, pre-existing] internal/api/handler_per_account_perms_test.go:337 - The comment says the guard "calls MatchesAccount"; it calls AccountScope.Allows.

Against #624:

• The leak came from resolveSingleAccountFilterIDs (internal/api/scoping.go:412-414), which builds {"": {id}}. accountFilterClause turns that into account_id = ANY with no provider condition (internal/api/analytics_postgres.go:117-120, internal/analytics/postgres_analytics.go:65-68).
• The new scopedAnalyticsAccountFilter (handler_analytics.go:275-301) puts a uniquely owned raw id under its owner's provider.
• All three endpoints use it (lines 47, 155 and 199), and no caller of the removed functions remains.
• The other resolveSingleAccountFilterIDs callers (dashboard:123, inventory:108, history:1016) are intersected elsewhere, per the issue.

Attacked:

• Edge cases:
  • An empty account_id returns 400 for scoped users, and an ambiguous id returns 404.
  • An unregistered id with a literal grant keeps the documented "" bucket.
  • A registered account with no provider fails closed.
  • Provider values are lowercase on both sides (CHECK in migration 000011:14).
• The explicit match.Provider == "" guard is necessary: without it the filter becomes nil/nil, which reads as all accounts. Probe M2 confirms this, and a test covers it.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 6 were caught:
  • M1, the owner filter set to "";
  • M2, the provider fail-closed dropped;
  • M3, providerScopedExternalID accepting an empty provider;
  • M4, an ambiguous id not refused;
  • M5, the unrestricted path dropping the filter;
  • M6, the UUID branch skipping Allows.

Verified locally, on a git archive export of HEAD with no cloud credentials:

• go build ./..., go vet ./internal/api/ and go vet -tags integration ./internal/api/: clean.
• gofmt -l on the 4 changed files: clean.
• go test -count=1 ./internal/api/: ok.
• go test -race -tags integration -count=1 ./internal/api/ -run 'TestAnalyticsScoped.*_DB' against real Postgres: PASS. The concurrency test answered 30 requests with aws rows only and refused 10 as ambiguous.
• Fail-before, with the head tests on the base export:
  • The unit tests for the providerless, unique raw id and every-endpoint cases FAIL.
  • The raw-id integration subtests FAIL with gcp and "" rows included.
  • The concurrency test FAILS ("aggregated a gcp row").

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 eb8c974f7f037258403589c469fa0e66ec928f99 merged onto current main (f4387e4c; merge-tree 463c76e8; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at eb8c974f7f037258403589c469fa0e66ec928f99).