Check permissions of queries via EXPLAIN EXECUTE <stmt>;
#563
Conversation
| } 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, | ||
| }; |
There was a problem hiding this comment.
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.
|
Another potential issue in this PR: 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();`; |
| @@ -43,6 +43,7 @@ export async function startup( | |||
| user: options.user, | |||
| database: options.dbName, | |||
| client_encoding: "'utf-8'", | |||
| application_name: 'pgtyped', | |||
There was a problem hiding this comment.
application_name(string)
Theapplication_namecan be any string of less thanNAMEDATALENcharacters (64 characters in a standard build). It is typically set by an application upon connection to the server. The name will be displayed in thepg_stat_activityview 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.
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.NOINHERITin 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
explainQueryshould 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_testto use instead of thepostgressuperuser role, making sure all previous tests still pass, adding a passing test about a new constraintinsertquery 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_emailsbut I don't know how to check that or mark it as expectedNote: for this to work for me I had to change the
testscript 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.sqlscript, 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.