deepujain commented on code in PR #44558:
URL: https://github.com/apache/superset/pull/44558#discussion_r4079757503
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1087,6 +1094,100 @@ def generate(
return strategy.generate()
+async def _generate_png_preview(
+ chart_id: int, request: GetChartPreviewRequest
+) -> PNGPreview | ChartError:
+ """Render saved charts as the caller in an isolated browser and app
context."""
+ if (
+ not chart_id
+ or request.form_data_key
+ or request.extra_form_data
+ or guest_scope.is_guest_read()
+ ):
+ return ChartError(
+ error="PNG previews require a saved chart and a non-guest user, "
+ "without unsaved state or extra filters.",
+ error_type="UnsupportedFormat",
+ )
+ user_id = getattr(getattr(g, "user", None), "id", None)
+ if not isinstance(user_id, int):
+ return ChartError(error="Authentication required",
error_type="Forbidden")
+ width = 800 if request.width is None else request.width
+ height = 600 if request.height is None else request.height
+ if not (64 <= width <= 4096 and 64 <= height <= 4096):
+ return ChartError(
+ error="PNG dimensions must be between 64 and 4096 pixels.",
+ error_type="ValidationError",
+ )
+
+ def render() -> PNGPreview | ChartError:
+ from superset import is_feature_enabled
+ from superset.mcp_service.auth import _mcp_tool_call_context
+ from superset.utils.core import override_user
+ from superset.utils.screenshots import validate_screenshot_image
+ from superset.utils.urls import get_url_path
+ from superset.utils.webdriver import (
+ _PlaywrightBrowserManager,
+ WebDriverPlaywright,
+ )
+
+ # Resolve ORM objects in a fresh context; neither the request session
nor
+ # Flask's mutable g is shared with the rendering worker.
+ with _mcp_tool_call_context():
+ user = security_manager.find_user(id=user_id)
+ if user is None or not user.is_active:
+ return ChartError(
+ error="Authentication required", error_type="Forbidden"
+ )
+ with override_user(user):
+ chart = find_chart_by_identifier(chart_id)
+ if chart is None:
+ return ChartError(error="Chart not found",
error_type="NotFound")
+ security_manager.raise_for_access(chart=chart)
Review Comment:
Valid catch. Fixed in 3522c875: `SupersetSecurityException` from
`raise_for_access` is now caught explicitly around the render worker and mapped
to `Forbidden`, instead of falling into the broad `except Exception` that
returned `RenderError`.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -1839,3 +1839,250 @@ def
test_saved_gauge_preview_skips_empty_aggregate_groups(
else:
assert "Blue" in result.ascii_content
assert "Empty" not in result.ascii_content
+
+
[email protected]
[email protected]("user_id", [11, 29])
+async def test_png_preview_renders_as_caller_in_isolated_context(app_context,
user_id):
+ import base64
+ import threading
+ from io import BytesIO
+
+ from flask import g
+ from PIL import Image
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ caller_thread = threading.get_ident()
+ g.user = SimpleNamespace(id=user_id)
+ g.request_marker = "request-only"
+ user = SimpleNamespace(id=user_id, is_active=True)
+ chart = SimpleNamespace(id=104)
+ png = BytesIO()
+ Image.new("RGB", (80, 60), "white").save(png, format="PNG")
+
+ def screenshot(url, element, *, user):
+ assert threading.get_ident() != caller_thread
+ assert g.user.id == user_id
+ assert not hasattr(g, "request_marker")
+ assert user.id == user_id
+ assert url == "http://localhost/superset/slice/104/?standalone=true";
+ assert element == "chart-container"
+ return png.getvalue()
+
+ with (
+ patch.object(module, "security_manager", new=MagicMock()) as manager,
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(module.guest_scope, "is_guest_read", return_value=False),
+ patch("superset.is_feature_enabled", return_value=False),
+ patch(
+ "superset.utils.urls.get_url_path",
+
return_value="http://localhost/superset/slice/104/?standalone=true";,
+ ),
+ patch("superset.utils.webdriver._PlaywrightBrowserManager") as
browser_manager,
+ patch("superset.utils.webdriver.WebDriverPlaywright") as driver,
+ ):
+ manager.find_user.return_value = user
+ driver.return_value.get_screenshot.side_effect = screenshot
+ result = await module._generate_png_preview(
+ 104, GetChartPreviewRequest(identifier=104, format="png")
+ )
+ assert base64.b64decode(result.data) == png.getvalue()
+ assert (result.width, result.height) == (80, 60)
+ manager.raise_for_access.assert_called_once_with(chart=chart)
+ browser_manager.return_value._cleanup.assert_called_once()
+ assert (
+ driver.call_args.kwargs["browser_manager"] is
browser_manager.return_value
+ )
+ assert g.request_marker == "request-only"
+ assert g.user.id == user_id
+
+
[email protected]
[email protected](
+ "guest,kwargs",
+ [
+ (True, {}),
+ (False, {"form_data_key": "unsaved"}),
+ (False, {"extra_form_data": {"filters": []}}),
+ ],
+)
+async def test_png_preview_rejects_unpropagated_context(app_context, guest,
kwargs):
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ with (
+ patch.object(module.guest_scope, "is_guest_read", return_value=guest),
+ patch("superset.utils.webdriver._PlaywrightBrowserManager") as manager,
+ ):
+ result = await module._generate_png_preview(
+ 104, GetChartPreviewRequest(identifier=104, format="png", **kwargs)
+ )
+ assert result.error_type == "UnsupportedFormat"
+ manager.assert_not_called()
+
+
[email protected]
[email protected]("failure", ["denied", "missing", "inactive",
"export"])
+async def test_png_preview_authorizes_before_browser(app_context, failure):
+ from flask import g
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ g.user = SimpleNamespace(id=11)
+ with (
+ patch.object(module, "security_manager", new=MagicMock()) as manager,
+ patch.object(
+ module,
+ "find_chart_by_identifier",
+ return_value=None if failure == "missing" else
SimpleNamespace(id=104),
+ ),
+ patch.object(module.guest_scope, "is_guest_read", return_value=False),
+ patch("superset.is_feature_enabled", return_value=failure == "export"),
+ patch("superset.utils.webdriver._PlaywrightBrowserManager") as browser,
+ ):
+ manager.find_user.return_value = SimpleNamespace(
+ id=11, is_active=failure != "inactive"
+ )
+ manager.can_access.return_value = False
+ if failure == "denied":
+ manager.raise_for_access.side_effect = ValueError("secret denied
URL")
+ result = await module._generate_png_preview(
+ 104, GetChartPreviewRequest(identifier=104, format="png")
+ )
+ assert isinstance(result, ChartError)
+ assert "secret" not in result.error
+ browser.assert_not_called()
Review Comment:
Agreed. Fixed in 3522c875: the denied case now raises the real
`SupersetSecurityException` and asserts `error_type == "Forbidden"`, so a
regression to `RenderError` would fail the test. The no-secret-leak assertion
is kept.
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1367,7 +1468,11 @@ def __init__(self, form_data: Dict[str, Any]):
action="mcp.get_chart_preview.preview_generation"
):
preview_generator = PreviewFormatGenerator(chart, request)
- content = preview_generator.generate()
+ content = (
+ await _generate_png_preview(chart.id, request)
+ if request.format == "png"
+ else preview_generator.generate()
+ )
Review Comment:
Checked the ordering: every PNG-unsupported precondition (unsaved chart,
guest, form_data_key, extra_form_data) still resolves to `UnsupportedFormat`,
just after lookup. A nonexistent chart returns `NotFound`, which is the correct
contract (the resource does not exist regardless of format). No wrong error
type is reachable, so leaving the ordering as is.
--
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]
