Repository navigation
fix: extension authorization fix - #237
Conversation
|
I agree with this. I raised it on before #116 (comment) but I believe there was a blocker mentioned on #118 (comment). |
Ah so its a known issue, thanks for the context. |
|
@steve-chavez I went ahead and increased the scope of this PR to every extension operation (ADD, DROP, ALTER, etc). Some of them were locked down, some were fine and I Just added tests for. Open questions as this PR seems easy enough: how would a breaking change like this be handled at the platform level? Is it sufficient to add this as a breaking change in the change log or do we need something at the supabase platform level to warn people before they upgrade the extension? I don't think there is anything we can do in an automated way to prevent breaking changes for customers unless we expand the scope of this PR to include some mechanism to grandfather existing users into be able to do extension operations, I'm opposed to this idea though. |
@steve-chavez maybe I misunderstood, how is it a blocker for this PR? As I understood that comment, it's saying that extension custom scripts need to run as superuser to avoid some security problems, and that is still the case after this PR's change no? |
I think this PR sidesteps this issue because of this line |
| static bool create_extension(CreateExtensionStmt *stmt, | ||
| const utility_hook_args *args, | ||
| const extension_policy *policy) { | ||
|
|
There was a problem hiding this comment.
The change looks good for today. One thing I'd adjust for the future: a role that isn't a member of supabase_privileged_role returns early from the new check and the statement falls through to Postgres, which installs trusted extensions (hstore, pgcrypto) for any role with CREATE on the database. Right now we have no overrides, constraints or scripts for those extensions, so nothing is lost. But the day we add one, it would be skipped silently for those roles.
I'd suggest to move the check so it only decides whether to elevate, not whether supautils runs at all. In create_extension that means keeping restrict_version_specification, constrain_extension and the custom scripts unconditional and folding the membership check into the is_extension_privileged condition in front of the elevated create. Same in alter_extension too.
Please also add a test for this case?
There was a problem hiding this comment.
I thought about this over the weekend and agree that this needs to be fixed before approval (as well as tested as this is certainly breaking stuff that isn't being picked up in the regression suite).
There was a problem hiding this comment.
I updated the C code implementation so I think it's correct now. I was not able to write unit tests to cover all of this though.
17688d7 to
c7fa9b2
Compare
70cb21f to
a8ebbab
Compare
This commit prevents unauthorized users from creating extensions. Current state is that any authorized user is able to create, drop, etc extensions. Lets bolt this down a bit.
a8ebbab to
817ec0a
Compare
This PR prevents unauthorized users from creating extensions.
Current state is that any authorized user is able to create, drop, etc
extensions. Lets bolt this down a bit.