codeant-ai-for-open-source[bot] commented on code in PR #43389:
URL: https://github.com/apache/superset/pull/43389#discussion_r3831839218
##########
tests/unit_tests/utils/test_core.py:
##########
@@ -1942,13 +1942,28 @@ def test_sanitize_svg_content_safe():
def test_sanitize_svg_content_removes_scripts():
- """Test that nh3 removes dangerous script content."""
+ """Test that dangerous script content is removed."""
malicious_svg = '<svg><script>alert("xss")</script><rect/></svg>'
result = sanitize_svg_content(malicious_svg)
assert "script" not in result.lower()
assert "alert" not in result
+def test_sanitize_svg_content_removes_script_with_attributes_on_closer():
+ """A closing </script foo> tag is still a valid closer to browsers."""
+ malicious_svg = "<svg><script>fetch('/api/v1/me/')</script foo></svg>"
+ result = sanitize_svg_content(malicious_svg)
+ assert "script" not in result.lower()
+ assert "fetch" not in result
+
+
+def test_sanitize_svg_content_removes_unterminated_script():
+ """An unterminated <script> opener with no closing tag is still
stripped."""
+ malicious_svg = "<svg><script>alert('xss')"
+ result = sanitize_svg_content(malicious_svg)
+ assert "script" not in result.lower()
Review Comment:
**Suggestion:** The unterminated-script regression test only checks that the
literal `script` tag is removed, so it would still pass if the sanitizer left
the script payload (`alert('xss')`) in the returned SVG. Assert that the
payload is removed as well, as the paired-script test does, to ensure this test
detects incomplete sanitization. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Unterminated-script regression coverage is incomplete.
- โ ๏ธ Theme SVG sanitization uses this sanitizer at
`superset/themes/utils.py:178`.
- โ ๏ธ Future sanitizer regressions could leave script payloads undetected.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<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_core.py
**Line:** 1960:1964
**Comment:**
*Incomplete Implementation: The unterminated-script regression test
only checks that the literal `script` tag is removed, so it would still pass if
the sanitizer left the script payload (`alert('xss')`) in the returned SVG.
Assert that the payload is removed as well, as the paired-script test does, to
ensure this test detects incomplete sanitization.
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%2F43389&comment_hash=f2697825ab026deb4d0e3ad970c0e023cd2fc88ca71bab6a755c768bb2377407&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43389&comment_hash=f2697825ab026deb4d0e3ad970c0e023cd2fc88ca71bab6a755c768bb2377407&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]
