fix(query): #168 review iter 5 — forbidden-table check uses registry IDs
Devin Review iter #5 flagged a pre-existing class of name/id mismatch in app/api/query.py:131-136 — the SAME root cause as the bq.* RBAC issue I fixed in iter #3 (line 332/362). Devin called it out as "NOT introduced by this PR" / "might merit follow-up", but it's exactly the same security-boundary pattern this PR is hardening, so fixing here keeps the RBAC story consistent across the handler. The `forbidden = all_views - set(allowed)` comparison mixed types: - `all_views` carries DuckDB master view names (= registry display `name` from the orchestrator's CREATE VIEW) - `set(allowed)` carries registry IDs (resource_grants.resource_id) When `id != name` (e.g. id="bq.finance.ue", name="ue"), authorized users got spurious 403s — the view name landed in `forbidden` even though the caller had a valid grant on the registry id. Build a name->id map from the registry, then the forbidden check compares apples to apples: allowed_view_names = {r["name"] for r in registry_rows if r.get("name") and r.get("id") in allowed_ids} forbidden = all_views - allowed_view_names 107 affected tests pass; 487 pass in wider RBAC/query/access/admin domain — no regressions.
This commit is contained in:
parent
a43533ca44
commit
4bd1919f77
1 changed files with 15 additions and 1 deletions
|
|
@ -128,8 +128,22 @@ async def execute_query(
|
|||
"SELECT table_name FROM information_schema.tables WHERE table_type='VIEW'"
|
||||
).fetchall()}
|
||||
|
||||
# `allowed` carries registry IDs (resource_grants.resource_id);
|
||||
# DuckDB master views are named by registry display `name`.
|
||||
# Build a name->id map so the forbidden check compares apples to
|
||||
# apples — when id != name, the prior `all_views - set(allowed)`
|
||||
# over-denied authorized users (Devin Review iter #5 on PR #168;
|
||||
# pre-existing class of name/id mismatch flagged across this
|
||||
# PR's BQ guardrail too).
|
||||
allowed_ids = set(allowed)
|
||||
registry_rows = TableRegistryRepository(conn).list_all()
|
||||
allowed_view_names = {
|
||||
r["name"] for r in registry_rows
|
||||
if r.get("name") and r.get("id") in allowed_ids
|
||||
}
|
||||
|
||||
# Check if query references any forbidden tables (word-boundary match)
|
||||
forbidden = all_views - set(allowed)
|
||||
forbidden = all_views - allowed_view_names
|
||||
for table in forbidden:
|
||||
pattern = r'\b' + re.escape(table.lower()) + r'\b'
|
||||
if re.search(pattern, sql_lower):
|
||||
|
|
|
|||
Loading…
Reference in a new issue