Skip to content

Commit fb0ba86

Browse files
seawindeCopilot
andcommitted
[fix](auth) Fix CTE privilege bypass due to shared privChecked flag on StatementContext
### What problem does this PR solve? Issue Number: N/A Related PR: #44621 Problem Summary: When a query uses a Common Table Expression (CTE) that references an unauthorized table, the privilege check is incorrectly skipped, allowing the query to succeed without proper authorization. **Root cause:** `CheckPrivileges.rewriteRoot()` uses `StatementContext.isPrivChecked()` as a guard to run only once. However, CTE processing (`RewriteCteChildren.visitLogicalCTEAnchor()`) creates separate `CascadesContext` subtrees for consumer and producer that **share** the same `StatementContext`. The consumer subtree is processed first, its `CheckPrivileges` sets `privChecked=true` on the shared `StatementContext`, and when the producer subtree runs, `CheckPrivileges` sees the flag and skips — leaving unauthorized tables in the CTE body unchecked. ``` StatementContext (shared) ├─ CascadesContext (consumer) ── CheckPrivileges runs ──> sets privChecked=true └─ CascadesContext (producer) ── CheckPrivileges sees true ──> SKIPPED (BUG) ``` **Fix:** Move the `privChecked` flag from `StatementContext` to `CascadesContext`. Since `newSubtreeContext()` creates a **new** `CascadesContext` instance for each CTE subtree, each subtree now has its own independent flag. This ensures both producer and consumer run their own privilege checks. Within the same `CascadesContext`, the flag still prevents duplicate checks (e.g., after view inlining — preserving #44621 semantics). ``` StatementContext (shared, no privChecked) ├─ CascadesContext (consumer) ── privChecked=false ── CheckPrivileges runs ── privChecked=true └─ CascadesContext (producer) ── privChecked=false ── CheckPrivileges runs ── privChecked=true ✓ ``` | File | Change Description | |------|-------------------| | `CascadesContext.java` | Add `privChecked` boolean field + getter/setter | | `StatementContext.java` | Remove `privChecked` field and accessors | | `CheckPrivileges.java` | Read/write `privChecked` on `CascadesContext` instead of `StatementContext` | | `TestCheckPrivileges.java` | Add `testCtePrivilegeCheck()` with 4 CTE authorization scenarios | | `TestCtePrivCheckGranularity.java` (new) | 4 unit tests verifying per-CascadesContext flag independence | | `test_cte_privilege_check.groovy` (new) | 6 regression test cases covering CTE privilege bypass | ### Release note Fixed a privilege bypass where queries using CTE (Common Table Expression) could access unauthorized tables without being denied. The privilege check was incorrectly shared across CTE subtrees, causing the producer's check to be skipped after the consumer's check ran. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - Behavior changed: - [x] No. - Does this need documentation? - [x] No. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 60f736e commit fb0ba86

6 files changed

Lines changed: 336 additions & 14 deletions

File tree

