adelsz / pgtyped

pgTyped - Typesafe SQL in TypeScript
https://pgtyped.dev
MIT License
2.91k stars 94 forks source link

Check permissions of queries via `EXPLAIN EXECUTE <stmt>;` #563

Open benjie opened 8 months ago

benjie commented 8 months ago

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 be EXPLAIN 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:

  1. 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.
  2. This is currently a breaking change for people who are using 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 via SET 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)
  3. I'm unsure that the way I raised the errors is correct; will this give users the right level of visibility into issues?
  4. Undocumented

Also, potentially the logic of explainQuery should be merged into getTypeData? 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 the postgres superuser role, making sure all previous tests still pass, adding a passing test about a new constraint insert query and adding what should be negative assertions which I can see are picked up by the system: 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 expected adding some negative assertions, rough though they are.

Note: for this to work for me I had to change the test script in packages/example/package.json` to:

-    "test": "docker-compose run build && docker-compose run test && docker-compose run test-cjs",
+    "test": "docker compose down && docker compose run build && docker compose run test && docker compose run test-cjs",

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 have docker-compose, only docker 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.

benjie commented 8 months ago

Another potential issue in this PR: STABLE functions can be called at planning time, so if users have STABLE functions that they call in their SQL queries and those functions RAISE EXCEPTION then the query may be invalidated due to that exception; for example:

create function current_user_id() returns int as $$
declare
  id int;
begin
  id := nullif(current_setting('jwt.claims.user_id', true), '');
  if id is null then
    raise exception 'FORBIDDEN'; -- WORKAROUND: just return `null`!
  end if;
  return id;
end;
$$ language plpgsql stable;
const query = sql`select * from users where id = current_user_id();`;

(Solved below)