github-actions[bot] commented on PR #61741:
URL: https://github.com/apache/doris/pull/61741#issuecomment-4132720816
## Code Review Summary
**Verdict: No issues found.** The change is correct, minimal, and
well-tested.
### Change Overview
This PR fixes a security bug (Ranger column-level privilege bypass with
CTE+JOIN) by moving the `privChecked` flag from `StatementContext` (shared
across all CTE subtrees) to `CascadesContext` (per-subtree). This ensures each
CTE subtree gets its own independent privilege check.
### Critical Checkpoint Conclusions
- **Goal and correctness**: The goal is to fix a privilege bypass when CTE
is referenced multiple times via JOIN (preventing CTE inlining). The fix
correctly moves `privChecked` to `CascadesContext`, since `RewriteCteChildren`
creates a new `CascadesContext` per CTE subtree (via `newSubtreeContext()`),
each starting with `privChecked = false`. Tests prove the fix works.
- **Modification minimality**: The change touches 4 files with +41/-14
lines. It is focused solely on moving the flag and adding tests. No unnecessary
changes.
- **Concurrency**: `privChecked` is a non-volatile boolean on
`CascadesContext`. This is safe because each `CascadesContext` is created and
used within a single thread during the rewrite phase. No concurrent access
concerns.
- **View privilege regression risk**: The original comment ("Only enter
once, if repeated, the permissions of the table in the view will be checked")
referred to avoiding re-checking after `InlineLogicalView` exposes base tables.
This is still safe because: (1) `CheckPrivileges` runs before
`InlineLogicalView` in the same topic block in `Rewriter.java:497-507`, (2) the
per-`CascadesContext` guard prevents re-execution within the same context, and
(3) `CheckPrivileges` appears only once in the entire job list
(`CTE_CHILDREN_REWRITE_JOBS_BEFORE_SUB_PATH_PUSH_DOWN`).
- **Other callers**: `isPrivChecked`/`setPrivChecked` are only called from
`CheckPrivileges.rewriteRoot()`. The `CreateMaterializedViewCommand.validate()`
also invokes `CheckPrivileges` but uses its own fresh `CascadesContext`, so the
fix is compatible.
- **Parallel code paths**: No other parallel code paths affected. The change
is isolated to the Nereids privilege checking mechanism.
- **Incompatible changes**: None. No storage format, function symbol, or
protocol changes.
- **Configuration items**: None added.
- **Test coverage**: Four new CTE+JOIN test cases added covering: (1)
fully-privileged CTE+LEFT JOIN succeeds, (2) restricted column CTE+LEFT JOIN
denied, (3) allowed-columns-only CTE+LEFT JOIN succeeds, (4) restricted column
CTE+INNER JOIN denied. Tests exercise the per-`CascadesContext` privilege
checking path since multi-reference CTEs trigger `RewriteCteChildren` with
separate subtree contexts. Existing view and base table privilege tests
continue to validate no regression.
- **Performance**: No performance impact. The `privChecked` flag is a simple
boolean check.
- **Other issues**: None identified.
--
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]
