Repository navigation
sec(auth): fail closed when a user's account scope cannot be established - #1752
Conversation
A user scoped to specific cloud accounts got unrestricted access to ALL of
them whenever their group memberships failed to resolve.
An empty allowed-accounts list means UNRESTRICTED (IsUnrestrictedAccess), a
deliberate backward-compat default so a group with no allowed_accounts
configured grants full access. But collectGroupsAndAccounts also skips a
group it cannot load -- pgx.ErrNoRows, or a store returning (nil, nil) -- so
a resolution failure produced the SAME empty value. Absence and unrestricted
shared a representation, and every failure silently granted everything. No
error, no log.
Verified by execution before fixing:
scoped user, group loads fine (control) -> unrestricted=false
scoped user, group missing (ErrNoRows) -> unrestricted=TRUE
scoped user, group returns (nil, nil) -> unrestricted=TRUE
Three producers of the empty value are closed:
1. h.auth == nil in getAllowedAccounts returned (nil, nil). Auth components
must fail closed when nil, never fall through.
2. a group that fails to load with pgx.ErrNoRows
3. a store path returning (nil, nil)
2 and 3 are closed at the single point where the scope is produced rather
than per-caller: ResolveAllowedAccounts requires at least one group to have
actually resolved. Partial resolution still under-reports scope, which makes
access STRICTER, so only total failure had to be refused.
The design constraint that shaped this: there are THREE legitimate
unrestricted principals, not two, and the third shares its representation
with the failures. The stateless admin API key resolves to empty, an
Administrators member carries "*", and a group with NO allowed_accounts
configured also resolves to empty. Making absence fail closed blindly would
have broken every legacy group. Requiring a resolved GROUP rather than a
resolved account list separates them cleanly -- verified across all six
cases.
Every failure test is paired with a control proving a legitimate unrestricted
principal still passes, because a fix that refused everyone would satisfy a
refusal-only suite. Mutation-verified per guard: removing the no-group-
resolved guard kills only its own test, reverting the nil-auth guard kills
only its own, and an inverse mutation that refuses everyone kills the
legitimate-principal controls.
Closes #1748.
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
Independent adversarial review — producer enumeration, consumer verdict, separator, mutationsReviewed at head Verdict: no blocking findings. The load-bearing claim holds with one correction to how it is stated. 1. The "single point of production" claim — verified, with a correctionThe claim under test: the three producers are closed at the single point where the scope is produced, so a fourth failure mode in that resolver cannot reintroduce the bug. Method. I enumerated producers rather than consumers, three ways:
The complete producer list:
Things I checked that are not producers:
The correction. The claim is true of the auth-side resolver — P-B and P-C both funnel into P-D, so the ≥1-group guard is genuinely single-point and a fourth resolver-level failure mode cannot slip past it. It is not true of the API layer: there are two API-layer producers (P-A and P-C), and the nil-auth guard added by this PR is only on P-A. P-C dereferences That is not exploitable today: 2. The two hand-rolled consumers — deferral is correctEnumerating every way an empty-and-nil-error list can reach them post-fix:
So empty is reachable only from a legitimately-unrestricted principal. Two accuracy notes on the reasoning, neither changing the verdict:
3. The separator ("a resolved GROUP, not a resolved account list")
I traced the field the guard reads rather than the one it is about:
Is there a legitimate zero-group principal? I went looking for one. There is not:
The only zero-group rows possible are legacy pre-#907 records or direct DB writes. Those hold zero permissions as well (permissions come solely from "Resolved" means the same thing on every path: one predicate, one append site. 4. Mutations — re-run independently, all three reproduceSeparate worktree, running only the 10 new top-level tests. Baseline: 10 PASS / 0 FAIL.
On the "5/10" in the PR body: the number 5 is real, the denominator is a different unit. P3 kills 3 top-level tests, which expand to 5 leaf cases: the three subtests of The substantive point stands and is the important one: the positive controls bite. An over-blocking implementation is caught by named legitimate-principal tests, so this is not a refusal-only suite. One structural note on P3: the On P1/P2 being killed only by their own test: that is inherent, not a coverage gap. Each is a specific guard on a specific branch, and the pairing is deliberate (P1 at the resolver, P2 at the API seam). The alternative — a guard killed by many tests — would mean the guard sits on a hot shared path, which this one does not. 5. Pre-existing-suite claimVerified rather than accepted: I applied the source fix without the two new test files and ran |
Independent adversarial review, part 2 — pre-fix demonstration, code-quality findings, gatesContinues the previous comment. Same worktree, head Pre-fix demonstration (the test must fail on the bug)Copied the two new test files onto base
"The pre-existing suite passed unchanged" — verifiedApplied the source fix, removed both new test files, ran Every consumer checks the errorThe new guard only protects callers that check "Resolved" means the same thing on both axesThere is a second, duplicated group-resolution loop: Findings (all minor, none blocking)F1 — _ = mock.AnythingNothing else in the file uses F2 — require.Error(t, err, "...")
...
assert.False(t, IsUnrestrictedAccess(got) && err == nil,
"a failed resolution must never yield an unrestricted scope")
The comment above it ("The property in its own terms: whatever comes back must not be readable as 'all accounts'") also does not describe what happens: F3 — same file, Not a finding, but checked because it was asked: F4 — for the follow-up issue, not this PR. Two items to fold in alongside the two hand-rolled emptiness checks already named in the PR description:
Gates (re-run here at head, exit codes captured separately from stdout)
On the lint gate specifically: the first two attempts here were tool failures, not clean runs, and I did not report them as passes. Attempt 1 died on Note on scope of the test evidence: CI's test job runs bare OverallThe fix is correct and the reasoning behind choosing the minimal resolver-level guard over the type change holds up under the enumeration. F1-F3 are test hygiene; F4 belongs in the follow-up. No blocking findings. |
BLOCKING — #1752 has the partial-resolution hole. Correcting my own earlier verdict.My first two comments concluded "no blocking findings". That was wrong, and the error is mine rather than a gap in the brief: I examined this exact configuration during the separator analysis and dismissed it with a bad argument — I reasoned that a group with no Prompted by the finding on the sibling PR #1737, I built the configuration against #1752 specifically and ran it. It reproduces. Proof 1 — the resolver widens
Proof 2 — the widening reaches enforcementSame probe driven end-to-end through the real seams: a
Proof 3 — the guard closes the unreachable halfMeasured on the same principal: Under total failure the principal also loses every permission, so Where the PR's reasoning goes wrong
That premise holds only when the surviving groups still contribute entries — which is the exact configuration the test pins ( Precondition, stated precisely (this tempers severity, it does not remove it)The widening requires ≥1 group with a genuinely empty I checked what ships: all seven seeded groups carry It is reachable on operator-configured groups: Cross-PR question: the adapterSettled by reading both trees at head:
The #1737 reviewer was correct for #1737's tree, which is Shape of a fix (author's call; noting a trap in each)The separator has to be "no group the principal belongs to failed to resolve", not "at least one resolved". Two options:
Whichever lands, the regression test must be the multi-group shape above — the current partial-resolution test passes with the bug present. Method / hygieneFresh worktree at head |
The previous guard closed only TOTAL resolution failure. It rested on a premise that is false for AllowedAccounts: "partial resolution under-reports scope, which makes access stricter". AllowedAccounts is a UNION in which the empty set means EVERYTHING, so dropping a contributing group does not narrow it. The union of [] and ["acct-A"] is restricted; lose the group carrying ["acct-A"] and it collapses to [], which reads as every account. Dropping a group WIDENS. Reproduced by execution with a two-group actor, one granting update:groups with no allowed_accounts, one carrying the restriction: baseline: both groups resolve scope=[acct-A] unrestricted=false PARTIAL: restricting group ErrNoRows scope=[] unrestricted=TRUE PARTIAL: restricting group (nil,nil) scope=[] unrestricted=TRUE TOTAL failure (the old guard's case) refused The open half was also the more reachable one: under total failure the actor loses every permission too, so requirePermission denies at the gate before scope is consulted. A single-group configuration cannot exhibit this, which is why the earlier "verified across all six cases" found nothing -- all six were single-group. The guard is now: refuse when the union is empty AND at least one group was skipped. That targets exactly the widening. It deliberately does NOT refuse on any unresolved group, which would reverse the intentional "group was deleted; skip it rather than failing the entire request" behaviour and lock out a user with one stale membership everywhere until an admin cleaned up -- an unconditional availability regression traded for a conditional security hole, on a path with ~37 consumers. The skip count is recorded in collectGroupsAndAccounts at the point of skipping, NOT derived by comparing len(Groups) to len(User.GroupIDs): GroupIDs may contain duplicates, so that comparison reports phantom skips and refuses legitimate principals. Covered by a regression test. TestResolveAllowedAccounts_PartialResolutionIsAllowedAndNarrower asserted the false invariant and passed with the bug present. It is INVERTED rather than supplemented: a test encoding a false invariant is worse than no test, because it tells the next reader the case is covered. Its replacement uses the two-group shape and fails against the old guard. Preconditions, stated precisely: all seven seeded groups ship allowed_accounts = ARRAY['*'], which is already unrestricted, so nothing widens by default and this is not exploitable out of the box. It is reachable on operator-configured groups, because CreateGroupAPI accepts an omitted allowed_accounts (yielding []) and UpdateGroupAPI accepts an explicit [] -- the documented backward-compat default. A deployment with one such group alongside per-account scoped groups is exposed, on the same triggers as the rest of #1748. Refs #1748.
Delta review of
|
| shape | widened? | guard | correct? |
|---|---|---|---|
U non-empty, no * |
no — U ⊆ C, strictly narrower |
allow | ✅ |
U empty, skips > 0 |
yes — [] reads as everything |
refuse | ✅ |
U empty, skips = 0 |
no — genuinely unrestricted | allow | ✅ |
U contains *, skips > 0 |
no — * ∈ U ⟹ * ∈ C, unrestricted either way |
refuse | ❌ |
The last row is the problem. The guard's first condition is IsUnrestrictedAccess(authCtx.AllowedAccounts), which is true for empty or for a union containing "*". A principal whose surviving union contains "*" was already maximally wide at baseline; no loss can widen them further. Refusing them is provably zero security benefit and pure availability cost.
Measured — survivor carries ["*"], which all seven seeded groups do:
=== survivor carries ['*']; one stale membership ===
baseline (both resolve) scope=[* acct-A] unrestricted=true reqAcctAccess(acct-B)=GRANTED
stale membership ErrNoRows scope=REFUSED reqAcctAccess(acct-B)=REFUSED <<< regression
stale membership (nil,nil) scope=REFUSED reqAcctAccess(acct-B)=REFUSED <<< regression
Reachability is high, and it is the ordinary path. PostgresStore.DeleteGroup (store_postgres.go:585) is a bare DELETE FROM groups WHERE id = $1. users.group_ids is a plain UUID[] with no foreign key and no delete trigger — the migrations say so themselves in 000024_seed_default_groups.down.sql ("group_ids is a plain UUID[] with..."), and the only array_remove calls in the tree are inside migrations, never at runtime. So deleting any group leaves a dangling ID on every member permanently, until an admin edits each affected user.
Consequence: an admin deletes one custom group → every member of it who is also in any seeded group is refused on every account-scoped endpoint, and since the error is a plain fmt.Errorf these surface as 500s, not a clean 403. They still hold their permissions, so they pass requirePermission and then fail at the scope check.
There is an irony worth stating plainly: the "*" that makes the widening unreachable out of the box is the same "*" that triggers this refusal. A default deployment gets none of the new guard's security benefit and all of its availability cost.
Fix is one token — the condition wants "the union is empty", which is what the docstring already says three times ("refuse when the union is empty AND at least one group was skipped"):
if len(authCtx.AllowedAccounts) == 0 && authCtx.SkippedGroups > 0 {That still refuses every widening (the widening is the empty case) and stops refusing principals who were already unrestricted. The code and its own documentation currently disagree; the docstring is right.
3. The duplicates trap — it does not exist here, and the test guarding it is vacuous ⚠️
This one is my error, and the author implemented against it in good faith. I warned that detecting skips via len(Groups) vs len(User.GroupIDs) would misfire on duplicate IDs. I did not check the append semantics. collectGroupsAndAccounts appends to Groups once per ID, with no dedup — so a duplicated ID produces two entries and the counts stay aligned.
Measured on four shapes:
duplicate ids [g1,g1], g1 resolves len(GroupIDs)=2 len(Groups)=2 SkippedGroups=0 lenDiff=0 agree=true
distinct [g1,g2], both resolve len(GroupIDs)=2 len(Groups)=2 SkippedGroups=0 lenDiff=0 agree=true
distinct [g1,g2], g2 skipped len(GroupIDs)=2 len(Groups)=1 SkippedGroups=1 lenDiff=1 agree=true
dupes + a skip [g1,g1,g2] len(GroupIDs)=3 len(Groups)=2 SkippedGroups=1 lenDiff=1 agree=true
len(GroupIDs) - len(Groups) == SkippedGroups on every shape — the two are equivalent, duplicates included. Confirmed by mutation: swapping the counted skip for len(authCtx.Groups) != len(authCtx.User.GroupIDs) leaves all 7 tests passing, so TestResolveAllowedAccounts_DuplicateGroupIDsAreNotSkips passes under the very implementation it exists to exclude. It cannot fail.
To be precise, the test is vacuous with respect to its stated purpose, not inert in general: a third mutation that also refuses any multi-group principal (len(authCtx.Groups) > 1 || ...) does kill it, along with MultiGroupBaselineStaysRestricted. So it catches over-blocking; it just cannot catch the len-comparison it was written to exclude.
Two consequences:
- Retitle/rewrite it to assert what it actually guards, or drop it — as named and commented it tells the next reader a trap is covered when it is not.
service_group.go:188-190and theAuthContext.SkippedGroupscomment intypes.goboth state as fact that the len comparison "would report phantom skips and refuse legitimate principals". That is false for this code, and it is my incorrect claim now recorded as a justification. Please correct both.
Keeping the counted field is fine — it is more direct and stays correct if dedup is ever added to collectGroupsAndAccounts. Only the test and the two comments need fixing.
Minor, same class: the new comment says migration 000057's users_min_one_group CHECK makes len(Groups) == 0 unreachable. The CHECK is cardinality(group_ids) >= 1 — it guarantees one group ID, not one resolvable group. A user whose sole group was deleted satisfies it and resolves zero groups; measured, that case fires the first guard ("1 group(s) could not be resolved"), not the second. The second guard is indeed unreachable, but because the first one shadows it, not because of the CHECK alone.
4. The false-invariant test was inverted, not supplemented ✅
TestResolveAllowedAccounts_PartialResolutionIsAllowedAndNarrower is gone — git grep for the name across all *.go returns nothing. Baseline at head: 7 top-level tests, 7 PASS.
Mutation M1, reverting to the total-failure-only guard:
--- FAIL: TestResolveAllowedAccounts_PartialResolutionThatWidensIsRefused
--- PASS: (the other 6)
Exactly as claimed: 1/7, and it is the new test. The finding is covered, not described.
The MultiGroupBaselineStaysRestricted control also bites rather than decorating — mutation M3, adding len(authCtx.Groups) > 1 || to the guard so any multi-group principal is refused, kills it (2/7 with the duplicates test). So the refusal test cannot be satisfied by an implementation that simply refuses the two-group shape.
5. app.go:1159 — confirmed fixed by this PR ✅
At 86d1164b9 the adapter reads return a.service.ResolveAllowedAccounts(ctx, userID), so it inherits both the total-failure and the partial guard. On origin/main it is still BuildAuthContext + return authCtx.AllowedAccounts, nil. The cross-PR discrepancy was two reviewers reading different trees, as diagnosed.
6. Severity framing — accurate in both directions, with one refinement
- "Not exploitable out of the box" — correct. All seven seeded groups ship
ARRAY['*'](000024×4,000057,000059,000064); a union containing*is unrestricted at baseline, so nothing widens. - "Reachable on operator-configured groups" — correct.
CreateGroupAPI(service_api.go:283) appliesreq.AllowedAccountswith no validation,UpdateGroupAPI(:321) accepts an explicit[]. - Refinement: the exposed shape is narrower than "any deployment with an operator-configured group". The widening needs every surviving group to contribute nothing — so a user who also holds a seeded group is protected by its
*. The exposed deployment is one that moved onto custom groups, at least one carrying noallowed_accounts. Real, and not overclaimed.
7. Gates at 86d1164b9
| Gate | Result |
|---|---|
go build ./... |
exit 0 |
go vet ./... |
exit 0 |
go test ./... |
still running — follow-up comment |
gocyclo -over 10 -ignore "_test\.go" . |
exit 0, 0 lines; collectGroupsAndAccounts 7, ResolveAllowedAccounts 5 |
golangci-lint v2.10.1 (CI-pinned) |
still running — follow-up comment |
Summary
Point 1 ✅, point 4 ✅, point 5 ✅. Point 2 holds for restricted principals but the guard over-fires on the "*" case — the one change I would want before merge, one token. Point 3 is a vacuous test plus two comments documenting a trap that does not exist in this code, from a warning I got wrong.
misspell enforces US English; two comments carried 'behaviour'. The wording came from a briefing message written in British English and was transcribed into code without being re-checked against the repo's own gate. Refs #1748.
Gates at
|
| Gate | Result |
|---|---|
go build ./... |
exit 0 |
go vet ./... |
exit 0 |
go test ./... |
exit 0 — 32 packages ok, 0 FAIL (internal/api 23.7s, internal/auth 8.9s, internal/server 19.3s) |
gocyclo -over 10 -ignore "_test\.go" . |
exit 0, 0 lines; collectGroupsAndAccounts 7, ResolveAllowedAccounts 5 |
golangci-lint v2.10.1 (CI-pinned) |
exit 1 — 2 issues ❌ |
internal/auth/account_scope_fail_closed_test.go:225:46: `behaviour` is a misspelling of `behavior` (misspell)
// absorbed rather than refused. This is the behaviour option 1 ("refuse on any
^
internal/auth/service_group.go:181:4: `behaviour` is a misspelling of `behavior` (misspell)
// behaviour and lock out a user with one stale membership everywhere until an
^
2 issues:
* misspell: 2
Non-empty findings block plus a nonzero exit — a genuine failing run, not a tool error (contrast the earlier lock/timeout failures I discarded on the previous head, where exit 3 and 4 came with no findings). The CI Lint job will go red on this commit.
Both are introduced by 86d1164b9. git diff 6318fc4ce..86d1164b9 shows them as added lines (+// behaviour and lock out... in service_group.go, +// ... the behaviour option 1 ... in the test) — they are in the new prose explaining why option 1 was rejected.
Worth knowing before anyone reaches for misspell --fix: a Python scan finds 52 occurrences of behaviour across 38 files, but the other 50 all live in providers/* and pkg/*, which are outside the root module that golangci-lint lints from the repo root — which is why only these two are reported. So this is not pre-existing debt leaking into your PR; these two are genuinely yours, and they are the only two the linter can see. Fix them by hand (plain comment prose, no identifiers involved) rather than running an autofix across the tree.
Where this leaves the merge
Three things to change, none of them large:
IsUnrestrictedAccess(...)→len(authCtx.AllowedAccounts) == 0in the guard's first condition — the false refusal for"*"-carrying principals, which every seeded group produces. This is the one with user-visible consequences: delete any group and its members who also hold a seeded group get 500s on every account-scoped endpoint until an admin edits each of them.- The two
behaviourspellings — CI blocker. - The duplicates test and the two comments citing my incorrect trap claim — accuracy, no behavioural change.
The security fix itself is correct and, per mutation M1, genuinely covered by its test. Once (1) and (2) land I have nothing further.
Head moved to
|
The partial-resolution guard tested IsUnrestrictedAccess(AllowedAccounts), which is true for an empty union OR for one containing "*". A principal whose surviving union carries "*" was ALREADY maximally wide at baseline, so no lost group can widen them. Refusing them is zero security benefit and pure availability cost. All seven seeded groups ship allowed_accounts = ARRAY['*'], so this hit the default shape: delete any group, and its members who also hold a seeded group got errors on every account-scoped endpoint until an admin edited each of them individually. That is the unconditional availability regression that option 1 was rejected to avoid, arriving through the predicate instead. Measured, survivor carrying ["*"]: baseline (both resolve) scope=[* acct-A] unrestricted=true stale membership ErrNoRows REFUSED <-- regression stale membership (nil,nil) REFUSED <-- regression The test is now len(AllowedAccounts) == 0. Only an EMPTY union can have been widened by a loss. Both directions are mutation-verified and each is guarded by exactly one test: reverting the predicate kills only WildcardSurvivorToleratesSkippedGroup; removing the guard entirely kills only PartialResolutionThatWidensIsRefused. Neither test would catch the other's defect, so both are load-bearing. Also retracts an incorrect claim recorded in two comments and a test name. The skip count was documented as NOT derivable from len(GroupIDs) - len(Groups) because GroupIDs may contain duplicates. That is wrong: Groups is appended once per ID with no dedup, so duplicate IDs produce duplicate entries and the counts stay aligned on every shape. The counted-skip implementation is kept because it states the intent directly rather than inferring it from two lengths, but the comments no longer assert a trap that does not exist. TestResolveAllowedAccounts_DuplicateGroupIDsAreNotSkips could not fail under the implementation it claimed to exclude -- swapping the counted skip for the length subtraction left it passing. It does catch over-blocking, so it is renamed to DuplicateMembershipIsAllowed rather than deleted: a test whose name promises something it cannot check is worse than one scoped to what it proves. Refs #1748.
Delta review of
|
| Mutation | Kills |
|---|---|
M-A predicate reverted to IsUnrestrictedAccess(...) |
1/9 — WildcardSurvivorToleratesSkippedGroup only |
| M-B partial guard removed entirely | 1/9 — PartialResolutionThatWidensIsRefused only |
Neither is a superset of the other. Both directions are independently guarded, and neither test is doing the other's work.
4. Nothing else regressed ✅
The two predicates differ on exactly one input class: a union that is non-empty and contains "*". Everywhere else len(U) == 0 and IsUnrestrictedAccess(U) agree, so the change is provably confined to that class. Every row of the shape table re-measured:
| shape | expected | measured |
|---|---|---|
U empty, skips > 0 (the widening) |
refuse | REFUSED ✅ |
U empty, skips = 0 (legacy default) |
allow, unrestricted | scope=[], unrestricted=true, GRANTED ✅ |
U non-empty no *, skips > 0 (narrower) |
allow | scope=[acct-A], restricted, acct-B REFUSED ✅ |
U contains *, skips > 0 |
allow (was the bug) | scope=[*], GRANTED ✅ |
Two extra shapes for the changed class:
['*'] survivor, no skips scope=[*] unrestricted=true reqAcctAccess(acct-B)=GRANTED
WILDCARD group lost, [acct-A] survives scope=[acct-A] unrestricted=false reqAcctAccess(acct-B)=REFUSED
Losing the wildcard group correctly narrows rather than widening — the guard does not fire, and the survivor's restriction is enforced.
One observation, not a regression: in the wildcard rows revokeOwn(acct-B) is REFUSED even though the principal is unrestricted. That is the pre-existing stringInSlice divergence at handler_purchases_revoke.go:398 I raised as F4 in my first review — it honours neither "*" nor account-name entries, unlike MatchesAccount. The baseline row shows the same, so this PR changes nothing about it. Still one for the follow-up.
5. The renamed test still bites ✅
Mutation M-C (len(authCtx.Groups) > 1 || added, refusing any multi-entry membership) kills 3/9, including DuplicateMembershipIsAllowed — plus MultiGroupBaselineStaysRestricted and WildcardSurvivorBaselineIsAlreadyUnrestricted. So the rename is honest: it now guards over-blocking, which it does catch, rather than a trap it never could.
The finding-3 corrections are accurate
The revised text now says len(User.GroupIDs) - len(Groups) would give the same answer, because Groups is appended once per ID with no dedup, and justifies counting at the skip site as stating intent rather than inferring it. That matches what I measured (agree=true on all four shapes, and M2 previously leaving all tests green). The non-existent trap is no longer recorded as fact in either service_group.go or the AuthContext.SkippedGroups doc.
Gates at 07e88eebd
| Gate | Result |
|---|---|
go build ./... |
exit 0 |
go vet ./... |
exit 0 |
gocyclo -over 10 -ignore "_test\.go" . |
exit 0, 0 lines; collectGroupsAndAccounts 7, ResolveAllowedAccounts 5 |
golangci-lint v2.10.1 (CI-pinned, re-run at this head) |
exit 0 and 0 issues. ✅ |
per-module go test -race -short × 6 |
running — follow-up comment |
I re-ran lint at this head rather than carrying the earlier result forward; the two behaviour misspellings are gone and nothing replaced them.
One thing to know before merging: the branch is behind main
git rev-list --left-right --count origin/main...07e88eebd → 2 behind, 4 ahead. #1755 landed on main and rewrote the Unit Tests job to loop go test -race -short ./... over all six workspace modules (., pkg, providers/{aws,azure,gcp}, tests/e2e), with tests/e2e type-checked under -tags=e2e instead. This branch still carries the pre-#1755 ci.yml, but pull_request workflows run from the merge ref, so the new contract applies to this PR.
Two consequences worth flagging:
- The Lint job was not changed by ci: run unit and integration tests in every workspace module #1755 — it still runs
golangci-lintonce from the root, so only the root module is linted. My lint reproduction above is therefore the right one. (Relevant becauseproviders/*andpkg/*contain 50behaviouroccurrences that stay invisible to it.) - The five previously-ungated modules now gate this merge for the first time. Any failure there would be inherited from
main, not caused by this PR — but it would still redden this PR. I am running the same six-module loop locally and will report it.
|
Merging. Closes #1748 — a live cross-account read bypass on The original bug: Two defects were found in the fix itself, both by independent review, both fixed here. 1. The first fix closed the less reachable half. Its premise — partial resolution under-reports scope, making the ceiling stricter, so only total failure needed closing — is false when the scope is a union whose empty value means everything. An actor holding two groups, one carrying the permission with no And the open half was the more reachable one: under total failure the actor also loses every permission, so The reviewer who found this had examined that exact configuration earlier and dismissed it, by comparing what one group grants in isolation against what the configured combination enforces. The union of Worth recording why the original verification missed it: "verified across all six cases" meant six single-group cases. The bug requires two. A single-group configuration cannot exhibit it, which is exactly why the check felt thorough. 2. The second fix introduced a false refusal hitting every default deployment. The guard predicate was Both directions are independently guarded, confirmed by mutation rather than asserted: Neither test is a superset of the other. The full shape table was re-verified on every row, including the legacy A false-invariant test was inverted, not supplemented. One constraint given to the implementer turned out not to exist, and is recorded because it shaped the code: a warning that counting skips via Not a regression, flagged for follow-up: Gates at Six-module CI confirmed applied: #1755 merged at 05:05:12Z, this PRs checks ran at 05:20:58Z from the merge ref, and the per-module markers are present in the job log. So the five previously-ungated modules gated this merge. #1764 (representing account scope as a type, where the compiler found a 19th call site no scan could reach) is stacked on this and follows. |
…iling grantCeilingAccounts closed only TOTAL resolution failure, on the premise that partial resolution under-reports the actor's scope and is therefore stricter. That premise is false for AllowedAccounts, which is a UNION in which the empty set means EVERYTHING: dropping a contributing group does not narrow it. The union of [] and ["acct-A"] is restricted; lose the group carrying ["acct-A"] and it collapses to [], and the actor may then widen any group to ["*"]. Verified by execution against the write path: baseline (both groups resolve) REFUSED widening to [*] PARTIAL (scoping group ErrNoRows) ACCEPTED widening to [*] The configuration needs TWO groups -- one granting update:groups with no allowed_accounts, one carrying the restriction -- which is why the earlier six-case single-group verification could not find it. The guard is now: refuse when the union is empty AND at least one group was skipped. The emptiness test is len(AllowedAccounts) == 0, deliberately NOT IsUnrestrictedAccess: a union containing "*" was already maximally wide at baseline, so no loss can widen it, and refusing it would be zero security benefit and pure availability cost. All seven seeded groups ship allowed_accounts = ARRAY['*'], so the broader predicate would have refused every seeded-group member with one stale membership. Both directions are mutation-verified and each is guarded by exactly one test: widening the predicate kills only WildcardActorToleratesSkippedGroup; removing the guard kills only PartialActorResolutionThatWidensIsRefused. Neither would catch the other's defect. The total-failure case is the degenerate partial one -- every group skipped -- so it is now caught by the skipped-group guard and carries that message. The existing test asserted the other guard's exact wording; its assertion is relaxed to the sentinel plus a substring true of both. NOTE ON OVERLAP: AuthContext.SkippedGroups and the counting in collectGroupsAndAccounts are also added by PR #1752 for the read path. The two changes are identical; whichever merges second should see no divergence. Refs #1550, #1748.
Closing the one gate that was still running: six-module suite is greenMirrored All six pass, including the five that had never gated a merge before #1755. Nothing inherited from That completes the gate set at |
…iling grantCeilingAccounts closed only TOTAL resolution failure, on the premise that partial resolution under-reports the actor's scope and is therefore stricter. That premise is false for AllowedAccounts, which is a UNION in which the empty set means EVERYTHING: dropping a contributing group does not narrow it. The union of [] and ["acct-A"] is restricted; lose the group carrying ["acct-A"] and it collapses to [], and the actor may then widen any group to ["*"]. Verified by execution against the write path: baseline (both groups resolve) REFUSED widening to [*] PARTIAL (scoping group ErrNoRows) ACCEPTED widening to [*] The configuration needs TWO groups -- one granting update:groups with no allowed_accounts, one carrying the restriction -- which is why the earlier six-case single-group verification could not find it. The guard is now: refuse when the union is empty AND at least one group was skipped. The emptiness test is len(AllowedAccounts) == 0, deliberately NOT IsUnrestrictedAccess: a union containing "*" was already maximally wide at baseline, so no loss can widen it, and refusing it would be zero security benefit and pure availability cost. All seven seeded groups ship allowed_accounts = ARRAY['*'], so the broader predicate would have refused every seeded-group member with one stale membership. Both directions are mutation-verified and each is guarded by exactly one test: widening the predicate kills only WildcardActorToleratesSkippedGroup; removing the guard kills only PartialActorResolutionThatWidensIsRefused. Neither would catch the other's defect. The total-failure case is the degenerate partial one -- every group skipped -- so it is now caught by the skipped-group guard and carries that message. The existing test asserted the other guard's exact wording; its assertion is relaxed to the sentinel plus a substring true of both. NOTE ON OVERLAP: AuthContext.SkippedGroups and the counting in collectGroupsAndAccounts are also added by PR #1752 for the read path. The two changes are identical; whichever merges second should see no divergence. Refs #1550, #1748.
…writes (#1737) * sec(auth): enforce a grant ceiling and system-managed guard on group writes CreateGroupAPI / UpdateGroupAPI wrote the client-supplied permission list onto a group verbatim, with no check that the caller may grant what they are granting and no consultation of the system_managed column. Because update:groups is not one of the pairs carved out of the admin:* wildcard, any admin could void the #923 money separation-of-duties control tenant-wide in a single request: PUT /api/groups/<administrators> {"permissions":[{admin,*},{execute,purchases},{approve-any,purchases}, {retry-any,purchases}]} Two rules now gate every group-permission write: 1. Ceiling: a caller may only grant permissions their own effective set already holds, matched through the same carve-out-aware logic used at enforcement time, and at constraints no broader than their own (a holder capped at $100 cannot hand out an uncapped grant). 2. Non-grantable: the three money verbs in adminCarvedOuts may never be ADDED to a group, whoever the caller is. This is load-bearing rather than belt-and-braces: migrations 000059/000064 backfill every Administrators member into the Purchaser group, so a default-deployment admin explicitly holds those verbs and rule 1 alone would let them relay the verbs onto the Administrators group. A carved-out permission already stored on the target group may be carried through an unrelated edit, but not widened, so a rename is not forced to strip it. Refusals fail closed and name the offending permission; the list is never silently narrowed to the allowed subset, which is the corruption mode #1629 reports on the frontend side. An unidentified actor, or any error resolving the actor's permissions, refuses the write. The stateless admin API key has no user row, so the ceiling measures it against a bare {admin, *} holding. It can still seed ordinary groups but is now subject to the same money carve-out as a human admin, closing the third vector in the report. system_managed is enforced on update and on delete. Delete is the third write path to a group's permissions and was named in neither issue: dropping the seeded Purchaser group destroys the only holder of the carved-out verbs, which nothing can then re-grant, so the purchase path would be dead tenant-wide. CreateGroupAPI / UpdateGroupAPI now take the acting principal, matching UpdateUserAPI's existing shape. updateGroup previously discarded its session. Refs #1550, #1629. * sec(auth): reject blank action or resource in a group permission write A blank resource is not a request for the "*" wildcard, but that is what it became. The group-edit form picks its option with `isDefault = !currentValue && resource === '*'`, so an empty stored resource renders as the selected "All (*)" entry and saves back as view:*. Nothing validated the list on the way in, so the same widening was reachable from any API client with no form involved. The grant ceiling added in the previous commit does NOT close this. Its admin:* branch grants any (action, resource) pair that is not carved out, and ("view", "") is not carved out, so an admin wrote a blank resource straight through. Verified by execution before adding the guard: the write returned nil and reached the store. validateRequestedPermissions runs ahead of the ceiling on both write paths and refuses blank (empty or whitespace-only) actions and resources, naming the offending entry index. It fails before the actor lookup, so the refusal does not depend on who is asking. The two fields fail differently in the form, which is what shows the defect is in the defaulting rather than the parsing: a blank action is silently DROPPED (its index 0 is an empty placeholder) while a blank resource is silently WIDENED (its index 0 is the wildcard). Only the resource side escalates; both are refused, because a silently dropped permission is a different bug rather than an acceptable one. Unknown-but-non-blank values are deliberately still accepted: vocabulary validation is a separate concern and rejecting values this endpoint can already have stored would break edits of existing groups. An explicit "*" stays a legitimate value, gated by the ceiling rather than by this check. Mapped to 400, not 403: malformed input rather than an authorization failure. Refs #1730, #1550, #1629. * sec(auth): block self-granting carved-out money verbs via group membership #1550's report names a one-request alternative that needs no group edit at all: PUT /api/users/{self} adding DefaultPurchaserGroupID. The #907 self-escalation guard gates self-added groups on update:users, which admin:* grants, so it passed. An admin could join the Purchaser group and pick up execute / approve-any / retry-any on purchases in a single request, voiding the #923 separation of duties exactly as writing those verbs onto their own group would. Closing only the group-permission write path would have left this open while the issue auto-closed over it. guardSelfCarvedOutGrant applies the grant ceiling's own rule to membership: you cannot grant yourself a carved-out verb you do not already hold. It keys off the permission rather than off DefaultPurchaserGroupID, so a custom group carrying a money verb is blocked identically. Deliberately still allowed, each with a negative-control test: adding a second group carrying a verb already held (not an escalation); an admin adding ANOTHER user to Purchaser (the two-person control separation of duties exists to create); and trusted internal callers with an empty actor, so bootstrap and seeding paths are unaffected. Also fixes a latent fragility this surfaced. The guard resolved the actor's permissions by re-reading their row, but applyUpdateUserRequest has already mutated the in-memory user by that point; it gave the right answer only because the write had not been committed yet. A pre-existing test had to hand back a distinct unmutated copy on the second read to avoid aliasing the just-mutated object. guardSelfEscalation now resolves permissions from the prior membership snapshot, which is the question a self-escalation guard has to ask anyway, so the second read and the test's workaround are both gone. GetUserPermissions delegates to the new permissionsForGroups helper rather than duplicating the group-walk loop. Refs #1550, #923, #907. * docs(auth): record why guardSelfEscalation reads the prior membership The shorter spelling of this guard is to re-read the actor's row, and that spelling is a silent regression: UpdateUser calls applyUpdateUserRequest before the guards run, so the in-memory user already carries the new membership and a re-read returns the old values only because the write has not been committed yet. A future caller that passes the mutated user makes the guard authorize the escalation it exists to block. Record that on the function so the next reader does not "simplify" it back, and point at the mutation that enforces it: swapping prior for next fails the suite. Refs #1550. * sec(auth): bound AllowedAccounts on group writes, the fifth write path An allowed_accounts-only PUT never reached the ceiling at all. checkGrantCeiling opens with `if len(requested) == 0 { return nil }`, and APIUpdateGroupRequest's "empty means not sent" contract makes an accounts-only request the natural shape to send. Verified by execution with an actor holding only update:groups in one group scoped to one account: widening to more accounts, to [] and to ["*"] were all ACCEPTED, while the control -- widening Permissions[].Constraints.AccountIDs on the same call -- was correctly refused. One account dimension was bounded carefully and its sibling left open on the same request. checkAccountCeiling is deliberately a separate call rather than a branch inside checkGrantCeiling, so it cannot inherit that early return. A write is in ceiling if it does not widen the group's existing scope, or if it stays inside the acting principal's own. Two traps are handled explicitly. Empty and "*" both mean UNRESTRICTED, so this cannot be a subset test: the empty set is a subset of everything and means the opposite of narrow. And on create there is no prior scope, where a nil existing would read as unrestricted and swallow every check -- so CreateGroupAPI calls checkAccountGrant directly, and an omitted allowed_accounts is checked too, because it produces an unrestricted group. Every refusal test sends allowed_accounts with no permissions key. A test that included permissions would pass with the bug present, since the permission ceiling would then run and refuse for an unrelated reason. Also strengthens three mutation kills that rested on a missing stub rather than on an assertion. M5, M7 and TestUpdateUser_SelfEscalationDenied panicked on an unstubbed READ downstream of the guard, a kill that disappears the moment someone adds a permissive stub while tidying fixtures. Those now register the downstream reads with .Maybe() so removing the guard cannot panic, leaving the test's own assertions as the only thing that can fail it; M7 now dies on require.Error. FailsClosedOnGroupLoadError asserts errors.Is(err, loadErr) rather than message text. Splits group_ceiling_test.go, which exceeded the repo's 500-line guideline, by concern rather than by line count: fixtures, the permission ceiling, blank-field validation, and system_managed. Verified no test was lost -- 707 passing test names before and after, sorted and identical. Closes #1738. Refs #1550, #1629, #1730. * sec(auth): fail closed when the actor's account scope cannot be resolved grantCeilingAccounts read the acting principal's scope from BuildAuthContext and returned it unconditionally. collectGroupsAndAccounts skips a missing or deleted group silently -- pgx.ErrNoRows, or a store returning (nil, nil) -- so an actor whose groups all failed to load produced an EMPTY account list, which IsUnrestrictedAccess reads as "all accounts". The account ceiling became a no-op on exactly the path it guards. Verified by execution across the four resolution-failure modes. A store error on the user and a missing user row both already failed closed; the two group-resolution failures did not: store error on user -> refused user missing (nil, nil) -> refused actor's only group missing (ErrNoRows) -> ACCEPTED widening to ["*"] actor's only group returns (nil, nil) -> ACCEPTED widening to ["*"] Note the asymmetry this closes: the PERMISSION ceiling already failed closed on the same input, because an empty permission set grants nothing. Only the account ceiling failed open, which is the harder direction to notice because nothing errors. Requiring at least one resolved group is the precise guard. Partial resolution still under-reports the actor's scope, which makes the ceiling stricter rather than looser, so only total failure needed closing. Refs #1550, #1738. * sec(auth): refuse a partial actor resolution that widens the grant ceiling grantCeilingAccounts closed only TOTAL resolution failure, on the premise that partial resolution under-reports the actor's scope and is therefore stricter. That premise is false for AllowedAccounts, which is a UNION in which the empty set means EVERYTHING: dropping a contributing group does not narrow it. The union of [] and ["acct-A"] is restricted; lose the group carrying ["acct-A"] and it collapses to [], and the actor may then widen any group to ["*"]. Verified by execution against the write path: baseline (both groups resolve) REFUSED widening to [*] PARTIAL (scoping group ErrNoRows) ACCEPTED widening to [*] The configuration needs TWO groups -- one granting update:groups with no allowed_accounts, one carrying the restriction -- which is why the earlier six-case single-group verification could not find it. The guard is now: refuse when the union is empty AND at least one group was skipped. The emptiness test is len(AllowedAccounts) == 0, deliberately NOT IsUnrestrictedAccess: a union containing "*" was already maximally wide at baseline, so no loss can widen it, and refusing it would be zero security benefit and pure availability cost. All seven seeded groups ship allowed_accounts = ARRAY['*'], so the broader predicate would have refused every seeded-group member with one stale membership. Both directions are mutation-verified and each is guarded by exactly one test: widening the predicate kills only WildcardActorToleratesSkippedGroup; removing the guard kills only PartialActorResolutionThatWidensIsRefused. Neither would catch the other's defect. The total-failure case is the degenerate partial one -- every group skipped -- so it is now caught by the skipped-group guard and carries that message. The existing test asserted the other guard's exact wording; its assertion is relaxed to the sentinel plus a substring true of both. NOTE ON OVERLAP: AuthContext.SkippedGroups and the counting in collectGroupsAndAccounts are also added by PR #1752 for the read path. The two changes are identical; whichever merges second should see no divergence. Refs #1550, #1748. * fix(auth): make ceiling comparisons exact, and land the third .Maybe() fix F2 -- two comparison helpers were MORE permissive than the enforcement their comments claimed to mirror. accountScopeGap documented "comparison is exact, matching MatchesAccount" but trimmed both sides. MatchesAccount is `a == accountID`: no trim, no fold. An actor whose stored scope is " acct-A" matches nothing at enforcement, yet the ceiling treated them as holding "acct-A" and let them grant it. listCovers documented values as "trimmed and lower-cased, matching matchAllRegionsConstraint" -- true for Regions only. AccountIDs, Providers and Services are enforced by containsAny, an exact case-sensitive lookup, so normalising made the ceiling looser there: a holder constrained to ["ACCT-1"] could grant ["acct-1"], a value they cannot themselves use. Both are now exact everywhere. Exact is stricter than enforcement on Regions rather than looser, so it fails closed. The rule now stated in both comments: a ceiling may exceed enforcement in strictness, never fall short. This leaves normalizeConstraintValue with no callers, so it is removed. F3 -- the third .Maybe() strengthening had landed in the wrong test. The stubs went to TestGroupOnlyAuthz_NonAdminDenied, which calls only HasPermission and UserHasAdminCapability: it never writes, never adds a group, never touches DefaultAdminGroupID, and has no AssertNotCalled for the comment to refer to. Both stubs were dead, and TestUpdateUser_SelfEscalationDenied -- the test that needed them -- was still killed by a panic on an unstubbed UpdateUser, the exact weakness the change claimed to remove. Cause: the anchor `On("GetGroup", ctx, viewerGroup().ID)` appears in three tests in that file, and a first-match replace put the stubs in the first one. The same first-match error recurred on the first attempt to fix it; the stubs are now placed by locating the target function's body explicitly. Verified by mutation rather than by the test passing: with the self-escalation guard removed, TestUpdateUser_SelfEscalationDenied is now killed by an assertion (killed-by-PANIC false, killed-by-ASSERTION true), where before the move it was killed by a panic. Refs #1550, #1748.
1 participant
Closes #1748.
A scoped user got access to every cloud account whenever their group lookup failed
Verified by execution before writing the fix:
An empty list means unrestricted (
IsUnrestrictedAccess) — a deliberate backward-compat default. ButcollectGroupsAndAccountsalso skips a group it cannot load, so a resolution failure produced the same empty value. Absence and unrestricted shared a representation, and every failure silently granted everything, with no error and no log.Triggers are ordinary rather than exotic: a group deleted while a session is live, a replica lagging a group insert, any store path returning
(nil, nil).Blast radius was every account-scoped handler — 18 call sites across 10 files (17
getAllowedAccountsplus one directGetAllowedAccountsAPIinhandler_purchases_revoke.go), includingscoping.go'srequireAccountAccessandrequirePlanAccessthat the per-record scoping depends on.Three producers, closed at the point of production
h.auth == nilreturned(nil, nil)pgx.ErrNoRows)ResolveAllowedAccountsrequires ≥1 group to have resolved(nil, nil)2 and 3 are closed at the single point where the scope is produced, not per-caller, so a fourth failure mode in the same resolver cannot reintroduce it. Partial resolution still under-reports scope, which makes access stricter, so only total failure needed refusing.
The design constraint: there are THREE legitimate unrestricted principals, not two
This is what shaped the fix, and it is why "make absence fail closed" could not be applied literally. Established by execution before designing:
[]"*"["*"]allowed_accountsconfigured[](nil, nil)/ no groups[]The third collides exactly with the failure modes. Making an empty account list fail closed would have broken every legacy group.
The separator is to require a resolved group, not a resolved account list. Verified across all six cases — the three legitimate principals resolve ≥1 group and keep working; all three failure modes resolve none and are refused.
Verification: refusal and control, because refusal alone proves nothing
A fix that refused everyone would pass a refusal-only suite. Every failure case is paired with a control:
(nil, nil), several groups none resolving, user with no groups,h.auth == nil, resolver error propagated, and end-to-end throughrequireAccountAccess."*"wildcard, group with noallowed_accountsconfigured, scoped principal reaching its own account — and a scoped principal still refused an account outside its scope, so the fix collapsed nothing into either extreme.Per-guard mutation
TestResolveAllowedAccounts_FailsClosed(1/10)(nil, nil)TestGetAllowedAccounts_FailsClosedWhenAuthMissing(1/10)P3 is the one that matters: it proves the controls actually bite. Without it, an over-blocking implementation would look correct.
Gates
Note: the pre-existing suite passed unchanged before these tests were added — no existing test covered any of the three producers, which is consistent with #1596's finding that restricted-account paths were structurally untestable.
Scope
Deliberately not changing the representation itself (a distinct type or explicit flag). That remains attractive as defence-in-depth, and two consumers still hand-roll the emptiness check rather than routing through
IsUnrestrictedAccess—handler_marketplace.go:435andhandler_purchases_revoke.go:398. Both are correct now that empty can only arise from a successful resolution, but they would not stop a future producer. Worth a follow-up; not worth growing a live-on-maincritical fix.Related: #1737 (the same shape on the group write path), #950/#956 (the account-filter regression class), #1596 (why this class had no coverage).