bito-code-review[bot] commented on code in PR #41120:
URL: https://github.com/apache/superset/pull/41120#discussion_r3424198413
##########
superset/sql/parse.py:
##########
@@ -866,15 +900,135 @@ def check_functions_present(self, functions: set[str])
-> bool:
return any(function.upper() in present for function in functions)
- def check_tables_present(self, tables: set[str]) -> bool:
+ def check_tables_present(
+ self, tables: set[str], default_schema: str | None = None
+ ) -> bool:
"""
Check if any of the given tables are present in the statement.
+ Denylist entries may be bare (``pg_stat_activity``) or
+ schema-qualified (``information_schema.tables``). Bare entries
+ match by table name regardless of schema; qualified entries
+ require the schema to match too. This lets us block all access
+ to ``information_schema`` without also blocking any
+ user-authored table that happens to be named ``tables``.
+
:param tables: Set of table names to check for (case-insensitive)
- :return: True if any of the tables are present
+ :param default_schema: Schema unqualified references resolve to at
+ runtime (e.g. the session ``search_path`` / selected schema)
+ :return: True if any of the given tables is referenced
+ """
+ return bool(self.get_disallowed_tables(tables, default_schema))
+
+ def changes_search_path(self) -> bool:
+ """
+ Return True if the statement changes the session ``search_path``.
+
+ A ``SET search_path = ...`` makes unqualified references in later
+ statements resolve to a schema other than the caller's
+ ``default_schema``, so denylist matching against ``default_schema``
+ alone becomes unreliable once such a statement is present.
"""
- present = {table.table.lower() for table in self.tables}
- return any(table.lower() in present for table in tables)
+ # `SET search_path = schema` (and the `TO`/`SESSION`/`LOCAL` variants)
+ # parse as a structured exp.Set, surfaced by get_settings(). Strip any
+ # identifier quoting so `SET "search_path" = ...` (equivalent to the
+ # unquoted form in Postgres) is still recognized.
+ if any(key.strip('"').lower() == "search_path" for key in
self.get_settings()):
+ return True
+ # `set_config('search_path', ...)` rebinds the search path through a
+ # function call rather than a SET statement, so it never reaches
+ # get_settings() and must be detected on the parsed tree.
+ for func in self._parsed.find_all(exp.Anonymous):
+ if (
+ func.name.lower() == "set_config"
+ and func.expressions
+ and isinstance(func.expressions[0], exp.Literal)
+ and func.expressions[0].name.lower() == "search_path"
+ ):
+ return True
+ # Exotic forms (e.g. `SET search_path TO "$user", public`) fall back to
+ # an opaque exp.Command. Match the leading setting name rather than
+ # scanning the whole expression, so `SET ROLE my_search_path_role`
+ # (whose value merely contains the substring) is not misclassified.
+ parsed = self._parsed
+ if isinstance(parsed, exp.Command) and parsed.name.upper() == "SET":
+ tokens = str(parsed.expression).replace("=", " ").split()
+ while tokens and tokens[0].upper() in {"SESSION", "LOCAL"}:
+ tokens.pop(0)
+ return bool(tokens) and tokens[0].strip('"').lower() ==
"search_path"
+ return False
+
+ def get_disallowed_tables(
+ self,
+ tables: set[str],
+ default_schema: str | None = None,
+ schema_indeterminate: bool = False,
+ ) -> set[str]:
+ """
+ Return the subset of ``tables`` referenced by this statement.
+
+ Matching mirrors :meth:`check_tables_present`: bare entries match by
+ table name regardless of schema, while schema-qualified entries
+ require the schema to match too. Entries are returned in their
+ original denylist form so callers can report exactly which
+ denylisted tables were hit.
+
+ A reference without an explicit schema is resolved against
+ ``default_schema`` when one is supplied, so an unqualified ``tables``
+ run under ``search_path = information_schema`` still matches the
+ ``information_schema.tables`` entry, while the same name under a
+ user schema does not.
+
+ :param tables: Set of table names to check for (case-insensitive)
+ :param default_schema: Schema unqualified references resolve to at
+ runtime (e.g. the session ``search_path`` / selected schema)
+ :param schema_indeterminate: When True, the effective ``search_path``
+ cannot be pinned to ``default_schema`` (e.g. the script contains a
+ ``SET search_path``), so an unqualified reference is matched
+ against the bare-name portion of schema-qualified entries too, to
+ avoid bypassing a qualified denylist entry
+ :return: The matched entries, in their original denylist form
+ """
+ fallback = default_schema.lower() if default_schema else None
+ present_bare: set[str] = set()
+ present_qualified: set[str] = set()
+ present_unqualified: set[str] = set()
+ for t in self.tables:
+ bare = t.table.lower()
+ present_bare.add(bare)
+ if t.schema:
+ present_qualified.add(f"{t.schema.lower()}.{bare}")
+ # Also index the fully-qualified (catalog.schema.table) form
so a
+ # three-part denylist entry can match; without this, qualified
+ # entries deeper than schema.table would silently never match.
+ if t.catalog:
+ present_qualified.add(
+ f"{t.catalog.lower()}.{t.schema.lower()}.{bare}"
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test for three-part matching</b></div>
<div id="fix">
The new three-part matching logic (lines 1004-1007) lacks dedicated test
coverage in `test_get_disallowed_tables`. The existing
`test_check_tables_present_schema_qualified` covers this for
`check_tables_present`, but `get_disallowed_tables` has no equivalent test case
with a three-part denylist entry like `{"cat.sys.sql_logins"}` against a Trino
query `SELECT * FROM cat.sys.sql_logins`. Without this, a regression to the
pre-change behavior (where three-part entries silently never matched) could go
undetected.
</div>
</div>
<small><i>Code Review Run #0c2d63</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
