Re: [PR] fix(superset_app_root): when used with oauth [superset]

["codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>]

codeant-ai-for-open-source[bot] commented on code in PR #38033:
URL: https://github.com/apache/superset/pull/38033#discussion_r3503508962


##########
superset/app.py:
##########
@@ -74,6 +74,9 @@ def create_app(
             # value of app_root so things work out of the box
             if not app.config["STATIC_ASSETS_PREFIX"]:
                 app.config["STATIC_ASSETS_PREFIX"] = app_root
+            # Prefix APP_ICON path with subdirectory root for subdirectory deployments
+            if app.config.get("APP_ICON", "").startswith("/static/"):
+                app.config["APP_ICON"] = f"{app_root}{app.config['APP_ICON']}"

Review Comment:
   **Suggestion:** The new prefixing logic only handles icons that start with `/static/`, so valid root-relative custom icon paths (for example `/assets/logo.svg`) are left unprefixed and will still break when the app runs under a non-root `SUPERSET_APP_ROOT`. Prefix all root-relative local icon paths (while avoiding already-prefixed values) instead of hard-coding `/static/`. [incomplete implementation]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   ⚠️ Subpath deployments show broken logo in Superset UI.
   ⚠️ MCP hello page logo URL missing app_root prefix.
   ⚠️ AppRootMiddleware rejects logo requests not starting app_root.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. In `superset/config.py:14`, note the default `APP_ICON =
   "/static/assets/images/superset-logo-horiz.png"` and the comment at
   `superset/config.py:11-13` explaining users may override `APP_ICON` via
   `superset_config.py`; configure `APP_ICON = "/assets/logo.svg"` (a root-relative but
   non-`/static/` path) so at runtime `app.config["APP_ICON"]` is `"/assets/logo.svg"`.
   
   2. Start Superset with a non-root application path, e.g. `SUPERSET_APP_ROOT=/superset`; in
   `superset/app.py:64-71` `create_app()` resolves `app_root` to `"/superset"` from
   `superset_app_root` / `SUPERSET_APP_ROOT` / `APPLICATION_ROOT`.
   
   3. In `create_app()`, when `app_root != "/"`, the new icon prefixing logic at
   `superset/app.py:75-79` runs: it prefixes `APP_ICON` only if `app.config.get("APP_ICON",
   "").startswith("/static/")`; because `APP_ICON` is `"/assets/logo.svg"`, this condition is
   false and the value remains unprefixed.
   
   4. The UI and services then use this unprefixed icon path: the theme base in
   `superset/config.py:10-15` sets `brandLogoUrl` to `APP_ICON`, and the MCP server in
   `superset/mcp_service/server.py:10-27` builds `logo_url` from `APP_ICON` for root-relative
   paths; when the browser requests `"/assets/logo.svg"` under
   `SUPERSET_APP_ROOT="/superset"`, `AppRootMiddleware.__call__` at `superset/app.py:26-35`
   only serves requests whose `PATH_INFO` starts with `"/superset"` and returns `NotFound`
   for others, so the logo image fails to load for subpath deployments using such
   root-relative custom icons.
   ```
   </details>
   
   [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ec76c63126a1464ca51228adcabe2a8a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ec76c63126a1464ca51228adcabe2a8a&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/app.py
   **Line:** 77:79
   **Comment:**
   	*Incomplete Implementation: The new prefixing logic only handles icons that start with `/static/`, so valid root-relative custom icon paths (for example `/assets/logo.svg`) are left unprefixed and will still break when the app runs under a non-root `SUPERSET_APP_ROOT`. Prefix all root-relative local icon paths (while avoiding already-prefixed values) instead of hard-coding `/static/`.
   
   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%2F38033&comment_hash=14835c608fed3b66a2820127ab09c0accba41719836b5f49bb384124344dbb27&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38033&comment_hash=14835c608fed3b66a2820127ab09c0accba41719836b5f49bb384124344dbb27&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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org