codeant-ai-for-open-source[bot] commented on code in PR #37104:
URL: https://github.com/apache/superset/pull/37104#discussion_r2704718006
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -117,6 +117,27 @@ class CeleryConfig:
log_level_text = os.getenv("SUPERSET_LOG_LEVEL", "INFO")
LOG_LEVEL = getattr(logging, log_level_text.upper(), logging.INFO)
+# Allow iframes in Markdown components (for embedding YouTube videos, etc.)
+HTML_SANITIZATION_SCHEMA_EXTENSIONS = {
+ "tagNames": ["iframe"],
+ "attributes": {
+ "iframe": [
+ "src",
+ "width",
+ "height",
+ "allow",
+ "allowfullscreen",
+ "title",
+ "referrerpolicy",
+ "sandbox",
+ "loading",
+ ],
+ },
+ "protocols": {
+ "src": ["https"],
Review Comment:
**Suggestion:** Security bug: the `protocols` mapping is the wrong shape for
rehype-sanitize-like schemas — it maps attribute names at the top level instead
of mapping by tag then attribute. As written the protocol restriction will be
ignored, potentially allowing unsafe protocols (e.g. javascript:) in `iframe`
`src`. Move `src` under the `iframe` key so the sanitizer can enforce allowed
protocols. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard Markdown iframe XSS if unsafe protocols allowed.
- ⚠️ Chart/Handlebars embeds may accept unsafe URLs.
- ⚠️ Dev images expose permissive sanitizer configuration.
```
</details>
```suggestion
"iframe": {"src": ["https"]},
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset using the Docker development image that includes
docker/pythonpath_dev/superset_config.py (file present at
docker/pythonpath_dev/superset_config.py:120-139).
2. In the web UI, create or edit a Dashboard and add a Markdown component
(follow PR's
"First scenario" testing instructions to reach the Markdown editor).
3. Paste an iframe with a non-HTTPS/malicious protocol, e.g. <iframe
src="javascript:alert(1)"></iframe>, into the Markdown and save the
dashboard.
4. Observe whether the iframe src protocol is rejected or allowed. Because
the current
config places "protocols": {"src": ["https"]} at top-level
(docker/pythonpath_dev/superset_config.py:136-138), sanitizer
implementations expecting
per-tag attribute mappings may ignore it, potentially allowing the unsafe
protocol to
pass.
Note: The reproduction steps use the actual config location and the
Dashboard Markdown
editor entry point referenced in the PR testing instructions; the behavior
depends on how
the server/frontend merges this dict into the sanitizer schema.
```
</details>
<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:** 137:137
**Comment:**
*Security: Security bug: the `protocols` mapping is the wrong shape for
rehype-sanitize-like schemas — it maps attribute names at the top level instead
of mapping by tag then attribute. As written the protocol restriction will be
ignored, potentially allowing unsafe protocols (e.g. javascript:) in `iframe`
`src`. Move `src` under the `iframe` key so the sanitizer can enforce allowed
protocols.
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]
