Skip to content
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.

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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

benjie
Copy link
Contributor

@benjie benjie commented Jan 9, 2024

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.

Comment on lines +310 to +326
} catch (e: any) {
// This is most likely from the `runQuery` statement
/* Example error:
* ```
* {
* type: 'ServerError',
* bufferOffset: 100,
* severity: 'ERROR',
* message: 'permission denied for table user_emails'
* }
* ```
*/
console.error('Error occurred whilst testing query:', e);
return {
errorCode: '_ERROR',
message: e.message,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not feel right, but it makes the error handled by insert into user_emails(created_at) be handled in the same way as insert into user_emails(id), thereby making it so I can add the functioning tests. The former being an RBAC permission which is thrown at EXPLAIN EXECUTE time, whereas the latter is a generated column error which is thrown (I think) at parse time.

@benjie
Copy link
Contributor Author

benjie commented Jan 9, 2024

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)

@@ -43,6 +43,7 @@ export async function startup(
user: options.user,
database: options.dbName,
client_encoding: "'utf-8'",
application_name: 'pgtyped',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

application_name (string)
The application_name can be any string of less than NAMEDATALEN characters (64 characters in a standard build). It is typically set by an application upon connection to the server. The name will be displayed in the pg_stat_activity view and included in CSV log entries. It can also be included in regular log entries via the log_line_prefix parameter. Only printable ASCII characters may be used in the application_name value. Other characters are replaced with C-style hexadecimal escapes.
-- https://www.postgresql.org/docs/current/runtime-config-logging.html#GUC-APPLICATION-NAME

It's good practice to set this; but it also give people a way to return null rather than raise exception from their STABLE functions if it's pgtyped running.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does this library reflect permissions?
1 participant