Repository navigation
sec(api): remaining tenant-scoping gaps after #499 #520
Description
Summary
Tenant-scoping gaps left after #499. Each item is a separate fix; tick them off as they land.
Checklist (origin/main 496d9d7)
- Plan mutations use
requirePlanAccess(internal/api/scoping.go:57), which passes if ANY of the plan's accounts is in scope. A user scoped to [A] can change a plan on {A,B}. Callers:updatePlan(internal/api/handler_plans.go:287),deletePlan(:384),createPlannedPurchases(:410),patchPlan(:683), andsetPlanAccountsviarequirePlanAccountsAccess(internal/api/scoping.go:102). Mutations should require every plan account in scope. -
GET /api/ri-exchange/target-offerings(listTargetOfferings,internal/api/handler_ri_exchange.go:116) has noreshapeCloudAccountInScopecheck, and its 404 vs 200 reveals whether an RI exists. - Analytics and inventory with a raw external
account_idmatch across providers through the empty-name bucket:validateAnalyticsAccountScope(internal/api/handler_analytics.go:245) andlistActiveCommitments(internal/api/handler_inventory.go:60) look up names by ID only. Related: sec(auth): AccountScope.Allows grants every account that shares an allowed display name #433. -
POST /api/recommendations/refresh(postRefreshRecommendations,internal/api/handler_recommendations_refresh.go:62) requires onlyview:recommendationsand triggers collection for every account. -
PUT /api/config/service/{provider}/{service}(updateServiceConfig,internal/api/handler_config.go:293) has norequireGlobalConfigScopecall, unlikeupdateConfig(:82). -
POST /api/accounts/discover-org(discoverOrgAccounts,internal/api/handler_accounts.go:1449) usesrequireAdminbut neverrequireAccountAccesson the org root account. - RI utilization and reshape from a payer account may include linked members' RIs (unverified;
getRIUtilizationinternal/api/handler_ri_exchange.go:1411,listConvertibleRIs:1375).
Context
Related: #384 (closed), #386, #433.
Acceptance
Each checklist item has a scoped-user test that fails before the fix and passes after.
Activity
cristim commentedon Oct 6, 2026
Plan for #520 (re-checked against origin/main e4c5cfc). No existing owner: no linked PR or branch for these items (open PRs #475 #482 #512 #535 are unrelated).
Checklist (state on current main, size, proposed PR):
requirePlanAccess(internal/api/scoping.go:57-75; callershandler_plans.go:287,384,410,683,scoping.go:102): STILL PRESENT. S. PR 1 (this one): add an every-account check for mutations; reads stay any-account.listTargetOfferingshas noreshapeCloudAccountInScopecheck (handler_ri_exchange.go:116): to be re-verified before PR 2. S. PR 2.account_idmatches across providers invalidateAnalyticsAccountScope(handler_analytics.go:245) andlistActiveCommitments(handler_inventory.go:60): to be re-verified; overlaps owner decision sec(auth): AccountScope.Allows grants every account that shares an allowed display name #433, so only the non-sec(auth): AccountScope.Allows grants every account that shares an allowed display name #433 part. S. PR 3.postRefreshRecommendations(handler_recommendations_refresh.go:62) collects for every account with only view:recommendations: to be re-verified. S. PR 4.updateServiceConfig(handler_config.go:293) lacksrequireGlobalConfigScope: to be re-verified. S. PR 4 or 5.discoverOrgAccounts(handler_accounts.go:1449) lacksrequireAccountAccesson the org root: to be re-verified. S. PR 5.Order is by security impact: 1 (write access to purchasing config), 2, 6, 5, 4, 3, 7. Each slice is well under 400 lines. Slices will use
Refs #520; the issue stays open.Decisions for PR 1: scoped mutation requires EVERY plan account in scope (
Allows), 404 (errNotFound) like the existing helper and #547/#553; a plan with zero accounts stays hidden from scoped users (unchanged); AllowsAll short-circuits before any store read; read endpoints (getPlan, listPlanAccounts) keep any-account visibility.cristim commentedon Oct 6, 2026
Slice 1 (plan mutations require EVERY plan account in scope) merged as #605. Review notes for the remaining slices: (1) NEW low finding: deletePlannedPurchase -> disablePlan (handler_purchases.go ~482) can disable a multi-account plan when the cancelled execution is a single-account child (it is gated by requireExecutionAccess on the execution only, not by requirePlanMutationAccess on the plan): add to the checklist. (2) Read gate stays any-account: getPlan returns the plan settings without an account list to a partially scoped user; listPlanAccounts filters per account scope, so no other tenant's accounts leak. (3) Optional test: a missing plan and a partially scoped plan return the same error for a scoped user (no existence oracle). (4) Account resolution is AccountScope.Allows(ID, Name): not provider-qualified; that is #433's owner decision. Remaining slices from the plan comment: listTargetOfferings (handler_ri_exchange.go ~116), discoverOrgAccounts (handler_accounts.go ~1449), updateServiceConfig (handler_config.go ~293), postRefreshRecommendations (handler_recommendations_refresh.go ~62), analytics/inventory cross-provider raw account_id (non-#433 part), payer RI utilization/reshape (unverified).
cristim commentedon Oct 6, 2026
Review notes from PR #607 (org-account discovery root now requires account access): (1) NEW follow-up item: with an in-scope root, persistDiscoveredMembers (handler_accounts.go ~1557) persists ALL org members as disabled cloud_accounts rows (new UUID, AWS name, AWSBastionID=root.ID). They are hidden from scoped users and every update/enable/credential route uses requireAccountAccess, so the direct impact is low, but (a) if an AWS account name equals a scope entry (name matching, #433) the new row falls into that scope, (b) the Created/Skipped counts come from the unscoped ListCloudAccounts (a minor existence oracle for other tenants' external ids), (c) unrestricted admins see unwanted disabled rows. Options: require an unrestricted scope for the persist step, or persist only members whose scope is allowed. (2) A scoped admin is a realistic configuration: a custom group with {admin,*} plus allowed_accounts [A] passes requireAdmin while getAccountScope returns [A]; the admin API key is unrestricted. (3) Other raw GetCloudAccount uses reviewed: handler_accounts.go ~1247 is gated by its callers; handler_marketplace.go ~74 and handler_purchases.go ~3630 take ids from already-gated rows (confirm the marketplace caller scopes the purchase row); no other missing scope checks in handler_accounts.go.
test(api): prove discover-org never loads an out-of-scope root's cred…
cristim commentedon Oct 6, 2026
Note from the #615 review (recommendation refresh now requires an unrestricted scope): residual exposure not covered there: in non-Lambda mode any scoped reader of ListRecommendations can trigger a global collection and take the in-flight marker through kickColdStartCollection (scheduler.go ~1385) and maybeKickBackgroundRefresh (~1430); both are TTL- and cold-start-gated, so they are not the on-demand abuse #615 closes, but they are the same class and belong in the checklist. Also new frontend follow-ups from #614 and #615: #617 (Settings Save toast for scoped admins) and the recommendations auto-refresh toast issue filed from the #615 review.
fix(api): fail loudly when the account list cannot be loaded
feat(api): report plan_disabled on planned purchase delete
cristim commentedon Oct 6, 2026
Status: every item of the plan checklist is now merged or tracked. Merged slices: #605 (plan mutations require every plan account in scope), #607 (org-discovery root gate), #611 (listTargetOfferings deployment-account gate), #614 (updateServiceConfig requires an unrestricted scope), #615 (recommendations refresh requires an unrestricted scope), #621 (deletePlannedPurchase only disables the plan with plan-wide scope; response carries plan_disabled), #622 (analytics and inventory provider-qualified account resolution), #625 (getRIUtilization post-filters to the deployment account's own RIs for scoped users). Tracked as separate follow-ups: #624 (analytics provider-unconstrained bucket), #626 (org-discovery persist step), #627 (non-Lambda cold-start/background refresh), #628 (404 vs 403 vs empty-list status differences), and the frontend UX issues #617 (Settings Save toast), #619 (recommendations auto-refresh toast), #623 (Disable plan on a multi-account plan). #433 (AccountScope.Allows name matching) remains an owner decision and is not part of this issue. Suggest closing #520 as completed once the owner agrees; leaving it open for that decision.