cockroachdb / cockroachdb/cockroach
sql: savetable check for non-root user would be bypassed if query cached disabled
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
We now only check in `optPlanningCtx.buildReusableMemo()` if the savetable feature is enabled, which means this check is only reachable if query cache is enabled. The following test, with query cache disabled, will actually not reach the check.
```sql
statement ok
SET CLUSTER SETTING sql.query_cache.enabled = false;
statement ok
CREATE DATABASE savetables; USE savetables
statement ok
CREATE TABLE t (k INT PRIMARY KEY, str STRING)
statement ok
INSERT INTO t SELECT i, to_english(i) FROM generate_series(1, 5) AS g(i)
# Only root may use the saveTableNode.
statement ok
GRANT ALL ON t TO testuser
user testuser
statement ok
USE savetables
query IT rowsort
SELECT * FROM t
----
1 one
2 two
3 three
4 four
5 five
statement ok
SET save_tables_prefix = 'tt'
statement error sub-expression tables creation may only be used by root
SELECT * FROM t
```
The last `SELECT * FROM t` is expected to return error `sub-expression tables creation may only be used by root`, but because we disabled query cache, this statement would falsely succeed.
I think we should pull the following check outside of the logic that is only reachable when query cache is enabled:
```
if p.SessionData().SaveTablesPrefix != "" && !p.SessionData().User().IsRootUser() {
return nil, memoTypeUnknown, pgerror.New(pgcode.InsufficientPrivilege,
"sub-expression tables creation may only be used by root",
)
}
```
But also as a caveat, we don't want this check of overkill all queries that doesn't interact with the savetable feature. For example, if we enforce this check even for a `SET save_tables_prefix` statement, we will fall into a deadlock -- user wouldn't even be able to disable using savetables.
Jira issue: CRDB-56426
Contributor guide
Assessment
This issue has not been assessed yet.