fe/fe-core/src/main/java/org/apache/doris/nereids/CascadesContext.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,8 @@ public class CascadesContext implements ScheduleContext {
120120

121121
private boolean isLeadingDisableJoinReorder = false;
122122

123+
private boolean privChecked = false;
124+
123125
private final Map<String, Hint> hintMap = Maps.newLinkedHashMap();
124126
private final ThreadLocal<Boolean> showPlanProcess = new ThreadLocal<>();
125127

@@ -520,6 +522,14 @@ public void setLeadingDisableJoinReorder(boolean leadingDisableJoinReorder) {
520522
isLeadingDisableJoinReorder = leadingDisableJoinReorder;
521523
}
522524

525+
public boolean isPrivChecked() {
526+
return privChecked;
527+
}
528+
529+
public void setPrivChecked(boolean privChecked) {
530+
this.privChecked = privChecked;
531+
}
532+
523533
public Map<String, Hint> getHintMap() {
524534
return hintMap;
525535
}

fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -263,8 +263,6 @@ public enum TableFrom {
263263

264264
private final Map<MvccTableInfo, MvccSnapshot> snapshots = Maps.newHashMap();
265265

266-
private boolean privChecked;
267-
268266
// if greater than 0 means the duration has used
269267
private long materializedViewRewriteDuration = 0L;
270268

@@ -1014,14 +1012,6 @@ public void setGroupCommitMergeBackend(
10141012
this.groupCommitMergeBackend = groupCommitMergeBackend;
10151013
}
10161014

1017-
public boolean isPrivChecked() {
1018-
return privChecked;
1019-
}
1020-
1021-
public void setPrivChecked(boolean privChecked) {
1022-
this.privChecked = privChecked;
1023-
}
1024-
10251015
public List<Backend> getUsedBackendsDistributing() {
10261016
return usedBackendsDistributing;
10271017
}

fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CheckPrivileges.java

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,20 +46,24 @@
4646

4747
/**
4848
* CheckPrivileges
49-
* This rule should only check once, because after check would set setPrivChecked in statementContext
49+
* This rule should only check once per CascadesContext, because after check would set privChecked
50+
* on the CascadesContext. Using CascadesContext-level (instead of StatementContext-level) flag
51+
* ensures CTE producer and consumer subtrees are checked independently, while still preventing
52+
* duplicate checks within the same subtree (e.g., after view inlining).
5053
*/
5154
public class CheckPrivileges extends ColumnPruning {
5255
private JobContext jobContext;
5356

5457
@Override
5558
public Plan rewriteRoot(Plan plan, JobContext jobContext) {
56-
// Only enter once, if repeated, the permissions of the table in the view will be checked
57-
if (jobContext.getCascadesContext().getStatementContext().isPrivChecked()) {
59+
// Only enter once per CascadesContext. If repeated within the same context,
60+
// the permissions of the table in the view will be checked incorrectly.
61+
if (jobContext.getCascadesContext().isPrivChecked()) {
5862
return plan;
5963
}
6064
this.jobContext = jobContext;
6165
super.rewriteRoot(plan, jobContext);
62-
jobContext.getCascadesContext().getStatementContext().setPrivChecked(true);
66+
jobContext.getCascadesContext().setPrivChecked(true);
6367
// don't rewrite plan, because Reorder expect no LogicalProject on LogicalJoin
6468
return plan;
6569
}

fe/fe-core/src/test/java/org/apache/doris/nereids/privileges/TestCheckPrivileges.java

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,76 @@ public void testPrivilegesAndPolicies() throws Exception {
225225
});
226226
}
227227

228+
@Test
229+
public void testCtePrivilegeCheck() throws Exception {
230+
FeConstants.runningUnitTest = true;
231+
// Use the same external catalog for privilege control infrastructure
232+
String catalogProvider
233+
= "org.apache.doris.nereids.privileges.TestCheckPrivileges$CustomCatalogProvider";
234+
String accessControllerFactory
235+
= "org.apache.doris.nereids.privileges.CustomAccessControllerFactory";
236+
String catalog = "custom_catalog_cte";
237+
238+
createCatalog("create catalog " + catalog + " properties("
239+
+ " \"type\"=\"test\","
240+
+ " \"catalog_provider.class\"=\"" + catalogProvider + "\","
241+
+ " \"" + CatalogMgr.ACCESS_CONTROLLER_CLASS_PROP + "\"=\"" + accessControllerFactory + "\""
242+
+ ")");
243+
244+
// Create internal OLAP tables (LogicalOlapScan supports withRelationId needed by CTE)
245+
String cteDb = "cte_priv_db";
246+
createDatabase(cteDb);
247+
useDatabase(cteDb);
248+
createTable("create table allowed_tbl (id int, name varchar(50)) "
249+
+ "distributed by hash(id) buckets 1 "
250+
+ "properties('replication_num' = '1')");
251+
createTable("create table denied_tbl (id int, name varchar(50)) "
252+
+ "distributed by hash(id) buckets 1 "
253+
+ "properties('replication_num' = '1')");
254+
255+
String user = "test_cte_privilege_user";
256+
addUser(user, true);
257+
useUser(user);
258+
259+
List<MakeTablePrivileges> privileges = ImmutableList.of(
260+
MakePrivileges.table("internal", cteDb, "allowed_tbl").allowSelectTable(user)
261+
);
262+
263+
AccessControllerManager accessManager = Env.getCurrentEnv().getAccessManager();
264+
CatalogAccessController catalogAccessController = accessManager.getAccessControllerOrDefault(catalog);
265+
new Expectations(accessManager) {
266+
{
267+
accessManager.getAccessControllerOrDefault("internal");
268+
minTimes = 0;
269+
result = catalogAccessController;
270+
}
271+
};
272+
273+
withPrivileges(privileges, () -> {
274+
// CTE with authorized table should succeed
275+
query("with cte as (select * from allowed_tbl) select * from cte");
276+
277+
// CTE with unauthorized table should fail (this was the bug:
278+
// consumer was checked first, setting privChecked=true on shared StatementContext,
279+
// so producer's CheckPrivileges was skipped)
280+
Assertions.assertThrows(AnalysisException.class, () ->
281+
query("with cte as (select * from denied_tbl) select * from cte")
282+
);
283+
284+
// CTE referenced twice forces no-inline path through RewriteCteChildren
285+
Assertions.assertThrows(AnalysisException.class, () ->
286+
query("with cte as (select * from denied_tbl) "
287+
+ "select * from cte union all select * from cte")
288+
);
289+
290+
// CTE authorized + direct unauthorized table should fail
291+
Assertions.assertThrows(AnalysisException.class, () ->
292+
query("with cte as (select * from allowed_tbl) "
293+
+ "select * from cte union all select * from denied_tbl")
294+
);
295+
});
296+
}
297+
228298
private void query(String sql) {
229299
PlanChecker.from(connectContext)
230300
.parse(sql)
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package org.apache.doris.nereids.privileges;
19+
20+
import org.apache.doris.nereids.CascadesContext;
21+
import org.apache.doris.nereids.properties.PhysicalProperties;
22+
23+
import org.junit.jupiter.api.Assertions;
24+
import org.junit.jupiter.api.Test;
25+
26+
import java.util.Optional;
27+
28+
/**
29+
* Tests that privChecked flag is per-CascadesContext, not per-StatementContext.
30+
* This ensures CTE producer and consumer subtrees check privileges independently.
31+
*/
32+
public class TestCtePrivCheckGranularity {
33+
34+
@Test
35+
public void testPrivCheckedDefaultFalse() {
36+
CascadesContext ctx = CascadesContext.initTempContext();
37+
Assertions.assertFalse(ctx.isPrivChecked(), "privChecked should default to false");
38+
}
39+
40+
@Test
41+
public void testPrivCheckedSetAndGet() {
42+
CascadesContext ctx = CascadesContext.initTempContext();
43+
44+
ctx.setPrivChecked(true);
45+
Assertions.assertTrue(ctx.isPrivChecked());
46+
47+
ctx.setPrivChecked(false);
48+
Assertions.assertFalse(ctx.isPrivChecked());
49+
}
50+
51+
@Test
52+
public void testSubtreeContextHasIndependentPrivChecked() {
53+
CascadesContext parentCtx = CascadesContext.initTempContext();
54+
55+
// Simulate CTE: create subtree context (shares StatementContext but different CascadesContext)
56+
CascadesContext subtreeCtx = CascadesContext.newSubtreeContext(
57+
Optional.empty(), parentCtx, parentCtx.getRewritePlan(), PhysicalProperties.ANY);
58+
59+
// Both should share the same StatementContext
60+
Assertions.assertSame(parentCtx.getStatementContext(), subtreeCtx.getStatementContext(),
61+
"Parent and subtree should share the same StatementContext");
62+
63+
// Both should start with privChecked = false
64+
Assertions.assertFalse(parentCtx.isPrivChecked());
65+
Assertions.assertFalse(subtreeCtx.isPrivChecked());
66+
67+
// Setting privChecked on subtree should NOT affect parent
68+
subtreeCtx.setPrivChecked(true);
69+
Assertions.assertTrue(subtreeCtx.isPrivChecked(), "subtree privChecked should be true");
70+
Assertions.assertFalse(parentCtx.isPrivChecked(), "parent privChecked should still be false");
71+
72+
// Setting privChecked on parent should NOT affect subtree
73+
parentCtx.setPrivChecked(true);
74+
Assertions.assertTrue(parentCtx.isPrivChecked());
75+
Assertions.assertTrue(subtreeCtx.isPrivChecked()); // still true from before
76+
}
77+
78+
@Test
79+
public void testTwoSubtreeContextsAreIndependent() {
80+
CascadesContext parentCtx = CascadesContext.initTempContext();
81+
82+
// Simulate CTE consumer and producer: two subtree contexts from same parent
83+
CascadesContext consumerCtx = CascadesContext.newSubtreeContext(
84+
Optional.empty(), parentCtx, parentCtx.getRewritePlan(), PhysicalProperties.ANY);
85+
CascadesContext producerCtx = CascadesContext.newSubtreeContext(
86+
Optional.empty(), parentCtx, parentCtx.getRewritePlan(), PhysicalProperties.ANY);
87+
88+
// Consumer runs CheckPrivileges first, sets its flag
89+
consumerCtx.setPrivChecked(true);
90+
91+
// Producer should NOT see consumer's flag — this is the bug fix
92+
Assertions.assertFalse(producerCtx.isPrivChecked(),
93+
"Producer CascadesContext should have independent privChecked from consumer");
94+
Assertions.assertFalse(parentCtx.isPrivChecked(),
95+
"Parent CascadesContext should not be affected by consumer's privChecked");
96+
}
97+
}

0 commit comments

Comments
 (0)