Conversation
Reviewer's GuideThis PR integrates DOMPurify into the profile editing routes to sanitize user-submitted HTML and CSS, establishing a jsdom-based sanitizer with a fallback and updating the POST handlers to apply sanitization before persisting. Sequence Diagram for Profile Update with SanitizationsequenceDiagram
actor User
participant Server as "Express Router (profile.js)"
participant Purifier as "DOMPurifyLogic"
participant UserModel as "User Model"
User->>Server: POST /edit/html (profileHtml) OR /edit/css (profileCss)
Server->>Purifier: sanitize(profileHtml/profileCss, options)
Purifier-->>Server: sanitizedOutput
Server->>UserModel: User.findById(req.user._id)
UserModel-->>Server: user
Server->>Server: user.profileHtml = sanitizedOutput OR user.profileCss = sanitizedOutput
Server->>UserModel: user.save()
UserModel-->>Server: save_success
Server-->>User: HTTP 200 / Redirect (Profile Updated)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| DOMPurify = createDOMPurify(window); | ||
| } catch (err) { | ||
| console.warn('DOMPurify module not found, using fallback sanitization'); | ||
| DOMPurify = { sanitize: (input) => input.replace(/</g, '<').replace(/>/g, '>') }; |
There was a problem hiding this comment.
The fallback sanitization method used here is very basic and might not be sufficient for all cases of XSS attacks. It only replaces < and > characters, which does not cover many other potential XSS vectors such as JavaScript event handlers or style injections.
Recommendation:
Consider using a more robust sanitization library as a fallback, or ensure that dompurify is always available in your deployment environment. If neither is possible, enhance the regex to cover more XSS patterns.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @numbpill3d - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Fallback sanitization is too naive (link)
- Enforce safe URL protocols in HTML sanitization (link)
- Allowing inline style attribute can introduce CSS-based attacks (link)
- DOMPurify doesn't sanitize CSS content (link)
General comments:
- The DOMPurify configuration uses
ALLOWED_ATTRandFORBID_ATTR, but the correct option names areALLOWED_ATTRSandFORBID_ATTRS—please update those keys so attributes are properly controlled. - Using DOMPurify to sanitize CSS text may not be sufficient—consider integrating a dedicated CSS sanitizer or whitelisting specific CSS properties rather than relying on tag/attribute rules.
- The fallback sanitizer in the
catchblock only escapes<and>, which won’t prevent XSS via attributes—either require DOMPurify or implement a more robust fallback to avoid security gaps.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try { | ||
| const createDOMPurify = require('dompurify'); | ||
| const { JSDOM } = require('jsdom'); | ||
| const window = new JSDOM('').window; | ||
| DOMPurify = createDOMPurify(window); | ||
| } catch (err) { | ||
| console.warn('DOMPurify module not found, using fallback sanitization'); | ||
| DOMPurify = { sanitize: (input) => input.replace(/</g, '<').replace(/>/g, '>') }; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Only fallback on missing DOMPurify dependency
Check specifically for err.code === 'MODULE_NOT_FOUND' and rethrow other errors to prevent masking unrelated issues.
| try { | |
| const createDOMPurify = require('dompurify'); | |
| const { JSDOM } = require('jsdom'); | |
| const window = new JSDOM('').window; | |
| DOMPurify = createDOMPurify(window); | |
| } catch (err) { | |
| console.warn('DOMPurify module not found, using fallback sanitization'); | |
| DOMPurify = { sanitize: (input) => input.replace(/</g, '<').replace(/>/g, '>') }; | |
| } | |
| try { | |
| const createDOMPurify = require('dompurify'); | |
| const { JSDOM } = require('jsdom'); | |
| const window = new JSDOM('').window; | |
| DOMPurify = createDOMPurify(window); | |
| } catch (err) { | |
| if (err.code === 'MODULE_NOT_FOUND') { | |
| console.warn('DOMPurify module not found, using fallback sanitization'); | |
| DOMPurify = { sanitize: (input) => input.replace(/</g, '<').replace(/>/g, '>') }; | |
| } else { | |
| throw err; | |
| } | |
| } |
| DOMPurify = createDOMPurify(window); | ||
| } catch (err) { | ||
| console.warn('DOMPurify module not found, using fallback sanitization'); | ||
| DOMPurify = { sanitize: (input) => input.replace(/</g, '<').replace(/>/g, '>') }; |
There was a problem hiding this comment.
🚨 issue (security): Fallback sanitization is too naive
This method only escapes angle brackets and can be bypassed. For production, use a robust sanitizer or fail closed to ensure security.
| const sanitizedHtml = DOMPurify.sanitize(profileHtml, { | ||
| ALLOWED_TAGS: ['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'p', 'br', 'ul', 'ol', 'li', | ||
| 'strong', 'em', 'a', 'img', 'div', 'span', 'blockquote', 'code', 'pre'], | ||
| ALLOWED_ATTR: ['href', 'src', 'alt', 'title', 'class', 'id', 'style'], | ||
| FORBID_TAGS: ['script', 'iframe', 'object', 'embed'], | ||
| FORBID_ATTR: ['onerror', 'onload', 'onclick', 'onmouseover'] | ||
| }); |
There was a problem hiding this comment.
🚨 suggestion (security): Enforce safe URL protocols in HTML sanitization
Configure DOMPurify with an ALLOWED_URI_REGEXP (e.g., /^https?:/) to prevent unsafe protocols like javascript: in href and src attributes.
| const sanitizedHtml = DOMPurify.sanitize(profileHtml, { | |
| ALLOWED_TAGS: ['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'p', 'br', 'ul', 'ol', 'li', | |
| 'strong', 'em', 'a', 'img', 'div', 'span', 'blockquote', 'code', 'pre'], | |
| ALLOWED_ATTR: ['href', 'src', 'alt', 'title', 'class', 'id', 'style'], | |
| FORBID_TAGS: ['script', 'iframe', 'object', 'embed'], | |
| FORBID_ATTR: ['onerror', 'onload', 'onclick', 'onmouseover'] | |
| }); | |
| const sanitizedHtml = DOMPurify.sanitize(profileHtml, { | |
| ALLOWED_TAGS: ['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'p', 'br', 'ul', 'ol', 'li', | |
| 'strong', 'em', 'a', 'img', 'div', 'span', 'blockquote', 'code', 'pre'], | |
| ALLOWED_ATTR: ['href', 'src', 'alt', 'title', 'class', 'id', 'style'], | |
| FORBID_TAGS: ['script', 'iframe', 'object', 'embed'], | |
| FORBID_ATTR: ['onerror', 'onload', 'onclick', 'onmouseover'], | |
| ALLOWED_URI_REGEXP: /^https?:/ | |
| }); |
| const sanitizedHtml = DOMPurify.sanitize(profileHtml, { | ||
| ALLOWED_TAGS: ['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'p', 'br', 'ul', 'ol', 'li', | ||
| 'strong', 'em', 'a', 'img', 'div', 'span', 'blockquote', 'code', 'pre'], | ||
| ALLOWED_ATTR: ['href', 'src', 'alt', 'title', 'class', 'id', 'style'], |
There was a problem hiding this comment.
🚨 issue (security): Allowing inline style attribute can introduce CSS-based attacks
Remove 'style' from ALLOWED_ATTR or ensure CSS in style attributes is properly sanitized to mitigate CSS injection risks.
| try { | ||
| const { profileCss } = req.body; | ||
|
|
||
| const sanitizedCss = DOMPurify.sanitize(profileCss, { |
There was a problem hiding this comment.
🚨 issue (security): DOMPurify doesn't sanitize CSS content
DOMPurify does not sanitize CSS rules. Use a dedicated CSS sanitizer to allow only safe properties, remove unsafe at-rules, and block dangerous expressions like url(javascript:...).
| @@ -147,10 +157,17 @@ router.get('/edit/html', ensureAuthenticated, (req, res) => { | |||
| router.post('/edit/html', ensureAuthenticated, async (req, res) => { | |||
| try { | |||
| const { profileHtml } = req.body; | |||
There was a problem hiding this comment.
suggestion (bug_risk): Validate that profileHtml is a string before sanitizing
Add a type check to return a 400 error if profileHtml is not a string to prevent runtime issues with invalid input.
| try { | ||
| const createDOMPurify = require('dompurify'); | ||
| const { JSDOM } = require('jsdom'); | ||
| const window = new JSDOM('').window; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const window = new JSDOM('').window; | |
| const {window} = new JSDOM(''); |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
1 participant
Summary
Testing
npm test(fails: Missing Supabase env vars and Item model)https://chatgpt.com/codex/tasks/task_e_6845129d6aa8832fa407b8d2198c3498
Summary by Sourcery
Integrate server-side sanitization for profile HTML and CSS inputs using DOMPurify (with specific tag/attribute rules) and fall back to a basic sanitizer when DOMPurify is missing.
New Features: