sec(auth): account scoping fails OPEN when a user's groups cannot be resolved (unrestricted access to all accounts)

[cristim]

Found while answering a review check on PR #1737. Not fixed there — #1737 hardens the group write path; this is the read path, and folding it in would mix an access-control fix into a privilege-escalation review.

What

A user scoped to specific cloud accounts gets unrestricted access to all of them whenever their group memberships fail to resolve.

The chain is Handler.getAllowedAccounts (internal/api/handler.go:527) -> authServiceAdapter.GetAllowedAccountsAPI (internal/server/app.go:1159) -> Service.BuildAuthContext -> authCtx.AllowedAccounts.

collectGroupsAndAccounts (internal/auth/service_group.go) deliberately skips a group it cannot load, both on pgx.ErrNoRows and on a (nil, nil) return:

if errors.Is(err, pgx.ErrNoRows) {
    // Group was deleted; skip it rather than failing the entire request.
    continue
}
if group == nil {
    continue
}

So a user whose groups all fail to load ends with AllowedAccounts == [], and auth.IsUnrestrictedAccess documents empty as "all accounts" (a deliberate backward-compat default). The failure is silent: no error is returned and nothing is logged.

Verified by execution

scoped user, group loads fine (control)  -> AllowedAccounts=[acct-A]  IsUnrestrictedAccess=false
scoped user, group missing (ErrNoRows)   -> AllowedAccounts=[]        IsUnrestrictedAccess=true   <<< ALL ACCOUNTS
scoped user, group returns (nil, nil)    -> AllowedAccounts=[]        IsUnrestrictedAccess=true   <<< ALL ACCOUNTS

Blast radius

Every account-scoped handler, because they all funnel through the same helper — 17 getAllowedAccounts call sites across 9 files: handler_accounts.go (2), handler_analytics.go (1), handler_dashboard.go (2), handler_history.go (1), handler_ladder.go (1), handler_marketplace.go (1), handler_recommendations.go (1), handler_ri_exchange.go (4), scoping.go (4) — plus a direct GetAllowedAccountsAPI call in handler_purchases_revoke.go. scoping.go's four cover requireAccountAccess and requirePlanAccess, which the per-record scoping depends on.

Triggering conditions are ordinary rather than exotic: a group deleted while a session is live, a replica lagging behind a group insert, or any store path returning (nil, nil).

Why the write path is already fixed and this is not

PR #1737 hit the identical shape in grantCeilingAccounts and closed it by requiring at least one group to actually resolve:

if len(authCtx.Groups) == 0 {
    return nil, fmt.Errorf("%w: the acting user's account scope could not be established (no group resolved)", ErrPermissionCeiling)
}

The same guard belongs on the read path. Note the asymmetry that makes this easy to miss: the permission side already fails closed on the same input, because an empty permission set grants nothing. Only the account side fails open, and it does so without erroring.

Fix direction

Distinguish "resolved to unrestricted" from "could not resolve". Options, in rough order of preference:

1. Make BuildAuthContext report when a user has GroupIDs but none resolved, and have getAllowedAccounts fail closed on it. Keeps the backward-compat empty-means-all default intact for genuinely unrestricted groups.
2. Represent unrestricted explicitly (a sentinel or a *[]string) so absent and unrestricted stop sharing a representation. Larger, but removes the trap rather than guarding it.

Whichever is chosen, the regression test must force a group-resolution failure for a scoped user and assert access is refused — a test using an unrestricted user passes with the bug present.

Related

• sec(auth): enforce a grant ceiling and system-managed guard on group writes #1737 — the same shape on the write path, fixed there.
• sec(purchases): standard user can pause/resume other users' scheduled purchases (no ownership gate) #950 / fix(api): filter purchases by account UUID AND external id (closes #701, #498, #866) #956 — the account-filter regression class this sits in.
• fix(test): grantAdmin authorization-decision stub — remainder after #1744 (pair pinning, .Maybe() per-handler, scoped-account conversion) #1596 — the test helper that made restricted-account paths untestable, so this class had no coverage.

[cristim]

Second fail-open vector in the same function, not covered by the report above.

getAllowedAccounts (internal/api/handler.go:527) returns unrestricted access on a nil auth component, not only on a failed group resolution:

if h.auth == nil {
    return nil, nil // -> empty -> IsUnrestrictedAccess == true -> all accounts
}

Combined with the docstring on the same function — "Empty slice means all access" — the representation itself is the hazard: every path that can produce an empty slice without erroring grants access to every account. There are now three known producers:

1. h.auth == nil
2. a group that fails to load (ErrNoRows)
3. a store path returning (nil, nil)

Only (1) is visible by reading; (2) and (3) require executing the failure.

This repo has already ruled on (1) as a class: auth middleware must fail closed when the auth component is nil — return 403, never fall through. A nil auth component is a misconfiguration, and a misconfigured deployment silently serving every account is strictly worse than one that refuses.

Whatever fix lands should close all three at the representation, rather than patching each producer. Suggested shape: make "unrestricted" an explicit value that cannot be produced by absence — a distinct type/flag, or an error return on every path that cannot establish scope. Patching producers one at a time leaves the fourth one to be discovered later, which is exactly how this was found in the first place.

Raising severity to critical: this is a live cross-account read bypass on main, reachable through ordinary operations (a group deleted mid-session, a lagging replica), across 17 call sites in 9 files including requireAccountAccess and requirePlanAccess.