codeant-ai-for-open-source[bot] commented on code in PR #40481:
URL: https://github.com/apache/superset/pull/40481#discussion_r3315532334
##########
superset/views/base.py:
##########
@@ -413,25 +413,28 @@ def get_default_spinner_svg() -> str | None:
Returns:
str | None: SVG content as string, or None if file not found
"""
- try:
- # Path to frontend source SVG file (used by both frontend and backend)
- svg_path = os.path.join(
- os.path.dirname(__file__),
- "..",
- "..",
- "superset-frontend",
- "packages",
- "superset-ui-core",
- "src",
- "components",
- "assets",
- "images",
- "loading.svg",
- )
+ # Path to frontend source SVG file (used by both frontend and backend)
+ svg_path = os.path.join(
+ os.path.dirname(__file__),
+ "..",
+ "..",
+ "superset-frontend",
+ "packages",
+ "superset-ui-core",
+ "src",
+ "components",
+ "assets",
+ "images",
+ "loading.svg",
+ )
+ if not os.path.exists(svg_path):
+ return None
Review Comment:
**Suggestion:** The existence pre-check can silently swallow filesystem
access problems and skip the warning path you intended to keep for unexpected
read failures. `os.path.exists()` returns `False` when `stat()` fails (for
example due to permission-denied on parent directories), so the function
returns `None` as if the file were missing instead of logging the underlying
error. Handle missing-file behavior in the `open()` exception flow (suppress
only file-not-found) so non-missing I/O failures are still reported. [incorrect
condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SPA endpoints hide spinner filesystem permission misconfigurations.
- ⚠️ Harder to diagnose asset permission issues from logs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a running Superset deployment, ensure the default spinner is using the
backend
helper by not setting `brandSpinnerSvg` or `brandSpinnerUrl` in the theme
tokens used by
`get_spa_template_context` in `superset/views/base.py:580-641`, so the code
at
`superset/views/base.py:63-70` (file lines 620-627 from the Read output)
follows the `elif
not theme_tokens.get("brandSpinnerUrl"):` branch and calls
`get_default_spinner_svg()`.
2. On the filesystem, create the SVG file at the path constructed in
`get_default_spinner_svg()` (`superset/views/base.py:37-49` in the snippet,
corresponding
to diff lines 416-428) but remove execute/read permission from one of its
parent
directories (for example `superset-frontend/`), so that `os.stat(svg_path)`
fails with a
`PermissionError` even though the file exists; per CPython semantics,
`os.path.exists(svg_path)` in `superset/views/base.py:431` will return
`False` when this
`stat()` call fails.
3. Trigger any SPA view that uses `render_app_template()`, for example send
an
authenticated `GET /extensions/list/` request, which is handled by
`ExtensionsView.list()`
in `superset/extensions/view.py:7-11`, calling
`super().render_app_template()`, which in
turn calls `get_spa_template_context()` in `superset/views/base.py:240-243`
and ultimately
`get_default_spinner_svg()` at `superset/views/base.py:409-60`.
4. Observe that `get_default_spinner_svg()` returns `None` early via the `if
not
os.path.exists(svg_path): return None` branch at
`superset/views/base.py:431-432`, so the
`try/except (OSError, UnicodeDecodeError)` block at
`superset/views/base.py:435-60` is
never entered and no `logger.warning("Could not load default spinner SVG:
%s", e)` message
is emitted, even though the underlying filesystem error is a permission
problem rather
than a genuinely missing file; if the pre-check were removed and
missing-file handling
moved into the `open()` exception flow (catching `FileNotFoundError`
separately), the same
misconfiguration would result in a warning log as originally intended for
unexpected I/O
failures.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=106b90d342ea400a8b6be0e5e8c0b8d2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=106b90d342ea400a8b6be0e5e8c0b8d2&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/views/base.py
**Line:** 431:432
**Comment:**
*Incorrect Condition Logic: The existence pre-check can silently
swallow filesystem access problems and skip the warning path you intended to
keep for unexpected read failures. `os.path.exists()` returns `False` when
`stat()` fails (for example due to permission-denied on parent directories), so
the function returns `None` as if the file were missing instead of logging the
underlying error. Handle missing-file behavior in the `open()` exception flow
(suppress only file-not-found) so non-missing I/O failures are still reported.
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%2F40481&comment_hash=0ba0d547e8b23490e214c9ca851c5c861b37fd74300b1a9b336edd833a9a802c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40481&comment_hash=0ba0d547e8b23490e214c9ca851c5c861b37fd74300b1a9b336edd833a9a802c&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]
