Repository navigation
sec(auth): bound account scope on self-membership changes, both directions - #1808
Merged
Merged
Conversation
…tions
An actor could raise their own account scope in a single self-edit through
PUT /api/users/{id}, by two routes running in opposite directions.
Joining a wider group: guardSelfEscalation gated self-added groups on
update:users, which the actor already holds, then checked only the carved-out
money verbs of the group being joined. The account dimension was never
checked, and a default deployment ships exactly one broader group -- the
seeded Administrators group, allowed_accounts = ARRAY['*'] (migrations
000024/000057).
Leaving the scoping group: guardGroupChange returned early unless the change
added a group, so a self-edit that only removed groups was not guarded at all.
Dropping the group carrying the restriction collapses the allowed_accounts
union to empty, and IsUnrestrictedAccess reads empty as every account.
Removing the restriction granted the restriction.
guardSelfAccountScope now compares the RESULTING scope against the PRIOR
scope, which is what closes both routes: inspecting only what is being added
can never see the removal. It reuses accountScopeGap from the group-write
ceiling, which treats empty and "*" as unrestricted on EITHER side, so the
comparison is not a subset test (the empty set is a subset of everything while
meaning the opposite of narrow).
The addsNewGroup screen moves from guardGroupChange into guardSelfEscalation,
where it now covers only the permission checks. That reach is correct for
permissions, which are a plain union that a removal can only shrink, and wrong
for accounts, whose union inverts on empty.
accountsForGroups resolves the union from a membership list and FAILS CLOSED
on any group it cannot load. Its permission-side twin permissionsForGroups
skips a missing group safely; here a skipped group widens the result, so
swallowing it would compute an unrestricted prior scope and wave every change
through -- issue #1748's mechanism in a new place.
Verification covers both refusals and, as importantly, both controls: a
removal that does not widen scope and a join of a subset-scoped group are
still allowed, so a guard that refused every self-membership edit fails the
suite. Coverage runs at the service layer and end to end through
Handler.HandleRequest -> Router.Route -> the real auth.Service, since a
handler-level test has missed router-level bypasses here before.
Closes #1756
cristim
added
triaged
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe authentication service now prevents self-membership changes from widening account scope, including removal of restricting groups. It fails closed when group scope resolution is incomplete. Unit and router tests cover rejected and allowed changes. ChangesSelf-membership account-scope enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant APIHandler
participant guardSelfEscalation
participant accountsForGroups
participant GroupStore
participant UpdateUser
APIHandler->>guardSelfEscalation: validate self-membership change
guardSelfEscalation->>accountsForGroups: resolve prior and resulting groups
accountsForGroups->>GroupStore: load group account scopes
GroupStore-->>accountsForGroups: scopes or resolution error
accountsForGroups-->>guardSelfEscalation: resolved scopes
guardSelfEscalation-->>APIHandler: allow or reject with ErrSelfEscalation
APIHandler->>UpdateUser: update memberships when allowed
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai full review The automatic review on this PR hit the adaptive rate limit and never ran. Requesting a full review rather than an incremental one, since an incremental review after a throttle can skip the commit that was in flight and report a false clean. Context worth having: this closes a p1/severity-critical account-scope escalation that is live on |
Contributor
|
🧠 Learnings used✅ Action performedFull review finished. |
accountsForGroups refused on ANY group it could not resolve. That is stricter than the widening it guards against, and the extra strictness has no security value while costing real availability. A skip can only widen the union when what SURVIVES is empty, because empty is the value that reads as every account. When the surviving union is non-empty the computed scope is exactly the scope enforcement will apply: BuildAuthContext skips an unresolvable group too, so both sides of the comparison already describe effective access rather than an approximation of it. The refused-for-nothing case is not hypothetical. DeleteGroup drops the groups row and never purges users.group_ids, an array column with no foreign key, so a dangling membership id is the ordinary state after any custom group is deleted. Under refuse-on-any-skip an administrator who is already unrestricted could not remove that dangling id from their own membership: a change that widens nothing, from a principal at maximum scope. In a single-admin deployment nobody could clean it up. The two conditions now match grantCeilingAccounts and ResolveAllowedAccounts exactly: refuse when the surviving union is empty and something was skipped, and refuse when no group resolved at all. The second is unreachable while migration 000057's users_min_one_group CHECK holds, and is carried for the same reason its siblings carry it, an unresolved scope being unknown rather than unrestricted. The new control test pins the availability half so a future tightening cannot quietly reintroduce it. Mutation-verified per test: tightening back to refuse-on-any-skip fails that control alone, and dropping the emptiness condition fails the fail-closed test alone.
Member
Author
|
@coderabbitai full review Retrying. A full review was acknowledged on this PR at 01:21:15Z but no review was ever delivered: zero inline comments and zero submitted reviews since. Re-requesting rather than treating the acknowledgement as a verdict. Note the head has moved since that acknowledgement, to Requesting full rather than incremental deliberately, since an incremental review after a throttle can skip the in-flight commit and report a false clean. What matters most here: the guard must refuse in both directions. The non-obvious one is that removing a group can widen scope, because an empty |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Checked-out revision ==='
git rev-parse HEAD
git status --short
printf '%s\n' '=== PR commits and changed files ==='
git diff --stat HEAD^ HEAD
git diff --name-only HEAD^ HEAD
git show --format='fuller' --no-ext-diff --no-renames --stat HEAD
printf '%s\n' '=== Changed-file outlines ==='
for f in internal/auth/service_group.go internal/auth/service_user.go internal/auth/self_account_scope_test.go internal/server/self_account_scope_dispatch_test.go; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all || true
fi
done
printf '%s\n' '=== Account scope definitions and call sites ==='
rg -n -C 5 --glob '*.go' \
'func\s+\(?[^)]*\)?\s*(accountScopeGap|accountsForGroups|IsUnrestrictedAccess)\s*\(|\b(accountScopeGap|accountsForGroups|IsUnrestrictedAccess)\s*\(' \
internalLength of output: 28671 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Full PR diff against merge-base ==='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
printf 'base=%s\n' "$base"
git diff --no-ext-diff --no-renames --unified=80 "$base"...HEAD -- \
internal/auth/service_group.go \
internal/auth/service_user.go \
internal/auth/self_account_scope_test.go \
internal/server/self_account_scope_dispatch_test.go \
internal/auth/group_ceiling.go \
internal/auth/account_scope_fail_closed_test.go
printf '%s\n' '=== Scope resolution and self-edit guards ==='
sed -n '95,225p' internal/auth/service_group.go
sed -n '360,515p' internal/auth/service_user.go
sed -n '245,290p' internal/auth/group_ceiling.go
printf '%s\n' '=== Self-scope tests ==='
sed -n '1,280p' internal/auth/self_account_scope_test.go
sed -n '1,210p' internal/server/self_account_scope_dispatch_test.go
printf '%s\n' '=== Related fail-closed tests ==='
sed -n '1,345p' internal/auth/account_scope_fail_closed_test.go
printf '%s\n' '=== Group deletion and group-membership database constraints ==='
rg -n -C 8 --glob '*.go' --glob '*.sql' \
'func\s+\(?[^)]*\)?\s*DeleteGroup\s*\(|DELETE\s+FROM\s+groups|users_min_one_group|group_ids' \
internal migrationsLength of output: 201951 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Exact resolution logic ==='
sed -n '125,215p' internal/auth/service_group.go
sed -n '270,305p' internal/auth/service_group.go
printf '%s\n' '=== Exact self-membership path ==='
sed -n '300,505p' internal/auth/service_user.go
sed -n '575,615p' internal/auth/service_user.go
printf '%s\n' '=== Exact scope comparison ==='
sed -n '225,290p' internal/auth/group_ceiling.go
sed -n '165,210p' internal/auth/types.go
printf '%s\n' '=== Full changed hunks, separated by file ==='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
for f in internal/auth/service_group.go internal/auth/service_user.go internal/auth/self_account_scope_test.go internal/server/self_account_scope_dispatch_test.go; do
echo "--- $f"
git diff --no-ext-diff --no-renames --unified=12 "$base"...HEAD -- "$f"
done
printf '%s\n' '=== Static patch integrity ==='
git diff --check "$base"...HEAD
git status --shortLength of output: 53265
I found no blocking issue. The guard rejects both escalation paths:
The unresolved-group relaxation does not fail open. If resolved groups produce a non-empty account union, skipped groups cannot make the computed scope broader. They can only make the computed scope narrower than effective access. The ceiling can then reject too strictly, but it cannot allow a scope widening. If the surviving union is empty and one or more groups were skipped,
I also used the repository guidance that a throttled review requires a full review rather than an incremental review. 🐇🔎 🧠 Learnings used✅ Action performedFull review finished. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1756
The bug
An actor could raise their own cloud-account scope in a single self-edit of
PUT /api/users/{id}, by two routes running in opposite directions. Both were live onmain.Join a wider group.
guardSelfEscalationgated self-added groups onupdate:users, which the actor already holds, then checked only the carved-out money verbs of the group being joined. The account dimension was never checked, so any broader-scoped group would do. #1756 names the seeded Administrators group as the one such group a default deployment ships; in fact every seeded group shipsallowed_accounts = ARRAY['*'](migrations000024,000057,000059/000064,000096), so the default deployment offers several laundering targets rather than one. The guard keys off the scope value rather than a group ID, so it covers all of them.Leave the scoping group.
guardGroupChangereturned early unless the change added a group, so a self-edit that only removed groups was not guarded at all. Dropping the group carrying the restriction collapses theallowed_accountsunion to empty, andIsUnrestrictedAccessreads empty as all accounts. Removing the restriction granted the restriction.Both routes cleared the permission-dimension guards correctly. The defect was that the account dimension had no equivalent on this endpoint, the same asymmetry #1737 closed on the group-write path.
The guard
guardSelfAccountScopecompares the resulting scope against the prior scope, rather than inspecting what is being added. That is what closes both routes at once: an add-oriented check can never see the removal, and the removal is the counter-intuitive half.It reuses
accountScopeGapfrom the group-write ceiling, which treats empty and"*"as unrestricted on either side. That is why it is not a subset test: the empty set is a subset of everything while meaning the opposite of narrow.["*"]or[](unrestricted)[]or contains"*"[A,B][A],[A,B][A][A,B]The
addsNewGroupscreen moves fromguardGroupChangeintoguardSelfEscalation, where it now covers only the permission checks. That reach is correct for permissions (a plain union, which a removal can only shrink) and wrong for accounts (a union whose meaning inverts on empty). Keeping it at the top is what left the removal route unguarded, so it had to move rather than be worked around.accountsForGroupsresolves the union from a membership list and fails closed when a group it cannot load leaves the surviving union empty, plus when no group resolved at all. Its permission-side twinpermissionsForGroupsskips a missing group safely; here a skipped group can widen the result, so swallowing that case would compute an unrestricted prior scope and wave every change through, which is #1748's mechanism in a new place.Those two conditions are deliberately identical to
grantCeilingAccountsandResolveAllowedAccounts. The emptiness qualifier is the load-bearing part and is not a softening: a skip can only widen the union when what survives is empty, because empty is the value that reads as every account. When the surviving union is non-empty, the computed scope is not an approximation at all, it is exactly the scope enforcement will apply, sinceBuildAuthContextskips an unresolvable group too. Both sides of the comparison therefore describe effective access.Refusing on any skip is the tempting stronger rule and was the first draft. It is wrong, and the case it breaks is ordinary rather than exotic:
DeleteGroupdrops thegroupsrow and never purgesusers.group_ids(an array column with no foreign key), so a dangling membership id is the normal state after any custom group is deleted. Under refuse-on-any-skip an administrator who is already unrestricted could not remove that dangling id from their own membership, a change that widens nothing from a principal at maximum scope, leaving a single-admin deployment with no way to clean up. That is pure availability cost for zero security benefit, which is the trade-offgrantCeilingAccountsalready documents rejecting.TestSelfAccountScope_UnrestrictedActorCanDropDanglingGrouppins it so a future tightening cannot quietly reintroduce it.Known tradeoff, deliberate: when the refusal does fire, it surfaces as a 500 with a count of unresolvable groups rather than a 4xx naming them. That matches
ResolveAllowedAccountsandgrantCeilingAccounts, which report the same unresolvable-scope condition the same way; giving only this one call site a sentinel and a 4xx would diverge from both siblings for no security gain. "We cannot establish your account scope" is genuinely a server-side condition, and after the emptiness qualifier above it requires a missing group and an otherwise-empty surviving union, so it is rare. Worth revisiting for all three call sites together, not for this one alone.Availability
A default deployment is unaffected: every seeded group ships
allowed_accounts = ARRAY['*'], so the prior scope is unrestricted and every self-edit passes. The same holds for any actor whose groups carry noallowed_accountsat all. Only a genuinely scoped actor is bounded, and only on a self-edit; an administrator editing another user is untouched, as are trusted internal callers (actorUserID == "").Verification
Refusals prove little on their own here, since an implementation that refused every membership edit would pass them. Each refusal is therefore paired with a control that must still be allowed, and the controls are the load-bearing half:
allowed_accounts = ['*']) -> refused["*"])Coverage runs at the service layer (
internal/auth/self_account_scope_test.go) and end to end throughHandler.HandleRequest->Router.Route->requireAdmin-> the realauth.Servicewith only the store mocked (internal/server/self_account_scope_dispatch_test.go), because a handler-level test has missed router-level bypasses in this repo before (#1757, #1773).Pre-fix reproduction
With the two production files restored to
origin/main(git checkout origin/main -- internal/auth/service_user.go internal/auth/service_group.go) and the new tests left in place, all five refusals fail and both controls still pass. That is the bug reproducing against the code as shipped, not against a mutant:Mutation matrix
Each test was run individually (
go test <pkg> -run '^Name$' -count=1), because an aggregate package run misreports: a mock panic kills the test binary. Every failure below was an assertion failure, never a panic; refusal tests carry a permissiveUpdateUserstub precisely so a guard removal fails by assertion rather than by an unstubbed-call panic.addsNewGroupscreenM2 is the one that matters most: it restores only the
addsNewGroupearly return and leaves the new guard in place, and the removal route reopens on its own. M3 keeps the controls honest, M5 pins the comparison direction rather than merely its presence, and M4/M6 pin the skip rule from both sides at once, which is what makes the emptiness qualifier a measured boundary rather than a preference. Every cell was measured on the current head; M1 and M3 were re-run after the dangling-group control was added rather than inferred for it.Full root module suite green (
go test ./...), race detector green on both changed packages,golangci-lint v2.10.1(the CI-pinned version) clean,gocyclo -over 10 -ignore "_test\.go"clean. CI was 20/20 green on the first commit; the second commit is under the same checks.Summary by CodeRabbit