sadpandajoe commented on code in PR #45089:
URL: https://github.com/apache/superset/pull/45089#discussion_r4219789280
##########
superset/sql/parse.py:
##########
@@ -1029,6 +1029,19 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
# last value, so it is intentionally not listed.)
"SETVAL",
"NEXTVAL",
+ # dblink functions open a separate connection and auto-commit,
+ # so writes persist even when the outer transaction is read-only.
+ "DBLINK",
Review Comment:
Nothing in this PR exercises the new names, so reverting either list would
leave the suite green while `SELECT dblink_exec('dbname=remote', 'DELETE FROM
t')` goes back to classifying as read-only. Could you add the new names as
cases next to the existing `lo_import`/`nextval` entries in
`test_is_mutating_postgres_function_and_select_into`
(`tests/unit_tests/sql/parse_tests.py`), asserting `SQLScript(sql,
"postgresql").has_mutation() is True`, and a case in
`tests/unit_tests/commands/sql_lab/test_estimate.py` that runs `SELECT
pg_stat_reset()` against the real `superset.config.DISALLOWED_SQL_FUNCTIONS`
and expects `SupersetDisallowedSQLFunctionException`? The existing estimate
test only injects `PG_SLEEP`, so it can't catch a regression in the shipped
default list.
##########
superset/config.py:
##########
@@ -2481,6 +2481,20 @@ def engine_context_manager( # pylint:
disable=unused-argument
# Other potentially dangerous functions
"pg_sleep",
"pg_terminate_backend",
+ # dblink functions can open separate connections and auto-commit
+ # writes, bypassing the read-only gate entirely
+ "dblink",
+ "dblink_exec",
+ "dblink_connect",
Review Comment:
Agreed—`dblink_connect_u` and `dblink_send_query` are in neither list, so on
a PostgreSQL connection with `allow_dml=False` a script like `SELECT
dblink_connect_u('c', '...'); SELECT dblink_send_query('c', 'DELETE FROM t');
SELECT * FROM dblink_get_result('c') AS r(status text);` still classifies as
read-only and passes the denylist, provided the DB role can execute
`dblink_connect_u`. Should those two be added to `DISALLOWED_SQL_FUNCTIONS` and
`_MUTATING_FUNCTION_NAMES` (for `dblink_send_query`) alongside the other dblink
entries?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
