codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3545204158
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -198,6 +237,20 @@ export const useDownloadMenuItems = (
];
const exportMenuItems: MenuItem[] = [
+ ...(userCanExport
+ ? [
+ {
+ key: 'export-xlsx',
+ label: t('Export Data to Excel'),
+ onClick: () => onExportXlsx('data'),
+ },
+ {
+ key: 'export-xlsx-images',
+ label: t('Export Images to Excel'),
+ onClick: () => onExportXlsx('images'),
+ },
+ ]
+ : []),
Review Comment:
**Suggestion:** The new image-based Excel export action is shown whenever
`userCanExport` is true, but it ignores `canExportImage`; this exposes an
image-export path to users who are explicitly denied image export under
granular export controls. Gate or disable this menu item with `canExportImage`
(and keep backend enforcement aligned). [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Image Excel export bypasses GRANULAR_EXPORT_CONTROLS permission gating.
- ⚠️ Users without image rights still receive chart screenshots in Excel.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dashboard download menu implementation at
`superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:195-237`;
note `imageDisabled = canExportImage === false` (line 195) and that
`screenshotMenuItems`
entries (lines 209-237) use `disabled: imageDisabled` and
`imageExportLabel(...)` to
hide/disable image/PDF screenshot actions when `canExportImage` is false.
2. In the same file, inspect the Excel export items at `exportMenuItems`
(lines 239-268);
observe that the new `export-xlsx` and `export-xlsx-images` menu items
(lines 240-253) are
created whenever `userCanExport` is truthy, with no reference to
`canExportImage`, so a
user with `userCanExport === true` and `canExportImage === false` will still
see and can
click "Export Images to Excel".
3. Follow the click handler `onExportXlsx('images')` (lines 170-193), which
posts to
`/api/v1/dashboard/${dashboardId}/export_xlsx/` with `mode: 'images'`; there
is no
client-side guard based on `canExportImage`, so the request is sent even for
users
explicitly denied image export.
4. On the backend, inspect `DashboardRestApi.export_xlsx` in
`superset/dashboards/api.py:1487-1588`; note that it is gated only by
`@permission_name("export")` (line 1490) and does not check
`GRANULAR_EXPORT_CONTROLS` or
`security_manager.can_access("can_export_image", "Superset")`, unlike the
screenshot/image
endpoints at `cache_dashboard_screenshot` (lines 1633-1636) and `screenshot`
(lines
1752-1755) which explicitly deny access when `can_export_image` is missing;
the Celery
task `export_dashboard_excel` in
`superset/tasks/export_dashboard_excel.py:251-335` then
calls `render_chart_image(...)` (lines 185-199 and `screenshot.py:44-91`) to
produce chart
images without any can_export_image checks, so users lacking image-export
rights can still
trigger and receive an image-based Excel export.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d71631e644064b9cbc10e3002f357ea1&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=d71631e644064b9cbc10e3002f357ea1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx
**Line:** 240:253
**Comment:**
*Security: The new image-based Excel export action is shown whenever
`userCanExport` is true, but it ignores `canExportImage`; this exposes an
image-export path to users who are explicitly denied image export under
granular export controls. Gate or disable this menu item with `canExportImage`
(and keep backend enforcement aligned).
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%2F41133&comment_hash=5fa19804c53359d777e1a887e1d85976772fa068314aff4af63d3478cb4b3331&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=5fa19804c53359d777e1a887e1d85976772fa068314aff4af63d3478cb4b3331&reaction=dislike'>👎</a>
##########
superset/dashboards/excel_export/email.py:
##########
@@ -0,0 +1,185 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Email rendering and delivery for dashboard Excel exports.
+
+Bodies use inline styles only (no external CSS, no logo) to match Superset's
+existing report notification emails, and all user-controlled values (dashboard
+title, chart names) are HTML-escaped to avoid injection.
+"""
+
+from __future__ import annotations
+
+from datetime import datetime
+
+from flask import current_app
+from flask_babel import gettext as __, ngettext
+from markupsafe import escape
+
+from superset.utils.core import send_email_smtp
+
+_DATETIME_FORMAT = "%Y-%m-%d %H:%M:%S"
+_FOOTER_STYLE = "color:#888;font-size:12px;"
+_BUTTON_STYLE = (
+ "display:inline-block;padding:10px 16px;background:#20a7c9;color:#ffffff;"
+ "text-decoration:none;border-radius:4px;"
+)
+
+# Reason keys under which the export task groups charts it could not export.
+# The task classifies each omitted chart under one of these; the email renders
a
+# separate, labelled section per non-empty group with its own remediation text.
+ERROR_NO_QUERY_CONTEXT = "no-query-context"
+ERROR_TIMEOUT = "timeout"
+ERROR_GENERAL = "general-exception"
+
+
+def _fmt(dt: datetime) -> str:
+ return dt.strftime(_DATETIME_FORMAT)
+
+
+def _humanize_ttl(seconds: int) -> str:
+ """Render a TTL as a human-readable, pluralized, translatable duration.
+
+ Whole hours read as "24 hours"; sub-hour and non-hour values keep their
+ minutes (e.g. "1 hour 30 minutes", "15 minutes") so the stated lifetime
+ always matches the real pre-signed URL expiration.
+ """
+ hours, remainder = divmod(seconds, 3600)
+ parts: list[str] = []
+ if hours:
+ parts.append(ngettext("%(num)d hour", "%(num)d hours", hours))
+ if minutes := remainder // 60:
+ parts.append(ngettext("%(num)d minute", "%(num)d minutes", minutes))
+ if not parts:
+ parts.append(ngettext("%(num)d second", "%(num)d seconds", seconds))
Review Comment:
**Suggestion:** The pluralized duration strings are built with `ngettext`
but the interpolation variable is never passed, so translated strings
containing `%(num)d` will not be formatted with the count and can render broken
placeholder text in emails. Pass `num=` (or equivalent formatting kwargs) on
each `ngettext` call. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Success emails show raw placeholders like %(num)d hours.
- ⚠️ Localized duration strings render broken in all export emails.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `_humanize_ttl` in
`superset/dashboards/excel_export/email.py:54-69`; note the
three `ngettext` calls at lines 64, 66, and 68 use message templates
`"%(num)d hour"`,
`"%(num)d minute"`, and `"%(num)d second"` but do not pass any interpolation
mapping (e.g.
`num=hours`), so the returned strings still contain the literal `%(num)d`
placeholder.
2. Observe how `_humanize_ttl` is used in `build_success_email` at
`email.py:123-154`; the
expiry line is built via `expiry = __("This link expires in %(duration)s
(%(when)s UTC).",
duration=_humanize_ttl(ttl_seconds), when=_fmt(expires_at))` (lines
136-140), meaning the
outer `gettext` only substitutes `%(duration)s` and `%(when)s`, leaving any
inner
`%(num)d` sequences untouched inside the `duration` string.
3. Follow the runtime path in
`superset/tasks/export_dashboard_excel.py:251-335`; when a
dashboard Excel export completes successfully, the task computes `ttl =
current_app.config["EXCEL_EXPORT_LINK_TTL_SECONDS"]` (line 303) and then
calls
`email.build_success_email(..., ttl_seconds=ttl, errored=errored)` (lines
309-321), which
in turn invokes `_humanize_ttl(ttl_seconds)` to format the link TTL for the
email body.
4. Run a dashboard export by POSTing from the frontend through
`useDownloadMenuItems.onExportXlsx('data')` or `'images'`
(index.tsx:170-193) to `POST
/api/v1/dashboard/<pk>/export_xlsx/` (api.py:1487-1588), wait for the Celery
task
`export_dashboard_excel` to finish, and inspect the received success email;
the expiry
line will show raw placeholders like `"This link expires in %(num)d hours
(2024-01-01
00:00:00 UTC)."` instead of the expected `"This link expires in 24 hours
(2024-01-01
00:00:00 UTC)."`, confirming that the `num` variable is never interpolated
into the
pluralized strings.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=11ddf7b416444694a1d4282f16cb6b7b&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=11ddf7b416444694a1d4282f16cb6b7b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/excel_export/email.py
**Line:** 63:68
**Comment:**
*Logic Error: The pluralized duration strings are built with `ngettext`
but the interpolation variable is never passed, so translated strings
containing `%(num)d` will not be formatted with the count and can render broken
placeholder text in emails. Pass `num=` (or equivalent formatting kwargs) on
each `ngettext` call.
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%2F41133&comment_hash=be0dc7197510596bf2b7ede8f6c964d57b328ad1f4999bebd53ae6b1bbb2d52e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=be0dc7197510596bf2b7ede8f6c964d57b328ad1f4999bebd53ae6b1bbb2d52e&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]
