Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (74)
📝 WalkthroughWalkthroughMigration tightens execution privileges on the public.record_build_time function (revokes PUBLIC/anon/authenticated; grants only to service_role). Tests updated to run many RPC cases concurrently and add checks that anonymous and authenticated clients are rejected and that negative build_time is rejected. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/build_time_tracking.test.ts`:
- Around line 348-366: Add a second test next to the anonymous denial case that
calls the same RPC record_build_time but using an authenticated Supabase client
instead of supabaseAnon: create a client via
createClient(process.env.SUPABASE_URL!, process.env.SUPABASE_SERVICE_ROLE_KEY!
or the project API key that maps to the authenticated role) with
auth.persistSession: false, invoke rpc('record_build_time', {...}) with the same
params (ORG_ID, USER_ID, randomUUID(), 'ios', 60) and assert the returned error
is truthy; place the test near the existing one so it covers the revoked
authenticated role path.
- Line 348: Replace all synchronous test declarations using it(...) with
parallel tests using it.concurrent(...) throughout the file; specifically update
the eight test cases including the one with description "should reject
record_build_time calls from anonymous clients" and the other tests at the
listed locations (e.g., tests with their respective descriptions on lines 138,
201, 254, 277, 300, 334, 368) so each call to it becomes it.concurrent,
preserving the test names and callback functions (no changes to assertions or
async handling).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supabase/migrations/20260227091500_restrict_record_build_time_access.sqltests/build_time_tracking.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/build_time_tracking.test.ts (2)
7-10:⚠️ Potential issue | 🟠 MajorUse per-test fixture identifiers instead of module-level shared constants.
All 9 tests in this describe block run concurrently (
it.concurrent()), but lines 7–10 define a single sharedORG_IDandAPPNAMEthat all tests reference. AlthoughbeforeEach()attempts to reset these rows (lines 62–120), concurrent execution means test A and test B can modify and read each other's data simultaneously, causing cross-test interference and flakes.Per the coding guidelines, create dedicated seed data with unique identifiers per test rather than sharing module-level fixtures. Refactor to generate scoped IDs per test (e.g., using test name or UUID suffix) and instantiate fixtures within each test's scope instead of in
beforeEach().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/build_time_tracking.test.ts` around lines 7 - 10, The tests currently use module-level shared constants (testRunId, ORG_ID, APPNAME, STRIPE_CUSTOMER_ID) which causes cross-test interference when tests run concurrently; change to generate unique identifiers per test by moving UUID generation into each test scope (or into a beforeEach that assigns to test-scoped variables) and instantiate seed data/fixtures (org/app/stripe customer) inside each test using those per-test IDs, updating all references to ORG_ID, APPNAME, STRIPE_CUSTOMER_ID and any setup/teardown that uses testRunId so each test operates on its own isolated data.
305-312:⚠️ Potential issue | 🟠 MajorAssert the first RPC call result in the upsert test.
The first
record_build_timecall (lines 305-312) does not capture or assert the error result. If that call fails, the test continues, and the second call can still succeed and update the record, causing the final assertion to pass despite the first insert failing. Upsert behavior cannot be properly validated without checking both calls.Proposed fix
// First call - await supabase.rpc('record_build_time', { + const { error: firstRpcError } = await supabase.rpc('record_build_time', { p_org_id: ORG_ID, p_user_id: USER_ID, p_build_id: buildId, p_platform: 'ios', p_build_time_unit: 600, }) + expect(firstRpcError).toBeFalsy()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/build_time_tracking.test.ts` around lines 305 - 312, Capture the result of the first supabase.rpc('record_build_time', {...}) call into a variable (e.g., insertRes) and assert its error is null (and/or check expected data) before making the second RPC call; then likewise capture and assert the second call result so the test actually verifies both the initial insert and the subsequent upsert. Refer to the rpc call invocation and use the result variable names around record_build_time to locate where to add the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/build_time_tracking.test.ts`:
- Line 4: The named imports on the import line are out of alphabetical order and
violate perfectionist/sort-named-imports; reorder the specifiers in the import
from './test-utils.ts' so they are sorted alphabetically (e.g., fetchWithRetry,
getEndpointUrl, getSupabaseClient, PRODUCT_ID, resetAppData, resetAppDataStats,
TEST_EMAIL, USER_ID, USER_PASSWORD) to satisfy the linter rule for the import
statement.
- Around line 365-366: Replace the weak truthy assertions in the two
auth-rejection tests with explicit permission-denied checks: in the anonymous
client rejection test and the authenticated client rejection test in
tests/build_time_tracking.test.ts, assert that the caught error indicates
permission denial (for example, expect(error.message).toContain("permission
denied for function") or expect((error as any).code).toBe("permission-denied")),
so the tests verify RLS semantics rather than any generic failure.
---
Outside diff comments:
In `@tests/build_time_tracking.test.ts`:
- Around line 7-10: The tests currently use module-level shared constants
(testRunId, ORG_ID, APPNAME, STRIPE_CUSTOMER_ID) which causes cross-test
interference when tests run concurrently; change to generate unique identifiers
per test by moving UUID generation into each test scope (or into a beforeEach
that assigns to test-scoped variables) and instantiate seed data/fixtures
(org/app/stripe customer) inside each test using those per-test IDs, updating
all references to ORG_ID, APPNAME, STRIPE_CUSTOMER_ID and any setup/teardown
that uses testRunId so each test operates on its own isolated data.
- Around line 305-312: Capture the result of the first
supabase.rpc('record_build_time', {...}) call into a variable (e.g., insertRes)
and assert its error is null (and/or check expected data) before making the
second RPC call; then likewise capture and assert the second call result so the
test actually verifies both the initial insert and the subsequent upsert. Refer
to the rpc call invocation and use the result variable names around
record_build_time to locate where to add the assertions.
| import { createClient } from '@supabase/supabase-js' | ||
| import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest' | ||
| import { fetchWithRetry, getEndpointUrl, getSupabaseClient, PRODUCT_ID, resetAppData, resetAppDataStats, TEST_EMAIL, USER_ID } from './test-utils.ts' | ||
| import { USER_PASSWORD, fetchWithRetry, getEndpointUrl, getSupabaseClient, PRODUCT_ID, resetAppData, resetAppDataStats, TEST_EMAIL, USER_ID } from './test-utils.ts' |
There was a problem hiding this comment.
Fix named-import order to satisfy ESLint.
Line 4 currently violates perfectionist/sort-named-imports, which will keep lint red.
Proposed fix
-import { USER_PASSWORD, fetchWithRetry, getEndpointUrl, getSupabaseClient, PRODUCT_ID, resetAppData, resetAppDataStats, TEST_EMAIL, USER_ID } from './test-utils.ts'
+import { fetchWithRetry, getEndpointUrl, getSupabaseClient, PRODUCT_ID, resetAppData, resetAppDataStats, TEST_EMAIL, USER_ID, USER_PASSWORD } from './test-utils.ts'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { USER_PASSWORD, fetchWithRetry, getEndpointUrl, getSupabaseClient, PRODUCT_ID, resetAppData, resetAppDataStats, TEST_EMAIL, USER_ID } from './test-utils.ts' | |
| import { fetchWithRetry, getEndpointUrl, getSupabaseClient, PRODUCT_ID, resetAppData, resetAppDataStats, TEST_EMAIL, USER_ID, USER_PASSWORD } from './test-utils.ts' |
🧰 Tools
🪛 ESLint
[error] 4-4: Expected "fetchWithRetry" to come before "USER_PASSWORD".
(perfectionist/sort-named-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/build_time_tracking.test.ts` at line 4, The named imports on the import
line are out of alphabetical order and violate perfectionist/sort-named-imports;
reorder the specifiers in the import from './test-utils.ts' so they are sorted
alphabetically (e.g., fetchWithRetry, getEndpointUrl, getSupabaseClient,
PRODUCT_ID, resetAppData, resetAppDataStats, TEST_EMAIL, USER_ID, USER_PASSWORD)
to satisfy the linter rule for the import statement.
| expect(error).toBeTruthy() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/build_time_tracking.test.ts | sed -n '350,410p'Repository: Cap-go/capgo
Length of output: 1940
🏁 Script executed:
find . -type f -name "*.sql" | xargs grep -l "record_build_time" | head -5Repository: Cap-go/capgo
Length of output: 223
🏁 Script executed:
cat ./supabase/migrations/20260227091500_restrict_record_build_time_access.sqlRepository: Cap-go/capgo
Length of output: 1032
🏁 Script executed:
grep -n "permission\|Permission\|PERMISSION" tests/build_time_tracking.test.tsRepository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
find tests -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "error.*message" | head -3Repository: Cap-go/capgo
Length of output: 133
🏁 Script executed:
grep -n "error.*message" tests/bundle.test.ts | head -10Repository: Cap-go/capgo
Length of output: 132
🏁 Script executed:
grep -n "\.rpc(" tests/*.test.ts | head -10Repository: Cap-go/capgo
Length of output: 1057
🏁 Script executed:
cat -n tests/build_time_tracking.test.ts | sed -n '200,220p'Repository: Cap-go/capgo
Length of output: 1045
🏁 Script executed:
cat -n tests/build_time_tracking.test.ts | sed -n '340,370p'Repository: Cap-go/capgo
Length of output: 1123
🌐 Web query:
Supabase RLS policy violation error message format
💡 Result:
Supabase’s “RLS policy violation” errors usually come from PostgREST, so the response body follows the PostgreSQL error shape:
{
"code": "42501",
"details": null,
"hint": null,
"message": "new row violates row-level security policy for table \"<table>\""
}This is commonly returned with HTTP 403 Forbidden (or 401 if unauthenticated) because 42501 maps to “insufficient privileges.” [1], [2]
For Storage endpoints, you may instead (or additionally) see Supabase’s gateway-style envelope like:
{
"statusCode": "403",
"error": "Unauthorized",
"message": "new row violates row-level security policy"
}[3], [4]
Sources: PostgREST error format + HTTP mapping [1]; Supabase/PostgREST example payload [2]; Storage-style envelope examples [3], [4].
[1] (docs.postgrest.org)
[2] (github.com)
[3] (reddit.com)
[4] (github.com)
Citations:
- 1: https://docs.postgrest.org/en/v14/references/errors.html?utm_source=openai
- 2: https://github.com/orgs/supabase/discussions/36243?utm_source=openai
- 3: https://www.reddit.com/r/Supabase/comments/1g6389c?utm_source=openai
- 4: https://github.com/orgs/supabase/discussions/33852?utm_source=openai
🌐 Web query:
Supabase PostgreSQL REVOKE EXECUTE function permission denied error message
💡 Result:
In Supabase (Postgres), the error:
ERROR: permission denied for function <fn>
means the role you’re using (often anon/authenticated via PostgREST) does not have EXECUTE on that function. By default, functions are executable by PUBLIC (so everyone can run them) unless you revoke it. (supabase.com)
Revoke EXECUTE (the Supabase-recommended pattern)
You typically need both revokes (because PUBLIC is the default grant path): (supabase.com)
revoke execute on function public.foo() from public;
revoke execute on function public.foo() from anon; -- and/or authenticated(Include the exact signature foo(argtype, ...) if the function is overloaded.)
If you still get “permission denied” after GRANT/REVOKE
Make sure the caller role also has USAGE on the schema that contains the function (or any function it calls); otherwise you can still see permission errors in SECURITY DEFINER / nested-function setups. (stackoverflow.com)
grant usage on schema public to anon, authenticated;Prevent future functions from being executable by default
Use default privileges (note: revoke at the “global” level, not per-schema). (stackoverflow.com)
alter default privileges for role postgres
revoke execute on functions from public;
alter default privileges for role postgres
revoke execute on functions from anon;If the REVOKE command itself fails
You must run REVOKE as the function owner (or a sufficiently privileged/admin role). On Supabase, the API commonly runs as anon/authenticated, not as an admin role. (supabase.com)
Citations:
- 1: https://supabase.com/docs/guides/troubleshooting/how-can-i-revoke-execution-of-a-postgresql-function-2GYb0A
- 2: https://supabase.com/docs/guides/troubleshooting/how-can-i-revoke-execution-of-a-postgresql-function-2GYb0A
- 3: https://stackoverflow.com/questions/57837817/permission-denied-when-calling-a-function-when-using-security-definer-even-after?utm_source=openai
- 4: https://stackoverflow.com/questions/40294881/how-to-revoke-execute-privileges-on-functions-in-postgresql-by-default?utm_source=openai
- 5: https://supabase.com/docs/guides/database/managing-passwords?utm_source=openai
Assert permission-denied semantics explicitly in auth-rejection tests.
Lines 365-366 and 392-393 only assert that error is truthy, which passes for any error (network failures, validation errors, etc.). To ensure these tests guard the migration intent, explicitly check for permission-denied semantics:
Proposed fix
- expect(error).toBeTruthy()
+ expect(error).toBeTruthy()
+ expect((error?.message ?? '').toLowerCase()).toContain('permission')Apply to both the anonymous client rejection test (line 365-366) and authenticated client rejection test (line 392-393).
This validates that the RLS policy correctly denies function execution with a "permission denied for function" error, rather than allowing the test to pass from unrelated failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/build_time_tracking.test.ts` around lines 365 - 366, Replace the weak
truthy assertions in the two auth-rejection tests with explicit
permission-denied checks: in the anonymous client rejection test and the
authenticated client rejection test in tests/build_time_tracking.test.ts, assert
that the caught error indicates permission denial (for example,
expect(error.message).toContain("permission denied for function") or
expect((error as any).code).toBe("permission-denied")), so the tests verify RLS
semantics rather than any generic failure.
…p-go/capgo into riderx/fix-rpc-build-log-auth
…p-go/capgo into riderx/fix-rpc-build-log-auth
…p-go/capgo into riderx/fix-rpc-build-log-auth
|
1 participant
Summary (AI generated)
tests/build_time_tracking.test.ts.it.concurrent()to align with the repo’s parallel test guidance.record_build_timeis rejected for authenticated callers, matching the permission posture enforced by the migration.Motivation (AI generated)
anonwas also vulnerable through authenticated sessions because permission changes revoke bothanonandauthenticated; adding dedicated regression coverage ensures both paths remain blocked and prevents future privilege regression.Business Impact (AI generated)
record_build_timefrom either unauthenticated or authenticated API-key clients.Test Plan (AI generated)
bun lintgit diff origin/main...HEADfor only the expected migration and backend test updatesbuild_time_tracking(not run in this pass)Screenshots
Checklist
bun run lint:backend && bun run lint.Generated with AI
Summary by CodeRabbit
Security
Tests