gabotorresruiz commented on code in PR #44558:
URL: https://github.com/apache/superset/pull/44558#discussion_r4137010074
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1087,6 +1105,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",
Review Comment:
The PNG genuinely reaches the caller now, I confirmed that end to end, thank
you. But with `get_chart_preview` out of the size guard, this clamp is the only
thing left bounding the response, and it bounds pixels rather than bytes.
I rendered the same ECharts line chart at both corners of the range you
allow here and pushed each result through a real `fastmcp.Client` with the real
`ResponseSizeGuardMiddleware` and the new default `excluded_tools`:
```text
viewport pixel_density response bytes delivered
800x600 1 128,944 yes
4096x4096 1 859,666 yes
4096x4096 2 2,014,190 yes
```
`pixel_density` is not caller controlled, it comes from `WEBDRIVER_WINDOW`
in `superset/config.py`, but a deployment that sets it to `2` doubles each
rendered dimension, so that last row is reachable. All three came back with
`is_error=False` and a base64 payload byte identical to what went in. A single
tool result of roughly two megabytes of base64 text is the exact shape
`ResponseSizeGuardMiddleware` was written to stop (its class docstring in
`superset/mcp_service/middleware.py` says so), and an agent asked for a large
preview will pass `4096` because the README advertises it.
Could we clamp the encoded output rather than only the viewport? A
configurable byte or megapixel ceiling that either downscales before encoding
or returns a `ValidationError` naming the largest size that fits would do it,
with the ceiling written into the README next to the `64..4096` line. A test
next to `test_png_preview_passes_default_response_size_guard` that renders at
`4096x4096` and asserts the response stays under that ceiling would lock it in.
This is the only thing I would still hold for, and I am happy to help if the
clamp turns out to be awkward to place.
##########
superset/mcp_service/mcp_config.py:
##########
@@ -443,6 +443,7 @@ class MCPAuthConfigError(ValueError):
"generate_explore_link", # Returns URLs
"open_sql_lab_with_context", # Returns URLs
"search_tools", # Returns tool schemas for discovery (intentionally
large)
+ "get_chart_preview", # Rendered PNG previews exceed the text budget
Review Comment:
Not a blocker, but worth on the record because it reaches past PNG.
The guard is keyed on the tool name, so this entry also turns it off for
`ascii`, `table` and `vega_lite`. `ascii` and `table` are row capped at 50 and
20 so they are fine, but `vega_lite` runs at `_preview_row_limit(form_data,
1000)` and then embeds the whole result set inline as `"data": {"values":
data}`, so it can get large. I built a 1000 row spec of plausible chart columns
and ran it through the same real client twice:
```text
1000 row vega_lite preview: 142,677 bytes
with master's excluded_tools -> is_error=True (blocked at 50,000)
with this branch's list -> is_error=False (130,618 bytes delivered)
```
Also worth knowing: #39719 removed `get_chart_preview` from this list on
purpose and added `test_default_config_checks_chart_preview` to pin it, which
is the assertion this PR inverts. That decision was about the non PNG formats,
so it may be worth looping in whoever owns it before flipping it.
If you would rather keep that invariant, skipping the guard on the payload's
`content.type == "png"` instead of on the tool name gives PNG its escape hatch
and leaves the other three formats checked.
--
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]
