fix: restrict admin credit grants to internal API#2113
Conversation
📝 WalkthroughWalkthroughThe PR moves admin credit grant authority from authenticated admin users to internal API secret requests. The ChangesAdmin Credits Grant Authorization Shift
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/private/admin_credits.ts (1)
96-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNew grants will disappear from
/grants-history.Dropping
admin_user_idfromsource_refmeans the existing history query later in this file no longer matches grants created by this route, because it still filters onsource_ref->admin_user_id IS NOT NULL. The dashboard stays read-only, but its history view will stop showing all post-change internal grants unless that query is updated to include the newgranted_via: 'internal_api'shape (and ideally keep the legacy predicate during migration).🤖 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 `@supabase/functions/_backend/private/admin_credits.ts` around lines 96 - 108, The new sourceRef object used when calling adminSupabase.rpc('top_up_usage_credits') omits admin_user_id which breaks the grants-history query (it still filters on source_ref->admin_user_id IS NOT NULL); restore the legacy field by adding admin_user_id (e.g., sourceRef.admin_user_id = currentAdmin.id or similar) when building sourceRef for top_up_usage_credits, and/or update the history query predicate to also accept grants with source_ref->>'granted_via' = 'internal_api' (preserve the existing admin_user_id check during migration).
🧹 Nitpick comments (1)
tests/admin-credits.test.ts (1)
317-331: ⚡ Quick winPlease keep one happy-path
/granttest for internal callers.Replacing the old success case with another rejection assertion leaves this endpoint with no visible coverage for the only allowed caller anymore. Add a companion test that sends the expected internal API secret and verifies a grant succeeds, otherwise the route could be accidentally broken and this suite would still stay green.
🤖 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 `@tests/admin-credits.test.ts` around lines 317 - 331, Add a companion happy-path test for the /private/admin_credits/grant endpoint: keep the existing "should reject admin JWT credit grants" test (which uses getAdminHeaders and asserts 400), then add a new test that uses the internal API secret header expected by the route (the same BASE_URL/PRIVATE path and TEST_ORG_ID/amount/notes payload as the rejected test), sends the correct internal auth credential instead of admin JWT, and asserts a successful response (e.g., 200/201) and that the response body contains confirmation of the grant; locate the endpoint usage in the test file around the existing test and mirror its request/response assertions but with the internal API secret header.
🤖 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 `@tests/admin-credits-auth-boundary.unit.test.ts`:
- Around line 5-20: The test currently only asserts the HTTP response; you must
also spy/mock the Supabase admin RPC to ensure top_up_usage_credits is never
invoked: in the test for 'does not allow platform-admin JWTs to reach credit
grants' obtain the mocked supabase admin client (supabaseAdmin) and attach a spy
or mock to its rpc method (the call name 'top_up_usage_credits'), then after
making adminCreditsApp.request assert that supabaseAdmin().rpc was never called
(or rpc was not invoked with 'top_up_usage_credits') so the test proves the
handler never reached the RPC path.
---
Outside diff comments:
In `@supabase/functions/_backend/private/admin_credits.ts`:
- Around line 96-108: The new sourceRef object used when calling
adminSupabase.rpc('top_up_usage_credits') omits admin_user_id which breaks the
grants-history query (it still filters on source_ref->admin_user_id IS NOT
NULL); restore the legacy field by adding admin_user_id (e.g.,
sourceRef.admin_user_id = currentAdmin.id or similar) when building sourceRef
for top_up_usage_credits, and/or update the history query predicate to also
accept grants with source_ref->>'granted_via' = 'internal_api' (preserve the
existing admin_user_id check during migration).
---
Nitpick comments:
In `@tests/admin-credits.test.ts`:
- Around line 317-331: Add a companion happy-path test for the
/private/admin_credits/grant endpoint: keep the existing "should reject admin
JWT credit grants" test (which uses getAdminHeaders and asserts 400), then add a
new test that uses the internal API secret header expected by the route (the
same BASE_URL/PRIVATE path and TEST_ORG_ID/amount/notes payload as the rejected
test), sends the correct internal auth credential instead of admin JWT, and
asserts a successful response (e.g., 200/201) and that the response body
contains confirmation of the grant; locate the endpoint usage in the test file
around the existing test and mirror its request/response assertions but with the
internal API secret header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59d26587-ead2-4df9-9648-1f00e23062c5
📒 Files selected for processing (5)
messages/en.jsonsrc/pages/admin/dashboard/credits.vuesupabase/functions/_backend/private/admin_credits.tstests/admin-credits-auth-boundary.unit.test.tstests/admin-credits.test.ts
| it('does not allow platform-admin JWTs to reach credit grants', async () => { | ||
| const response = await adminCreditsApp.request(new Request('http://localhost/grant', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Authorization': 'Bearer test.jwt.value', | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| org_id: '550e8400-e29b-41d4-a716-446655440000', | ||
| amount: 25, | ||
| }), | ||
| })) | ||
|
|
||
| expect(response.status).toBe(400) | ||
| await expect(response.text()).resolves.toContain('Cannot find authorization') | ||
| }) |
There was a problem hiding this comment.
This doesn't actually prove top_up_usage_credits was unreachable.
The test only checks the rejected response. If the handler ever regressed into calling supabaseAdmin().rpc('top_up_usage_credits', ...) before failing, this would still pass. Please mock/spy the RPC path and assert it is never invoked so the test matches its title and the PR objective.
🤖 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 `@tests/admin-credits-auth-boundary.unit.test.ts` around lines 5 - 20, The test
currently only asserts the HTTP response; you must also spy/mock the Supabase
admin RPC to ensure top_up_usage_credits is never invoked: in the test for 'does
not allow platform-admin JWTs to reach credit grants' obtain the mocked supabase
admin client (supabaseAdmin) and attach a spy or mock to its rpc method (the
call name 'top_up_usage_credits'), then after making adminCreditsApp.request
assert that supabaseAdmin().rpc was never called (or rpc was not invoked with
'top_up_usage_credits') so the test proves the handler never reached the RPC
path.
|
e4792200-stack
left a comment
e4792200-stack
left a comment
There was a problem hiding this comment.
Thanks for tightening this boundary. I verified the route/UI direction and the CI is green, but I found one blocker before merge.
New grants created by POST /private/admin_credits/grant now write source_ref with { granted_via: "internal_api", org_name }, but /grants-history still fetches only manual grants where source_ref->admin_user_id is not null. Because this PR removes admin_user_id from the new source_ref, internal grants created after this change will disappear from the read-only grants history dashboard even though the page still advertises grant history.
Please update the history query to include the new granted_via = internal_api shape while preserving legacy admin_user_id rows. A happy-path test using the internal apisecret header would also cover the only allowed caller and catch this kind of regression. I also checked the patch with git show --check; no whitespace issues.
digzrow-coder
left a comment
digzrow-coder
left a comment
There was a problem hiding this comment.
The grants-history endpoint still filters on the old UI shape, so the new internal grants disappear from the read-only admin page. /grant now writes source_ref = { granted_via: 'internal_api', org_name: ... } with no admin_user_id, but /grants-history keeps .not('source_ref->admin_user_id', 'is', null). After this change, any credits granted through the only remaining mutation path are excluded from the "Recent Admin Grants" table, so admins can no longer audit the grant history from this page. The filter needs to include the new internal grant shape, or be changed to match the intended manual-credit sources.
KCDaemon
left a comment
KCDaemon
left a comment
There was a problem hiding this comment.
Rechecked latest head (f7503e1). Moving the grant endpoint behind the internal API secret closes the user-JWT mutation path, but the successful grant is no longer attributable to any actor.
Before this PR, the grant wrote admin_user_id into source_ref and logged adminUserId. Now every grant records only { granted_via: "internal_api", org_name }, and the success/failure logs also omit who or which internal job/operator initiated the credit mutation. For a financial credit grant, a shared secret should authenticate the caller, but it should not be the only audit identity.
Please require an internal actor/source identifier (for example service name, job id, or operator id) and persist it in source_ref and logs, then add a regression asserting grant history retains that attribution for internal grants. Without that, admins can see balances changed but cannot audit who/what granted the credits.
|
I think this leaves the read-only history path out of sync with the new grant writer.
A quick repro from the code path is: call |
5 participants
Summary
Restricts the admin credit grant endpoint to internal operational callers and removes the browser-facing grant form from the platform admin credits dashboard.
POST /private/admin_credits/grantbehindmiddlewareAPISecretSecurity rationale
AGENTS.mdsays platform-admin capabilities must remain read-only except impersonation, and must not mutate billing state. Credit grants mutate billing/accounting state throughtop_up_usage_credits, so this path should be internal API-secret-only rather than reachable through platform-admin JWT/API-key middleware.Related to the rewarded security review thread: #1667
Test plan
git diff --check/home/joao/.bun/bin/bun test tests/admin-credits-auth-boundary.unit.test.ts/home/joao/.bun/bin/bun lintNote: I did not run the full
tests/admin-credits.test.tsintegration suite locally because the Supabase stack was not running on127.0.0.1:54321in this environment.Summary by CodeRabbit
New Features
Changes