michael-s-molina opened a new pull request, #36739:
URL: https://github.com/apache/superset/pull/36739
### Summary
Migrates the MCP `execute_sql` tool to use the unified `Database.execute()`
API introduced in #36529. This consolidates SQL execution code and **fixes a
critical security gap where Row-Level Security (RLS) was not being applied** to
MCP queries.
#### Key Changes
- **Security Fix**: MCP queries now have RLS applied automatically via AST
transformation
- **Simplified Architecture**: Removed `sql_lab_utils.py` and
`execute_sql_core.py` (~300 lines), replaced with direct `Database.execute()`
call
- **Jinja2 Templates**: The old `{placeholder}` syntax is removed. Use
Jinja2 `{{ placeholder }}` syntax with `template_params`.
- **Multi-Statement Support**: Response now includes per-statement execution
info
- **New Options**: Added `catalog` and `dry_run` parameters
#### Feature Comparison
| Feature | Before | After | Notes
|
| ------------------------ | ---------------- | ------------------- |
--------------------------------------- |
| **Security** |
| Row-Level Security (RLS) | ❌ Not applied | ✅ Applied via AST |
**Critical fix** |
| DML Permission Check | ✅ Basic | ✅ Full | Uses
database `allow_dml` setting |
| Disallowed Functions | ✅ Basic | ✅ Full per-engine |
Engine-specific checks |
| Database Access Check | ✅ Yes | ✅ Yes | No
change |
| **Query Processing** |
| Jinja2 Templates | ❌ Not supported | ✅ Full support | New
`template_params` field |
| SQL Mutation Hook | ❌ Not applied | ✅ Applied | Custom
SQL transformations |
| Multi-Statement SQL | ❌ No | ✅ Yes | New
`statements` field in response |
| LIMIT Application | ✅ Simple regex | ✅ Proper parsing | More
reliable |
| **Execution** |
| Sync Execution | ✅ Yes | ✅ Yes | No
change |
| Timeout Handling | ⚠️ Basic | ✅ Full | Uses
database config |
| Query Logging | ❌ No | ✅ Via QUERY_LOGGER | If
configured |
| **Results** |
| Result Caching | ❌ No | ❌ No |
Explicitly disabled via `force_refresh` |
| Query Audit Trail | ❌ No | ✅ Yes | Query
model created for audit |
| **Parameters** |
| `database_id` | ✅ Yes | ✅ Yes | No
change |
| `sql` | ✅ Yes | ✅ Yes | No
change |
| `schema` | ✅ Yes | ✅ Yes | No
change |
| `catalog` | ❌ No | ✅ Yes | **New**
|
| `limit` | ✅ 1-10000 | ✅ 1-10000 | No
change |
| `timeout` | ✅ 1-300s | ✅ 1-300s | No
change |
| `template_params` | ❌ No | ✅ Yes | **New**
- Jinja2 parameters |
| `dry_run` | ❌ No | ✅ Yes | **New**
- Returns transformed SQL only |
### TESTING INSTRUCTIONS
- [x] All 12 `execute_sql` unit tests pass
- [x] All 252 MCP service tests pass
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
