codeant-ai-for-open-source[bot] commented on code in PR #44558:
URL: https://github.com/apache/superset/pull/44558#discussion_r4079761566
##########
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)
+ if is_feature_enabled(
+ "GRANULAR_EXPORT_CONTROLS"
+ ) and not security_manager.can_access("can_export_image",
"Superset"):
+ return ChartError(
+ error="Image export is not permitted",
error_type="Forbidden"
+ )
+ url = get_url_path(
+ "Superset.slice", slice_id=chart_id, standalone="true"
+ )
+ manager = _PlaywrightBrowserManager()
+ try:
+ driver = WebDriverPlaywright(
+ "",
+ (width, height),
+ require_complete_capture=True,
+ browser_manager=manager,
+ )
+ image = driver.get_screenshot(url, "chart-container",
user=user)
+ if (
+ image is None
+ or not image.startswith(b"\x89PNG\r\n\x1a\n")
+ or validate_screenshot_image(image)
+ ):
+ return ChartError(
+ error="Chart rendering failed",
error_type="RenderError"
+ )
+ with Image.open(BytesIO(image)) as rendered:
+ image_width, image_height = rendered.size
+ return PNGPreview(
+ data=base64.b64encode(image).decode("ascii"),
+ width=image_width,
+ height=image_height,
+ )
+ finally:
+ manager._cleanup()
+
+ try:
+ return await asyncio.to_thread(render)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag cancellation-related resource leaks for the bounded render
operation in this module when the screenshot call has its own timeout and
browser resources are released in a finally cleanup block; early process
isolation is out of scope.
**Applied to:**
- `superset/mcp_service/chart/tool/get_chart_preview.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `3522c87`.
Added a dedicated `except SupersetSecurityException` handler around the
render call, returning a `ChartError` with `error_type="Forbidden"` instead of
treating the denial as a rendering failure.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `3522c87`.
The denied-case test now explicitly asserts `result.error_type ==
"Forbidden"` while still verifying that sensitive error text is not exposed.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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]
