fix(dashboard): decouple Excel export link lifetime from S3 credential expiry, and support guest/embedded sessions - #43336
Conversation
…l expiry, and support guest/embedded sessions The Excel export success email links to a pre-signed S3 URL that is only valid for as long as both its own ExpiresIn and the credentials that signed it remain valid. Deployments authenticating via short-lived, auto-refreshed credentials (e.g. an EKS IRSA role assumed through AssumeRoleWithWebIdentity, which AWS caps at 12 hours) can silently invalidate the link long before EXCEL_EXPORT_LINK_TTL_SECONDS elapses. The email now links to a Superset redirect (export_xlsx/download/<job_id>/) instead of a raw S3 URL. The link's own lifetime is enforced independently via the key_value store, and a fresh pre-signed URL is generated -- with then-current credentials -- at click time. Reusing the same job_id-keyed store also removes a separate limitation: export_xlsx previously hard-required the requester to have an email address on file, since email was the only way to deliver the link, which excluded guest/embedded dashboard sessions entirely. The frontend can now poll a new export_xlsx_status/<job_id>/ endpoint and auto-download once ready, so an embedded session (no email) gets a working export too; a logged-in session gets both the auto-download and, still, the email. export_xlsx itself needs a CSRF exemption for this to work end-to-end: it's a POST route reachable from embedded/guest sessions whose requests carry no CSRF token, the same reason chart/data is already exempted.
dosubot
Bot
added
change:backend
Code Review Agent Run #94aa37Actionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| WTF_CSRF_EXEMPT_LIST = [ | ||
| "superset.charts.data.api.data", | ||
| "superset.dashboards.api.cache_dashboard_screenshot", | ||
| "superset.dashboards.api.export_xlsx", |
There was a problem hiding this comment.
Suggestion: Exempting the authenticated POST export endpoint from CSRF protection allows a cross-site request to enqueue an export using a victim's Superset session whenever cookies are sent cross-site, such as deployments configured with SESSION_COOKIE_SAMESITE="None". Keep CSRF validation for cookie-authenticated requests and use a narrowly scoped guest/embedded authentication mechanism instead of globally exempting this endpoint. [security]
Severity Level: Major ⚠️
- ❌ Cross-site origins can trigger authenticated dashboard exports.
- ⚠️ Victim email and worker resources may be consumed.
- ⚠️ CSRF protection is bypassed for a state-changing endpoint.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 371:371
**Comment:**
*Security: Exempting the authenticated `POST` export endpoint from CSRF protection allows a cross-site request to enqueue an export using a victim's Superset session whenever cookies are sent cross-site, such as deployments configured with `SESSION_COOKIE_SAMESITE="None"`. Keep CSRF validation for cookie-authenticated requests and use a narrowly scoped guest/embedded authentication mechanism instead of globally exempting this endpoint.
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
| setTimeout( | ||
| () => pollExportStatus(jobId, startedAt), | ||
| EXPORT_STATUS_POLL_INTERVAL_MS, | ||
| ); |
There was a problem hiding this comment.
Suggestion: The polling timers are not retained or cancelled when the hook unmounts or the user navigates away. A pending export can therefore continue issuing status requests for up to five minutes, and a late successful response can display a toast or navigate the browser to the download URL after the originating dashboard has been destroyed. Track the timer and cancel it during cleanup, and ignore responses after unmount. [missing cleanup]
Severity Level: Major ⚠️
- ⚠️ Dashboard navigation leaves polling requests running.
- ⚠️ Late completion can navigate the current page unexpectedly.
- ⚠️ Stale callbacks can display export toasts after teardown.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx
**Line:** 212:215
**Comment:**
*Missing Cleanup: The polling timers are not retained or cancelled when the hook unmounts or the user navigates away. A pending export can therefore continue issuing status requests for up to five minutes, and a late successful response can display a toast or navigate the browser to the download URL after the originating dashboard has been destroyed. Track the timer and cancel it during cleanup, and ignore responses after unmount.
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| addSuccessToast( | ||
| t( | ||
| "Your export is being prepared. You'll receive an email when it's ready.", | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Suggestion: The pending message explicitly promises that the user will receive an email, but the newly supported guest and embedded sessions have no email address and rely exclusively on polling and automatic download. This gives those users an incorrect delivery expectation; use wording that reflects the automatic download or select the message based on the session's notification capability. [logic error]
Severity Level: Minor 🧹
- ⚠️ Guest users receive an impossible email notification promise.
- ⚠️ Embedded users may wait for an email that never arrives.
- ✅ Polling still enables automatic download completion.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx
**Line:** 242:246
**Comment:**
*Logic Error: The pending message explicitly promises that the user will receive an email, but the newly supported guest and embedded sessions have no email address and rely exclusively on polling and automatic download. This gives those users an incorrect delivery expectation; use wording that reflects the automatic download or select the message based on the session's notification capability.
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
|
The security concern regarding CSRF protection for the export endpoint is valid. Exempting state-changing endpoints from CSRF protection can indeed expose users to cross-site request forgery attacks, especially in configurations where cookies are sent cross-site. To resolve this, you should remove the global exemption for the endpoint and implement a more secure authentication mechanism, such as a guest token or a signed request parameter, for embedded or guest scenarios. Since the file |
|
Heads up: removing the guest guard here exposes an I opened #43340 against this branch with the fix (guest token passed to the task, guest user reconstructed via |
| @@ -1787,12 +1803,9 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse: | |||
| except SupersetSecurityException: | |||
| return self.response_403() | |||
|
|
|||
There was a problem hiding this comment.
Embedded guest requests now reach this path, but GuestUser has no persisted numeric id; building the lock key (and later enqueueing the task) raises before a job can be returned. Could this keep guest context separate from the user-id task contract, or continue rejecting guests until the worker can safely reconstruct that context?
| # upsert (not create) so a retried/duplicate write for the same job_id | ||
| # overwrites cleanly instead of colliding on the primary key. | ||
| KeyValueDAO.delete_expired_entries(RESOURCE) | ||
| KeyValueDAO.upsert_entry( |
There was a problem hiding this comment.
The ready/error status is only staged in the SQLAlchemy session here. With a Redis distributed lock, releasing the lock does not commit that session, so Celery teardown rolls the entry back: polling remains pending and the emailed redirect returns 410. Could this write be committed transactionally before reporting the export ready?
| def build_download_url(job_id: UUID) -> str: | ||
| """The browser-facing URL that redirects to a freshly pre-signed S3 URL | ||
| for ``job_id``, once its export is ready.""" | ||
| return headless_url(DOWNLOAD_PATH.format(job_id=job_id), user_friendly=True) |
There was a problem hiding this comment.
headless_url joins this absolute path to the host and bypasses APPLICATION_ROOT. Deployments mounted below a prefix therefore email a root-level download URL that does not route to Superset. Could this use the prefix-aware URL helper (or otherwise include the application root)?
| # KeyValueEntry.expires_on comparisons use naive datetime.now(), so | ||
| # the stored expiry must be naive UTC too, not tz-aware. | ||
| download_url = create_download_link( | ||
| uuid.UUID(job_id), bucket, key, expires_at.replace(tzinfo=None) |
There was a problem hiding this comment.
This stores a naive UTC timestamp, while KeyValueEntry.is_expired() compares it with naive local datetime.now(). On non-UTC servers the configured link lifetime is shifted by the timezone offset, so links can expire early or stay valid too long. Could the stored value and expiry comparison use the same timezone convention?
3 participants
SUMMARY
The dashboard Excel export success email (#41133) links to a pre-signed S3 URL that is only valid for as long as both its own
ExpiresInwindow and the credentials that signed it remain valid. Deployments whose S3 client authenticates via short-lived, auto-refreshed credentials (e.g. an EKS IRSA role assumed throughAssumeRoleWithWebIdentity, which AWS caps at 12 hours) can silently invalidate the pre-signed URL long beforeEXCEL_EXPORT_LINK_TTL_SECONDSelapses, since the credentials' session — not just the URL's ownExpiresIn— bounds how long it actually works.To keep that promise regardless of credential lifetime, the email now links to a small Superset redirect endpoint (
GET .../export_xlsx/download/<job_id>/) instead of a raw S3 URL. The link's own lifetime is enforced independently via thekey_valuestore'sexpires_on, and the actual pre-signed URL is generated fresh — with then-current credentials — at click time, valid only long enough to complete a single download. The redirect intentionally requires no login, matching a raw pre-signed URL's own access model: the unguessable job id is the credential, and the dashboard access check already ran once when the export was requested.Reusing this same job_id-keyed store also removes a separate limitation:
export_xlsxpreviously hard-required the requester to have an email address on file, since email was the only delivery channel — which excluded guest/embedded dashboard sessions (GuestUser) entirely. A newGET .../export_xlsx/status/<job_id>/polling endpoint lets the frontend discover completion without an email: theDownloadMenuItemscomponent now polls after enqueueing and auto-downloads once ready, so an embedded/guest session (no email) gets a working export too. A regular logged-in session gets both the browser auto-download and, still, the email — the polling arrives before the email does in practice.export_xlsxneeds a CSRF exemption for this to work end-to-end: it's a POST route now reachable from embedded/guest sessions whose fetch calls carry no CSRF token, the same reasonchart/datais already inWTF_CSRF_EXEMPT_LIST.BEFORE/AFTER
Before: the export email links directly to a pre-signed S3 URL that can silently stop working hours before its promised expiry on IRSA-style deployments, and
POST export_xlsx400s for any session without an email address (all embedded/guest dashboards).After: the export email (and, for guest sessions, browser polling) links to a Superset redirect that re-signs the S3 URL at click time; guest/embedded sessions can request and receive an export.
TESTING INSTRUCTIONS
pytest tests/integration_tests/dashboards/api_tests.py -k "download_xlsx or export_xlsx"— 17 new/updated testspytest tests/unit_tests/tasks/test_export_dashboard_excel.py— 47 testspytest tests/integration_tests/security_tests.py -k test_views_are_securednpx jest src/dashboard/components/menu/DownloadMenuItems/DownloadMenuItems.test.tsx— 18 tests (3 new, covering ready/pending/error polling states)ruff check/ruff format --check/oxlint/prettier --checkall clean on touched filesmypyclean on touched files (pre-existing, unrelated repo-wide errors aside)ADDITIONAL INFORMATION
key_valuetable via a newKeyValueResource.EXCEL_EXPORT_DOWNLOADenum member, no schema change