Skip to content

revoke excess permissions #298

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.

Sign up for GitHub

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

Open
AndrewJackson2020 wants to merge 7 commits into
base: master
Choose a base branch
from
+72 −71
Open

revoke excess permissions #298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e89441e
fix: revoke excess permissions
AndrewJackson2020 Sep 22, 2026
6578515
fix: fix most tests
AndrewJackson2020 Sep 22, 2026
5831c09
fix: refactor test to use psycopg
AndrewJackson2020 Sep 23, 2026
ec49b09
fix: fix tests, format, convert tests to psycopg
AndrewJackson2020 Sep 23, 2026
68acdec
fix: add truncate/delete permission for single table
AndrewJackson2020 Sep 23, 2026
6d89518
fix: grant truncate
AndrewJackson2020 Sep 23, 2026
d623154
fix: add checks on delete/truncate
AndrewJackson2020 Sep 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions sql/pg_net--0.20.5--0.20.6.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
revoke update on all sequences in schema net to PUBLIC;

revoke insert on all tables in schema net to PUBLIC;
revoke update on all tables in schema net to PUBLIC;
Comment on lines +3 to +4

@steve-chavez steve-chavez Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are likely not needed, also insert shouldn't work correctly since #199

@AndrewJackson2020 AndrewJackson2020 Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed as its okay to revoke and the above statements are fine?

@steve-chavez steve-chavez Sep 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, should be fine to revoke.


revoke delete on all tables in schema net to PUBLIC;

@steve-chavez steve-chavez Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same use case as TRUNCATE

@AndrewJackson2020 AndrewJackson2020 Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added delete above with reservation.

grant delete on net.http_request_queue to PUBLIC;

revoke truncate on all tables in schema net to PUBLIC;

@steve-chavez steve-chavez Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this is used sometimes by users to clear the http_request_que rows, if it grows too big I recall there were problems #216

@AndrewJackson2020 AndrewJackson2020 Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can only grant truncate on that one table then? It still seems odd granting that privilege to all as the pg_net queue is a shared resource and you are basically allowing any read only user to clobber everyone elses request.

@steve-chavez steve-chavez Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can only grant truncate on that one table then?

Sounds fine to me.

@AndrewJackson2020 AndrewJackson2020 Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and added delete/truncate to public on this table. TBH I still feel a bit weird doing this. pg_cron does not allow usage on the net schema at all by default so the user has to opt into a user being able to use any of the functionality. I wonder if we should do the same.

@steve-chavez steve-chavez Sep 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pg_cron does not allow usage on the net schema at all by default so the user has to opt into a user being able to use any of the functionality. I wonder if we should do the same.

Yeah, I've seen that as well. Perhaps we should do the same and manage the privileges on supabase/postgres?

grant truncate on net.http_request_queue to PUBLIC;

revoke references on all tables in schema net to PUBLIC;

@steve-chavez steve-chavez Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no PRIMARY or UNIQUE key on the tables then this should be fine

@AndrewJackson2020 AndrewJackson2020 Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the docs I think this needs to be revoked either way, seems live a vector for a privilege escalation attack:

Allows creation of a foreign key constraint referencing a table, or specific column(s) of a table. Great care should be taken when granting this privilege, since a user who creates a foreign key can arrange for enforcement of that foreign key to call an arbitrary function, such as a cast function, and such functions will be called with the privileges of the table owner.

https://www.postgresql.org/docs/current/ddl-priv.html#DDL-PRIV-REFERENCES

@steve-chavez steve-chavez Sep 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I agree in revoking this too.

revoke trigger on all tables in schema net to PUBLIC;

@steve-chavez steve-chavez Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some users have added triggers in the past #62 (comment)

@AndrewJackson2020 AndrewJackson2020 Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one again where I would want to consult what the default permissions are. A trigger will essentially highjack another users session with code provided by the client that created the trigger, correct? Seems like this opens up a plethora of security issues.

@steve-chavez steve-chavez Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, the case in #62 does self-hosting too so I guess they can use superuser anyway.

revoke maintain on all tables in schema net to PUBLIC;
Comment on lines +13 to +14

@steve-chavez steve-chavez Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would prevent users from running VACUUM, not sure if needed, it might.

@AndrewJackson2020 AndrewJackson2020 Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not postgres/superuser but obv supabase platform users don't have that anyway. I think what I should do here is validate all of the permissions and only revoke non default permissions that postgres wouldn't grant on a a create table.

@AndrewJackson2020 AndrewJackson2020 Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This permission would allow any authenticated user to lock the table. Also it seems like it is not a default permission in postgres which leads me to think we should not allow it.

11:22:15 aj@localhost:5432/postgres  96469 =#
create table something3 (c1 int);
CREATE TABLE
11:22:20 aj@localhost:5432/postgres  96469 =#
create user whatever3;
CREATE ROLE
11:22:28 aj@localhost:5432/postgres  96469 =#
SELECT * FROM has_table_privilege('whatever3', 'something3', 'MAINTAIN')
;
 has_table_privilege
---------------------
 f
