codeant-ai-for-open-source[bot] commented on code in PR #44558:
URL: https://github.com/apache/superset/pull/44558#discussion_r4079439907
##########
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:
**Suggestion:** Denied chart access raises an authorization exception inside
the broad handler, so callers receive `RenderError` instead of the expected
`Forbidden` response.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d16f1b876da44e6286ef05b4f94c54d4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d16f1b876da44e6286ef05b4f94c54d4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 1146:1146
**Comment:**
*Api Mismatch: Denied chart access raises an authorization exception
inside the broad handler, so callers receive `RenderError` instead of the
expected `Forbidden` response.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44558&comment_hash=b699ce30003afc5f5c93ca874d6072193bf55a2fbaa16419d9e5f0ce3d70c4e7&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44558&comment_hash=b699ce30003afc5f5c93ca874d6072193bf55a2fbaa16419d9e5f0ce3d70c4e7&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** Cancellation cannot stop `render`, leaving Chromium
processes and worker threads occupied until screenshot timeouts expire;
repeated cancelled requests can exhaust rendering capacity.
**Assessment:** ๐ด `Critical` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Resource leak`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c7b4bcb9bc474069a5789e0f0329a349&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c7b4bcb9bc474069a5789e0f0329a349&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 1184:1184
**Comment:**
*Resource Leak: Cancellation cannot stop `render`, leaving Chromium
processes and worker threads occupied until screenshot timeouts expire;
repeated cancelled requests can exhaust rendering capacity.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44558&comment_hash=45ae123df000d1066741789046fbba81a5ab37e384957a0cb18af14e73852e01&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44558&comment_hash=45ae123df000d1066741789046fbba81a5ab37e384957a0cb18af14e73852e01&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** PNG rejection occurs only after chart lookup and transient
form-data handling, so invalid PNG requests can return `NotFound` or other
errors instead of `UnsupportedFormat`.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Incomplete
implementation`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cc1831d054c243b39f57c4d328ceb907&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cc1831d054c243b39f57c4d328ceb907&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 1471:1475
**Comment:**
*Incomplete Implementation: PNG rejection occurs only after chart
lookup and transient form-data handling, so invalid PNG requests can return
`NotFound` or other errors instead of `UnsupportedFormat`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44558&comment_hash=1045f1724212941d7b01527f779cb46fd921eb679257fe898aa202565feaba8a&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44558&comment_hash=1045f1724212941d7b01527f779cb46fd921eb679257fe898aa202565feaba8a&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** The denied case accepts any `ChartError`, so authorization
failures converted to `RenderError` still pass and the test misses the required
`Forbidden` contract.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Often` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f1289e4ba9364da3b5644f02c116a78d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f1289e4ba9364da3b5644f02c116a78d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
**Line:** 1955:1957
**Comment:**
*Api Mismatch: The denied case accepts any `ChartError`, so
authorization failures converted to `RenderError` still pass and the test
misses the required `Forbidden` contract.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44558&comment_hash=56672edd7a86f5180afb572bce54bdacecc1809ce5dbed81ffb9bd71c71cc821&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44558&comment_hash=56672edd7a86f5180afb572bce54bdacecc1809ce5dbed81ffb9bd71c71cc821&reaction=dislike'>๐</a>
--
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]
