Repository navigation
fix(core): prune inaccessible CLS columns in wildcard expansion instead of denying - #2449
Conversation
…ad of denying A model referenced with a table alias (e.g. `FROM customer c`) makes ModelAnalyzeRule build a throwaway inner ModelPlanNode with empty required fields: the required columns are keyed by the alias, but the bottom-up transform processes the bare inner TableScan first under the model name and finds none. Empty required fields trigger the wildcard expansion in RelationChain::source, which expanded every physical column and hard-denied any column-level-access-controlled column the caller cannot read — even one the query never referenced. This made any aliased query against a model with a CLS column fail, and also broke `SELECT count(*)` on such models. Prune inaccessible CLS columns in the wildcard expansion (validate_clac_rule + continue), matching `SELECT *` semantics. Explicit references still deny: they arrive as named required fields via ModelPlanNodeBuilder::build, not this path. Pruning a column can never leak it. Adds a regression test covering aliased/unaliased joins and standalones, count(*), qualified/bare wildcards (all pruned), plus explicit references in SELECT/WHERE/GROUP BY/JOIN ON and a second CLS column (all still denied).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughWildcard expansion in ModelSourceNode::new now validates each physical column against column-level access-control rules (CLAC), pruning inaccessible columns instead of unconditionally including them. A regression test verifies unreferenced protected columns are pruned while explicitly referenced ones still deny. ChangesCLAC Wildcard Pruning
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
goldmedal
left a comment
There was a problem hiding this comment.
Thanks @fmcmac, makes sense to me.
One note for future readers (no change requested): the aliased-scan case here routes through a build → discard → rebuild — under the bottom-up transform the bare TableScan is visited before its SubqueryAlias, looked up by model name instead of the alias, so required columns miss → empty → wildcard expansion → a ModelPlanNode that's then discarded and rebuilt by analyze_subquery_alias_model.
That's a separate, behavior-neutral inefficiency (and fixing it wouldn't remove the need for this PR — genuine count(*) still needs the prune here). Opened #2451 to track it. Not a blocker.
… scans For a model scanned through a table alias (FROM customer AS e), the bottom-up ModelAnalyzeRule walk visits the bare TableScan before its SubqueryAlias parent. Required columns are keyed by the alias, so this first pass finds none and builds a ModelPlanNode via the wildcard- expansion path; analyze_subquery_alias_model then discards it and rebuilds the node correctly once the alias is known. The first build runs the full RLAC/CLAC parsing for nothing. PR Canner#2449 already fixed the correctness bug this caused (the throwaway build was denying access to columns under column-level security instead of pruning them); the wasted build itself remained. This adds a pre-order TreeNodeRewriter pass that recognizes SubqueryAlias -> TableScan for a model directly and builds the ModelPlanNode once, with the alias's real required columns, skipping the discarded intermediate build. Everything else keeps using the original bottom-up analyze_model_internal pass. Fixes Canner#2451
… scans For a model scanned through a table alias (FROM customer AS e), the bottom-up ModelAnalyzeRule walk visits the bare TableScan before its SubqueryAlias parent. Required columns are keyed by the alias, so this first pass finds none and builds a ModelPlanNode via the wildcard- expansion path; analyze_subquery_alias_model then discards it and rebuilds the node correctly once the alias is known. The first build runs the full RLAC/CLAC parsing for nothing. PR Canner#2449 already fixed the correctness bug this caused (the throwaway build was denying access to columns under column-level security instead of pruning them); the wasted build itself remained. This adds a pre-order TreeNodeRewriter pass that recognizes SubqueryAlias -> TableScan for a model directly and builds the ModelPlanNode once, with the alias's real required columns, skipping the discarded intermediate build. Everything else keeps using the original bottom-up analyze_model_internal pass. Fixes Canner#2451
… scans For a model scanned through a table alias (FROM customer AS e), the bottom-up ModelAnalyzeRule walk visits the bare TableScan before its SubqueryAlias parent. Required columns are keyed by the alias, so this first pass finds none and builds a ModelPlanNode via the wildcard- expansion path; analyze_subquery_alias_model then discards it and rebuilds the node correctly once the alias is known. The first build runs the full RLAC/CLAC parsing for nothing. PR #2449 already fixed the correctness bug this caused (the throwaway build was denying access to columns under column-level security instead of pruning them); the wasted build itself remained. This adds a pre-order TreeNodeRewriter pass that recognizes SubqueryAlias -> TableScan for a model directly and builds the ModelPlanNode once, with the alias's real required columns, skipping the discarded intermediate build. Everything else keeps using the original bottom-up analyze_model_internal pass. Fixes #2451
Problem
Column-level access control (CLAC/CLS) denies any query that references a model with a table alias, even when the protected column is never selected.
SELECT * FROM modelcorrectly prunes an inaccessible CLS column, butSELECT alias.other_col FROM model alias(or a join using an alias) fails withAccess denied to column … violates access control rule ….SELECT count(*) FROM modelon a model with a CLS column is denied for the same reason.Root cause
For
FROM customer c, the plan isProjection → SubqueryAlias(c) → TableScan(customer).ModelAnalyzeRulecollects required columns keyed by the aliasc, but the bottom-up transform processes the inner bareTableScan(customer)first with no alias, looks required columns up under the model name, and finds none. That builds a throwaway innerModelPlanNodewith empty required fields, which triggers the wildcard expansion inRelationChain::source(ModelSourceNode::new). That loop expanded every physical column and calledget_remote_column_expper column, which hard-denies viavalidate_clac_rule.analyze_subquery_alias_modellater rebuilds the node correctly with the alias's required columns, but the throwaway build errors first.SELECT *avoids this because it doesn't route through the empty-required wildcard path.Fix
In the wildcard-expansion loop, prune columns the caller cannot access (
validate_clac_rule+continue) instead of denying — matchingSELECT *semantics. This path is an implicit all-columns expansion (count(*), or the aliased throwaway plan); a protected column the query never explicitly selected should be dropped, not turned into a hard error. Explicit references still deny, because they arrive as named required fields throughModelPlanNodeBuilder::build(the non-wildcard branch), not this path. Pruning a column can only ever remove it, so it cannot widen access.Tests
Adds
test_clac_unreferenced_column_pruned_not_denied:count(*), model-qualifiedalias.*, and bare*— none reference the protected columns.Verified locally against
main: the new test passes and the fix is a minimal, localized change inModelSourceNode::new.Summary by CodeRabbit
count(*)/*scenarios.