Repository navigation
🛡️ Sentinel: [HIGH] Fix Regex-based XSS evasion in SVG validation - #1113
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Thanks for opening your first pull request to thesvg! We appreciate you taking the time to contribute. Please make sure you've read the README for contribution guidelines. A maintainer will take a look soon. |
github-actions
Bot
added
documentation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const allElements = doc.querySelectorAll("*"); | ||
| for (let i = 0; i < allElements.length; i++) { | ||
| const el = allElements[i]; | ||
| for (let j = 0; j < el.attributes.length; j++) { | ||
| const attr = el.attributes[j]; | ||
| const name = attr.name.toLowerCase(); | ||
| if (name.startsWith("on")) { | ||
| hasOnHandlers = true; | ||
| } | ||
| if (name === "href" || name.endsWith(":href")) { | ||
| const val = attr.value.replace(/[\s\x00-\x1F\x7F]+/g, "").toLowerCase(); | ||
| if (val.startsWith("javascript:")) { | ||
| hasJavascriptHref = true; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This security-sensitive rewrite has no regression test covering the behavior it introduces. Please add cases for numeric-entity-encoded and control-character-obfuscated javascript: values, namespaced xlink:href, ordinary JavaScript URLs, and event-handler attributes. Without these tests, a future refactor could silently reintroduce the documented XSS bypass.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
1 participant
🚨 Severity: HIGH
💡 Vulnerability:
validateSvginsrc/lib/svg-validation.tsrelied on Regex tests against the raw SVG string (/href\s*=\s*["']javascript:/i) to prevent malicious attributes. This is vulnerable to evasion using HTML entity encoding (e.g.,javascript:alert(1)) or control characters, which Regex fails to catch but the browser will execute when parsed.🎯 Impact: An attacker could submit a malicious SVG with entity-encoded XSS payloads that bypasses the validation and executes javascript when opened or rendered.
🔧 Fix: Changed the logic to use
DOMParserto parse the SVG and traverse all elements, inspectingel.attributesafter stripping whitespace/control characters. This ensures that the validation sees the same normalized attributes that the browser does.✅ Verification: Verify by running
pnpm testandpnpm lint. Attempting to validate<a href="javascript:alert(1)">will now correctly fail validation.PR created automatically by Jules for task 11245609607414445165 started by @thegdsks