From 4bd1919f77610cbdcc5f9c4f796dc77d490b05e6 Mon Sep 17 00:00:00 2001
From: ZdenekSrotyr <zdenek.srotyr@keboola.com>
Date: Mon, 4 May 2026 14:18:43 +0200
Subject: [PATCH] =?UTF-8?q?fix(query):=20#168=20review=20iter=205=20?=
 =?UTF-8?q?=E2=80=94=20forbidden-table=20check=20uses=20registry=20IDs?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 app/api/query.py | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/app/api/query.py b/app/api/query.py
index 549a31b..ff04d57 100644
--- a/app/api/query.py
+++ b/app/api/query.py
@@ -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):