Check access rights in EXPLAIN QUERY TREE and EXPLAIN SYNTAX - #110180
Check access rights in EXPLAIN QUERY TREE and EXPLAIN SYNTAX#110180alexey-milovidov wants to merge 49 commits into
Conversation
`EXPLAIN QUERY TREE` and `EXPLAIN SYNTAX` (in the analyzer) resolve the query and dump table metadata such as column names and types, but unlike `EXPLAIN PLAN` they do not build a query plan. The `SELECT` access check that the planner performs in `prepareBuildQueryPlanForTableExpression` was therefore skipped, so a user with no privileges could read the column names and data types (including `Enum` element lists) of tables they are not allowed to access. Add a `SELECT` access check for every table referenced anywhere in the query tree. The check mirrors the planner: it validates access to the columns that are actually read, with the trivial-count fallback (access is granted if at least one column is accessible) for queries that read no specific column, e.g. `SELECT count() FROM t`. `buildQueryTree` only builds the tree; table identifiers are bound to storages and columns are resolved by the query analysis pass. The check runs on the resolved tree: on `query_tree` directly when the passes already resolved it, or on a throwaway resolved copy otherwise (e.g. `EXPLAIN QUERY TREE run_passes = 0`, which intentionally dumps the unresolved tree). Closes: #78938 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Workflow [PR], commit [fcc47e8] Summary: ❌
AI ReviewSummaryThis PR substantially tightens access checks for Findings
Performance & Safety
Final VerdictNeeds changes before merge: the new nested-view check can still turn LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 394/414 (95.17%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
The `SELECT` access check added for `EXPLAIN QUERY TREE` and `EXPLAIN SYNTAX`
resolved a throwaway copy of the query tree in the branch that dumps the
*unresolved* tree (`EXPLAIN SYNTAX`, `EXPLAIN QUERY TREE run_passes = 0`).
Resolution throws for queries that these statements are specifically designed
to format without resolving - e.g. fuzzed or invalid expressions, a `JOIN`
subquery without an alias (`ALIAS_REQUIRED`), `GROUP BY grouping sets ...
WITH TOTALS` (`NOT_IMPLEMENTED`), or `executable('', ...)` whose arguments the
analyzer intentionally does not evaluate for `EXPLAIN SYNTAX` (`BAD_ARGUMENTS`).
This broke `03773_join_on_formatting`, `03761_inconsistent_ast_formatting`,
`03771_tuple_inconsistent_formatting`, `03601_inconsistent_table_names`,
`01883_with_grouping_sets` and `02377_executable_function_settings` in the
`Fast test`.
The dumped tree in this branch is the unresolved `query_tree`, which never
carries resolved column names or types, so the dump itself cannot leak table
metadata. When resolution of the throwaway copy fails, there is no resolved
metadata to protect and a real query would fail with the same resolution error
before the planner's access check, so we skip the access check instead of
turning a formatting request into a resolution error. An `ACCESS_DENIED`
raised during resolution is still propagated, so `EXPLAIN SYNTAX` of a table
the user cannot read stays denied.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
`checkAccessRightsForQueryTree` collected every `TableNode` in the resolved query tree and re-checked all of them against the outer `query_context`. That is wrong for scopes that do not execute under the top-level context: when a view is inlined (`analyzer_inline_views = 1`), its body is resolved under `StorageView::getViewSubqueryContext`, and the planner checks the base-table privileges with that per-scope context (`buildPlannerContext` / `prepareBuildQueryPlanForTableExpression` use `QueryNode::getContext`). Flattening every table into one global check made `EXPLAIN QUERY TREE` and analyzer `EXPLAIN SYNTAX` deny a valid `SELECT ... FROM definer_view` when the user has `SELECT` on a `SQL SECURITY DEFINER` / `NONE` view but not on its base tables. Track the scope context with `InDepthQueryTreeVisitorWithContext` and check each table with the context of its own scope, reproducing the planner. Columns are now collected per `TableNode` instance (`collectSelectedColumnsForTableNode`) so a base table referenced both directly and inside an inlined view is checked with the correct column set in each scope. Addresses the AI review on #110180 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Covers the per-scope access-check fix: a user with SELECT on a `SQL SECURITY DEFINER` / `NONE` view but not on its base table can still `EXPLAIN QUERY TREE` / `EXPLAIN SYNTAX` it with `analyzer_inline_views = 1`, matching what a plain SELECT through the view allows; a user with no access is still denied. Verified locally against a freshly built server.
|
Addressed the AI-review Major (per-scope access check for inlined Out of scope, but flagging a pre-existing issue found while testing this: with
|
The `run` helper in `04538_explain_query_tree_access_check.sh` and `04539_explain_query_tree_access_definer_view.sh` mapped every non-`ACCESS_DENIED` outcome to `OK`, so a positive case that started throwing a different exception (e.g. `UNKNOWN_IDENTIFIER`, `NOT_IMPLEMENTED`) would still pass and the tests would no longer prove the fix. Now `OK` is printed only when the client exits with status 0, `ACCESS_DENIED` only for that specific exception, and any other exception is emitted verbatim, which makes the reference diff fail. The expected `OK` / `ACCESS_DENIED` outputs are unchanged, so both reference files stay valid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…atabase tables The AI review found that `checkAccessRightsForQueryTree` returned early on `!storage_id.hasDatabase()` before it knew whether the query was a trivial-count / `SELECT 1` case. The planner's `PlannerJoinTree::checkAccessRights` guards only the explicit-column branch with `hasDatabase` and always runs the "at least one accessible column" fallback first, with `ContextAccess` resolving an empty database name to the current database. As written, `EXPLAIN QUERY TREE` and analyzer `EXPLAIN SYNTAX` could still succeed for `count()`-style queries on empty-database `TableNode`s (cross-replication) even when the real `SELECT` would be denied. Move the `hasDatabase` guard so it only skips the explicit-column check, and let the trivial-count fallback run unconditionally, matching the planner. Extend `04538_explain_query_tree_access_check` to cover the trivial-count path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…XPLAIN SYNTAX `EXPLAIN SYNTAX` inlines a `SQL SECURITY INVOKER` parameterized view call `pv(...)` into its inner subquery via `ExpandParameterizedViewsMatcher` before the query tree is built, so the view object is no longer present in the tree that `checkAccessRightsForQueryTree` inspects — only the base tables were checked. Real execution instead turns `pv(...)` into a fake `TableNode` for the view in `QueryAnalyzer::resolveTableFunction` and checks `SELECT` on it in `prepareBuildQueryPlanForTableExpression`, so a user with access to the base tables but no grant on the view could `EXPLAIN SYNTAX SELECT ... FROM pv(...)` even though the real `SELECT` is denied. Run the access check on the original (unexpanded) query when a parameterized view was expanded: there the view still resolves to its own `TableNode`, so its `SELECT` grant (and its selected columns) are enforced exactly as a real `SELECT` does, while the base tables inside its body are still checked under the per-scope context. The check on the expanded dump tree is skipped in that case to avoid a redundant, incomplete check. Non-parameterized-view queries are unaffected. Extracted the resolve-then-check logic into `resolveThenCheckAccessRights` shared by both paths. Test: `04600_explain_syntax_parameterized_view_access.sh`.
…ews in EXPLAIN A regular, non-parameterized view that is not inlined (`analyzer_inline_views = 0`, the default) stays as a single `TableNode` in the query tree, so `checkAccessRightsForQueryTree` only enforced `SELECT` on the view object and never re-checked the base tables. Real execution does check them: `StorageView::readImpl` builds the view's inner query under the view's own context (the definer for `SQL SECURITY DEFINER`, the invoker otherwise) and checks the base-table privileges there. As a result a user with `SELECT` on a default `SQL SECURITY INVOKER` view but not on its base table could `EXPLAIN QUERY TREE` / `EXPLAIN SYNTAX SELECT * FROM v` and learn the view's projected columns/types even though the actual `SELECT` is denied. Reproduce the inner access pass: when the check encounters a non-inlined regular view `TableNode`, build its inner query under `StorageView::getViewSubqueryContext`, resolve it, and run the access check recursively (which also covers nested views). Parameterized views are expanded and checked separately; inlined views already expose their base tables as `TableNode`s, so neither is double-checked. Test `04601_explain_query_tree_access_invoker_view`.
…yzer-only The AI review asked to run the parameterized-view object access check in the legacy (allow_experimental_analyzer = 0) EXPLAIN SYNTAX fallback as well. That would over-deny: the old interpreter never requires a SELECT grant on the parameterized view object — Context::executeTableFunction resolves pv(...) without any access check and InterpreterSelectQuery skips checkAccessRightsForSelect for table functions — so a real SELECT ... FROM pv(...) succeeds with only the base-table grants (verified against a live server). The legacy fallback already checks the base tables via InterpreterSelectQuery inside ExplainAnalyzedSyntaxVisitor, which is exactly what real legacy execution enforces, so EXPLAIN SYNTAX is allowed precisely when the real SELECT is allowed and denied (without leaking anything) when it is not. Add a comment explaining this at the analyzer gate and a legacy-path regression test 04602_explain_syntax_parameterized_view_access_legacy that pins the faithfulness contract: base-only user -> real SELECT and EXPLAIN SYNTAX both allowed; user without grants -> both denied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s in legacy EXPLAIN SYNTAX `ExplainAnalyzedSyntaxMatcher` (the `allow_experimental_analyzer = 0` fallback used by `EXPLAIN SYNTAX`) only checked `SELECT` on the view object itself, via `checkAccessRightsForSelect` inside `InterpreterSelectQuery::analyze`. A real `SELECT` through a regular, non-parameterized view goes on to check the base-table privileges too, but only once the pipeline is actually built, in `StorageView::readImpl` - which `EXPLAIN SYNTAX` never reaches. So a user with `SELECT` on the view but not on its base table could `EXPLAIN SYNTAX` the expanded view body (with `allow_experimental_analyzer = 0`) while the real `SELECT` is denied. Reproduce that inner check by resolving the view straight from the AST (before it is rewritten into a subquery) and running the base-table access check on its inner query under the view's own security context, recursing through nested views the same way the analyzer path (`checkAccessRightsForQueryTree`) already does for `EXPLAIN QUERY TREE` and the analyzer's `EXPLAIN SYNTAX`. Factored the shared recursion into `checkViewBaseTableAccess`, used by both paths. Test: `04603_explain_syntax_invoker_view_access_legacy.sh`.
Address the AI review on #110180. A parameterized view is not resolved into a `TableNode`: `QueryAnalyzer::resolveTableFunction` turns `pv(...)` into a `TableFunctionNode` that owns the view storage built by `Context::buildParameterizedViewStorage`, and the planner checks `SELECT` on that view object for exactly this node type in `prepareBuildQueryPlanForTableExpression`. `checkAccessRightsForQueryTree` collected only `TableNode`s, so every `EXPLAIN` form skipped the view object's own grant: a user with `SELECT` on the base table but none on the view could read the view definition through `EXPLAIN QUERY TREE`, `EXPLAIN SYNTAX` and `EXPLAIN AST optimize = 1` while a real `SELECT` is rejected. Collect parameterized-view `TableFunctionNode`s as well (other table functions stay out, as in the planner: their check happens in `ITableFunction::execute`), and check them with the same column-aware `SELECT` check and the same recursive base-table pass through `checkViewBaseTableAccess`. `collectSelectedColumnsForTableExpression` generalizes `collectSelectedColumnsForTableNode` to match columns by any table expression instance, because the columns selected from a parameterized view have its `TableFunctionNode` as their column source. `EXPLAIN AST optimize = 1` no longer ties the view-object pre-check to the explain-time expansion having happened: expansion is deliberately skipped for `FINAL` / `SAMPLE` and for `SQL SECURITY DEFINER` / `NONE` views, and the legacy rewriting visitor still inlines the view body for those, so the check now runs for every referenced parameterized view. New test `04656_explain_ast_optimize_definer_parameterized_view_object_access`. The mode the pre-check follows is now derived only from the fully applied context, not from `session || explained query`: `EXPLAIN AST` forces the legacy interpreter as an implementation detail, so the effective value is read from a copy of the session context that override never touched, with the explained statement's own `SETTINGS` applied. An explicit `SETTINGS allow_experimental_analyzer = 0` therefore no longer denies an `EXPLAIN AST optimize = 1` whose real `SELECT` succeeds with the base-table grants alone. New test `04657_explain_ast_optimize_analyzer_disabled_in_query_settings`. `joined_subquery_requires_alias` is relaxed for the throwaway tree the access check resolves: it is a restriction on how a query may be written, not an access rule, and an unexpanded parameterized view call in a `JOIN` carries no alias, which would otherwise skip the check and fall back to dumping the query unexpanded (`04105_explain_syntax_parameterized_view`).
…cy EXPLAIN Address review: the legacy formatting path (`ExplainAnalyzedSyntaxVisitor`, used by `EXPLAIN SYNTAX` with `enable_analyzer = 0` and by `EXPLAIN AST optimize = 1`) unconditionally rewrote the main-`FROM` parameterized view call into a subquery, even though `ExpandParameterizedViewsMatcher` deliberately keeps `pv(...)` intact when the table expression carries `FINAL` or `SAMPLE` (those modifiers are rejected on a subquery). The result was `(SELECT ...) FINAL` - a query the executor rejects - while the real `SELECT ... FROM pv(...) FINAL` is valid. Skipping the rewrite alone is not enough: the `analyze()`-mode `InterpreterSelectQuery` mutates the call in place (its own `StorageView::replaceWithSubquery` / `restoreViewName` round trip leaves a fake table identifier holding the view name instead of the original `pv(...)` call), so the original table expression is snapshotted up front and restored afterwards. Test `04660_explain_parameterized_view_final_sample_legacy` pins the executable output for `FINAL` and `SAMPLE` under both `EXPLAIN SYNTAX` and `EXPLAIN AST optimize = 1`, and that the view body is still expanded without those modifiers.
…ected columns `collectSelectedColumnsForTableExpression` matches a parameterized view's `TableFunctionNode`, but `isAliasColumn` only recognised `QueryTreeNodeType::TABLE` as a carrier of `ALIAS` columns. The planner's `CollectTableExpressionData::isAliasColumn` accepts `TABLE_FUNCTION` too, and keeps only the selected alias name in `TableExpressionData::getSelectedColumnsNames`. Descending into an alias column's defining expression here would collect the columns that expression reads on top of the alias name, so the access check could demand grants the real `SELECT` never asks for and `EXPLAIN` would reject a query that plain execution allows. The over-denial is not reachable through a parameterized view today, because `Context::buildParameterizedViewStorage` builds the view's `ColumnsDescription` from the inner query's sample block, which materialises the outputs as ordinary rather than `ALIAS` columns. The other caller, `IStorage::getDependentViewsByColumn`, only collects columns whose source is a `TableNode`, so it is unaffected either way. Keeping the two visitors in sync is still the right contract, and the new test pins `EXPLAIN` outcome to plain `SELECT` outcome so it keeps holding.
… setting The test dumps the expanded parameterized view body, and the harness randomizes `query_plan_optimize_prewhere`. When it is off, the move-to-prewhere rewrite is applied to the AST instead of to the query plan, so the body's `WHERE` is printed as `PREWHERE` and the reference no longer matches. This showed up as a reproducible failure in several flaky-check jobs. Pin `optimize_move_to_prewhere = 0` so the dumped body is stable. The prewhere placement is not what this test is about - it checks that a parameterized view call carrying `FINAL` or `SAMPLE` stays unexpanded on the legacy formatting path.
…PLE view calls Keeping a parameterized view call with `FINAL` or `SAMPLE` unexpanded on the legacy formatting path also changes `EXPLAIN SYNTAX` output when the server defaults to the old analyzer, and `04105_explain_syntax_parameterized_view` has a separate `.oldanalyzer.reference` that still expected the inlined body. Only the two `04105_modifiers_pv` blocks change, matching the already updated `.reference`; the reproducible failure was in the `old analyzer, s3 storage, DBReplicated` stateless job.
…gacy EXPLAIN path `ExplainAnalyzedSyntaxVisitor` stops descending at an `ASTSelectQuery`, so `checkNonParameterizedViewBaseTableAccess` only ever ran for the outermost `SELECT` of the explained query. A regular `SQL SECURITY INVOKER` view read from a nested subquery - `FROM (SELECT ... FROM v)`, `WHERE x IN (SELECT ... FROM v)`, a scalar subquery - therefore never got its base tables checked: `InterpreterSelectQuery::analyze` checks only `SELECT` on the view object, while the base-table denial of a real query happens in `StorageView::readImpl`, which `EXPLAIN` never executes. `EXPLAIN SYNTAX` / `EXPLAIN AST optimize = 1` were then allowed where the real `SELECT` is `ACCESS_DENIED`. The same check now runs for every nested `SELECT` that reads from such a view, with the same column-level precision: each nested `SELECT` is analyzed on a copy (so the query being dumped is not touched, and nested view references stay unexpanded as before) and the columns real execution requests from the view are taken from that analysis. Nested `SELECT`s that reference no regular view are not analyzed at all, and a table name that does not resolve here - a `WITH` table of the enclosing query, an unknown database - is left to the query's own analysis, so nothing that used to be explainable starts failing. New test `04666_explain_syntax_nested_subquery_view_access_legacy`.
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 378/399 (94.74%) · Uncovered code |
…LAIN SYNTAX `EXPLAIN SYNTAX` ran the analyzer-side access pre-check on the original query only when `ExpandParameterizedViewsMatcher` had actually expanded a parameterized view call. Expansion is deliberately skipped for `FINAL` / `SAMPLE` calls and for views created with `SQL SECURITY DEFINER` or `NONE`, and for a statement that merely wraps a `SELECT` (`INSERT INTO dst SELECT ... FROM pv(...)`) `explainQueryTree` declines the non-`SELECT` root, so the legacy formatting visitor was reached and inlined the view body through `StorageView::replaceWithSubquery` without ever enforcing the `SELECT` grant on the view object that `prepareBuildQueryPlanForTableExpression` requires for a real `SELECT ... FROM pv(...)`. Reproduced with `enable_analyzer = 1` and a user holding `SELECT` on the base table but not on the view: `INSERT INTO dst SELECT * FROM pv_def(n = 1)` was `ACCESS_DENIED` while `EXPLAIN SYNTAX INSERT INTO dst SELECT * FROM pv_def(n = 1)` printed the inlined body; the `FINAL` form was likewise dumped instead of being denied. Gate the pre-check on `referenced_parameterized_view` instead, matching what the `EXPLAIN AST optimize = 1` path already does. The check stays analyzer-only, so `enable_analyzer = 0` still dumps what its real execution allows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ck could not run `checkAccessRightsForQueryTree` recurses into a non-inlined view's inner query (`checkViewBaseTableAccess`) to reproduce the base-table `SELECT` check real execution runs in `StorageView::readImpl`. That recursive pass skips itself when the inner query cannot be resolved by the analyzer (e.g. a view created under `enable_analyzer = 0` with `GROUP BY GROUPING SETS (...) WITH TOTALS`), but the analyzer path dropped the signal: `EXPLAIN QUERY TREE SELECT * FROM v` (and the analyzer `EXPLAIN SYNTAX run_query_tree_passes = 1`) still dumped the resolved outer tree after only the view-object grant, handing out the view's resolved columns and types although the base-table pass never ran. Propagate the skip through `checkAccessRightsForQueryTree` / `resolveThenCheckAccessRights` / `checkAccessForExplainedSelect` and make `explainQueryTree` fall back to dumping a freshly built, unresolved tree - the user's own query text - matching the fail-close the legacy `EXPLAIN SYNTAX` formatter already implements by keeping such a view unexpanded. A real `SELECT` through such a view fails while resolving the inner query, so no successfully running query loses its resolved `EXPLAIN`. Test `04703_explain_query_tree_view_unresolvable_access` pins the matrix: outsider denied on the view object; view-only and full users both get the unresolved fallback with no resolved metadata revealed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕵 @groeneai, both remaining CI failures on cf1eac5 are fleet-wide master regressions unrelated to this PR (the PR only touches
|
|
Thanks, and your triage is right on both counts: neither is caused by this PR. 1. STID 2508-2e0c, hash-join residual filter: fixing PR #107957 (mine, open). The frame you quote is exactly the line that PR changes. From your Two corrections to the numbers, both in the direction you suspected. Keying on the stack frame rather than the STID (per-STID undercounts this family, and On the flare onset, the trigger is mine. #112831 (merged 08-04 11:04:38Z, 1h50m before that first master failure) makes 2. STID 4148-4291, zero client version: a fix task is at pending, and I will post the fixing PR link here when it opens. One correction that matters for your read: this row is not a stale base. CI builds the merge commit, and the job log for So #109408 is doing its job as an assertion and has uncovered a context that neither fill site covers. The stack is uniform across all 8: Nothing here needs a change to this PR. |
|
Following up on item 2 (STID 4148-4291, zero client version) with the fixing PR link I promised: #113220 (mine, open). It was already open when I wrote the analysis above and I missed it, because it was raised from the access-control angle and its title and body never mention the client version, so no keyword from this signature finds it. It changes the same line the stack ends at: I verified it covers this signature rather than assuming it: applying its Nothing here needs a change to this PR. |
…le expressions are all genuine table functions The throwaway resolution that reproduces the planner's access check for the unresolved dump also resolves every table function in the query, and resolving a remote one may connect to a remote server: `mysql(...)` fetches the table structure, so `EXPLAIN QUERY TREE run_passes = 0` - a form that deliberately dumps the unresolved tree so that no server is contacted - produced connection attempts and error-log noise, failing 04657_mysql_tls_credentials_query_tree across all stateless CI jobs on https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110180&sha=e3743bceb849a90e7d8e1517c9a57d9121467a66&name_0=PR&name_1=Stateless%20tests%20%28amd_debug%2C%20parallel%29 The check can only ever guard a plain table or view (only a table identifier resolves into a `TableNode`) or a parameterized view, which is called like a table function but with a name no registered table function has. Skip the throwaway resolution when the explained query references neither, so a query reading only from genuine table functions is dumped without resolving them - exactly as on master. PR: #110180
…t view access check too `checkNestedSelectsViewBaseTableAccess` swallowed only `DB::Exception`, so a nested legacy subquery mentioning both an `INVOKER` view and a table function that throws a non-ClickHouse exception during resolution (`paimonAzure` / `icebergAzure` raise `std::runtime_error`) turned `EXPLAIN SYNTAX` / `EXPLAIN AST optimize = 1` into a connection exception, although master dumps the query and nothing from the nested subquery beyond the user's own text is printed. Mirror the `catch (...)` of `resolveThenCheckAccessRights`: such an exception is not an access denial, and the nested `SELECT` is left unchecked exactly as an unresolvable one is. Add a regression test over a nested view plus a failing remote table function. PR: #110180
…allowing The style check requires a comment containing `Ok` in a `catch (...)` block that intentionally swallows exceptions, as the sibling block in `resolveThenCheckAccessRights` already has. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110180&sha=afc4af6ba10f88d4b9efa884ce8df17f544196ba&name_0=PR&name_1=Style%20check #110180 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The style check rejects test files whose name contains `fail`; rename `04823_explain_syntax_nested_view_failing_table_function_legacy` to `04823_explain_syntax_nested_view_throwing_table_function_legacy`. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110180&sha=afc4af6ba10f88d4b9efa884ce8df17f544196ba&name_0=PR&name_1=Style%20check #110180 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| ASTPtr nested_select_copy = nested_select_node->clone(); | ||
| try | ||
| { | ||
| InterpreterSelectQuery interpreter( |
There was a problem hiding this comment.
checkNestedSelectsViewBaseTableAccess now re-runs a full InterpreterSelectQuery analysis for every nested SELECT that mentions a regular view. That analysis resolves every other table expression in the subquery too, so a shape like ... IN (SELECT v.a FROM v JOIN paimonAzure(...) AS p ON ...) still opens the remote table-function connection and waits on its timeout even though the final dump leaves that nested subquery unexpanded. The new catch (...) only hides the exception; it does not restore the previous "dump without side effects" behavior.
Can we fail-close here the same way the top-level helpers do and skip this nested access pass when checking it would require resolving unrelated table functions, or otherwise derive the view-specific access inputs without analyzing the whole nested subquery?
There was a problem hiding this comment.
You are right about the side effect, and I agree it is still there: the analysis resolves the other table expressions, so the connection and its timeout are paid even though the dump leaves the nested subquery unexpanded. That is the same thing I measured in the earlier thread (0.14s on master vs 15.1s on this branch against a blackholed address, identical before and after the catch). The catch fixes the error, not the side effect.
But I measured the proposed remedy, and skipping the nested pass when checking would require resolving unrelated table functions reopens the leak this PR closes.
Debug builds, build IDs asserted against SELECT lower(buildId()): pristine master f9801d56 (c29b56ab..., 0 occurrences of checkNestedSelectsViewBaseTableAccess) and this branch's nested block (cf11c5b1...). Fixture: base(a, secret) which the reader cannot select, v = SELECT a FROM base, plus an outer table pub the reader can select, so any denial is attributable to the nested pass.
SET enable_analyzer = 0;
EXPLAIN SYNTAX SELECT x FROM pub
WHERE x IN (SELECT v.a FROM v AS v JOIN numbers(3) AS n ON v.a = n.number);| arm | reader |
|---|---|
| pristine master | dump produced (the leak) |
| this branch | Code: 497 ... grant SELECT ON base |
| this branch, full user | dump produced |
The real SELECT for that shape is denied with the same grant and succeeds for the full user, so EXPLAIN SYNTAX is denied exactly where execution is denied. That nested subquery does require resolving an unrelated table expression (numbers(3)), so a "skip when resolution would be required" predicate skips it and hands the reader the dump again.
The asymmetry with the top level is what makes referencesCheckableTables sound there but not here. At the top level the predicate is a whole-query one and checkAccessRightsForQueryTree can only deny on a table identifier or a parameterized view, so an all-table-functions query has nothing to lose. The nested pass is entered only after referencesNonParameterizedView already returned true, i.e. only when the subquery does read a view whose base tables are the thing to check. A subquery mixing a view with a table function has both a checkable part and an unresolvable one, and one predicate cannot drop the resolution without dropping the denial.
Also, a predicate that has to resolve in order to learn whether resolution was needed cannot avoid the connection, so it does not recover the no-side-effect property either. A purely syntactic predicate ("mentions any table function") does avoid it, and that is the over-broad form measured above.
On what is lost today when the analysis does fail: with paimonAzure at an unroutable address the subquery is left unchecked and the reader gets the fallback dump, but grep for the base table name and for the hidden column is 0 there, with a positive control scoring 1 for a view in the main FROM. So the swallow costs a skipped check on an unresolvable subquery, never a disclosure, matching the reasoning already in the comment at the top of that function.
That leaves a design fork rather than a defect, and it is yours to pick: keep the residual side effect, or derive the view's access inputs without a full InterpreterSelectQuery analysis. The second removes the side effect properly but is a much bigger change than catch placement. I have not pushed anything; the measurements above are all I am adding here.
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Object file sizes11 object files changed (+87.32 KiB total), 0 added.
737 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only Compile time of recompiled translation units585 translation units recompiled, 1041 s compile time in total, 70 of them have a recent master baseline. Translation units without a recent master baseline:
|
2 participants
Closes: #78938
EXPLAIN QUERY TREEandEXPLAIN SYNTAX(in the analyzer) resolve the query and dump table metadata such as column names and types, but unlikeEXPLAIN PLANthey do not build a query plan. TheSELECTaccess check that the planner performs inprepareBuildQueryPlanForTableExpressionwas therefore skipped, so a user with no privileges could read the column names and data types (includingEnumelement lists) of tables they are not allowed to access, whileSELECT,EXPLAIN PLANandEXPLAIN PIPELINEare correctly rejected.This adds a
SELECTaccess check for every table referenced anywhere in the query tree (including tables inside subqueries in expressions such asWHERE x IN (SELECT ... FROM t)). The check mirrors the planner: it validates access to the columns that are actually read, with the trivial-count fallback (access is granted if at least one column is accessible) for queries that read no specific column, e.g.SELECT count() FROM t. As a result, a user with a column-level grant sees the same behavior as for a plainSELECT(EXPLAIN QUERY TREE SELECT granted_col FROM tis allowed,... other_col ...is denied).buildQueryTreeonly builds the tree; table identifiers are bound to storages and columns are resolved by the query analysis pass. The check runs on the resolved tree: onquery_treedirectly when the passes already resolved it, or on a throwaway resolved copy otherwise (e.g.EXPLAIN QUERY TREE run_passes = 0, which intentionally dumps the unresolved tree, so the tree that gets dumped is unchanged).Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix
EXPLAIN QUERY TREEandEXPLAIN SYNTAXnot checking access rights on the referenced tables, which allowed a user without privileges to read table column names and data types.Documentation entry for user-facing changes
🤖 Generated with Claude Code