codeant-ai-for-open-source[bot] commented on code in PR #37973:
URL: https://github.com/apache/superset/pull/37973#discussion_r2819473419
##########
superset/mcp_service/auth.py:
##########
@@ -107,14 +116,32 @@ def get_user_from_request() -> User:
if hasattr(g, "user") and g.user:
return g.user
+ # Try API key authentication via FAB SecurityManager
+ # Only attempt when in a request context (not for MCP internal operations
+ # like tool discovery that run with only an application context)
+ from flask import has_request_context
+
+ api_key_enabled = current_app.config.get("FAB_API_KEY_ENABLED", False)
+ if api_key_enabled and has_request_context():
+ sm = current_app.appbuilder.sm
+ api_key_string = sm._extract_api_key_from_request()
+ if api_key_string is not None:
+ user = sm.validate_api_key(api_key_string)
+ if user:
+ return user
Review Comment:
**Suggestion:** When authenticating via API key, the user object returned by
the security manager is used directly without going through the
`load_user_with_relationships` helper, so nested relationships like
`Group.roles` may remain lazily loaded and cause detached-instance errors once
the session is closed or rolled back, which is exactly what the helper is meant
to prevent. Reload the user via `load_user_with_relationships` after successful
API key validation so all required relationships are eagerly loaded before MCP
tools run permission checks. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP tools may crash for API-key-authenticated requests.
- ❌ Permission checks can fail with DetachedInstanceError.
- ⚠️ API-key-based MCP automation becomes unreliable under errors.
- ⚠️ Debugging intermittent auth failures becomes significantly harder.
```
</details>
```suggestion
# Reload user with all relationships eagerly loaded to avoid
# detached-instance errors during later permission checks.
user_with_relationships = load_user_with_relationships(
username=user.username,
email=user.email,
)
return user_with_relationships or user
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. An MCP tool function is decorated with `mcp_auth_hook` defined in
`superset/mcp_service/auth.py` (see wrapper starting around line 176 in the
final file).
2. The tool is invoked over HTTP with `Authorization: Bearer sst_...` while
`FAB_API_KEY_ENABLED` is True and the call is in a Flask request context, so
`_setup_user_context()` (around line 120) calls `get_user_from_request()`
(around line
88).
3. Inside `get_user_from_request()` at lines 124–135, the code executes
`sm.validate_api_key(api_key_string)` and returns the resulting `user`
directly, without
using `load_user_with_relationships()` (defined earlier in the same file) to
eagerly load
`roles`, `groups`, and `Group.roles`.
4. Later in the same MCP call, permission logic (e.g. via
`has_dataset_access()` at line
152 which uses `security_manager.can_access_datasource(datasource=dataset)`
and relies on
`g.user`'s relationships) runs after the SQLAlchemy session has been rolled
back by
`_cleanup_session_on_error()` at line 212 or by other Superset DB lifecycle
code, causing
a `DetachedInstanceError` when lazy-loading `user.groups` or `group.roles`
from the user
obtained via the API key path. The explicit helper
`load_user_with_relationships()` exists
to avoid exactly these detached-instance errors, but is currently bypassed
for
API-key-authenticated users.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 131:131
**Comment:**
*Logic Error: When authenticating via API key, the user object returned
by the security manager is used directly without going through the
`load_user_with_relationships` helper, so nested relationships like
`Group.roles` may remain lazily loaded and cause detached-instance errors once
the session is closed or rolled back, which is exactly what the helper is meant
to prevent. Reload the user via `load_user_with_relationships` after successful
API key validation so all required relationships are eagerly loaded before MCP
tools run permission checks.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=91be4d49f03aa3ffe4de79a68b5b461f7e422b8bbc035369b0da1ca77adaf72e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=91be4d49f03aa3ffe4de79a68b5b461f7e422b8bbc035369b0da1ca77adaf72e&reaction=dislike'>👎</a>
##########
tests/unit_tests/security/api_test.py:
##########
@@ -29,6 +29,7 @@ def test_csrf_not_exempt(app_context: None) -> None:
Test that REST API is not exempt from CSRF.
"""
assert {blueprint.name for blueprint in csrf._exempt_blueprints} == {
+ "ApiKeyApi",
Review Comment:
**Suggestion:** Adding the API key management blueprint to the CSRF
exemption list means those endpoints can be called without a CSRF token when
using cookie-based authentication, which allows cross-site requests to create
or revoke API keys on behalf of a logged-in user; the API key endpoints should
not be CSRF-exempt. Update the expected exempt blueprints set to exclude the
API key API so that configuration and tests enforce CSRF protection for these
sensitive operations. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ API key creation bypasses CSRF for logged-in users.
- ❌ API key revocation vulnerable to cross-site request forgery.
- ⚠️ Admin UI API key management lacks CSRF protection.
- ⚠️ Security weaker than other protected REST endpoints.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest tests/unit_tests/security/api_test.py::test_csrf_not_exempt`;
the fixture
at `tests/unit_tests/security/api_test.py:22-26` creates an app with
`{"WTF_CSRF_ENABLED":
True}`.
2. In `test_csrf_not_exempt` at
`tests/unit_tests/security/api_test.py:31-41`, observe
that the assertion requires `"ApiKeyApi"` to be present in `{blueprint.name
for blueprint
in csrf._exempt_blueprints}`, proving that all routes in the `ApiKeyApi`
blueprint are
excluded from CSRF checks.
3. Start the Superset web server with `WTF_CSRF_ENABLED = True` (same
configuration used
in the test), then log in through the browser so that a valid session cookie
is set for
the Superset domain.
4. From a different origin (e.g., `https://attacker.example`), host an HTML
page that
auto-submits a POST form to any state-changing endpoint registered under the
`ApiKeyApi`
blueprint (the exact URL can be obtained by inspecting `app.url_map` entries
for the
`"ApiKeyApi"` blueprint in a Python shell against the running app); when the
logged-in
user visits this page, the browser sends the POST with Superset cookies but
no CSRF token,
and because `ApiKeyApi` is listed in `csrf._exempt_blueprints` per step 2,
the request is
accepted and the API key create/revoke action succeeds instead of being
rejected with a
CSRF error.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/api_test.py
**Line:** 32:32
**Comment:**
*Security: Adding the API key management blueprint to the CSRF
exemption list means those endpoints can be called without a CSRF token when
using cookie-based authentication, which allows cross-site requests to create
or revoke API keys on behalf of a logged-in user; the API key endpoints should
not be CSRF-exempt. Update the expected exempt blueprints set to exclude the
API key API so that configuration and tests enforce CSRF protection for these
sensitive operations.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=d9df211586571588dbf7849909735bf2144183b90d02ec4013eb3af5473256c7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=d9df211586571588dbf7849909735bf2144183b90d02ec4013eb3af5473256c7&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]
