fix: transfer deploy history ownership with app#1946
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughA new migration introduces a Changes
Sequence DiagramsequenceDiagram
participant Client
participant TransferApp as transfer_app()
participant Auth as auth.uid()
participant RBAC as rbac_check_permission()
participant DB as Database
Client->>TransferApp: Call transfer_app(p_app_id, p_new_org_id)
TransferApp->>DB: Load app & transfer_history from public.apps
TransferApp->>Auth: Verify authenticated user
Auth-->>TransferApp: Return auth.uid() or NULL
alt User Not Authenticated
TransferApp-->>Client: Log TRANSFER_NO_AUTH & raise exception
end
TransferApp->>RBAC: Check source org permissions
RBAC-->>TransferApp: Permission result
alt Source Org Denied
TransferApp-->>Client: Log TRANSFER_OLD_ORG_RIGHTS & raise exception
end
TransferApp->>RBAC: Check destination org permissions
RBAC-->>TransferApp: Permission result
alt Destination Org Denied
TransferApp-->>Client: Log TRANSFER_NEW_ORG_RIGHTS & raise exception
end
TransferApp->>TransferApp: Validate 32-day minimum interval
alt Interval Violated
TransferApp-->>Client: Raise exception
end
TransferApp->>DB: Update public.apps with new owner_org & transfer_history
TransferApp->>DB: Propagate owner_org to app_versions
TransferApp->>DB: Propagate owner_org to app_versions_meta
TransferApp->>DB: Propagate owner_org to channel_devices
TransferApp->>DB: Propagate owner_org to channels
TransferApp->>DB: Propagate owner_org to deploy_history
TransferApp->>DB: Repair stale deploy_history records
DB-->>TransferApp: Operations complete
TransferApp-->>Client: Return success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
supabase/migrations/20260424090941_fix_transfer_app_deploy_history_owner_org.sql (1)
14-88: Qualifypg_catalogcalls inside the definer function.With
search_path = '', this function still relies on implicit resolution forarray_length,jsonb_build_object, andnow(). Prefixing those withpg_catalog.keeps the function aligned with the repo SQL rule.As per coding guidelines, "
**/*.sql: Set an empty search path (search_path = '') in every PostgreSQL function and use fully qualified names for all references`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260424090941_fix_transfer_app_deploy_history_owner_org.sql` around lines 14 - 88, The function uses unqualified built-ins (array_length, jsonb_build_object, now(), COALESCE, jsonb operators and casts) while the function sets search_path = '', so change those calls to their pg_catalog-qualified equivalents (e.g. pg_catalog.array_length, pg_catalog.jsonb_build_object, pg_catalog.now, pg_catalog.coalesce, and any pg_catalog::jsonb casts/operators as needed) in the SELECT that reads transfer_history into v_last_transfer and in the UPDATE that builds/concats transfer_history; ensure all built-in functions/operators referenced in the SELECT, IF checks, and UPDATE (including interval arithmetic) are fully qualified with pg_catalog to comply with the SQL rule.supabase/tests/24_test_data_functions.sql (1)
448-459: Assert on the seeded deploy row instead of the app-wide count.This
count(*) = 1check will become brittle as soon as another fixture adds adeploy_historyrow forcom.demoadmin.app, even if the transfer behavior is still correct. Key the assertion to the seeded channel/deploy row from Lines 397-427 instead of the whole app.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/24_test_data_functions.sql` around lines 448 - 459, The test currently asserts count(*) on public.deploy_history for app_id = 'com.demoadmin.app' and owner_org, which is brittle; change the assertion to check the seeded deploy row inserted earlier in this SQL (the specific deploy_history row used by the transfer_app test) by selecting that row's unique identifier (e.g., deploy_id or channel identifier used in the seed) and asserting its owner_org equals the destination org ('34a8c55d-2d0f-4652-a43f-684c7a9403ac'), instead of asserting a global count; update the SELECT/is assertion to target deploy_history where app_id = 'com.demoadmin.app' AND the seeded deploy's unique key (deploy_id or channel) to validate ownership moved.
🤖 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/20260424090941_fix_transfer_app_deploy_history_owner_org.sql`:
- Around line 14-17: The SELECT that reads transfer_history into v_old_org_id
and v_last_transfer must lock the apps row to prevent concurrent reads passing
the cooldown check; change the query that selects owner_org,
transfer_history[array_length(...)] FROM public.apps WHERE app_id = p_app_id to
use a FOR UPDATE (or FOR NO KEY UPDATE) lock so the row is locked until the
transaction commits, and apply the same change to the analogous SELECT in the
later block (the logic around p_app_id, transfer_history, and the 32-day
cooldown in lines 69-89) so concurrent calls cannot observe the same
transfer_history tail.
- Around line 19-21: The procedure currently only checks for NULL v_old_org_id
and proceeds to write a transfer history even when p_new_org_id equals the
current owner; add an early guard that compares p_new_org_id to v_old_org_id (or
the retrieved current owner) and aborts (RAISE EXCEPTION or RETURN) before any
HISTORY insert or cooldown update runs so same-org "transfers" do not create a
history record or start the 32-day cooldown; update the logic around the
existing NULL check and the block that performs the HISTORY insert (the transfer
history/cooldown code referenced in the region handling v_old_org_id and the
subsequent insert/update between lines ~79-89) to enforce this equality check.
- Around line 114-124: The migration must explicitly set function ACLs for
public.transfer_app(p_app_id character varying, p_new_org_id uuid): after
setting OWNER use a deny-by-default approach by revoking all privileges from
PUBLIC (REVOKE ALL ON FUNCTION public.transfer_app(...) FROM PUBLIC) and then
grant only EXECUTE to the minimal set of caller roles that should be able to
invoke it (GRANT EXECUTE ON FUNCTION public.transfer_app(...) TO
<intended_roles>), replacing <intended_roles> with the actual service/auth roles
used by your RPC callers.
---
Nitpick comments:
In
`@supabase/migrations/20260424090941_fix_transfer_app_deploy_history_owner_org.sql`:
- Around line 14-88: The function uses unqualified built-ins (array_length,
jsonb_build_object, now(), COALESCE, jsonb operators and casts) while the
function sets search_path = '', so change those calls to their
pg_catalog-qualified equivalents (e.g. pg_catalog.array_length,
pg_catalog.jsonb_build_object, pg_catalog.now, pg_catalog.coalesce, and any
pg_catalog::jsonb casts/operators as needed) in the SELECT that reads
transfer_history into v_last_transfer and in the UPDATE that builds/concats
transfer_history; ensure all built-in functions/operators referenced in the
SELECT, IF checks, and UPDATE (including interval arithmetic) are fully
qualified with pg_catalog to comply with the SQL rule.
In `@supabase/tests/24_test_data_functions.sql`:
- Around line 448-459: The test currently asserts count(*) on
public.deploy_history for app_id = 'com.demoadmin.app' and owner_org, which is
brittle; change the assertion to check the seeded deploy row inserted earlier in
this SQL (the specific deploy_history row used by the transfer_app test) by
selecting that row's unique identifier (e.g., deploy_id or channel identifier
used in the seed) and asserting its owner_org equals the destination org
('34a8c55d-2d0f-4652-a43f-684c7a9403ac'), instead of asserting a global count;
update the SELECT/is assertion to target deploy_history where app_id =
'com.demoadmin.app' AND the seeded deploy's unique key (deploy_id or channel) to
validate ownership moved.
🪄 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: 241563a8-923c-4799-a691-d1eec330bd0c
📒 Files selected for processing (2)
supabase/migrations/20260424090941_fix_transfer_app_deploy_history_owner_org.sqlsupabase/tests/24_test_data_functions.sql
|
This was referenced Apr 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary (AI generated)
public.transfer_app(...)to movedeploy_history.owner_orgduring app transferstransfer_appfunction ACLs in the migration and tighten the pgTAP assertion to the seeded deploy rowdeploy_history.owner_orgvalues to match current app ownershipMotivation (AI generated)
App transfers were leaving historical deployment rows attached to the source organization. Because deploy history read access is authorized from the row's stored
owner_org, old orgs could retain access and destination orgs could lose access after a transfer. The follow-up review also identified concurrency and no-op transfer edge cases that could leave the cooldown logic inconsistent.Business Impact (AI generated)
This closes a cross-org authorization gap on a sensitive ownership flow, preserves correct deployment audit history after transfers, and reduces the risk of support incidents or trust issues around org-boundary isolation.
Test Plan (AI generated)
bun lintbunx supabase db query --workdir .context/supabase-worktrees/aec510ca --local "SELECT pg_get_functiondef('public.transfer_app(character varying, uuid)'::regprocedure) LIKE '%UPDATE public.deploy_history%' AS has_deploy_history_update;"bunx supabase test db .context/supabase-worktrees/aec510ca/supabase/tests/00-supabase_test_helpers.sql supabase/tests/24_test_data_functions.sql --workdir .context/supabase-worktrees/aec510ca --localChecklist (AI generated)
Generated with AI