rusackas commented on code in PR #44558:
URL: https://github.com/apache/superset/pull/44558#discussion_r4261616170
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1167,6 +1185,116 @@ def generate(
return strategy.generate()
+async def _run_png_render(
+ render: Callable[[], PNGPreview | ChartError],
+) -> PNGPreview | ChartError:
+ """Run the blocking PNG render in a worker thread with error mapping.
+
+ Chart-level access denials raised inside render() surface as Forbidden,
+ not as a rendering failure. Browser errors may contain URLs or page data,
+ so those stay server-side.
+ """
+ try:
+ context = copy_context()
+ return await asyncio.get_running_loop().run_in_executor(
+ _PNG_EXECUTOR, context.run, render
+ )
+ except SupersetSecurityException:
+ return ChartError(error="Chart access denied", error_type="Forbidden")
+ except Exception:
+ logger.exception("PNG chart rendering failed")
+ return ChartError(error="Chart rendering failed",
error_type="RenderError")
+
+
+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
Review Comment:
`superset/utils/screenshots.py` already defines `PNG_MAGIC_BYTES`, used by
`validate_screenshot_image` right below this. Importing it here too avoids the
two checks drifting if that byte sequence ever changes.
```suggestion
from superset.utils.screenshots import PNG_MAGIC_BYTES,
validate_screenshot_image
```
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1447,7 +1575,11 @@ def __init__(self, form_data: Dict[str, Any]):
action="mcp.get_chart_preview.preview_generation"
):
preview_generator = PreviewFormatGenerator(chart, request)
Review Comment:
NIT: `preview_generator` gets built and then thrown away on the `png`
branch. Not a big deal, but may as well only construct it where it's used.
```suggestion
content = (
await _generate_png_preview(chart.id, request)
if request.format == "png"
else PreviewFormatGenerator(chart, request).generate()
```
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1167,6 +1185,116 @@ def generate(
return strategy.generate()
+async def _run_png_render(
+ render: Callable[[], PNGPreview | ChartError],
+) -> PNGPreview | ChartError:
+ """Run the blocking PNG render in a worker thread with error mapping.
+
+ Chart-level access denials raised inside render() surface as Forbidden,
+ not as a rendering failure. Browser errors may contain URLs or page data,
+ so those stay server-side.
+ """
+ try:
+ context = copy_context()
+ return await asyncio.get_running_loop().run_in_executor(
+ _PNG_EXECUTOR, context.run, render
+ )
+ except SupersetSecurityException:
+ return ChartError(error="Chart access denied", error_type="Forbidden")
+ except Exception:
+ logger.exception("PNG chart rendering failed")
+ return ChartError(error="Chart rendering failed",
error_type="RenderError")
+
+
+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")
Review Comment:
```suggestion
or not image.startswith(PNG_MAGIC_BYTES)
```
--
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]
