Fix dropping role with table privileges granted by non-neon_superuser #10964
Conversation
7755 tests run: 7377 passed, 0 failed, 378 skipped (full report)Flaky tests (3)Postgres 17
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
fff6e6e at 2025-02-27T05:12:05.225Z :recycle: |
We were previously only revoking privileges granted by neon_superuser. However, we need to do it for all grantors. Signed-off-by: Tristan Partin <tristan@neon.tech>
tristan957
force-pushed
the
tristan957/ci-fixes
branch
from
2017f90 to
fff6e6e
Compare
| FOR grantor IN | ||
| SELECT DISTINCT rtg.grantor | ||
| FROM information_schema.role_table_grants AS rtg | ||
| WHERE grantee = '{role_name}' |
There was a problem hiding this comment.
Hm, isn't this prone to SQL-injection now? I.e. if you name role like ';DROP DATABASE template0;'
There was a problem hiding this comment.
Yes I think you're right. Hmmm
|
|
||
| EXECUTE revoke_query; | ||
| revoke_query := format( | ||
| 'REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA %I FROM "{role_name}" GRANTED BY %I', |
There was a problem hiding this comment.
Same here, if the grantee is some 'funny' name, I guess you need something like quote_ident() or so
| TEST_GRANTOR = "my_grantor" | ||
| TEST_GRANTEE = "my_grantee" |
There was a problem hiding this comment.
Regardless of whether the current code is prone to injections or not (I bet it does, though), let's make role names for both grantor and grantee to contain the injections vector like '; RAISE 'ouch'; ' or "; RAISE 'ouch'; " depending on the type of quotes you use in what case
|
Given the SQL injection comments, good catch, I will take another look at this. |
We were previously only revoking privileges granted by neon_superuser. However, we need to do it for all grantors.