codeant-ai-for-open-source[bot] commented on code in PR #35021: URL: https://github.com/apache/superset/pull/35021#discussion_r2874735223
########## superset/views/redirect.py: ########## @@ -0,0 +1,76 @@ +# 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. + +""" +View that handles external-link redirects with a warning page. + +Links in alert/report emails are rewritten to point here. Internal links +redirect immediately; external links are shown to the user for confirmation +via the React ``RedirectWarning`` page. +""" + +import logging +from urllib.parse import urlparse + +from flask import abort, redirect, request +from flask_appbuilder import expose + +from superset import is_feature_enabled +from superset.superset_typing import FlaskResponse +from superset.utils.link_redirect import is_safe_redirect_url +from superset.views.base import BaseSupersetView + +logger = logging.getLogger(__name__) + +DANGEROUS_SCHEMES: frozenset[str] = frozenset( + ("javascript", "data", "vbscript", "file") +) + + +class RedirectView(BaseSupersetView): + """ + Warning page for external links found in alert/report emails. + + This endpoint is publicly accessible (no authentication required) + because email recipients may not have an active Superset session. + """ + + route_base = "/redirect" + + @expose("/") + def redirect_warning(self) -> FlaskResponse: + """Validate the target URL and either redirect or show the warning page.""" + if not is_feature_enabled("ALERT_REPORTS"): + abort(404) Review Comment: **Suggestion:** The redirect endpoint is hard-gated on the `ALERT_REPORTS` feature flag, so if this flag is later disabled (for example after emails have already been sent), all existing `/redirect/?url=...` links in those emails will start returning 404 instead of showing the warning page or redirecting, breaking previously issued links; the redirect handler itself is safe and already explicitly whitelisted as a benign public view, so it should remain available regardless of whether alert/report creation is enabled. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Old alert/report email links 404 after disabling ALERT_REPORTS. - ⚠️ External-link phishing warning unavailable for legacy alert/report emails. ``` </details> ```suggestion # Keep this endpoint available even if ALERT_REPORTS is disabled so # previously sent alert/report email links using /redirect/ continue to work. ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. With `ALERT_REPORTS` enabled and `ALERT_REPORTS_ENABLE_LINK_REDIRECT` left at its default True (configured in `superset/config.py:595-599` and `superset/config.py:1968`), create a report/alert that sends an email containing an HTML description with an external link (e.g. `<a href="https://external.com";>Link</a>`). Email generation uses `EmailNotification._get_content` in `superset/reports/notifications/email.py:114-139`, which sanitizes HTML and then calls `process_html_links(description)` at line 138. 2. During email rendering, `process_html_links` in `superset/utils/link_redirect.py:90-118` calls `_HREF_RE.sub` with `_replace_href` (lines 70-87). For external `https://external.com`, `_replace_href` builds a redirect URL `f"{redirect_base}/redirect/?url={quote(href, safe='')}"` at line 86, so the email sent to the user now contains an `href` pointing to `/redirect/?url=https%3A%2F%2Fexternal.com` on the Superset host. 3. After these emails have been delivered, an administrator later disables alerts/reports by setting the `ALERT_REPORTS` feature flag to False (feature flag defined in `superset/config.py:595-599` and used across alerts code paths). Existing emails in users' inboxes are unchanged and still contain links to `/redirect/?url=...`. 4. A recipient opens an old email and clicks the rewritten link, issuing `GET /redirect/?url=https%3A%2F%2Fexternal.com` against Superset. This request is routed to `RedirectView.redirect_warning` (view registered in `superset/initialization/__init__.py:430-21` via `appbuilder.add_view_no_menu(RedirectView)` and implemented in `superset/views/redirect.py:44-76`). Because `ALERT_REPORTS` is now disabled, the guard at `superset/views/redirect.py:57-58` executes `if not is_feature_enabled("ALERT_REPORTS"): abort(404)`, causing a 404 instead of a redirect or warning page. This behavior is verified by `TestRedirectView.test_feature_flag_disabled_returns_404` in `superset/tests/integration_tests/views/test_redirect_view.py:61-66`, which expects `/redirect/?url=https://external.com` to return 404 when `ALERT_REPORTS=False`. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/views/redirect.py **Line:** 57:58 **Comment:** *Logic Error: The redirect endpoint is hard-gated on the `ALERT_REPORTS` feature flag, so if this flag is later disabled (for example after emails have already been sent), all existing `/redirect/?url=...` links in those emails will start returning 404 instead of showing the warning page or redirecting, breaking previously issued links; the redirect handler itself is safe and already explicitly whitelisted as a benign public view, so it should remain available regardless of whether alert/report creation is enabled. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35021&comment_hash=a94260ff7d27dc3d151fe49643b13def116724ebc8f494c1aa9fc1e3f0035db8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35021&comment_hash=a94260ff7d27dc3d151fe49643b13def116724ebc8f494c1aa9fc1e3f0035db8&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]
