codeant-ai-for-open-source[bot] commented on code in PR #35021: URL: https://github.com/apache/superset/pull/35021#discussion_r2590542280
########## docs/docs/configuration/alerts-reports.mdx: ########## @@ -233,6 +233,48 @@ def alert_dynamic_minimal_interval(**kwargs) -> int: ALERT_MINIMUM_INTERVAL = alert_dynamic_minimal_interval ``` +## Security Configuration + +### External Link Redirection + +For security purposes, Superset automatically processes HTML content in alert and report emails to redirect external links through a warning page. This helps protect users from potentially malicious links. + +#### Configuration Options + +```python +# Enable/disable external link redirection (default: True) +ALERT_REPORTS_ENABLE_LINK_REDIRECT = True + +# Show visual indicators for external links in emails (default: True) +ALERT_REPORTS_EXTERNAL_LINK_INDICATOR = True + +# Custom URL for external redirect warning page (optional) +# If not set, uses the built-in Superset redirect warning page +# Example: REDIRECT_URL_PAGE = "https://mycompany.com/external-link-warning"; +REDIRECT_URL_PAGE = None +``` + +#### How it Works + +1. **HTML Processing**: When generating alert/report emails, Superset scans HTML content for external links +2. **Link Replacement**: External links (those not pointing to your Superset instance) are replaced with redirect URLs +3. **Warning Page**: Users who click external links see a warning page before being redirected. By default, this uses Superset's built-in warning page, but you can customize this by setting `REDIRECT_URL_PAGE` to your own URL +4. **Internal Links**: Links pointing to your Superset instance are not modified + +#### Security Features + +- **Dangerous Scheme Blocking**: Automatically blocks `javascript:`, `data:`, `vbscript:`, and `file:` URLs +- **Host Validation**: Uses your `WEBDRIVER_BASEURL_USER_FRIENDLY` or `WEBDRIVER_BASEURL` configuration to determine internal vs external links +- **Logging**: All redirect attempts are logged for security monitoring + +#### Disabling the Feature + +To disable external link redirection entirely: + +```python +ALERT_REPORTS_ENABLE_LINK_REDIRECT = False +``` + Review Comment: **Suggestion:** The docs show how to disable the feature but do not warn about setting the redirect page to an arbitrary external domain; using a third-party or user-controlled redirect URL can enable phishing/open-redirect attacks. Add a short example and recommendation to prefer an internal, HTTPS-only, allowlisted domain and show a safe example configuration variable (and an allowlist) so operators do not accidentally configure an unsafe redirect endpoint. [security] **Severity Level:** Critical 🚨 ```suggestion # Recommended: restrict custom redirect pages to internal/allowlisted domains only. # Example: prefer an internal HTTPS warning page and declare an allowlist to avoid open-redirects. # ALERT_REPORTS_REDIRECT_URL_PAGE = "https://mycompany.com/external-link-warning"; # ALERT_REPORTS_REDIRECT_ALLOWLIST = ["mycompany.com"] ``` <details> <summary><b>Why it matters? ⭐ </b></summary> This is a purely documentation improvement that warns operators about an important security consideration: pointing the redirect page at arbitrary external domains can open the door to phishing/open-redirect attacks. Adding a short recommendation to prefer an internal HTTPS-only page and to consider an allowlist is helpful, non-breaking, and actionable for operators. It doesn't conflict with the current code and improves security guidance. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/docs/configuration/alerts-reports.mdx **Line:** 277:277 **Comment:** *Security: The docs show how to disable the feature but do not warn about setting the redirect page to an arbitrary external domain; using a third-party or user-controlled redirect URL can enable phishing/open-redirect attacks. Add a short example and recommendation to prefer an internal, HTTPS-only, allowlisted domain and show a safe example configuration variable (and an allowlist) so operators do not accidentally configure an unsafe redirect 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. ``` </details> ########## tests/unit_tests/utils/test_link_redirect.py: ########## @@ -0,0 +1,349 @@ +# 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. + + +import pytest +from flask import Flask + +from superset.utils.link_redirect import is_safe_redirect_url, process_html_links + + [email protected] +def app(): + """Create a Flask app for testing""" + app = Flask(__name__) + app.config["WEBDRIVER_BASEURL"] = "http://superset.example.com/"; + app.config["WEBDRIVER_BASEURL_USER_FRIENDLY"] = "http://superset.example.com/"; + app.config["ALERT_REPORTS_EXTERNAL_LINK_INDICATOR"] = True + return app + + +def test_process_html_links_external_urls(app): + """Test that external URLs are replaced with redirect URLs""" + with app.app_context(): + html = """ + <div> + <a href="https://evil.com/phishing";>Click here</a> + <a href="http://external.site/page";>External link</a> + </div> + """ + + result = process_html_links(html) + + # Check that external links are redirected + assert "/redirect/?url=https%3A%2F%2Fevil.com%2Fphishing" in result + assert "/redirect/?url=http%3A%2F%2Fexternal.site%2Fpage" in result + + # Check that external-link class is added + assert 'class="external-link"' in result or "class='external-link'" in result + + +def test_process_html_links_internal_urls(app): + """Test that internal URLs are not modified""" + with app.app_context(): + html = """ + <div> + <a href="http://superset.example.com/dashboard/1";>Dashboard</a> + <a href="http://superset.example.com/chart/list";>Charts</a> + </div> + """ + + result = process_html_links(html) + + # Check that internal links are NOT redirected + assert "http://superset.example.com/dashboard/1"; in result + assert "http://superset.example.com/chart/list"; in result + assert "/redirect/?" not in result + + +def test_process_html_links_mixed_urls(app): + """Test processing HTML with both internal and external URLs""" + with app.app_context(): + html = """ + <div> + <a href="http://superset.example.com/dashboard/1";>Internal Dashboard</a> + <a href="https://external.com/page";>External Page</a> + <a href="/relative/path">Relative Path</a> + <a href="mailto:[email protected]";>Email</a> + </div> + """ + + result = process_html_links(html) + + # Internal link should not be redirected + assert "http://superset.example.com/dashboard/1"; in result + + # External link should be redirected + assert "/redirect/?url=https%3A%2F%2Fexternal.com%2Fpage" in result + + # Relative paths and non-http(s) links should not be modified + assert "/relative/path" in result + assert "mailto:[email protected]"; in result + + +def test_process_html_links_empty_or_none(app): + """Test handling of empty or None HTML content""" + with app.app_context(): + assert process_html_links(None) is None + assert process_html_links("") == "" + assert process_html_links(" ") == " " + + +def test_process_html_links_no_links(app): + """Test HTML without any links""" + with app.app_context(): + html = "<div><p>This is some text without any links.</p></div>" + result = process_html_links(html) + assert "This is some text without any links." in result + assert "/redirect/?" not in result + + +def test_process_html_links_already_redirected(app): + """Test that already redirected URLs are not double-redirected""" + with app.app_context(): + html = """ + <div> + <a href="/redirect/?url=https%3A%2F%2Fexternal.com">Already redirected</a> + </div> + """ + + result = process_html_links(html) + + # Should not double-redirect + assert result.count("/redirect/?") == 1 Review Comment: **Suggestion:** The test checks the number of "/redirect/?" substrings in the rendered HTML to assert no double-redirection, which is brittle; instead parse the resulting HTML and assert the anchor's href equals the original redirected path so you verify the link was left unchanged (no double-redirect) reliably. [possible bug] **Severity Level:** Critical 🚨 ```suggestion from bs4 import BeautifulSoup with app.app_context(): html = """ <div> <a href="/redirect/?url=https%3A%2F%2Fexternal.com">Already redirected</a> </div> """ result = process_html_links(html) # Should not double-redirect: the href should remain the original redirect path soup = BeautifulSoup(result, "html.parser") hrefs = [a.get("href") for a in soup.find_all("a", href=True)] assert hrefs == ["/redirect/?url=https%3A%2F%2Fexternal.com"] ``` <details> <summary><b>Why it matters? ⭐ </b></summary> Counting substring occurrences in the serialized HTML is fragile (serialization may change spacing, attribute quoting, or include other occurrences). A robust test should parse the returned HTML and inspect the anchor's href attribute directly to ensure the href remained the original redirect path (no double-redirect). Additionally, process_html_links/_process_link_element use BeautifulSoup and treat relative URLs and already-redirected links such that inspecting the anchor href is semantically correct. The suggested change improves test reliability and correctness. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_link_redirect.py **Line:** 117:127 **Comment:** *Possible Bug: The test checks the number of "/redirect/?" substrings in the rendered HTML to assert no double-redirection, which is brittle; instead parse the resulting HTML and assert the anchor's href equals the original redirected path so you verify the link was left unchanged (no double-redirect) reliably. 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> ########## tests/unit_tests/utils/test_link_redirect.py: ########## @@ -0,0 +1,349 @@ +# 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. + + +import pytest +from flask import Flask + +from superset.utils.link_redirect import is_safe_redirect_url, process_html_links + + [email protected] +def app(): + """Create a Flask app for testing""" + app = Flask(__name__) + app.config["WEBDRIVER_BASEURL"] = "http://superset.example.com/"; + app.config["WEBDRIVER_BASEURL_USER_FRIENDLY"] = "http://superset.example.com/"; + app.config["ALERT_REPORTS_EXTERNAL_LINK_INDICATOR"] = True + return app + + +def test_process_html_links_external_urls(app): + """Test that external URLs are replaced with redirect URLs""" + with app.app_context(): + html = """ + <div> + <a href="https://evil.com/phishing";>Click here</a> + <a href="http://external.site/page";>External link</a> + </div> + """ + + result = process_html_links(html) + + # Check that external links are redirected + assert "/redirect/?url=https%3A%2F%2Fevil.com%2Fphishing" in result + assert "/redirect/?url=http%3A%2F%2Fexternal.site%2Fpage" in result + + # Check that external-link class is added + assert 'class="external-link"' in result or "class='external-link'" in result + + +def test_process_html_links_internal_urls(app): + """Test that internal URLs are not modified""" + with app.app_context(): + html = """ + <div> + <a href="http://superset.example.com/dashboard/1";>Dashboard</a> + <a href="http://superset.example.com/chart/list";>Charts</a> + </div> + """ + + result = process_html_links(html) + + # Check that internal links are NOT redirected + assert "http://superset.example.com/dashboard/1"; in result + assert "http://superset.example.com/chart/list"; in result + assert "/redirect/?" not in result + + +def test_process_html_links_mixed_urls(app): + """Test processing HTML with both internal and external URLs""" + with app.app_context(): + html = """ + <div> + <a href="http://superset.example.com/dashboard/1";>Internal Dashboard</a> + <a href="https://external.com/page";>External Page</a> + <a href="/relative/path">Relative Path</a> + <a href="mailto:[email protected]";>Email</a> + </div> + """ + + result = process_html_links(html) + + # Internal link should not be redirected + assert "http://superset.example.com/dashboard/1"; in result + + # External link should be redirected + assert "/redirect/?url=https%3A%2F%2Fexternal.com%2Fpage" in result + + # Relative paths and non-http(s) links should not be modified + assert "/relative/path" in result + assert "mailto:[email protected]"; in result + + +def test_process_html_links_empty_or_none(app): + """Test handling of empty or None HTML content""" + with app.app_context(): + assert process_html_links(None) is None + assert process_html_links("") == "" + assert process_html_links(" ") == " " + + +def test_process_html_links_no_links(app): + """Test HTML without any links""" + with app.app_context(): + html = "<div><p>This is some text without any links.</p></div>" + result = process_html_links(html) + assert "This is some text without any links." in result + assert "/redirect/?" not in result + + +def test_process_html_links_already_redirected(app): + """Test that already redirected URLs are not double-redirected""" + with app.app_context(): + html = """ + <div> + <a href="/redirect/?url=https%3A%2F%2Fexternal.com">Already redirected</a> + </div> + """ + + result = process_html_links(html) + + # Should not double-redirect + assert result.count("/redirect/?") == 1 + + +def test_process_html_links_complex_html(app): + """Test processing complex HTML with various elements""" + with app.app_context(): + html = """ + <table> + <tr> + <td><a href="https://evil.com";>External</a></td> + <td><a href="http://superset.example.com/dashboard";>Internal</a></td> + </tr> + </table> + <p>Some text with <a href="https://google.com";>a link</a> in it.</p> + <ul> + <li><a href="https://github.com";>GitHub</a></li> + <li><a href="#anchor">Anchor link</a></li> + </ul> + """ + + result = process_html_links(html) + + # External links should be redirected + assert "/redirect/?url=https%3A%2F%2Fevil.com" in result + assert "/redirect/?url=https%3A%2F%2Fgoogle.com" in result + assert "/redirect/?url=https%3A%2F%2Fgithub.com" in result + + # Internal link should not be redirected + assert "http://superset.example.com/dashboard"; in result + + # Anchor link should not be modified + assert "#anchor" in result + + +def test_process_html_links_with_custom_base_url(app): + """Test processing with a custom base URL""" + with app.app_context(): + html = """ + <a href="https://mycompany.com/page";>Company Page</a> + <a href="https://external.com/page";>External Page</a> + """ + + result = process_html_links(html, base_url="https://mycompany.com";) + + # mycompany.com link should not be redirected + assert "https://mycompany.com/page"; in result + + # external.com link should be redirected + assert "/redirect/?url=https%3A%2F%2Fexternal.com%2Fpage" in result + + +def test_process_html_links_disable_indicator(app): + """Test disabling the external link indicator""" + app.config["ALERT_REPORTS_EXTERNAL_LINK_INDICATOR"] = False + + with app.app_context(): + html = '<a href="https://external.com";>External</a>' + result = process_html_links(html) + + # Should have redirect but no external-link class + assert "/redirect/?url=https%3A%2F%2Fexternal.com" in result + assert "external-link" not in result + + +def test_process_html_links_malformed_html(app): + """Test that malformed HTML is handled gracefully""" + with app.app_context(): + html = """ + <div> + <a href="https://external.com";>Unclosed link + <p>Some text</p> + """ + + # Should not raise an exception + result = process_html_links(html) + + # Should still process the link + assert "/redirect/?" in result + + +def test_is_safe_redirect_url(app): + """Test is_safe_redirect_url function""" + with app.app_context(): + # Internal URLs should be safe + assert is_safe_redirect_url("http://superset.example.com/dashboard";) + assert is_safe_redirect_url("http://superset.example.com/chart/1";) + + # External URLs should not be safe + assert not is_safe_redirect_url("https://evil.com";) + assert not is_safe_redirect_url("http://external.site/page";) + + # Empty or None URLs should not be safe + assert not is_safe_redirect_url("") + assert not is_safe_redirect_url(None) + + +def test_is_safe_redirect_url_with_custom_base(app): + """Test is_safe_redirect_url with custom base URL""" + with app.app_context(): + base_url = "https://mycompany.com"; + + # mycompany.com URLs should be safe + assert is_safe_redirect_url("https://mycompany.com/page";, base_url) + + # Other URLs should not be safe + assert not is_safe_redirect_url("https://external.com/page";, base_url) + + +def test_process_html_links_no_base_url_configured(app): + """Test behavior when no base URL is configured""" + app.config["WEBDRIVER_BASEURL"] = "" + app.config["WEBDRIVER_BASEURL_USER_FRIENDLY"] = "" + + with app.app_context(): + html = '<a href="https://external.com";>External</a>' + result = process_html_links(html) + + # Should return unchanged HTML when no base URL configured + assert result == html + + +def test_process_html_links_whitespace_only_content(app): + """Test processing whitespace-only content""" + with app.app_context(): + assert process_html_links(" ") == " " + assert process_html_links("\n\t \n") == "\n\t \n" + + +def test_process_html_links_empty_href_values(app): + """Test handling of empty href values""" + with app.app_context(): + html = '<a href="">Empty href</a><a href=" ">Whitespace href</a>' + result = process_html_links(html) + + # Should not modify empty or whitespace-only hrefs + assert 'href=""' in result + assert 'href=" "' in result Review Comment: **Suggestion:** The test asserts both that the whitespace-only href is preserved as two spaces and that an empty href exists; BeautifulSoup often normalizes attribute values, so the second assertion can fail. Parse the resulting HTML and assert that each anchor's href, when stripped, is empty — this verifies the intent (empty/whitespace-only hrefs are not treated as external) without depending on exact serialization. [possible bug] **Severity Level:** Critical 🚨 ```suggestion from bs4 import BeautifulSoup with app.app_context(): html = '<a href="">Empty href</a><a href=" ">Whitespace href</a>' result = process_html_links(html) # Should not modify empty or whitespace-only hrefs. Normalize hrefs and ensure stripped value is empty. soup = BeautifulSoup(result, "html.parser") hrefs = [a.get("href") for a in soup.find_all("a", href=True)] assert all((h is None) or (h.strip() == "") for h in hrefs) ``` <details> <summary><b>Why it matters? ⭐ </b></summary> The test relies on exact string serialization of attributes in the HTML returned by BeautifulSoup (process_html_links builds a BeautifulSoup and returns str(soup)). BeautifulSoup may normalize or re-serialize attribute values (especially whitespace-only values), making exact substring checks brittle. The suggested change (parse the output with BeautifulSoup and assert that each anchor's href, when stripped, is empty) verifies the actual intent — that empty/whitespace hrefs remain empty — without depending on serialization details. This is a behavioral / robustness improvement, not merely cosmetic. Evidence: process_html_links uses BeautifulSoup and returns str(soup) (link_redirect.py lines ~130-142), so the output format is controlled by BeautifulSoup and can differ from the input. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_link_redirect.py **Line:** 257:263 **Comment:** *Possible Bug: The test asserts both that the whitespace-only href is preserved as two spaces and that an empty href exists; BeautifulSoup often normalizes attribute values, so the second assertion can fail. Parse the resulting HTML and assert that each anchor's href, when stripped, is empty — this verifies the intent (empty/whitespace-only hrefs are not treated as external) without depending on exact serialization. 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> ########## tests/unit_tests/utils/test_link_redirect.py: ########## @@ -0,0 +1,349 @@ +# 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. + + +import pytest +from flask import Flask + +from superset.utils.link_redirect import is_safe_redirect_url, process_html_links + + [email protected] +def app(): + """Create a Flask app for testing""" + app = Flask(__name__) + app.config["WEBDRIVER_BASEURL"] = "http://superset.example.com/"; + app.config["WEBDRIVER_BASEURL_USER_FRIENDLY"] = "http://superset.example.com/"; + app.config["ALERT_REPORTS_EXTERNAL_LINK_INDICATOR"] = True + return app + + +def test_process_html_links_external_urls(app): + """Test that external URLs are replaced with redirect URLs""" + with app.app_context(): + html = """ + <div> + <a href="https://evil.com/phishing";>Click here</a> + <a href="http://external.site/page";>External link</a> + </div> + """ + + result = process_html_links(html) + + # Check that external links are redirected + assert "/redirect/?url=https%3A%2F%2Fevil.com%2Fphishing" in result + assert "/redirect/?url=http%3A%2F%2Fexternal.site%2Fpage" in result + + # Check that external-link class is added + assert 'class="external-link"' in result or "class='external-link'" in result + + +def test_process_html_links_internal_urls(app): + """Test that internal URLs are not modified""" + with app.app_context(): + html = """ + <div> + <a href="http://superset.example.com/dashboard/1";>Dashboard</a> + <a href="http://superset.example.com/chart/list";>Charts</a> + </div> + """ + + result = process_html_links(html) + + # Check that internal links are NOT redirected + assert "http://superset.example.com/dashboard/1"; in result + assert "http://superset.example.com/chart/list"; in result + assert "/redirect/?" not in result + + +def test_process_html_links_mixed_urls(app): + """Test processing HTML with both internal and external URLs""" + with app.app_context(): + html = """ + <div> + <a href="http://superset.example.com/dashboard/1";>Internal Dashboard</a> + <a href="https://external.com/page";>External Page</a> + <a href="/relative/path">Relative Path</a> + <a href="mailto:[email protected]";>Email</a> + </div> + """ + + result = process_html_links(html) + + # Internal link should not be redirected + assert "http://superset.example.com/dashboard/1"; in result + + # External link should be redirected + assert "/redirect/?url=https%3A%2F%2Fexternal.com%2Fpage" in result + + # Relative paths and non-http(s) links should not be modified + assert "/relative/path" in result + assert "mailto:[email protected]"; in result + + +def test_process_html_links_empty_or_none(app): + """Test handling of empty or None HTML content""" + with app.app_context(): + assert process_html_links(None) is None + assert process_html_links("") == "" + assert process_html_links(" ") == " " + + +def test_process_html_links_no_links(app): + """Test HTML without any links""" + with app.app_context(): + html = "<div><p>This is some text without any links.</p></div>" + result = process_html_links(html) + assert "This is some text without any links." in result + assert "/redirect/?" not in result + + +def test_process_html_links_already_redirected(app): + """Test that already redirected URLs are not double-redirected""" + with app.app_context(): + html = """ + <div> + <a href="/redirect/?url=https%3A%2F%2Fexternal.com">Already redirected</a> + </div> + """ + + result = process_html_links(html) + + # Should not double-redirect + assert result.count("/redirect/?") == 1 + + +def test_process_html_links_complex_html(app): + """Test processing complex HTML with various elements""" + with app.app_context(): + html = """ + <table> + <tr> + <td><a href="https://evil.com";>External</a></td> + <td><a href="http://superset.example.com/dashboard";>Internal</a></td> + </tr> + </table> + <p>Some text with <a href="https://google.com";>a link</a> in it.</p> + <ul> + <li><a href="https://github.com";>GitHub</a></li> + <li><a href="#anchor">Anchor link</a></li> + </ul> + """ + + result = process_html_links(html) + + # External links should be redirected + assert "/redirect/?url=https%3A%2F%2Fevil.com" in result + assert "/redirect/?url=https%3A%2F%2Fgoogle.com" in result + assert "/redirect/?url=https%3A%2F%2Fgithub.com" in result + + # Internal link should not be redirected + assert "http://superset.example.com/dashboard"; in result + + # Anchor link should not be modified + assert "#anchor" in result + + +def test_process_html_links_with_custom_base_url(app): + """Test processing with a custom base URL""" + with app.app_context(): + html = """ + <a href="https://mycompany.com/page";>Company Page</a> + <a href="https://external.com/page";>External Page</a> + """ + + result = process_html_links(html, base_url="https://mycompany.com";) + + # mycompany.com link should not be redirected + assert "https://mycompany.com/page"; in result + + # external.com link should be redirected + assert "/redirect/?url=https%3A%2F%2Fexternal.com%2Fpage" in result + + +def test_process_html_links_disable_indicator(app): + """Test disabling the external link indicator""" + app.config["ALERT_REPORTS_EXTERNAL_LINK_INDICATOR"] = False + + with app.app_context(): + html = '<a href="https://external.com";>External</a>' + result = process_html_links(html) + + # Should have redirect but no external-link class + assert "/redirect/?url=https%3A%2F%2Fexternal.com" in result + assert "external-link" not in result + + +def test_process_html_links_malformed_html(app): + """Test that malformed HTML is handled gracefully""" + with app.app_context(): + html = """ + <div> + <a href="https://external.com";>Unclosed link + <p>Some text</p> + """ + + # Should not raise an exception + result = process_html_links(html) + + # Should still process the link + assert "/redirect/?" in result + + +def test_is_safe_redirect_url(app): + """Test is_safe_redirect_url function""" + with app.app_context(): + # Internal URLs should be safe + assert is_safe_redirect_url("http://superset.example.com/dashboard";) + assert is_safe_redirect_url("http://superset.example.com/chart/1";) + + # External URLs should not be safe + assert not is_safe_redirect_url("https://evil.com";) + assert not is_safe_redirect_url("http://external.site/page";) + + # Empty or None URLs should not be safe + assert not is_safe_redirect_url("") + assert not is_safe_redirect_url(None) + + +def test_is_safe_redirect_url_with_custom_base(app): + """Test is_safe_redirect_url with custom base URL""" + with app.app_context(): + base_url = "https://mycompany.com"; + + # mycompany.com URLs should be safe + assert is_safe_redirect_url("https://mycompany.com/page";, base_url) + + # Other URLs should not be safe + assert not is_safe_redirect_url("https://external.com/page";, base_url) + + +def test_process_html_links_no_base_url_configured(app): + """Test behavior when no base URL is configured""" + app.config["WEBDRIVER_BASEURL"] = "" + app.config["WEBDRIVER_BASEURL_USER_FRIENDLY"] = "" + + with app.app_context(): + html = '<a href="https://external.com";>External</a>' + result = process_html_links(html) + + # Should return unchanged HTML when no base URL configured + assert result == html + + +def test_process_html_links_whitespace_only_content(app): + """Test processing whitespace-only content""" + with app.app_context(): + assert process_html_links(" ") == " " + assert process_html_links("\n\t \n") == "\n\t \n" + + +def test_process_html_links_empty_href_values(app): + """Test handling of empty href values""" + with app.app_context(): + html = '<a href="">Empty href</a><a href=" ">Whitespace href</a>' + result = process_html_links(html) + + # Should not modify empty or whitespace-only hrefs + assert 'href=""' in result + assert 'href=" "' in result + assert "/redirect/?" not in result + + +def test_is_safe_redirect_url_case_insensitive(app): + """Test that host comparison is case-insensitive""" + with app.app_context(): + # Should be safe regardless of case + assert is_safe_redirect_url("http://SUPERSET.EXAMPLE.COM/dashboard";) + assert is_safe_redirect_url("http://Superset.Example.Com/chart";) + + +def test_is_safe_redirect_url_relative_urls(app): + """Test that relative URLs are considered safe""" + with app.app_context(): + assert is_safe_redirect_url("/dashboard/1") + assert is_safe_redirect_url("../chart/list") + assert is_safe_redirect_url("page.html") + + +def test_is_safe_redirect_url_no_base_configured(app): + """Test behavior when no base URL is configured""" + app.config["WEBDRIVER_BASEURL"] = "" + app.config["WEBDRIVER_BASEURL_USER_FRIENDLY"] = "" + + with app.app_context(): + # Should return False when no base URL is configured + assert not is_safe_redirect_url("https://external.com";) + + +def test_process_html_links_invalid_base_url(app): + """Test behavior with invalid base URL""" + app.config["WEBDRIVER_BASEURL"] = "not-a-valid-url" + Review Comment: **Suggestion:** The test sets only `WEBDRIVER_BASEURL` to an invalid value but the implementation prefers `WEBDRIVER_BASEURL_USER_FRIENDLY` when available — since the fixture leaves that valid, the function will still process links and the test will fail. Set both base URL config keys to the invalid value (or pass the invalid base URL explicitly) so the test exercises the intended invalid-base-url behavior. [logic error] **Severity Level:** Minor ⚠️ ```suggestion app.config["WEBDRIVER_BASEURL_USER_FRIENDLY"] = "not-a-valid-url" ``` <details> <summary><b>Why it matters? ⭐ </b></summary> This test intends to exercise the "invalid base URL" early-return behavior, but _get_base_url_and_host() prefers WEBDRIVER_BASEURL_USER_FRIENDLY and only falls back to WEBDRIVER_BASEURL (see link_redirect.py lines ~32-36). The fixture sets WEBDRIVER_BASEURL_USER_FRIENDLY to a valid URL, so changing only WEBDRIVER_BASEURL in the test will not trigger the invalid-base-url path. The proposed fix (set both WEBDRIVER_BASEURL and WEBDRIVER_BASEURL_USER_FRIENDLY to an invalid value, or pass base_url explicitly) makes the test actually exercise the intended branch — this fixes a real test logic bug. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_link_redirect.py **Line:** 296:296 **Comment:** *Logic Error: The test sets only `WEBDRIVER_BASEURL` to an invalid value but the implementation prefers `WEBDRIVER_BASEURL_USER_FRIENDLY` when available — since the fixture leaves that valid, the function will still process links and the test will fail. Set both base URL config keys to the invalid value (or pass the invalid base URL explicitly) so the test exercises the intended invalid-base-url behavior. 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> -- 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]
