Re: [PR] fix(database): move prequeries from event listeners to direct execution [superset]

via GitHub Thu, 16 Jul 2026 05:30:31 -0700


codeant-ai-for-open-source[bot] commented on code in PR #42079:
URL: https://github.com/apache/superset/pull/42079#discussion_r3595395202



##########
superset/models/core.py:
##########
@@ -686,6 +644,18 @@ def get_raw_connection(
         ) as engine:
             with check_for_oauth2(self):
                 with closing(engine.raw_connection()) as conn:
+                    prequeries = self.db_engine_spec.get_prequeries(
+                        database=self,
+                        catalog=catalog,
+                        schema=schema,
+                    )
+                    if prequeries:
+                        cursor = conn.cursor()
+                        try:
+                            for prequery in prequeries:
+                                cursor.execute(prequery)
+                        finally:
+                            cursor.close()

Review Comment:
   **Suggestion:** This change limits `get_prequeries` execution to 
`get_raw_connection`, so any code path that uses `get_sqla_engine` directly 
(for example inspector-based metadata calls) no longer runs required 
pre-session statements. That breaks engine specs that implement impersonation 
or session setup via prequeries (e.g. StarRocks `EXECUTE AS`), and can expose 
metadata using the service account instead of the effective user. Reintroduce 
prequery application for non-raw-connection engine usage (without dynamic 
listener mutation), such as applying prequeries on explicit connections used by 
inspector/metadata paths. [security]
   
   <details>
   <summary><b>Severity Level:</b> Critical 🚨</summary>
   
   ```mdx
   - ❌ StarRocks impersonation not applied for metadata inspector connections.
   - ❌ Database tables API uses service account for listing tables.
   - ⚠️ Streaming export queries ignore prequeries, bypassing user 
impersonation.
   - ⚠️ Schema default prequeries skipped for engine-only metadata operations.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Configure a Database row using the StarRocks engine spec with user 
impersonation
   enabled, so that `Database.db_engine_spec` is `StarRocksEngineSpec` and
   `database.impersonate_user` is True; see 
`superset/db_engine_specs/starrocks.py` lines
   380–439 where `get_prequeries()` returns an `EXECUTE AS` statement and the 
comment
   explicitly states “User impersonation is actually achieved via 
`get_prequeries`”.
   
   2. Observe that `Database.get_raw_connection()` in `superset/models/core.py` 
lines 632–659
   calls `self.db_engine_spec.get_prequeries(database=self, catalog=catalog, 
schema=schema)`
   (lines 647–651), then executes each returned prequery on the DBAPI 
connection cursor
   before yielding `conn`, so any SQL execution paths that use 
`get_raw_connection()` (e.g.
   SQL Lab execution paths in `superset/sql/execution/executor.py:538` and
   `superset/sql_lab.py:499`) still run the StarRocks `EXECUTE AS` 
impersonation prequeries.
   
   3. Follow an inspector-based metadata path, for example
   `Database.get_all_table_names_in_schema()` in `superset/models/core.py` 
lines 979–1005,
   which calls `self.get_inspector(catalog=catalog, schema=schema)` (line 997);
   `get_inspector()` (lines 1085–1091) uses `with 
self.get_sqla_engine(catalog=catalog,
   schema=schema) as engine:` and then `sqla.inspect(engine)`, meaning the 
Inspector obtains
   connections directly from the engine without going through 
`get_raw_connection()`.
   
   4. Inspect `Database.get_sqla_engine()` in `superset/models/core.py` lines 
455–507 and
   note that it now only calls `_get_sqla_engine()` and yields the shared 
engine (line 507)
   without invoking `get_prequeries()` or registering any `connect` event 
listener; a Grep
   for `get_prequeries` in the repo shows the only runtime callsite is inside
   `get_raw_connection()` (lines 647–651). Therefore, when the database tables 
API runs
   `TableListCommand.run()` in `superset/commands/database/tables.py` lines 
60–70, it calls
   `self._model.get_all_table_names_in_schema(...)`, which goes through 
`get_inspector()` and
   `get_sqla_engine()` and opens connections where StarRocks `EXECUTE AS` 
prequeries are
   never executed, so metadata inspection and similar engine-only operations 
run under the
   service account instead of the impersonated user.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7cf46323584240e9b727a1456ebe6d7d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7cf46323584240e9b727a1456ebe6d7d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/models/core.py
   **Line:** 647:658
   **Comment:**
        *Security: This change limits `get_prequeries` execution to 
`get_raw_connection`, so any code path that uses `get_sqla_engine` directly 
(for example inspector-based metadata calls) no longer runs required 
pre-session statements. That breaks engine specs that implement impersonation 
or session setup via prequeries (e.g. StarRocks `EXECUTE AS`), and can expose 
metadata using the service account instead of the effective user. Reintroduce 
prequery application for non-raw-connection engine usage (without dynamic 
listener mutation), such as applying prequeries on explicit connections used by 
inspector/metadata paths.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42079&comment_hash=8887cd81dc091cd993ce5f95f70a1b226d7253c67eeb17ab66b562a13571cb3c&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42079&comment_hash=8887cd81dc091cd993ce5f95f70a1b226d7253c67eeb17ab66b562a13571cb3c&reaction=dislike'>👎</a>



-- 
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]

Re: [PR] fix(database): move prequeries from event listeners to direct execution [superset]

Reply via email to