bito-code-review[bot] commented on code in PR #43388:
URL: https://github.com/apache/superset/pull/43388#discussion_r3832238331
##########
tests/unit_tests/mcp_service/chart/tool/test_restore_chart.py:
##########
@@ -240,3 +244,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 = [
+ _mock_chart(chart_id=10, slice_name="Secret KPI"), # unfiltered lookup
+ None, # base-filtered re-lookup
+ ]
Review Comment:
<!-- Bito Reply -->
The decision to keep the tests self-contained is appropriate here. Since
each test covers a distinct scenario (RBAC-scope-invisible versus
visible-non-editor), maintaining them as independent units avoids unnecessary
coupling and keeps the test logic clear and focused on its specific requirement.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_restore_dashboard.py:
##########
@@ -248,3 +252,71 @@ async def
test_restore_dashboard_rejects_boolean_identifier(
await client.call_tool(
"restore_dashboard", {"request": {"identifier": True}}
)
+
+
+@patch(_FIND)
[email protected]
+async def test_restore_dashboard_inaccessible_dashboard_reads_as_not_found(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """A dashboard 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 = [
+ _mock_dashboard(dashboard_id=10, title="Secret Board"),
+ None, # base-filtered re-lookup
+ ]
Review Comment:
<!-- Bito Reply -->
The decision to keep the test logic separate is reasonable. Given that the
two scenarios represent distinct test cases with only minor shared setup,
avoiding coupling them through a helper function maintains test isolation and
clarity.
##########
tests/unit_tests/mcp_service/system/tool/test_find_users.py:
##########
@@ -336,3 +336,31 @@ async def
test_find_users_escapes_literal_backslash(mcp_server):
assert ilike_call is not None
assert ilike_call.args[0] == "%\\\\%"
assert ilike_call.kwargs.get("escape") == "\\"
+
+
[email protected]
+async def test_find_users_does_not_match_on_email(mcp_server):
+ """Email must not be a searchable column: substring or exact email
+ matching would let any MCP credential confirm which addresses have
+ accounts (an email-disclosure oracle the web API reserves for admins)."""
+ session, _ = _patch_user_query([])
Review Comment:
<!-- Bito Reply -->
The decision to maintain independent test cases is appropriate here. Keeping
each test case self-contained ensures that specific wildcard and escaping
scenarios remain clearly readable and avoids the potential complexity of shared
fixtures for these distinct test requirements.
--
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]
