fix: escape HTML attributes to prevent XSS attacks#1741
Conversation
- Add escapeHtml() utility function to sanitize attribute values - Escape double quotes and special characters in alt, srcset, sizes attributes - Escape iframe title attribute to prevent XSS - Escape auto-generated captions from title/alt attributes - Add comprehensive security test suite with 8 XSS test cases This fixes a critical XSS vulnerability where unescaped double quotes in the alt attribute allowed attackers to break out of the attribute context and inject malicious HTML/JavaScript (e.g., "><img src=x onerror=alert(1)>). The escapeHtml() function now properly encodes: - & to & - < to < - > to > - " to " - ' to ' All security tests now pass, confirming the vulnerability is fixed.
|
|
📝 WalkthroughWalkthroughThis PR adds XSS prevention by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (1)src/lg-utils.ts (1)
🪛 ast-grep (0.40.3)test/lightgallery.test.ts[warning] 308-312: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify. (unsafe-html-content-assignment) [warning] 338-342: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify. (unsafe-html-content-assignment) [warning] 367-371: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify. (unsafe-html-content-assignment) [warning] 392-396: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify. (unsafe-html-content-assignment) [warning] 419-423: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify. (unsafe-html-content-assignment) [warning] 445-449: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify. (unsafe-html-content-assignment) [warning] 471-475: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify. (unsafe-html-content-assignment) [warning] 497-501: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify. (unsafe-html-content-assignment) [warning] 308-312: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) [warning] 338-342: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) [warning] 367-371: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) [warning] 392-396: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) [warning] 419-423: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) [warning] 445-449: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) [warning] 471-475: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) [warning] 497-501: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) 🔇 Additional comments (7)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lg-utils.ts (1)
437-443: Critical XSS vulnerability: source attribute values are not escaped.The source attributes are built using string concatenation without escaping the values. An attacker could inject malicious content through the
sourcesarray attributes (e.g.,srcset,media,sizes,type).Example exploit:
sources: [{srcset: '" onload="alert(1)" x="'}] // Results in: <source srcset="" onload="alert(1)" x=""></source>🔎 Proposed fix
sourceTag = sourceObj.map((source: any) => { let attrs = ''; Object.keys(source).forEach((key) => { // Do not remove the first space as it is required to separate the attributes - attrs += ` ${key}="${source[key]}"`; + attrs += ` ${key}="${this.escapeHtml(source[key])}"`; }); return `<source ${attrs}></source>`; });src/lightgallery.ts (1)
1278-1294: Critical XSS vulnerability: alt attribute not escaped in zoom animation path.Line 1281 constructs the alt attribute without escaping, creating an XSS vulnerability when
zoomFromOriginis enabled. This is inconsistent with the fix applied at line 1007.const altAttr = alt ? 'alt="' + alt + '"' : ''; // Line 1281 - NOT ESCAPEDAn attacker could exploit this with a payload like:
alt='"><img src=x onerror=alert(1)>'🔎 Proposed fix
if (this.getSlideType(currentGalleryItem) === 'image') { const { alt } = currentGalleryItem; - const altAttr = alt ? 'alt="' + alt + '"' : ''; + const altAttr = alt ? 'alt="' + utils.escapeHtml(alt) + '"' : ''; $currentSlide .find('.lg-img-wrap') .append(
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lg-utils.tssrc/lightgallery.tstest/lightgallery.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/lg-utils.ts (1)
site/assets/js/app.js (1)
dynamicEl(590-623)
🪛 ast-grep (0.40.3)
test/lightgallery.test.ts
[warning] 308-312: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt='"><img src=x onerror=alert(1)>' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 338-342: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt='" onload="alert(1)" x="' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 367-371: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt='"><script>alert("XSS")</script><img x="' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 392-396: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt="Testing & signs < and > symbols" /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 419-423: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt="It's a nice image with 'quotes'" /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 445-449: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" srcset='" onload="alert(1)" x="' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 471-475: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" sizes='" onload="alert(1)" x="' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 497-501: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt='Test \\ backslash and "quotes" and <tags>' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 308-312: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt='"><img src=x onerror=alert(1)>' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 338-342: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt='" onload="alert(1)" x="' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 367-371: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt='"><script>alert("XSS")</script><img x="' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 392-396: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt="Testing & signs < and > symbols" /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 419-423: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt="It's a nice image with 'quotes'" /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 445-449: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" srcset='" onload="alert(1)" x="' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 471-475: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" sizes='" onload="alert(1)" x="' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 497-501: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = <div id="lightGallery"> <a href="a.png"> <img src="b.png" alt='Test \\ backslash and "quotes" and <tags>' /> </a> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (7)
src/lg-utils.ts (4)
391-403: LGTM! Well-implemented HTML escaping utility.The escaping function correctly handles all critical characters for XSS prevention in HTML attributes. The order of replacements is correct (ampersand first to prevent double-encoding).
413-415: LGTM! Iframe title properly escaped.The iframe title attribute is correctly escaped before being inserted into the markup, preventing XSS attacks through this vector.
429-430: LGTM! Image attributes properly escaped.The srcset and sizes attributes are correctly escaped, preventing XSS attacks through these attributes.
593-597: LGTM! Dynamic captions properly escaped.The caption text derived from title/alt attributes is correctly escaped before being used as subHtml, preventing XSS attacks through this vector. The comment clearly explains the security rationale.
src/lightgallery.ts (2)
1007-1007: LGTM! Alt attribute properly escaped in setImgMarkup.The alt attribute value is correctly escaped using the new
escapeHtmlutility, preventing XSS attacks through the alt attribute.
1013-1013: LGTM! Safe alt handling in getDummyImageContent.Passing the raw alt value to
getDummyImageContentis safe because it uses DOM property assignment (dummyImgContentImg.alt = alt || '') rather than string interpolation, which automatically handles escaping.test/lightgallery.test.ts (1)
306-523: LGTM! Comprehensive XSS prevention test suite.The security test suite is well-designed and covers critical XSS attack vectors:
- Attribute breakout via double quotes
- Event handler injection
- Script tag injection
- HTML entity handling
- Special characters in various attributes
The tests validate that the escaping logic correctly prevents XSS attacks while preserving legitimate content.
Note: Static analysis warnings about
innerHTMLusage are false positives - this is test code that intentionally sets up malicious content to verify proper escaping.
|
@sachinchoolur is there any plan to merge this fix ? |
3 participants
This fixes a critical XSS vulnerability where unescaped double quotes in the
altattribute allow attackers to break out of the attribute context and inject malicious HTML/JavaScript (e.g.,"><img src=x onerror=alert(1)>).Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.