[fix](auth) Fix CTE privilege bypass due to shared privChecked flag on StatementContext by seawinde · Pull Request #62339 · apache/doris

seawinde · 2026-04-10T09:13:20Z

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
- Regression test
- Unit Test
Behavior changed:
- No.
Does this need documentation?
- No.

Check List (For Reviewer who merge this PR)

Confirm the release note
Confirm test cases
Confirm document
Add branch pick label

hello-stephen · 2026-04-10T09:13:27Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

What problem was fixed (it's best to include specific error reporting information). How it was fixed.
Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
What features were added. Why was this function added?
Which code was refactored and why was this part of the code refactored?
Which functions were optimized and what is the difference before and after the optimization?

seawinde · 2026-04-10T09:14:06Z

run buildall

seawinde · 2026-04-10T09:15:01Z

run buildall

seawinde · 2026-04-10T09:16:20Z

run buildall

seawinde · 2026-04-13T02:35:40Z

run buildall

seawinde · 2026-04-13T02:36:06Z

run buildall

hello-stephen · 2026-04-13T04:02:05Z

FE UT Coverage Report

Increment line coverage 100.00% (6/6) 🎉
Increment coverage report
Complete coverage report

### What problem does this PR solve? Issue Number: N/A Related PR: apache#62339 Problem Summary: Fix two test failures introduced in the CTE privilege bypass fix: 1. **test_cte_privilege_check.groovy**: The regression test user lacked database-level access to `regression_test`, causing `connect()` to fail before any CTE query was executed. Added `grant select_priv on regression_test` to the test setup. 2. **TestCheckPrivileges.testCtePrivilegeCheck**: The new test method called `useUser("test_cte_privilege_user")` on the shared `connectContext` but never restored it. When `testPrivilegesAndPolicies` ran afterward, the unprivileged user caused `createCatalog` to throw `AnalysisException` instead of the expected `DdlException`. Wrapped the test body in try-finally to restore root user. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test fix - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

seawinde · 2026-04-13T06:55:08Z

run buildall

### What problem does this PR solve? Issue Number: N/A Related PR: apache#62339 Problem Summary: Fix two test failures introduced in the CTE privilege bypass fix: 1. **test_cte_privilege_check.groovy**: The regression test user lacked database-level access to `regression_test`, causing `connect()` to fail before any CTE query was executed. Added `grant select_priv on regression_test` to the test setup. 2. **TestCheckPrivileges.testCtePrivilegeCheck**: The new test method called `useUser("test_cte_privilege_user")` on the shared `connectContext` but never restored it. When `testPrivilegesAndPolicies` ran afterward, the unprivileged user caused `createCatalog` to throw `AnalysisException` instead of the expected `DdlException`. Wrapped the test body in try-finally to restore root user. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test fix - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

seawinde · 2026-04-13T07:50:34Z

run buildall

seawinde · 2026-04-13T09:20:31Z

run buildall

hello-stephen · 2026-04-13T12:33:18Z

FE UT Coverage Report

Increment line coverage 42.86% (6/14) 🎉
Increment coverage report
Complete coverage report

seawinde · 2026-04-14T09:36:44Z

/review

github-actions

Requesting changes for one blocking issue in the new tests.

Critical checkpoints:

Goal: The production change does address the reported CTE privilege bypass by making privChecked scoped to each CascadesContext, and the follow-up DeleteFromCommand change correctly restores the intended skipAuth behavior for the fallback planner. However, the new unit test still introduces shared-state leakage, so the PR is not yet safe as submitted.
Scope/minimality: The production fix is small and focused. The delete follow-up is also narrowly scoped.
Concurrency/locking: I did not find new lock-order or concurrency issues in the touched production paths.
Lifecycle/static state: This is the blocking area. The new unit test mutates the process-wide Env.accessManager singleton and does not restore it, so later tests can observe a mocked access controller.
Config/compatibility: No config or compatibility concerns in the production changes.
Parallel code paths: The delete fallback path was updated consistently with the initial DELETE planner auth policy.
Special conditions: The skipAuth condition in DeleteFromCommand now matches the existing DELETE semantics.
Test coverage: Regression and unit coverage were added for the CTE bug and for the per-context flag behavior, but the new unit test is order-dependent because it leaves global FE auth state modified.
Observability: No additional observability appears necessary for this fix.
Transaction/persistence/data writes: No persistence changes; the DELETE auth regression path is handled in-memory only.
FE/BE variable passing: Not applicable.
Performance: No material performance concerns in the touched production code.
Other issues: See the inline comment on the leaking test setup.

Because TestWithFeService runs against one shared FE environment, the test isolation bug is enough to block merge until the original AccessControllerManager is restored in finally (or the mocking is otherwise scoped without mutating Env).

github-actions · 2026-04-14T09:44:37Z

+                        query("with cte as (select * from denied_tbl) "
+                                + "select * from cte union all select * from cte")
+                );
+


Env.getCurrentEnv().accessManager is process-global shared FE state, but this test never restores the original instance after swapping in the spy. TestWithFeService keeps one FE env across test methods, so any later test in this class or another class can now observe the mocked controller and become order-dependent. testPrivilegesAndPolicies() already showed this class of leak with useUser(...); this is the same problem at global-auth scope. Please snapshot the original AccessControllerManager before replacing it and restore it in finally after the assertions complete.

### What problem does this PR solve? Issue Number: None Related PR: apache#62339 Problem Summary: Restore the original AccessControllerManager and test user after CTE privilege tests replace process-global FE auth state, preventing order-dependent leakage across TestWithFeService test methods. ### Release note None ### Check List (For Author) - Test: No test (not run locally) - Behavior changed: No - Does this need documentation: No

