sadpandajoe commented on code in PR #43388:
URL: https://github.com/apache/superset/pull/43388#discussion_r3834760049
##########
superset/mcp_service/__main__.py:
##########
@@ -77,12 +79,16 @@ def _add_default_middlewares() -> None:
for middleware in build_middleware_list():
mcp.add_middleware(middleware)
- # Response size guard is innermost (added last)
+ # Response size guard is innermost (added last), then response caching.
if size_guard := create_response_size_guard_middleware():
mcp.add_middleware(size_guard)
limit = size_guard.token_limit
sys.stderr.write(f"[MCP] Response size guard enabled
(token_limit={limit})\n")
+ if caching_middleware := create_response_caching_middleware():
Review Comment:
This installs response caching for this entrypoint, but the configured
exclusions do not include mutating tools such as `restore_chart` and
`restore_dashboard`. A repeated identical restore within the TTL can return the
cached success without executing the handler, leaving an object deleted after
it was deleted again. Could caching be limited to an explicit read-only
allowlist (or otherwise exclude every mutation) before this middleware is
enabled?
##########
superset/mcp_service/mcp_config.py:
##########
@@ -495,6 +506,16 @@ def create_default_mcp_auth_factory(app: Flask) ->
Optional[Any]:
if not (auth_enabled or api_key_enabled or guest_enabled):
return None
+ # MCP_DEV_USERNAME makes user resolution fall back to a fixed user for
+ # requests that carry no resolvable identity, which defeats the point of
+ # having transport auth enabled. Refuse the combination outright.
+ if auth_enabled and app.config.get("MCP_DEV_USERNAME"):
Review Comment:
This now refuses startup for a configuration the MCP README currently
presents as valid (`MCP_DEV_USERNAME = 'admin'` together with `MCP_AUTH_ENABLED
= True`). Deployments following that example will fail immediately on upgrade.
Could the documented development/production configurations and upgrade guidance
be updated with the required migration?
##########
tests/unit_tests/mcp_service/chart/tool/test_restore_chart.py:
##########
@@ -240,3 +271,70 @@ async def
test_restore_chart_rejects_boolean_identifier(mcp_server: object) -> N
async with Client(mcp_server) as client:
with pytest.raises(ToolError):
await client.call_tool("restore_chart", {"request": {"identifier":
True}})
+
+
+@patch(_FIND)
[email protected]
+async def test_restore_chart_inaccessible_chart_reads_as_not_found(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """A chart outside the caller's RBAC scope must not leak its existence or
+ title: the unfiltered restore lookup finds it, the base-filtered re-lookup
+ does not, so the tool must answer exactly as if it does not exist."""
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+
+ mock_find.side_effect = [
Review Comment:
This test predetermines the re-lookup result by call order, so it would
still pass if the second DAO query accidentally kept `skip_base_filter=True`.
That regression would turn the intended NotFound response for an inaccessible
chart into a disclosure. Could the test assert the exact second call (or make
the mock depend on `skip_base_filter`) to pin that boundary?
##########
superset/mcp_service/system/tool/find_users.py:
##########
@@ -75,7 +77,6 @@ async def find_users(request: FindUsersRequest, ctx: Context)
-> FindUsersRespon
user_model.username.ilike(needle, escape="\\"),
Review Comment:
The new privacy contract can still be bypassed where usernames are email
addresses, which is common with OAuth provisioning: querying
`[email protected]` matches `username` and confirms the account while
returning its identity fields. Could email-shaped queries be rejected (and
covered by a regression test) so the documented non-enumeration guarantee holds?
--
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]
