codeant-ai-for-open-source[bot] commented on code in PR #42495: URL: https://github.com/apache/superset/pull/42495#discussion_r3661975773
########## docker/pythonpath_dev/superset_config.py: ########## @@ -114,7 +114,11 @@ class CeleryConfig: } EXTENSIONS_PATH = "/app/docker/extensions" ALERT_REPORTS_NOTIFICATION_DRY_RUN = True -WEBDRIVER_BASEURL = f"http://superset_app{os.environ.get('SUPERSET_APP_ROOT', '/')}/" # When using docker compose baseurl should be http://superset_nginx{ENV{BASEPATH}}/ # noqa: E501 +# The Docker Compose app service is named "superset" and listens on 8088. Only the +# scheme/host/port matter here (paths are joined with urljoin). For screenshots in +# the dev stack (unbuilt static assets) point this at the nginx service instead: +# http://nginx{SUPERSET_APP_ROOT}/ +WEBDRIVER_BASEURL = f"http://superset:8088{os.environ.get('SUPERSET_APP_ROOT', '/')}/" Review Comment: **Suggestion:** The default still points report browsers at the `superset` backend, while the dev Compose stack serves the dynamically built frontend assets through the `nginx` service. Screenshot navigation therefore loads the application HTML from `superset` but receives missing or stale static assets, causing screenshot reports to fail. The comment acknowledges that screenshots require `nginx`, but the executable default does not apply that routing; use the asset-serving endpoint for screenshot execution or provide separate defaults for data requests and screenshots. [api mismatch] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Dev screenshot reports load without webpack frontend assets. - ❌ Alerts and Reports screenshot delivery fails or renders unusable pages. - ⚠️ The adjacent comment documents nginx routing but the executable default does not use it. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the development stack with `docker compose up`; `docker-compose.yml:50-56` defines the `nginx` service, while `docker-compose.yml:72-83` exposes the `superset` backend on port 8088. 2. Create or execute a screenshot report through the worker path; the worker is defined at `docker-compose.yml:218-238`, and report browser URLs are read from `WEBDRIVER_BASEURL` by `superset/utils/urls.py:24-31`. 3. The new configuration at `docker/pythonpath_dev/superset_config.py:121` directs the browser to `http://superset:8088`, bypassing nginx. 4. The HTML loaded from the backend references frontend assets under `/static`; however, `docker/nginx/templates/superset.conf.template:40-44` explicitly routes those assets to the webpack dev server at `host.docker.internal:9000`, while the backend URL does not provide that dev-server routing. Screenshot rendering therefore encounters missing frontend assets, whereas navigating through the `nginx` service would apply the required static-asset proxy. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b044135564443b0ba3dae11f05cade0&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=0b044135564443b0ba3dae11f05cade0&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:** docker/pythonpath_dev/superset_config.py **Line:** 121:121 **Comment:** *Api Mismatch: The default still points report browsers at the `superset` backend, while the dev Compose stack serves the dynamically built frontend assets through the `nginx` service. Screenshot navigation therefore loads the application HTML from `superset` but receives missing or stale static assets, causing screenshot reports to fail. The comment acknowledges that screenshots require `nginx`, but the executable default does not apply that routing; use the asset-serving endpoint for screenshot execution or provide separate defaults for data requests and screenshots. 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%2F42495&comment_hash=46a4fd8f6a5149e08d3808efba10389fa4002f69928a875f9a8059f8649ab96e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42495&comment_hash=46a4fd8f6a5149e08d3808efba10389fa4002f69928a875f9a8059f8649ab96e&reaction=dislike'>👎</a> ########## docs/admin_docs/configuration/alerts-reports.mdx: ########## @@ -402,9 +402,9 @@ Possible fixes: The worker may be unable to reach the report. It will use the value of `WEBDRIVER_BASEURL` to browse to the report. If that route is invalid, or presents an authentication challenge that the worker can't pass, the report screenshot will fail. -Check this by attempting to `curl` the URL of a report that you see in the error logs of your worker. For instance, from the worker environment, run `curl http://superset_app:8088/superset/dashboard/1/`. You may get different responses depending on whether the dashboard exists - for example, you may need to change the `1` in that URL. If there's a URL in your logs from a failed report screenshot, that's a good place to start. The goal is to determine a valid value for `WEBDRIVER_BASEURL` and determine if an issue like HTTPS or authentication is redirecting your worker. +Check this by attempting to `curl` the URL of a report that you see in the error logs of your worker. For instance, from the worker environment, run `curl http://superset:8088/superset/dashboard/1/`. You may get different responses depending on whether the dashboard exists - for example, you may need to change the `1` in that URL. If there's a URL in your logs from a failed report screenshot, that's a good place to start. The goal is to determine a valid value for `WEBDRIVER_BASEURL` and determine if an issue like HTTPS or authentication is redirecting your worker. -In a deployment with authentication measures enabled like HTTPS and Single Sign-On, it may make sense to have the worker navigate directly to the Superset application running in the same location, avoiding the need to sign in. For instance, you could use `WEBDRIVER_BASEURL="http://superset_app:8088"` for a docker compose deployment, and set `"force_https": False,` in your `TALISMAN_CONFIG`. +In a deployment with authentication measures enabled like HTTPS and Single Sign-On, it may make sense to have the worker navigate directly to the Superset application running in the same location, avoiding the need to sign in. For instance, you could use `WEBDRIVER_BASEURL="http://superset:8088"` for a docker compose deployment, and set `"force_https": False,` in your `TALISMAN_CONFIG`. Review Comment: **Suggestion:** This copyable example recommends setting `force_https` to `False` together with an HTTP internal URL, but it does not restrict that setting to a dev-only or otherwise explicitly protected internal configuration. Operators can apply the example to a deployment that relies on Talisman for HTTPS enforcement and silently disable an important transport-security control. Clarify that this is only appropriate when HTTPS is enforced by a trusted upstream proxy or for an isolated development stack, rather than presenting it as a general Docker Compose fix. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Operators may disable HTTPS enforcement in production. - ❌ Authenticated report traffic may use unencrypted HTTP. - ⚠️ Talisman protection becomes weaker than intended. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Follow the Alerts and Reports guidance at `docs/admin_docs/configuration/alerts-reports.mdx:407` in an installation that uses HTTPS and has `TALISMAN_CONFIG` configured with HTTPS enforcement. 2. Set `WEBDRIVER_BASEURL` to the documented internal HTTP URL and copy the suggested `"force_https": False` setting into `TALISMAN_CONFIG`. 3. Talisman no longer upgrades HTTP requests; the security documentation confirms that `force_https` controls this behavior at `docs/admin_docs/security/security.mdx:644-646`. 4. If the deployment does not enforce HTTPS at a trusted load balancer or application gateway, report-browser traffic can remain HTTP, weakening transport security for authenticated dashboard/report requests. The existing security guidance at `docs/admin_docs/security/security.mdx:645-647` distinguishes upstream HTTPS enforcement, but this new example does not state that prerequisite. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=64e50b1f02d84b8984a80dd76612d2e4&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=64e50b1f02d84b8984a80dd76612d2e4&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:** docs/admin_docs/configuration/alerts-reports.mdx **Line:** 407:407 **Comment:** *Security: This copyable example recommends setting `force_https` to `False` together with an HTTP internal URL, but it does not restrict that setting to a dev-only or otherwise explicitly protected internal configuration. Operators can apply the example to a deployment that relies on Talisman for HTTPS enforcement and silently disable an important transport-security control. Clarify that this is only appropriate when HTTPS is enforced by a trusted upstream proxy or for an isolated development stack, rather than presenting it as a general Docker Compose fix. 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%2F42495&comment_hash=b42dbc16074347d1b889e1c57a7fcc9e93660deb68741f09319a4f576a572a51&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42495&comment_hash=b42dbc16074347d1b889e1c57a7fcc9e93660deb68741f09319a4f576a572a51&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]
