[codex] restore transfer_app guard flag and CI stability#1960
Conversation
📝 WalkthroughWalkthroughAdds a new database migration that creates Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client (authenticated user)
participant DB as PostgreSQL (public.transfer_app)
participant RBAC as RBAC check
participant Tables as Related Tables (apps, app_versions, channels, deploy_history)
Note over Client,DB: Call transfer_app(p_app_id, p_new_org_id)
Client->>DB: invoke transfer_app
DB->>DB: LOCK target app row
DB->>RBAC: check permissions for current owner_org (source)
RBAC-->>DB: allow/deny
DB->>RBAC: check permissions for p_new_org_id (destination)
RBAC-->>DB: allow/deny
DB->>DB: read latest transfer_history -> enforce 32-day cooldown if present
alt validations pass
DB->>DB: set_config('capgo.allow_owner_org_transfer', 'true')
DB->>Tables: UPDATE apps.owner_org, updated_at, append transfer_history entry
DB->>Tables: UPDATE app_versions, app_versions_meta, channel_devices, channels, deploy_history owner_org
DB-->>Client: success (void)
else validation fails
DB-->>Client: raise exception
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.1.0)supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: 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
🧹 Nitpick comments (2)
supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sql (2)
28-82: Consider extracting duplicate JSON keys to constants.SonarCloud flags that
'app_id','new_org_id', and'old_org_id'are duplicated across multiplejsonb_build_objectcalls. While functional, extracting these toCONSTANTdeclarations would improve maintainability.♻️ Proposed refactor to reduce duplication
DECLARE v_old_org_id uuid; v_user_id uuid; v_last_transfer jsonb; v_last_transfer_date timestamp; v_transfer_request_error constant text := 'Unable to process transfer request.'; + k_app_id constant text := 'app_id'; + k_new_org_id constant text := 'new_org_id'; + k_old_org_id constant text := 'old_org_id'; + k_uid constant text := 'uid'; BEGINThen use
k_app_id,k_new_org_id, etc. injsonb_build_objectcalls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sql` around lines 28 - 82, The repeated literal JSON keys ('app_id', 'new_org_id', 'old_org_id', 'uid') used in multiple pg_log calls should be extracted into constants to reduce duplication and ease future changes; add constant declarations (e.g., k_app_id, k_new_org_id, k_old_org_id, k_uid) near the top of the function or script and replace the string literals in every pg_catalog.jsonb_build_object call inside the public.pg_log invocations (references: public.pg_log, pg_catalog.jsonb_build_object, and the error-check blocks using v_old_org_id, p_new_org_id, p_app_id, v_user_id) so each call uses the new constants instead of repeated literal keys.
94-112: Consider logging successful transfers for audit completeness.The function logs denial cases via
public.pg_logbut doesn't log successful transfers. Whiletransfer_historyprovides a record, an explicit audit log entry would provide consistency with the denial logging pattern.📋 Proposed addition for success logging
Add after the final UPDATE (before
END;):PERFORM public.pg_log( 'success: TRANSFER_COMPLETED', pg_catalog.jsonb_build_object( 'app_id', p_app_id, 'old_org_id', v_old_org_id, 'new_org_id', p_new_org_id, 'uid', v_user_id ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sql` around lines 94 - 112, The function logs transfer denials but not successful transfers; add an explicit audit log call after the UPDATE that records the success so it matches the denial pattern: call public.pg_log with a success tag (e.g. 'success: TRANSFER_COMPLETED') and a jsonb payload containing p_app_id, v_old_org_id, p_new_org_id and v_user_id; insert this PERFORM public.pg_log(...) immediately after the UPDATE that modifies owner_org/transfer_history so successful transfers are auditable alongside existing denial logs.
🤖 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/20260427092630_restore_transfer_app_guard_flag.sql`:
- Around line 137-140: The SECURITY DEFINER function transfer_app needs explicit
deny-by-default permissions: after setting OWNER (transfer_app), add a REVOKE
ALL ON FUNCTION public.transfer_app(character varying, uuid) FROM PUBLIC; and
then GRANT EXECUTE ON FUNCTION public.transfer_app(character varying, uuid) TO
the minimal role (e.g., authenticated) so only required roles can call
transfer_app; ensure the function signature in the REVOKE/GRANT matches the
existing transfer_app(p_app_id character varying, p_new_org_id uuid)
declaration.
---
Nitpick comments:
In `@supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sql`:
- Around line 28-82: The repeated literal JSON keys ('app_id', 'new_org_id',
'old_org_id', 'uid') used in multiple pg_log calls should be extracted into
constants to reduce duplication and ease future changes; add constant
declarations (e.g., k_app_id, k_new_org_id, k_old_org_id, k_uid) near the top of
the function or script and replace the string literals in every
pg_catalog.jsonb_build_object call inside the public.pg_log invocations
(references: public.pg_log, pg_catalog.jsonb_build_object, and the error-check
blocks using v_old_org_id, p_new_org_id, p_app_id, v_user_id) so each call uses
the new constants instead of repeated literal keys.
- Around line 94-112: The function logs transfer denials but not successful
transfers; add an explicit audit log call after the UPDATE that records the
success so it matches the denial pattern: call public.pg_log with a success tag
(e.g. 'success: TRANSFER_COMPLETED') and a jsonb payload containing p_app_id,
v_old_org_id, p_new_org_id and v_user_id; insert this PERFORM public.pg_log(...)
immediately after the UPDATE that modifies owner_org/transfer_history so
successful transfers are auditable alongside existing denial logs.
🪄 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: 021dd919-1609-46c4-9f40-ff9cdb09cefa
📒 Files selected for processing (2)
supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sqltests/email-preferences.test.ts
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/20260427092630_restore_transfer_app_guard_flag.sql`:
- Around line 8-13: Create a new migration (run bunx supabase migration new
<feature_slug>) that updates public.transfer_app() to save the previous value of
the session setting before enabling the bypass and restore it before any
return/exit: fetch the current value with
current_setting('capgo.allow_owner_org_transfer', true) into a local variable
(e.g., v_prev_allow_transfer), call
set_config('capgo.allow_owner_org_transfer','true', true) to enable the bypass,
and ensure you call set_config('capgo.allow_owner_org_transfer',
v_prev_allow_transfer, true) to restore the original value right before the
function returns and also in any EXCEPTION blocks so
public.guard_owner_org_reassignment() cannot see the bypass after transfer_app()
completes.
🪄 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: b6132939-7e13-4f9e-8e93-4479c5a6255e
📒 Files selected for processing (1)
supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sql
| DECLARE | ||
| v_old_org_id uuid; | ||
| v_user_id uuid; | ||
| v_last_transfer jsonb; | ||
| v_last_transfer_date timestamp; | ||
| v_transfer_request_error constant text := 'Unable to process transfer request.'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PostgreSQL, does set_config('capgo.allow_owner_org_transfer', 'true', true) remain active for the rest of the current transaction after a PL/pgSQL function returns, and would a later trigger calling current_setting('capgo.allow_owner_org_transfer', true) still see 'true'?
💡 Result:
Yes, the setting set by set_config('capgo.allow_owner_org_transfer', 'true', true) inside a PL/pgSQL function remains active for the rest of the current transaction after the function returns, and a later trigger in the same transaction calling current_setting('capgo.allow_owner_org_transfer', true) will see the value 'true', provided the set_config call is not within a subtransaction (like a BEGIN...EXCEPTION block) that rolls back or ends before the function returns.
Citations:
- 1: https://www.postgresql.org/docs/current/interactive/sql-set.html
- 2: http://www.postgresql.org/docs/current/interactive/functions-admin.html
- 3: https://postgresql.org/docs/current/functions-admin.html
- 4: https://stackoverflow.com/questions/71242103/why-does-postgres-set-configs-is-local-true-not-persist-the-variable-for-the
🏁 Script executed:
# First, check the current state of the migration file
cat -n supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sql | head -150Repository: Cap-go/capgo
Length of output: 5159
🏁 Script executed:
# Check git status to see if file is uncommitted
git status supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sqlRepository: Cap-go/capgo
Length of output: 122
Create a new migration to restore the guard flag before returning.
The set_config('capgo.allow_owner_org_transfer', 'true', true) call at line 95 remains active for the rest of the current transaction after public.transfer_app() returns. This leaves the bypass open for any raw owner_org update in the same transaction after the function completes, allowing public.guard_owner_org_reassignment() to still see the flag as enabled.
Per the migration guidelines, create a new migration file using bunx supabase migration new <feature_slug> to fix this by saving the previous setting value before enabling the bypass and restoring it before the function returns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260427092630_restore_transfer_app_guard_flag.sql`
around lines 8 - 13, Create a new migration (run bunx supabase migration new
<feature_slug>) that updates public.transfer_app() to save the previous value of
the session setting before enabling the bypass and restore it before any
return/exit: fetch the current value with
current_setting('capgo.allow_owner_org_transfer', true) into a local variable
(e.g., v_prev_allow_transfer), call
set_config('capgo.allow_owner_org_transfer','true', true) to enable the bypass,
and ensure you call set_config('capgo.allow_owner_org_transfer',
v_prev_allow_transfer, true) to restore the original value right before the
function returns and also in any EXCEPTION blocks so
public.guard_owner_org_reassignment() cannot see the bypass after transfer_app()
completes.
|
No longer needed. The TUS HEAD fix PR has been retargeted directly to main so the security fix stays isolated. |
1 participant
Summary (AI generated)
capgo.allow_owner_org_transferguard flag insidepublic.transfer_appMotivation (AI generated)
Current
mainfailssupabase test dbbecausepublic.transfer_appupdatesowner_orgwithout setting the bypass flag expected by the new reassignment guard trigger. After that database issue is fixed, the Cloudflare email-preferences suite can still fail from a CI-only setup timeout, so the prerequisite branch also carries the minimal timeout adjustment needed for stable checks.Business Impact (AI generated)
This restores green CI on the current base branch and unblocks focused application fixes from shipping without mixing unrelated changes into their PRs.
Test Plan (AI generated)
mainfail inRun Supabase Test DBGenerated with AI
Summary by CodeRabbit
New Features
Tests