fix: Security removal dependency svg-prep#3236
Merged
mtrezza merged 2 commits intoparse-community:alphafrom Feb 20, 2026
Merged
fix: Security removal dependency svg-prep#3236mtrezza merged 2 commits intoparse-community:alphafrom
mtrezza merged 2 commits intoparse-community:alphafrom
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughRemoved the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
webpack/plugins/svg-prep.js (1)
19-23: Synchronous file I/O inside anasynctapPromisehandler blocks the event loop.Both
fs.readdirSyncandfs.readFileSyncare synchronous. Inside atapPromise(async) hook, the idiomatic approach isfs.promises.readdir/fs.promises.readFileso Webpack can overlap I/O with other work. For a small icon set at build time the impact is negligible, but it does defeat the async contract.♻️ Proposed async refactor
-const fs = require('fs'); +const fs = require('fs'); +const fsPromises = require('fs').promises; async function buildSvgSprite(files) { const symbols = await Promise.all(files.map(async file => { const name = path.basename(file, '.svg'); - const svg = fs.readFileSync(file, 'utf-8'); + const svg = await fsPromises.readFile(file, 'utf-8'); // ... rest unchanged })); return [ '<?xml version="1.0" encoding="UTF-8" standalone="yes"?>', '<svg id="sprites" xmlns="http://www.w3.org/2000/svg" style="display:none">', symbols.join('\n'), '</svg>', ].join('\n'); }In
SvgPrepPlugin.prototype.apply:- const files = fs - .readdirSync(this.options.source) + const files = (await fsPromises.readdir(this.options.source)) .filter(name => name.endsWith('.svg')) .sort() .map(name => path.join(this.options.source, name)); - const sprited = buildSvgSprite(files); + const sprited = await buildSvgSprite(files);Also applies to: 81-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webpack/plugins/svg-prep.js` around lines 19 - 23, The buildSvgSprite function and the SvgPrepPlugin.prototype.apply tapPromise handler currently use synchronous I/O (fs.readdirSync, fs.readFileSync) which blocks the event loop; change these to the Promise-based APIs (fs.promises.readdir and fs.promises.readFile) and make buildSvgSprite async so it returns a Promise of symbols, then await it inside the tapPromise handler (or return the Promise directly from the tapPromise callback). Update references to path.basename and any synchronous parsing to work with the async flow and ensure error handling uses try/catch inside the async function so the tapPromise rejects on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webpack/plugins/svg-prep.js`:
- Around line 34-45: The sanitization pipeline building the `cleaned` string
from `inner` must explicitly remove <script> tags and their content, strip all
event-handler attributes matching /^on[a-z]+/ (both double- and single-quoted
forms), and neutralize href/xlink:href attributes that contain javascript: URIs;
update the chain of .replace(...) calls that produce `cleaned` to include
regexes that remove /<script[\s\S]*?<\/script>/gi, attributes like
/\s+on[a-z]+="[^"]*"/gi and /\s+on[a-z]+='[^']*'/gi, and href/xlink:href
patterns that match javascript: URIs (both quoted variants) so SVG sprites
cannot carry script or event XSS vectors while preserving the existing
style/defs/title/desc/comment removal behavior.
- Line 47: The symbol id is used verbatim from name and can produce malformed
XML/invalid NCName values; create and call a sanitizeSymbolId(name) helper (or
inline logic) before embedding it in the template: 1) escape XML attribute
characters (&, <, >, ", ') for the id attribute value, 2) normalize/replace
invalid NCName characters (whitespace, colons, slashes, punctuation) with safe
characters (e.g. hyphen), and 3) ensure the id does not start with a digit or
other invalid starting char (prefix with a safe string like "svg-" when needed);
then replace the existing id="${name}" usage with id="${sanitizeSymbolId(name)}"
and keep the cleaned content insertion as-is.
---
Nitpick comments:
In `@webpack/plugins/svg-prep.js`:
- Around line 19-23: The buildSvgSprite function and the
SvgPrepPlugin.prototype.apply tapPromise handler currently use synchronous I/O
(fs.readdirSync, fs.readFileSync) which blocks the event loop; change these to
the Promise-based APIs (fs.promises.readdir and fs.promises.readFile) and make
buildSvgSprite async so it returns a Promise of symbols, then await it inside
the tapPromise handler (or return the Promise directly from the tapPromise
callback). Update references to path.basename and any synchronous parsing to
work with the async flow and ensure error handling uses try/catch inside the
async function so the tapPromise rejects on error.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webpack/plugins/svg-prep.js (1)
25-26:viewBoxextraction silently fails for single-quoted attributes.The regex only matches double-quoted
viewBoxvalues. Any icon SVG usingviewBox='...'(single-quoted, which is valid per the XML spec) will silently fall back to0 0 100 100and may render distorted.♻️ Proposed fix to handle both quote styles
- const viewBoxMatch = svg.match(/viewBox="([^"]*)"/); + const viewBoxMatch = svg.match(/viewBox=["']([^"']*)["']/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webpack/plugins/svg-prep.js` around lines 25 - 26, The viewBox extraction in svg-prep.js only matches double-quoted attributes and falls back incorrectly; update the regex used with the svg string (the code creating viewBoxMatch and viewBox) to accept either single or double quotes (e.g., match viewBox=(["'])(.*?)\1), then set viewBox = viewBoxMatch ? viewBoxMatch[2] : '0 0 100 100' so single-quoted attributes are correctly captured by the existing viewBoxMatch/viewBox logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webpack/plugins/svg-prep.js`:
- Around line 34-50: The sanitization pipeline building the cleaned string
(variable cleaned in svg-prep.js) currently strips scripts, event handlers,
hrefs, etc., but misses <foreignObject> elements; update the chain of .replace
calls that produce cleaned to also remove any <foreignObject>...</foreignObject>
blocks (case-insensitive) by adding a regex replacement for
/<foreignObject[\s\S]*?<\/foreignObject>/gi so embedded HTML/iframes inside SVGs
are stripped before writing sprites.svg.
---
Duplicate comments:
In `@webpack/plugins/svg-prep.js`:
- Line 52: The symbol id is generated directly from filename (variable name) and
can produce invalid XML/NCName values; implement a sanitize function (e.g.,
sanitizeSvgId(name)) used before interpolation to: 1) remove or replace
XML-special characters (&, <, >, ", ') and also remove colons, whitespace and
other non-name characters (replace them with hyphens), 2) ensure the result
starts with a letter or underscore (prefix with '_' or 'id-' if it starts with a
digit or other invalid start), and 3) collapse repeated hyphens and trim
boundary hyphens so the final id is a valid NCName; then use sanitizeSvgId(name)
instead of name in the template that returns the <symbol> string.
---
Nitpick comments:
In `@webpack/plugins/svg-prep.js`:
- Around line 25-26: The viewBox extraction in svg-prep.js only matches
double-quoted attributes and falls back incorrectly; update the regex used with
the svg string (the code creating viewBoxMatch and viewBox) to accept either
single or double quotes (e.g., match viewBox=(["'])(.*?)\1), then set viewBox =
viewBoxMatch ? viewBoxMatch[2] : '0 0 100 100' so single-quoted attributes are
correctly captured by the existing viewBoxMatch/viewBox logic.
Comment on lines
+34
to
+50
| const cleaned = inner | ||
| .replace(/<style[\s\S]*?<\/style>/gi, '') | ||
| .replace(/<defs[\s\S]*?<\/defs>/gi, '') | ||
| .replace(/<title[\s\S]*?<\/title>/gi, '') | ||
| .replace(/<desc[\s\S]*?<\/desc>/gi, '') | ||
| .replace(/<script[\s\S]*?<\/script>/gi, '') | ||
| .replace(/<!--[\s\S]*?-->/g, '') | ||
| .replace(/\s+id="[^"]*"/g, '') | ||
| .replace(/\s+fill="[^"]*"/g, '') | ||
| .replace(/\s+class="[^"]*"/g, '') | ||
| .replace(/\s+style="[^"]*"/g, '') | ||
| .replace(/\s+stroke="[^"]*"/g, '') | ||
| .replace(/\s+stroke-[a-z]+="[^"]*"/g, '') | ||
| .replace(/\s+on[a-zA-Z]+="[^"]*"/g, '') | ||
| .replace(/\s+on[a-zA-Z]+='[^']*'/g, '') | ||
| .replace(/\s+href="[^"]*"/g, '') | ||
| .replace(/\s+xlink:href="[^"]*"/g, ''); |
There was a problem hiding this comment.
<foreignObject> elements are not stripped.
All previously flagged XSS vectors (<script>, on* attributes, href/xlink:href) are now removed. However, <foreignObject> — which allows embedding arbitrary HTML content inside SVG — is still passed through. Even though descendant on* attributes would be caught by the attribute regexes, the element itself can carry HTML <iframe>, navigation anchors, and other non-script vectors that survive into sprites.svg.
🔒 Proposed addition to strip <foreignObject>
const cleaned = inner
.replace(/<style[\s\S]*?<\/style>/gi, '')
.replace(/<defs[\s\S]*?<\/defs>/gi, '')
.replace(/<title[\s\S]*?<\/title>/gi, '')
.replace(/<desc[\s\S]*?<\/desc>/gi, '')
.replace(/<script[\s\S]*?<\/script>/gi, '')
+ .replace(/<foreignObject[\s\S]*?<\/foreignObject>/gi, '')
.replace(/<!--[\s\S]*?-->/g, '')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webpack/plugins/svg-prep.js` around lines 34 - 50, The sanitization pipeline
building the cleaned string (variable cleaned in svg-prep.js) currently strips
scripts, event handlers, hrefs, etc., but misses <foreignObject> elements;
update the chain of .replace calls that produce cleaned to also remove any
<foreignObject>...</foreignObject> blocks (case-insensitive) by adding a regex
replacement for /<foreignObject[\s\S]*?<\/foreignObject>/gi so embedded
HTML/iframes inside SVGs are stripped before writing sprites.svg.
parseplatformorg
pushed a commit
that referenced
this pull request
Feb 20, 2026
## [9.0.1-alpha.7](9.0.1-alpha.6...9.0.1-alpha.7) (2026-02-20) ### Bug Fixes * Security removal dependency svg-prep ([#3236](#3236)) ([abb08c6](abb08c6))
Contributor
|
🎉 This change has been released in version 9.0.1-alpha.7 |
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
2 participants
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.
Pull Request
Issue
Security removal dependency svg-prep
Summary by CodeRabbit