Harden upsert_version_meta RPC against abuse#1760
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a SECURITY DEFINER PostgreSQL function Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RPC as upsert_version_meta()
participant Auth as Auth/RoleCheck
participant DB as DB (app, app_versions, version_meta)
Client->>RPC: CALL(p_app_id, p_version_id, p_size)
RPC->>Auth: verify caller identity & service_role/postgres exemption
Auth-->>RPC: allow / deny
alt allowed
RPC->>DB: SELECT app, app_versions, owner_org
DB-->>RPC: app/version found / not found
alt app/version found
RPC->>DB: check existing version_meta row (app_id, version_id, sign(size))
DB-->>RPC: row exists / not exists
alt not exists
RPC->>DB: INSERT INTO version_meta (app_id, version_id, size)
DB-->>RPC: inserted / unique_violation => error
alt inserted
RPC-->>Client: returns true
else unique_violation
RPC-->>Client: returns false
end
else exists
RPC-->>Client: returns false
end
else not found
RPC-->>Client: returns false
end
else denied
RPC-->>Client: raise permission error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 740755153b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| FROM public.app_versions | ||
| WHERE public.app_versions.app_id = p_app_id | ||
| AND public.app_versions.id = p_version_id | ||
| AND public.app_versions.deleted = false |
There was a problem hiding this comment.
The new public.app_versions.deleted = false predicate prevents the delete path from recording compensating negative metadata entries: deleteIt in supabase/functions/_backend/triggers/on_version_update.ts calls createStatsMeta(..., -data.size) after a soft delete, when the target row is already marked deleted. With this filter, upsert_version_meta returns false and skips the insert, so version_meta no longer captures bundle removals and storage/billing aggregates that depend on it remain overstated.
Useful? React with 👍 / 👎.
riderx
force-pushed
the
riderx/fix-version-meta-rpc
branch
from
9398519 to
e9bf46a
Compare
![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
🧹 Nitpick comments (1)
tests/upsert-version-meta-rpc.test.ts (1)
55-117: Useit.concurrent()for this test file.These cases use
it(...); the repo test guideline expectsit.concurrent(...)in*.test.tsfiles to maximize in-file parallelism.🔧 Suggested fix
- it('must reject unauthenticated (anon) execution', async () => { + it.concurrent('must reject unauthenticated (anon) execution', async () => { @@ - it('returns false for unknown app ids', async () => { + it.concurrent('returns false for unknown app ids', async () => { @@ - it('returns false for unknown version ids', async () => { + it.concurrent('returns false for unknown version ids', async () => { @@ - it('upserts only once per app/version/size sign', async () => { + it.concurrent('upserts only once per app/version/size sign', async () => {As per coding guidelines: “ALL TEST FILES RUN IN PARALLEL; use
it.concurrent()instead ofit()to run tests in parallel within the same file.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/upsert-version-meta-rpc.test.ts` around lines 55 - 117, Replace all synchronous Jest test declarations in this file with concurrent variants: change each it('...') block to it.concurrent('...') so tests run in-file in parallel; specifically update the four test cases that call anonSupabase.rpc and serviceRoleSupabase.rpc (the tests named "must reject unauthenticated (anon) execution", "returns false for unknown app ids", "returns false for unknown version ids", and "upserts only once per app/version/size sign") to use it.concurrent while keeping their bodies and assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260312202239_harden_upsert_version_meta_authz.sql`:
- Around line 33-37: The code checks app ownership into v_owner_org for p_app_id
but later inserts metadata for p_version_id without verifying that p_version_id
belongs to the same app; update the function to first SELECT owner_org (or
app_id) FROM public.versions WHERE version_id = p_version_id INTO a local (e.g.,
v_version_owner_org or v_version_app_id) and compare it to v_owner_org (or
p_app_id), and if they don’t match return FALSE (or raise a controlled notice
and return FALSE) before executing the INSERT that currently appears around
lines 83-88; ensure you reference p_app_id, p_version_id, v_owner_org and the
INSERT into version metadata when adding this guard.
In `@tests/upsert-version-meta-rpc.test.ts`:
- Line 18: The tests currently hardcode VERSION_ID and insert into app_versions
without capturing the created ID, causing flaky assertions; change the setup to
capture the inserted row's id (e.g., use the insert call that returns the
inserted id or query the created row) and assign it to a local variable (replace
VERSION_ID usage) so subsequent RPC calls and assertions use that actual id;
update each occurrence that uses the hardcoded VERSION_ID (the insert block and
later assertions/RPC calls around the references in the test file) to reference
the captured id variable instead.
---
Nitpick comments:
In `@tests/upsert-version-meta-rpc.test.ts`:
- Around line 55-117: Replace all synchronous Jest test declarations in this
file with concurrent variants: change each it('...') block to
it.concurrent('...') so tests run in-file in parallel; specifically update the
four test cases that call anonSupabase.rpc and serviceRoleSupabase.rpc (the
tests named "must reject unauthenticated (anon) execution", "returns false for
unknown app ids", "returns false for unknown version ids", and "upserts only
once per app/version/size sign") to use it.concurrent while keeping their bodies
and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 168b1f42-915e-4df7-9671-fcb5fdc47617
📒 Files selected for processing (2)
supabase/migrations/20260312202239_harden_upsert_version_meta_authz.sqltests/upsert-version-meta-rpc.test.ts
riderx
force-pushed
the
riderx/fix-version-meta-rpc
branch
from
d66f989 to
8d52d3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260313130044_harden_upsert_version_meta_authz.sql`:
- Around line 30-31: The check using "p_size = 0" misses NULL and lets the
sign-based dedup logic skip both branches, leaving v_existing_count NULL and
causing an unintended insert; change the guard to explicitly reject NULLs (e.g.
check "p_size IS NULL OR p_size = 0") before the sign-based branches so the
function returns FALSE for NULL/zero sizes, and apply the same NULL-aware change
to the other identical size-check later in the function that controls the
sign-based dedup path (ensure checks reference the parameter p_size and preserve
the existing control flow around v_existing_count).
- Around line 2-7: The REVOKE statement targeting the function
upsert_version_meta currently uses the identifiers "public" and public which
refer to a regular role named public; replace both occurrences with the
PostgreSQL pseudo-role PUBLIC (unquoted, uppercase) so the statement reads
REVOKE ... FROM PUBLIC, "anon", "authenticated" to ensure it revokes from the
built-in all-roles pseudo-role rather than a literal role named public.
In `@supabase/tests/28_test_new_migration_functions.sql`:
- Around line 6-8: Add explicit assertions that the anon and authenticated roles
do NOT have execute rights on the migration function to prevent grant
regressions: after invoking tests.authenticate_as_service_role() and any
service-role checks, add has_function_privilege checks for both 'anon' and
'authenticated' against the target function (e.g.,
has_function_privilege('anon', 'public.upsert_version_meta()') = false and
has_function_privilege('authenticated', 'public.upsert_version_meta()') = false)
and fail the test if either returns true so the test covers revoked-grants
scenarios as well.
In `@tests/upsert-version-meta-rpc.test.ts`:
- Around line 86-95: The test currently only checks a non-existent p_version_id
(999999999) and should also assert the RPC rejects a real version id that
belongs to a different app; update or add a case that calls
serviceRoleSupabase.rpc('upsert_version_meta', { p_app_id: APP_ID, p_version_id:
<real_version_id_for_other_app>, p_size: 123456 }) where
<real_version_id_for_other_app> is obtained from a fixture or by inserting a
version tied to another app (e.g., create/query a version record for
OTHER_APP_ID), and assert expect(error).toBeNull() and expect(data).toBe(false)
so ownership checks are covered.
- Around line 63-125: Don't convert the "upserts only once per app/version/size
sign" test to it.concurrent(): this test mutates shared state in the
version_meta table using the shared APP_ID and versionId so it must remain
sequential or be fully isolated. Either keep the test as a normal it(...) and do
not change it to it.concurrent(), or refactor the test to create and use its own
dedicated app/version identifiers (e.g., generate new values with randomUUID and
create the corresponding app/version seed) and then call
serviceRoleSupabase.rpc('upsert_version_meta', ...) and verify via
serviceRoleSupabase.from('version_meta').select(...) against those dedicated IDs
so the test can safely run concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfeda966-57c5-44cc-8ded-a6c5c970689d
📒 Files selected for processing (3)
supabase/migrations/20260313130044_harden_upsert_version_meta_authz.sqlsupabase/tests/28_test_new_migration_functions.sqltests/upsert-version-meta-rpc.test.ts
| REVOKE ALL ON FUNCTION "public"."upsert_version_meta"("p_app_id" character varying, "p_version_id" bigint, "p_size" bigint) | ||
| FROM | ||
| "public", | ||
| public, | ||
| "anon", | ||
| "authenticated"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PostgreSQL GRANT/REVOKE syntax, does FROM "public"refer to the specialPUBLICpseudo-role, or to a separately named rolepublic?
💡 Result:
In GRANT/REVOKE, the special grantee PUBLIC (unquoted) refers to PostgreSQL’s built-in “pseudo-role” that means every role. (postgresql.org)
But "public" is a delimited identifier, so it is treated as an ordinary role name (case-sensitive) — i.e., it means a separately named role literally called public, not the PUBLIC pseudo-role. (postgresql.org)
Citations:
- 1: https://www.postgresql.org/docs/current/ddl-priv.html
- 2: https://www.postgresql.org/docs/current/sql-syntax-lexical.html?utm_source=openai
🏁 Script executed:
cd /tmp && find . -name "20260313130044_harden_upsert_version_meta_authz.sql" 2>/dev/null || echo "File not in /tmp"
# Try to find the file in common locations
fd "20260313130044_harden_upsert_version_meta_authz.sql" 2>/dev/nullRepository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Search for the migration file
fd "20260313130044" --type fRepository: Cap-go/capgo
Length of output: 127
🏁 Script executed:
# List supabase migrations to find the file
ls -la supabase/migrations/ 2>/dev/null | head -20Repository: Cap-go/capgo
Length of output: 1695
🏁 Script executed:
cat -n supabase/migrations/20260313130044_harden_upsert_version_meta_authz.sqlRepository: Cap-go/capgo
Length of output: 3577
Replace "public" and public with the PUBLIC pseudo-role in the REVOKE statement.
In PostgreSQL GRANT/REVOKE syntax, PUBLIC (unquoted, uppercase) is the built-in pseudo-role meaning "all roles". The identifiers "public" and public refer to a regular role literally named public. Unless your cluster defines such a role, this migration will fail before the function can be hardened.
Proposed fix
REVOKE ALL ON FUNCTION "public"."upsert_version_meta"("p_app_id" character varying, "p_version_id" bigint, "p_size" bigint)
FROM
- "public",
- public,
+ PUBLIC,
"anon",
"authenticated";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260313130044_harden_upsert_version_meta_authz.sql`
around lines 2 - 7, The REVOKE statement targeting the function
upsert_version_meta currently uses the identifiers "public" and public which
refer to a regular role named public; replace both occurrences with the
PostgreSQL pseudo-role PUBLIC (unquoted, uppercase) so the statement reads
REVOKE ... FROM PUBLIC, "anon", "authenticated" to ensure it revokes from the
built-in all-roles pseudo-role rather than a literal role named public.
| IF p_size = 0 THEN | ||
| RETURN FALSE; |
There was a problem hiding this comment.
Reject NULL sizes before the sign-based dedup logic.
p_size = 0 does not catch NULL. In that case neither sign branch runs, v_existing_count stays null, and the function falls through to the insert path instead of returning FALSE.
🛡️ Proposed fix
- IF p_size = 0 THEN
+ IF p_size IS NULL OR p_size = 0 THEN
RETURN FALSE;
END IF;Also applies to: 77-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260313130044_harden_upsert_version_meta_authz.sql`
around lines 30 - 31, The check using "p_size = 0" misses NULL and lets the
sign-based dedup logic skip both branches, leaving v_existing_count NULL and
causing an unintended insert; change the guard to explicitly reject NULLs (e.g.
check "p_size IS NULL OR p_size = 0") before the sign-based branches so the
function returns FALSE for NULL/zero sizes, and apply the same NULL-aware change
to the other identical size-check later in the function that controls the
sign-based dedup path (ensure checks reference the parameter p_size and preserve
the existing control flow around v_existing_count).
| -- Test upsert_version_meta function | ||
| SELECT tests.authenticate_as_service_role(); | ||
|
|
There was a problem hiding this comment.
Assert the revoked grants directly in this migration test.
This setup only exercises the service-role happy path. Please add has_function_privilege(...)=false checks for both anon and authenticated; otherwise a grant regression can slip through while these tests still pass.
🧪 Suggested test additions
-SELECT plan(30);
+SELECT plan(32);
-- Test upsert_version_meta function
+SELECT
+ is(
+ has_function_privilege(
+ 'anon'::name,
+ 'public.upsert_version_meta(character varying, bigint, bigint)'::regprocedure,
+ 'EXECUTE'
+ ),
+ false,
+ 'anon role has no execute privilege on upsert_version_meta'
+ );
+
+SELECT
+ is(
+ has_function_privilege(
+ 'authenticated'::name,
+ 'public.upsert_version_meta(character varying, bigint, bigint)'::regprocedure,
+ 'EXECUTE'
+ ),
+ false,
+ 'authenticated role has no execute privilege on upsert_version_meta'
+ );
+
SELECT tests.authenticate_as_service_role();📝 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.
| -- Test upsert_version_meta function | |
| SELECT tests.authenticate_as_service_role(); | |
| -- Test upsert_version_meta function | |
| SELECT | |
| is( | |
| has_function_privilege( | |
| 'anon'::name, | |
| 'public.upsert_version_meta(character varying, bigint, bigint)'::regprocedure, | |
| 'EXECUTE' | |
| ), | |
| false, | |
| 'anon role has no execute privilege on upsert_version_meta' | |
| ); | |
| SELECT | |
| is( | |
| has_function_privilege( | |
| 'authenticated'::name, | |
| 'public.upsert_version_meta(character varying, bigint, bigint)'::regprocedure, | |
| 'EXECUTE' | |
| ), | |
| false, | |
| 'authenticated role has no execute privilege on upsert_version_meta' | |
| ); | |
| SELECT tests.authenticate_as_service_role(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/tests/28_test_new_migration_functions.sql` around lines 6 - 8, Add
explicit assertions that the anon and authenticated roles do NOT have execute
rights on the migration function to prevent grant regressions: after invoking
tests.authenticate_as_service_role() and any service-role checks, add
has_function_privilege checks for both 'anon' and 'authenticated' against the
target function (e.g., has_function_privilege('anon',
'public.upsert_version_meta()') = false and
has_function_privilege('authenticated', 'public.upsert_version_meta()') = false)
and fail the test if either returns true so the test covers revoked-grants
scenarios as well.
| it('returns false for unknown version ids', async () => { | ||
| const { data, error } = await serviceRoleSupabase.rpc('upsert_version_meta', { | ||
| p_app_id: APP_ID, | ||
| p_version_id: 999999999, | ||
| p_size: 123456, | ||
| }) | ||
|
|
||
| expect(error).toBeNull() | ||
| expect(data).toBe(false) | ||
| }) |
There was a problem hiding this comment.
Add a regression case for a real version_id from another app.
999999999 only proves the version is missing. It does not protect the security fix that rejects a valid version_id belonging to a different app, so a future ownership-query regression could still pass this suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/upsert-version-meta-rpc.test.ts` around lines 86 - 95, The test
currently only checks a non-existent p_version_id (999999999) and should also
assert the RPC rejects a real version id that belongs to a different app; update
or add a case that calls serviceRoleSupabase.rpc('upsert_version_meta', {
p_app_id: APP_ID, p_version_id: <real_version_id_for_other_app>, p_size: 123456
}) where <real_version_id_for_other_app> is obtained from a fixture or by
inserting a version tied to another app (e.g., create/query a version record for
OTHER_APP_ID), and assert expect(error).toBeNull() and expect(data).toBe(false)
so ownership checks are covered.
|
1 participant
Summary (AI generated)
public.upsert_version_metaRPC execution toservice_roleonly.get_identity_org_appidandcheck_min_rights, while preserving existing dedupe behavior.Test plan (AI generated)
bun lint:backend.POST /rest/v1/rpc/upsert_version_metato confirm inserts are rejected.Screenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.Summary by CodeRabbit
Tests
Chores