Security: Unsanitized SVG content injected via dangerouslySetInnerHTML (XSS risk)#647
Open
tuanaiseo wants to merge 1 commit intositeboon:mainfrom
Conversation
…erouslys The plugin icon renderer fetches SVG text from `/api/plugins/.../assets/...` and injects it directly into the DOM using `dangerouslySetInnerHTML` after only checking that the payload starts with `<svg`. This does not remove malicious attributes/elements (e.g., event handlers, scriptable SVG payloads), enabling DOM-based XSS if a plugin asset is malicious or compromised. Affected files: PluginIcon.tsx Signed-off-by: tuanaiseo <221258316+tuanaiseo@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThe PluginIcon component now includes SVG sanitization logic that parses fetched SVG content, removes dangerous elements and unsafe attributes, and validates the result before rendering to prevent security risks. Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/plugins/view/PluginIcon.tsx (2)
35-36: Consider usinglocalNamefor more robust namespace handling.The
xlink:hrefattribute check relies on the qualified name. While this works in modern browsers, usingattr.localName === 'href'is more robust for namespaced attributes, as namespace prefixes can vary.♻️ Suggested improvement
const name = attr.name.toLowerCase(); + const localName = attr.localName?.toLowerCase() ?? name; const value = attr.value.trim().toLowerCase(); if ( name.startsWith('on') || - name === 'href' || - name === 'xlink:href' || + localName === 'href' || value.startsWith('javascript:') ) { el.removeAttribute(attr.name); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/plugins/view/PluginIcon.tsx` around lines 35 - 36, The attribute check in PluginIcon.tsx that tests for name === 'href' || name === 'xlink:href' is fragile; update the logic to use the attribute object's localName (e.g., attr.localName === 'href') so namespaced prefixes are handled robustly. Locate the check inside the PluginIcon component (the function that iterates SVG attributes) and replace the qualified-name comparisons with a single localName comparison, keeping existing behavior for truthy/non-empty values and ensuring any code paths that relied on 'xlink:href' continue to run when localName === 'href'.
13-48: Consider using DOMPurify for SVG sanitization instead of custom implementation.Custom SVG sanitizers are prone to bypasses due to SVG's large attack surface. The current implementation has specific limitations:
- Removes all
hrefandxlink:hrefattributes, breaking legitimate SVG links and image references- Doesn't handle data URI protocols (e.g.,
data:text/html), which can still be malicious- Doesn't strip inline
styleattributes- May miss edge cases in namespace or protocol handling
DOMPurify is battle-tested, actively maintained against new bypass techniques, and handles these cases correctly. It would require adding DOMPurify as a dependency.
♻️ Recommended approach using DOMPurify
import DOMPurify from 'dompurify'; function sanitizeSvg(svgText: string): string | null { try { const clean = DOMPurify.sanitize(svgText, { USE_PROFILES: { svg: true, svgFilters: false }, FORBID_TAGS: ['foreignObject', 'style'], FORBID_ATTR: ['style'], }); // Ensure result is an SVG if (!clean.trim().startsWith('<svg')) return null; return clean; } catch { return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/plugins/view/PluginIcon.tsx` around lines 13 - 48, The sanitizeSvg function uses a fragile custom sanitizer; replace its logic with DOMPurify: add DOMPurify as a dependency and in sanitizeSvg call DOMPurify.sanitize(svgText, { USE_PROFILES: { svg: true, svgFilters: false }, FORBID_TAGS: ['foreignObject','style'], FORBID_ATTR: ['style'] }) (or equivalent config) then verify the cleaned output starts with an <svg> root and return it or null on failure; remove the manual DOMParser/TreeWalker/attribute-removal code in sanitizeSvg and ensure data: and javascript: protocols are handled by DOMPurify configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/plugins/view/PluginIcon.tsx`:
- Around line 19-21: The SVG sanitizer in PluginIcon.tsx currently removes
elements via
doc.querySelectorAll('script,foreignObject,iframe,object,embed,link,meta,style')
but omits SVG animation elements which can inject event handlers; update the
selector used on the parsed document (the call where doc.querySelectorAll(...)
is invoked inside the PluginIcon component/cleanup logic) to also remove
'animate', 'set', and 'animateTransform' nodes so those SVG animation elements
are stripped before insertion.
---
Nitpick comments:
In `@src/components/plugins/view/PluginIcon.tsx`:
- Around line 35-36: The attribute check in PluginIcon.tsx that tests for name
=== 'href' || name === 'xlink:href' is fragile; update the logic to use the
attribute object's localName (e.g., attr.localName === 'href') so namespaced
prefixes are handled robustly. Locate the check inside the PluginIcon component
(the function that iterates SVG attributes) and replace the qualified-name
comparisons with a single localName comparison, keeping existing behavior for
truthy/non-empty values and ensuring any code paths that relied on 'xlink:href'
continue to run when localName === 'href'.
- Around line 13-48: The sanitizeSvg function uses a fragile custom sanitizer;
replace its logic with DOMPurify: add DOMPurify as a dependency and in
sanitizeSvg call DOMPurify.sanitize(svgText, { USE_PROFILES: { svg: true,
svgFilters: false }, FORBID_TAGS: ['foreignObject','style'], FORBID_ATTR:
['style'] }) (or equivalent config) then verify the cleaned output starts with
an <svg> root and return it or null on failure; remove the manual
DOMParser/TreeWalker/attribute-removal code in sanitizeSvg and ensure data: and
javascript: protocols are handled by DOMPurify configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6b086f71-c90d-4176-acbc-da80ba4edb12
📒 Files selected for processing (1)
src/components/plugins/view/PluginIcon.tsx
Comment on lines
+19
to
+21
| doc | ||
| .querySelectorAll('script,foreignObject,iframe,object,embed,link,meta,style') | ||
| .forEach((el) => el.remove()); |
Contributor
There was a problem hiding this comment.
SVG animate/set elements can bypass event handler sanitization.
The removal list is missing animate, set, and animateTransform elements. These SVG animation elements can dynamically set attribute values, including event handlers:
<svg xmlns="http://www.w3.org/2000/svg">
<rect width="100" height="100">
<set attributeName="onclick" to="alert('XSS')" begin="0s" fill="freeze"/>
</rect>
</svg>When the user clicks the rect, the injected handler executes.
🛡️ Proposed fix to include animation elements
doc
- .querySelectorAll('script,foreignObject,iframe,object,embed,link,meta,style')
+ .querySelectorAll('script,foreignObject,iframe,object,embed,link,meta,style,animate,set,animateTransform,animateMotion')
.forEach((el) => el.remove());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/plugins/view/PluginIcon.tsx` around lines 19 - 21, The SVG
sanitizer in PluginIcon.tsx currently removes elements via
doc.querySelectorAll('script,foreignObject,iframe,object,embed,link,meta,style')
but omits SVG animation elements which can inject event handlers; update the
selector used on the parsed document (the call where doc.querySelectorAll(...)
is invoked inside the PluginIcon component/cleanup logic) to also remove
'animate', 'set', and 'animateTransform' nodes so those SVG animation elements
are stripped before insertion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The plugin icon renderer fetches SVG text from
/api/plugins/.../assets/...and injects it directly into the DOM usingdangerouslySetInnerHTMLafter only checking that the payload starts with<svg. This does not remove malicious attributes/elements (e.g., event handlers, scriptable SVG payloads), enabling DOM-based XSS if a plugin asset is malicious or compromised.Severity:
highFile:
src/components/plugins/view/PluginIcon.tsxSolution
Sanitize SVG content before injection using a strict sanitizer (e.g., DOMPurify with SVG profile and forbidden tags/attributes), or render icons as safe image sources (
<img src=...>) with server-side content-type validation and CSP hardening. AvoiddangerouslySetInnerHTMLfor untrusted plugin-provided markup.Changes
src/components/plugins/view/PluginIcon.tsx(modified)Testing
Summary by CodeRabbit