Uh oh!
There was an error while loading. Please reload this page.
fix(cli): restore postgres after role reverts (CLI-2205) - #6246
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@802bece9ff819ad3c02181f842ffe3337ef78f92Preview package for commit |
avallete
left a comment
There was a problem hiding this comment.
Thank's for looking into this !
One issue raised by Claude on this one, dug a bit:
The end-of-file restoreRoleSql correctly fixes the filed bug: after RESET ROLE, the history insert no longer dies with 42501 and later files start as postgres. Password / local / non-stepped-down --db-url streams stay byte-identical. That part is good.
An issue is residual in go-cli-divergences.md as the product behavior.
Statements betweenRESET ROLE and the end of the same file still run as the login role. For the class of migrations in #6236 (SET ROLE / RESET ROLE / REVOKE … GRANTED BY current_user), this PR changes the failure mode the wrong way:
- before: fail-closed —
42501, migration not recorded, retryable - after: succeed, history row written, grant leftover, stderr
WARN:that CI will ignore
The pooler rationale does not apply here. You do not need a connection-time role=postgres default. restoreRoleSql is already SET SESSION ROLE postgres. Inject it after each top-levelRESET ROLE / SET ROLE NONE / SET SESSION ROLE NONE in the same batch/sequence, keep recording the original statements, and keep the trailing restore as a backstop. At statement: N should skip those internal ops the same way the trailing restore already does.
I reproduced this on a real passwordless db push --linked against staging (Initialising login role…, login role cli_login_postgres, NOINHERIT):
createtableif not exists public.dogfood_probe (
at textprimary key,
current_user_name textnot null,
session_user_name textnot null
);
insert intopublic.dogfood_probevalues ('start', current_user, session_user);
create role repro_writer nologin;
grant repro_writer to postgres;
grant usage on schema public to repro_writer;
grant insert onpublic.dogfood_probe to repro_writer;
set role repro_writer;
insert intopublic.dogfood_probevalues ('as_writer', current_user, session_user);
reset role;
revoke repro_writer from postgres granted by current_user;Result: WARN: statements after RESET ROLE…, exit 0, schema_migrations recorded dogfood_reset_role. Probe at start was current_user=postgres / session_user=cli_login_postgres. After the push, pg_auth_members still had repro_writer → postgresgrantor postgres. The same REVOKE … GRANTED BY current_user on the password path (current_user actually postgres, no login-role init) dropped that row.
The warn can stay as defense-in-depth for dynamic SQL / siblings the detector misses. It should not be the answer for a static RESET ROLE already in the statement list.
Request: mid-file restore, then this is good to land.
Uh oh!
There was an error while loading. Please reload this page.
TL;DR
Passwordless
db push --linkedbreaks on any migration containingreset role:the login role path relies on a session level
SET SESSION ROLE postgresthat the migration itself undoes.File runners now re-assert the step-down immediately after each role-reverting statement, at the end of each file, and before every CLI owned ledger write, so the whole file behaves the same on both auth paths.
whats biting?
The passwordless path connects as a temp
cli_login_*role and steps down topostgresonce at connect. A migration'sreset rolereverts the session to the login role, so:permission denied for schema supabase_migrations (SQLSTATE 42501)and rolls the migration back, even though every user statement succeededreset roleand the end of the same file run as the login role, sogranted by current_usercleanup silently no-ops while the push exits 0 (reproduced on staging: the stalepg_auth_membersgrant survives)seed_filesupsert run as the login role toofixed now by:
LegacyDbSession.restoreRoleSql(set only when the step-down ran) is injected by every file runner right after each top-level role revert (RESET ROLE,SET [SESSION] ROLE [TO|=] NONE|DEFAULTincluding a case-sensitively quoted'none',RESET SESSION AUTHORIZATION,SET SESSION AUTHORIZATION DEFAULT,DISCARD ALL), and again at end of file and before the history insert and bothseed_filesupserts, socurrent_usermatches a password session for the whole fileAt statement: Nand are never recorded in the history row; deliberateset role <x>choreography is untouched, and password, local and plain--db-urlsessions see a byte identical statement streamSET LOCAL ROLE NONE,session_useritself) is documented indocs/go-cli-divergences.mdwith the end-of-file restore protecting every CLI owned write; bothSIDE_EFFECTS.mdtables record the new statementsref:
reset rolefails the ledger INSERT with 42501 and silently no-opsgranted by current_usercleanup #6236