Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong ownership for some built-in event triggers #1437

Open
steve-chavez opened this issue Feb 7, 2025 · 6 comments · Fixed by #1489
Open

Wrong ownership for some built-in event triggers #1437

steve-chavez opened this issue Feb 7, 2025 · 6 comments · Fixed by #1489
Labels
bug Something isn't working

Comments

@steve-chavez
Copy link
Member

steve-chavez commented Feb 7, 2025

Problem

See the functions and event trigger owned by postgres:

select proname, proowner::regrole from pg_proc where prorettype = 'event_trigger'::regtype;
proname proowner
event_trigger_in supabase_admin
pgrst_drop_watch supabase_admin
grant_pg_graphql_access supabase_admin
trg_mask_update supabase_admin
pgrst_ddl_watch supabase_admin
grant_pg_net_access postgres
set_graphql_placeholder supabase_admin
increment_schema_version supabase_admin
grant_pg_cron_access postgres
select evtname, evtowner::regrole, evtfoid::regproc from pg_event_trigger;
evtname evtowner evtfoid
issue_pg_net_access postgres grant_pg_net_access
issue_pg_graphql_access supabase_admin grant_pg_graphql_access
issue_graphql_placeholder supabase_admin set_graphql_placeholder
pgrst_ddl_watch supabase_admin pgrst_ddl_watch
pgrst_drop_watch supabase_admin pgrst_drop_watch
pgsodium_trg_mask_update supabase_admin pgsodium.trg_mask_update
graphql_watch_ddl supabase_admin graphql.increment_schema_version
graphql_watch_drop supabase_admin graphql.increment_schema_version
issue_pg_cron_access supabase_admin grant_pg_cron_access

This means that any user can DROP those and cause services to malfunction:

drop function grant_pg_cron_access cascade;
drop function grant_pg_net_access cascade;

Both DROPs above work.

@steve-chavez steve-chavez added the bug Something isn't working label Feb 7, 2025
@steve-chavez
Copy link
Member Author

Furthermore, it looks like it's possible to DROP any evtrig by doing CASCADE:

-- this works
drop function grant_pg_graphql_access cascade;

This despite being owned by the supabase_admin.

@soedirgo
Copy link
Member

So resolution is to reassign the owner for BOTH the event trigger and the event trigger function to supabase_admin (CMIIW)

@steve-chavez
Copy link
Member Author

@soedirgo Yes, correct. They all should be owned by supabase_admin.

I don't know if these evtrigs are created inside the dashboard somehow to have the postgres user, but if they are, it would be good to put them in some file under supabase/postgres instead. (any SQL inside dashboard code is too hard to find).

@soedirgo
Copy link
Member

I think it's owned by postgres because of an erratic migrations setup - will look into this tmr

@steve-chavez
Copy link
Member Author

steve-chavez commented Mar 10, 2025

The steps required for this:

  1. Add a migration file for new projects on https://github.com/supabase/postgres/tree/develop/migrations/db/migrations

  2. backpatch to existing projects using https://github.com/supabase/playbooks/blob/main/playbooks/postgres/running-ad-hoc-sql-on-user-projects.md (private link)

Notes

The problem seems to arise from:

steve-chavez added a commit that referenced this issue Mar 21, 2025
Fixes #1437 (comment).

Moves migrations/db/init-scripts to migrations/db/migrations.
steve-chavez added a commit that referenced this issue Mar 21, 2025
Fixes #1437 (comment).

Moves migrations/db/init-scripts to migrations/db/migrations.
steve-chavez added a commit that referenced this issue Mar 21, 2025
Fixes #1437 (comment).

Moves migrations/db/init-scripts to migrations/db/migrations.
steve-chavez added a commit that referenced this issue Mar 22, 2025
Fixes #1437

+ Moves migrations/db/init-scripts to migrations/db/migrations.
+ Make initial migrations idempotent.
+ Adds test for event triggers.
steve-chavez added a commit that referenced this issue Mar 25, 2025
Fixes #1437

+ Moves migrations/db/init-scripts to migrations/db/migrations.
+ Make initial migrations idempotent.
+ Adds test for event triggers.
steve-chavez added a commit that referenced this issue Mar 26, 2025
Fixes #1437

+ Moves migrations/db/init-scripts to migrations/db/migrations.
+ Make initial migrations idempotent.
+ Adds test for event triggers.
steve-chavez added a commit that referenced this issue Mar 26, 2025
Fixes #1437

+ Moves migrations/db/init-scripts to migrations/db/migrations.
+ Make initial migrations idempotent.
+ Adds test for event triggers.
@steve-chavez steve-chavez reopened this Mar 27, 2025
@steve-chavez
Copy link
Member Author

Reopening since it was reverted on #1500

steve-chavez added a commit that referenced this issue Apr 5, 2025
Fixes #1437.

Now that the effects of the `migrations/db/init-scripts` are fully
tested on the following files:

- [nix/tests/sql/auth.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/auth.out)
- [nix/tests/sql/storage.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/storage.out)
- [nix/tests/sql/roles.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/roles.out)
- [nix/tests/sql/evtrigs.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/evtrigs.out)
- [nix/tests/sql/extensions_schema.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/extensions_schema.out)
- [nix/tests/sql/realtime.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/realtime.out)

This reattempts to move `migrations/db/init-scripts` to `migrations/db/migrations` and have a single directory for migrations.

Of the above tests, the only change pertains to the event triggers owners which is the problem on #1437.

This requires modifying migrations, but the changes are more about following best practices:

+ Makes migrations idempotent.
+ [alter default privileges](https://www.postgresql.org/docs/current/sql-alterdefaultprivileges.html) explicitly list postgres as the target role.
+ Also:
  - adds a conditional for pgjwt which is not available on the 17 version
  - Realtime publication maintains the same postgres owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants