sec(plans): listPlans does not scope account_ids to caller's allowed_accounts #29

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p2Backlog-worthyBacklog-worthy

   severity/mediumModerate harmModerate harm

   urgency/this-sprintWithin the current sprintWithin the current sprint

   impact/internalTeam-internal onlyTeam-internal only

   effort/mDaysDays

   type/securitySecurity findingSecurity finding

   on Jun 5, 2026
2. cristim commented on Jul 28, 2026

   

   cristim

   on Jul 28, 2026

   MemberAuthor

   More actions

   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.

   Re-verified live during the 2026-07-28 full review of internal/api (all 40 non-test Go files, every route in router.go:99-371 traced to its handler). Adding evidence rather than filing a second issue.

   Where: internal/api/handler_plans.go:21-49 (listPlans), client-supplied account_ids read at :29.

   What the review pins down that this issue currently hedges. The issue text says a scoped user "may be able to enumerate plans ... depending on how view:plans is granted", and asks the fixer to "decide whether listPlans should intersect". On be11bdcb5 there is nothing to decide and no dependency on the grant shape:

   • getAllowedAccounts is never called, and the session returned by requirePermission is discarded with _.
   • The client-supplied account_ids is the only filter passed to ListPurchasePlans; every returned row is emitted.
   • Eight sibling handlers in this same package resolve caller-allowed accounts. This one does not.

   Failure scenario (concrete): Read-Only user U with allowed_accounts = [acct-A] calls GET /api/plans with no params and receives every tenant's purchase plans: names, provider/service maps, term and payment options, coverage percentages, ramp schedules, auto_purchase flags and next_execution_date. Supplying ?account_ids=<acct-B-uuid> narrows the response to the other tenant rather than excluding it.

   The per-plan read (getPlan, handler_plans.go:158) correctly 404s for the same user, so the list endpoint is the sole disclosure path for this data. It is also the endpoint the UI calls on page load, so the leak fires without any deliberate probing.

   Downstream amplifier: the plan UUIDs harvested here are the input for GET /api/plans/{id}/accounts (handler_accounts.go:1351-1370), which is likewise unscoped and returns full config.CloudAccount rows (UUIDs, display names, provider, and external IDs such as AWS account numbers / Azure subscription IDs) for other tenants. Those account UUIDs are in turn the input the dashboard explicit-filter leak needs. Filed in the grouped medium/low issue from this review; worth fixing in the same change as this one.

   Triage note: this issue is currently severity/medium, impact/internal. Given the above is unconditional cross-tenant disclosure of commercial commitment strategy to any view:plans holder in a multi-team deployment, impact/many and a severity re-look seem warranted.

   Fix direction: intersect accountIDs with getAllowedAccounts and filter the returned plans through requirePlanAccess / GetPlanAccounts before responding. Regression test must cover both the no-param case and the ?account_ids=<out-of-scope> case; a test covering only one of them can stay green with the other still leaking.
3. cristim commented on Jul 28, 2026

   

   cristim

   on Jul 28, 2026

   MemberAuthor

   More actions

   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.

   Adding the store-layer half of this issue from the 2026-07-28 full review of internal/config (store_postgres.go read in full). A separate comment on this issue covers the handler layer (listPlans never intersecting with getAllowedAccounts); this one is about the query underneath it, because fixing the handler alone does not close the leak.

   Where: internal/config/store_postgres.go:754-783 (buildListPlansQuery), specifically the OR NOT EXISTS (SELECT 1 FROM plan_accounts WHERE plan_id = pp.id) disjunct at :779.

   What: when filter.AccountIDs is non-empty the WHERE clause is

   pa.account_id IN (...) OR NOT EXISTS (SELECT 1 FROM plan_accounts WHERE plan_id = pp.id)

   The second disjunct is unconditional. Any plan with zero plan_accounts rows matches for every caller, regardless of the account scope passed in. So even after listPlans starts intersecting the requested account_ids with the caller's allowed accounts, every unassigned plan still comes back to everyone: the scoping is applied to a filter that the query then ORs its way around.

   The "legacy plans" framing does not hold. The docstring frames zero-account plans as legacy rows predating plan_accounts, but nothing prevents new ones: SetPlanAccounts (store_postgres.go:3335) accepts an empty accountIDs slice, and validatePlanAccountProvidersTx returns early at :3375 without requiring any account. A user can create an unassigned plan today, or clear the assignment on an existing one.

   Failure scenario: tenant A creates a purchase plan named "Q3 EC2 ramp - prod" and never assigns an account to it (or clears the assignment). Tenant B, whose session is scoped to a disjoint allowed_accounts set, opens the Plans page. ListPurchasePlans(filter{AccountIDs: [B's uuids]}) returns tenant A's plan including its services JSONB, which carries the regions, instance families, coverage targets and payment options tenant A is committing to, plus its next_execution_date. That is cross-tenant disclosure of commercial configuration through a query written to be account-scoped.

   Fix direction: gate the unassigned disjunct behind an explicit caller opt-in (filter.IncludeUnassigned) that only admin or unrestricted sessions set, rather than applying it to every scoped query. That answers the open question in this issue's acceptance criteria ("how should Unassigned plans be scoped") in the store rather than leaving it to each caller.

   The regression test needs three cases, not two: a scoped caller must not see another account's assigned plan, must not see an unassigned plan, and an admin caller must still see unassigned plans. A test covering only assigned plans stays green with this defect intact.
4. cristim

   transferred this issue fromLeanerCloud/cloud-commitments-clion Sep 27, 2026
5. cristim

   closed this as completedin #499on Oct 5, 2026