(1 row)

Allows VACUUM, ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, REINDEX, LOCK TABLE, and database object statistics manipulation functions (see Table 9.105) on a relation.

https://www.postgresql.org/docs/current/ddl-priv.html#DDL-PRIV-MAINTAIN

@steve-chavez steve-chavez Sep 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of LOCK, given that I agree with revoking MAINTAIN.

10 changes: 8 additions & 2 deletions sql/pg_net.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,5 +353,11 @@ end;
$$;

grant usage on schema net to PUBLIC;
grant all on all sequences in schema net to PUBLIC;
grant all on all tables in schema net to PUBLIC;

grant usage on all sequences in schema net to PUBLIC;
grant select on all sequences in schema net to PUBLIC;

grant select on all tables in schema net to PUBLIC;

grant delete on net.http_request_queue to PUBLIC;
grant truncate on net.http_request_queue to PUBLIC;
2 changes: 1 addition & 1 deletion test/test_engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@ def test_connect(conn):
"""Sanity test verifying connection to postgres works"""

conn.execute("select 1")
assert [(1, )] == conn.execute("select 1").fetchall()
assert [(1,)] == conn.execute("select 1").fetchall()
117 changes: 49 additions & 68 deletions test/test_privileges.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from sqlalchemy import text
from common import collect_response_sync, http_request
import pytest
import psycopg


def test_net_on_postgres_role(sess):
Expand All @@ -25,87 +27,66 @@ def test_net_on_postgres_role(sess):
assert response["status"] == "SUCCESS"


def test_net_on_pre_existing_role(sess):
"""Check that a pre existing role can use the net schema"""
def test_net_on_pre_existing_role(conn):
"""Check permissions on pre-existing role"""

role = sess.execute(text("select current_user;")).fetchone()
assert role[0] == "postgres"
role = conn.execute("select current_user;").fetchall()
assert role[0][0] == "postgres"

sess.execute(text("set local role to pre_existing;"))
(request_id, current_user) = sess.execute(
text(
conn.execute("set local role to pre_existing;")
with pytest.raises(
psycopg.errors.InsufficientPrivilege,
match="permission denied for table http_request_queue",
):
conn.execute(
"""
select net.http_get(
'http://localhost:8080/anything'
), current_user;
"""
select net.http_get(
'http://localhost:8080/anything'
), current_user;
"""
)
).fetchone()
assert request_id == 1
assert current_user == "pre_existing"

# Commit to wakeup background worker
sess.commit()

# Confirm that the request was retrievable
sess.execute(text("set local role to pre_existing;"))
response = collect_response_sync(sess, request_id)
current_user = sess.execute(text("select current_user;")).scalar()
assert response["status"] == "SUCCESS"
assert current_user == "pre_existing"
conn.rollback()
conn.execute("set local role to pre_existing;")
conn.execute("DELETE FROM net.http_request_queue WHERE 1 = 0;")
conn.execute("TRUNCATE net.http_request_queue;")


def test_net_on_new_role(sess):
"""Check that a newly created role can use the net schema"""
def test_net_on_new_role(conn):
"""Check permissions on newly created role"""

role = sess.execute(text("select current_user;")).fetchone()
assert role[0] == "postgres"
role = conn.execute("select current_user;").fetchall()
assert role[0][0] == "postgres"

sess.execute(
text("""
create role another;
""")
)
sess.execute(text("set local role to another;"))
conn.execute("create role another;")
conn.commit()
conn.execute("set local role to another;")

(request_id, current_user) = sess.execute(
text(
with pytest.raises(
psycopg.errors.InsufficientPrivilege,
match="permission denied for table http_request_queue",
):
conn.execute(
"""
select net.http_get(
'http://localhost:8080/anything'
), current_user;
"""
select net.http_get(
'http://localhost:8080/anything'
), current_user;
"""
)
).fetchone()
assert request_id == 1
assert current_user == "another"

# Commit to wakeup background worker
sess.commit()
conn.rollback()

# Confirm that the request was retrievable
sess.execute(text("set local role to another;"))
response = collect_response_sync(sess, request_id)
current_user = sess.execute(text("select current_user;")).scalar()
assert response["status"] == "SUCCESS"
assert current_user == "another"
conn.execute("set local role to another;")
conn.execute("DELETE FROM net.http_request_queue WHERE 1 = 0;")
conn.execute("TRUNCATE net.http_request_queue;")

sess.execute(text("set local role to another;"))
conn.execute("set local role to another;")
# can use the net.worker_restart function
(res, current_user) = sess.execute(
text(
"""
select net.worker_restart(), current_user;
"""
)
).fetchone()
assert res
assert current_user == "another"

sess.execute(
text("""
select net.wait_until_running();
set local role postgres;
drop role another;
""")
)
res = conn.execute("select net.worker_restart(), current_user;").fetchone()
assert res[0]
assert res[1] == "another"

conn.execute("select net.wait_until_running();")

conn.execute("set local role postgres;")
conn.execute("drop role another;")
Loading