Repository navigation
fix(auth): scope ladder configs to the caller's allowed accounts - #1489
Conversation
Migration 000088 granted view:config to the Standard-User and Read-Only-User groups (previously admin-only), but getLadderConfigs returned every account's ladder config unconditionally, a cross-account data leak: any scoped non-admin user with view:config could enumerate other accounts' spend caps and ramp schedules. Add filterLadderConfigsByAllowedAccounts, mirroring the existing filterPurchaseHistoryByAllowedAccounts pattern from issue #1030. Admin/unrestricted sessions pass through unchanged; scoped sessions are filtered by allowed_accounts. Fails closed: if getAllowedAccounts errors, the request errors out instead of falling through to unfiltered data. Adds two regression tests: a scoped user must see only their own account's config when configs exist for multiple accounts, and an admin session must still see every account's config.
cristim
added
triaged
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe ladder configuration endpoint now filters results by the caller’s allowed cloud accounts. New tests cover scoped users and admins, and an existing test adds mock expectation cleanup. ChangesLadder configuration account scoping
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handler_per_account_perms_test.go`:
- Around line 1383-1413: Add a mock-first regression test alongside
TestPerAccountPerms_GetLadderConfigs_ScopedUserSeesOnlyOwnAccount that
configures GetAllowedAccountsAPI to return an error, then invokes
handler.getLadderConfigs with the scoped request. Assert the error is returned
and the response is nil, covering the fail-closed allowlist-lookup branch.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0f34d08-3b73-459e-92b9-44fa50e5fa03
📒 Files selected for processing (3)
internal/api/handler_config_test.gointernal/api/handler_ladder.gointernal/api/handler_per_account_perms_test.go
| func TestPerAccountPerms_GetLadderConfigs_ScopedUserSeesOnlyOwnAccount(t *testing.T) { | ||
| ctx := context.Background() | ||
|
|
||
| cfgA := config.LadderConfigDB{ID: "cfg-a", CloudAccountID: permsAccA, Provider: "aws"} | ||
| cfgB := config.LadderConfigDB{ID: "cfg-b", CloudAccountID: permsAccB, Provider: "aws"} | ||
|
|
||
| mockStore := new(MockConfigStore) | ||
| mockStore.On("GetLadderConfigs", ctx).Return([]config.LadderConfigDB{cfgA, cfgB}, nil) | ||
| mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { | ||
| return permsAccountList(), nil | ||
| } | ||
|
|
||
| handler := &Handler{ | ||
| auth: scopedAuthMock(ctx), | ||
| config: mockStore, | ||
| } | ||
|
|
||
| resp, err := handler.getLadderConfigs(ctx, scopedReq()) | ||
| require.NoError(t, err) | ||
| body, ok := resp.(map[string]any) | ||
| require.True(t, ok, "expected map[string]any, got %T", resp) | ||
| configs, ok := body["configs"].([]config.LadderConfigDB) | ||
| require.True(t, ok, "expected []config.LadderConfigDB, got %T", body["configs"]) | ||
|
|
||
| ids := make([]string, len(configs)) | ||
| for i, c := range configs { | ||
| ids[i] = c.ID | ||
| } | ||
| assert.ElementsMatch(t, []string{"cfg-a"}, ids, | ||
| "scoped user must see only account-A's ladder config; account-B config must be filtered out") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
go build ./...
go test ./...
if [[ -f pkg/go.mod ]]; then
(cd pkg && go test ./...)
fiRepository: LeanerCloud/CUDly
Length of output: 147
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n## File outline\n'
ast-grep outline internal/api/handler_per_account_perms_test.go --view expanded || true
printf '\n## Search for GetAllowedAccountsAPI and getLadderConfigs\n'
rg -n "GetAllowedAccountsAPI|getLadderConfigs|GetLadderConfigs" internal/api -S
printf '\n## Relevant test region\n'
sed -n '1320,1460p' internal/api/handler_per_account_perms_test.go
printf '\n## Relevant handler implementation region\n'
sed -n '1,260p' internal/api/handler_per_account_perms.goRepository: LeanerCloud/CUDly
Length of output: 16632
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n "GetAllowedAccountsAPI|allowed-account|allowlist|GetLadderConfigs" internal/api/handler_per_account_perms_test.go internal/api/handler_per_account_perms.go -S
sed -n '1360,1455p' internal/api/handler_per_account_perms_test.goRepository: LeanerCloud/CUDly
Length of output: 1540
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n## Locate ladder handler\n'
fd -a "handler_ladder.go|handler_per_account_perms.go" internal/api
printf '\n## Outline ladder handler\n'
ast-grep outline internal/api/handler_ladder.go --view expanded || true
printf '\n## Read ladder handler\n'
sed -n '1,220p' internal/api/handler_ladder.go
printf '\n## Read auth interface and any shared permission helper\n'
sed -n '180,260p' internal/api/types.go
sed -n '380,460p' internal/api/handler.goRepository: LeanerCloud/CUDly
Length of output: 16417
Cover the allowlist-lookup failure path. Add a mock-first regression test for GetAllowedAccountsAPI returning an error, and assert getLadderConfigs returns an error with a nil response. The current tests only cover successful scoped and unrestricted paths, so the fail-closed branch stays unguarded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/handler_per_account_perms_test.go` around lines 1383 - 1413, Add
a mock-first regression test alongside
TestPerAccountPerms_GetLadderConfigs_ScopedUserSeesOnlyOwnAccount that
configures GetAllowedAccountsAPI to return an error, then invokes
handler.getLadderConfigs with the scoped request. Assert the error is returned
and the response is nil, covering the fail-closed allowlist-lookup branch.
Source: Coding guidelines
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Summary
Fixes a cross-account data leak in
getLadderConfigs(GET /ladder/configs), found via adversarial review of #1428.view:configpermission to the Standard-User and Read-Only-User groups, which previously only admins held.getLadderConfigsininternal/api/handler_ladder.gowas never updated to account for that: it returned every account'sLadderConfigDB(spend caps, ramp schedules) unconditionally, regardless of the caller'sallowed_accountsscope.view:config.Root cause
getLadderConfigscalledh.config.GetLadderConfigs(ctx)and returned the result unfiltered. There was no scoping step, unlike the analogous purchase-history endpoint which already went throughfilterPurchaseHistoryByAllowedAccounts(added for issue #1030).Fix
getLadderConfigsnow captures thesessionfromrequirePermissionand filters the result through a newfilterLadderConfigsByAllowedAccountsmethod, mirroring the existingfilterPurchaseHistoryByAllowedAccountspattern.auth.IsUnrestrictedAccess) pass through unfiltered.auth.MatchesAccount(allowed, c.CloudAccountID, nameByID[c.CloudAccountID]).getAllowedAccountserrors, the whole request errors out rather than falling through to unfiltered data.Tests
Two new regression tests in
internal/api/handler_per_account_perms_test.go:TestPerAccountPerms_GetLadderConfigs_ScopedUserSeesOnlyOwnAccount: the real failing scenario. A scoped user withallowed_accounts: [accA]must not see account B's config when the store returns configs for both accounts. Verified this fails on the pre-fix code (account-B config leaks through) and passes post-fix.TestPerAccountPerms_GetLadderConfigs_AdminSeesAll: an admin/unrestricted session still receives every account's config.Also added
t.Cleanup(mockStore/mockAuth.AssertExpectations)to an existing test inhandler_config_test.goper this repo's mock-assertion convention.Test plan
go build ./...go vet ./...go test ./internal/api/...(full package, all green)golangci-lint run(pinned v2.10.1, matching CI) — 0 issuesgocyclo -over 10on changed files — cleanContext only: #1428 (already merged; this is a fix-forward PR for a scoping gap it introduced, not a fix to #1428 itself).
Summary by CodeRabbit