Check permissions of queries via EXPLAIN EXECUTE <stmt>;
#563
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds an additional check before generating the types of a query, it runs something like
EXPLAIN EXECUTE prepared_statement_name (null, null, null, ...);
which doesn't actually run the query (that would beEXPLAIN ANALYZE EXECUTE prepa...
) but does make the DB attempt to form a query plan for it, which will force some rudimentary query checks including, critically, the assertion of role-based access control.This PR is not merge-able for (at least) the following reasons:
There are negative assertions that I don't understand how to make with your test suite (I want to assert that a given SQL query fails to generate types)I'm not confident in the negative assertions in the tests; it feels like there should be a better way.NOINHERIT
in their role configuration, since it only checks that the user in the connection string is capable of making the query, not the other roles it might become viaSET ROLE
(solution: opt-in via configuration setting? I've added a dummy variable for this - let me know how you'd like me to address it / what the option should be called)Also, potentially the logic of
explainQuery
should be merged intogetTypeData
? That's where I started, but the changes got too large so I figured it would be easier to review separately, plus easier to opt in/out of that way also.I've done what I can to add to the tests, including creating a database role
pgtyped_test
to use instead of thepostgres
superuser role, making sure all previous tests still pass, adding a passing test about a new constraintinsert
query andadding what should be negative assertions which I can see are picked up by the system:adding some negative assertions, rough though they are.Error processing src/user_emails/assert_fail.ts: permission denied for table user_emails
but I don't know how to check that or mark it as expectedNote: for this to work for me I had to change the
test
script in packages/example/package.json` to:for two reasons: a) I needed the DB to shut down so it would re-initialize with the
schema.sql
script, and b) I don't havedocker-compose
, onlydocker compose
.Please feel very free to take over this pull request if you wish. Alternatively, if you're interested in supporting this feature, please let me know the changes you'd like to see.