seawinde · 2026-04-15T02:10:39Z

run buildall

hello-stephen · 2026-04-15T05:12:55Z

FE Regression Coverage Report

Increment line coverage 9.85% (13/132) 🎉
Increment coverage report
Complete coverage report

…n StatementContext ### What problem does this PR solve? Issue Number: N/A Related PR: apache#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 apache#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>

### What problem does this PR solve? Issue Number: N/A Related PR: apache#62339 Problem Summary: Fix two test failures introduced in the CTE privilege bypass fix: 1. **test_cte_privilege_check.groovy**: The regression test user lacked database-level access to `regression_test`, causing `connect()` to fail before any CTE query was executed. Added `grant select_priv on regression_test` to the test setup. 2. **TestCheckPrivileges.testCtePrivilegeCheck**: The new test method called `useUser("test_cte_privilege_user")` on the shared `connectContext` but never restored it. When `testPrivilegesAndPolicies` ran afterward, the unprivileged user caused `createCatalog` to throw `AnalysisException` instead of the expected `DdlException`. Wrapped the test body in try-finally to restore root user. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test fix - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

### What problem does this PR solve? Problem Summary: After PR apache#62221 removed all JMockit usage from the codebase, the testCtePrivilegeCheck method still used JMockit Expectations pattern which causes compilation failure. Convert to Mockito spy pattern consistent with the rest of the test file. ### Release note None ### Check List (For Author) - Test: Unit Test (compilation verified) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…Command ### What problem does this PR solve? Problem Summary: Moving privChecked from StatementContext to CascadesContext (for the CTE privilege bypass fix) caused a regression in DELETE commands with complex WHERE clauses (e.g. NOT EXISTS subquery). DeleteFromCommand sets skipAuth=true during initial Nereids planning because DELETE does not need SELECT privilege. Previously, the privChecked flag on StatementContext was shared across planners, so when DeleteFromUsingCommand created a new planner, CheckPrivileges would see privChecked=true and skip. After moving privChecked to CascadesContext, the new planner gets a fresh CascadesContext with privChecked=false, causing CheckPrivileges to run auth checks without skipAuth=true, failing on tables the user only has LOAD privilege for. Fix: Wrap both DeleteFromUsingCommand.run() call sites in skipAuth=true, consistent with the initial planning phase's auth policy. ### Release note None ### Check List (For Author) - Test: Regression test (test_dml_delete_table_auth) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

seawinde · 2026-04-15T09:44:25Z

run buildall

hello-stephen · 2026-04-15T12:13:51Z

FE Regression Coverage Report

Increment line coverage 92.86% (13/14) 🎉
Increment coverage report
Complete coverage report

morrySnow · 2026-04-16T03:42:16Z

duplicate to #61741

### What problem does this PR solve? Issue Number: N/A Related PR: apache#62339 Problem Summary: Fix two test failures introduced in the CTE privilege bypass fix: 1. **test_cte_privilege_check.groovy**: The regression test user lacked database-level access to `regression_test`, causing `connect()` to fail before any CTE query was executed. Added `grant select_priv on regression_test` to the test setup. 2. **TestCheckPrivileges.testCtePrivilegeCheck**: The new test method called `useUser("test_cte_privilege_user")` on the shared `connectContext` but never restored it. When `testPrivilegesAndPolicies` ran afterward, the unprivileged user caused `createCatalog` to throw `AnalysisException` instead of the expected `DdlException`. Wrapped the test body in try-finally to restore root user. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test fix - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

seawinde · 2026-04-16T11:53:16Z

Rebased on latest upstream/master. run buildall

seawinde force-pushed the fix_cte_privilege_bypass branch from fb0ba86 to f0c436f Compare April 13, 2026 02:35

seawinde force-pushed the fix_cte_privilege_bypass branch from a205c6c to 378911c Compare April 13, 2026 07:50

github-actions bot requested changes Apr 14, 2026

View reviewed changes

seawinde and others added 4 commits April 15, 2026 17:43

seawinde force-pushed the fix_cte_privilege_bypass branch from cc3ede7 to e4facdf Compare April 15, 2026 09:44

morrySnow closed this Apr 16, 2026

Conversation

seawinde commented Apr 10, 2026

What problem does this PR solve?

Release note

Check List (For Author)

Check List (For Reviewer who merge this PR)

Uh oh!

hello-stephen commented Apr 10, 2026

Uh oh!

seawinde commented Apr 10, 2026

Uh oh!

seawinde commented Apr 10, 2026

Uh oh!

seawinde commented Apr 10, 2026

Uh oh!

seawinde commented Apr 13, 2026

Uh oh!

seawinde commented Apr 13, 2026

Uh oh!

hello-stephen commented Apr 13, 2026

FE UT Coverage Report

Uh oh!

seawinde commented Apr 13, 2026

Uh oh!

seawinde commented Apr 13, 2026

Uh oh!

seawinde commented Apr 13, 2026

Uh oh!

hello-stephen commented Apr 13, 2026

FE UT Coverage Report

Uh oh!

seawinde commented Apr 14, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

seawinde commented Apr 15, 2026

Uh oh!

hello-stephen commented Apr 15, 2026

FE Regression Coverage Report

Uh oh!

seawinde commented Apr 15, 2026

Uh oh!

hello-stephen commented Apr 15, 2026

FE Regression Coverage Report

Uh oh!

morrySnow commented Apr 16, 2026

Uh oh!

seawinde commented Apr 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants