Skip to content

Sanitize profile HTML and CSS #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
numbpill3d wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Sanitize profile HTML and CSS #40

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions server/routes/profile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@ const router = express.Router();
const User = require('../models/User');
const ScrapyardItem = require('../models/ScrapyardItem');
const Feed = require('feed').Feed;
let DOMPurify;
try {
const createDOMPurify = require('dompurify');
const { JSDOM } = require('jsdom');
const window = new JSDOM('').window;
Copy link
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)

Suggested change
const window = new JSDOM('').window;
const {window} = new JSDOM('');


ExplanationObject destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

DOMPurify = createDOMPurify(window);
} catch (err) {
console.warn('DOMPurify module not found, using fallback sanitization');
DOMPurify = { sanitize: (input) => input.replace(/</g, '&lt;').replace(/>/g, '&gt;') };
Copy link
Contributor

@codereviewbot-ai codereviewbot-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

}
Comment on lines +7 to +15
Copy link
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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, '&lt;').replace(/>/g, '&gt;') };
}
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, '&lt;').replace(/>/g, '&gt;') };
} else {
throw err;
}
}


// Middleware to ensure user is authenticated
const ensureAuthenticated = (req, res, next) => {
Expand Down Expand Up @@ -147,10 +157,17 @@ router.get('/edit/html', ensureAuthenticated, (req, res) => {
router.post('/edit/html', ensureAuthenticated, async (req, res) => {
try {
const { profileHtml } = req.body;
Copy link
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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'],
Copy link
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

FORBID_TAGS: ['script', 'iframe', 'object', 'embed'],
FORBID_ATTR: ['onerror', 'onload', 'onclick', 'onmouseover']
});
Comment on lines +160 to +166
Copy link
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

Suggested change
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?:/
});


// Update the user's profile HTML
const user = await User.findById(req.user._id);
user.profileHtml = profileHtml;
user.profileHtml = sanitizedHtml;
await user.save();

req.flash('success_msg', 'Profile HTML updated successfully');
Expand Down Expand Up @@ -179,10 +196,15 @@ router.get('/edit/css', ensureAuthenticated, (req, res) => {
router.post('/edit/css', ensureAuthenticated, async (req, res) => {
try {
const { profileCss } = req.body;

const sanitizedCss = DOMPurify.sanitize(profileCss, {
Copy link
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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:...).

ALLOWED_TAGS: [],
ALLOWED_ATTR: [],
FORBID_TAGS: ['script', 'iframe', 'object', 'embed']
});

// Update the user's profile CSS
const user = await User.findById(req.user._id);
user.profileCss = profileCss;
user.profileCss = sanitizedCss;
await user.save();

req.flash('success_msg', 'Profile CSS updated successfully');
Expand Down