cure53 / DOMPurify

DOMPurify - a DOM-only, super-fast, uber-tolerant XSS sanitizer for HTML, MathML and SVG. DOMPurify works with a secure default, but offers a lot of configurability and hooks. Demo:
https://cure53.de/purify
Other
13.61k stars 695 forks source link

Question about Sanitize Css Hook example #902

Closed Ste35 closed 7 months ago

Ste35 commented 7 months ago

This issue proposes a question/bug

Hi! I'd like to ask a question abound the Sanitize Css Hook for sanitizing elements' inline css, such as:

<div style="color: red; background-color: green; font-family: monospace;">Lorem</div>

I tested on Chrome 120, Windows 10.

Background & Context

I've downloaded the html page and tried to sanitize the above string, without changing the allowed_properties (so with the font-family property in list). I was expecting the color and font-family properties to be kept, while the background-color property to be stripped, but only color was kept. I also tried adding background-color to the allowed_properties list, but it was stripped again (same as font-family). I suppose the issue is that in this function

    function validateStyles(output, styles) {
        // Validate regular CSS properties
        for (var prop in styles) {
            if (typeof styles[prop] === 'string') {
                if (styles[prop] && allowed_properties.indexOf(prop) > -1) {
                    console.log(prop)
                    if (allow_css_functions || !/\w+\(/.test(styles[prop])) {
                        output.push(prop + ':' + styles[prop] +';');
                    }
                }
            }
        }
    }

when a property contains a -, prop is camelCase and not kebab-case (so we have fontFamily and backgroundColor). If I add fontFamily to the allowed_properties, than font-family will be kept, but the result is:

<div style="color:red;fontFamily:monospace;">Lorem</div>

that is not accepted by the browser.

Bug

Input

<div style="color: red; background-color: green; font-family: monospace;">Lorem</div>

Given output

<div style="color:red;">Lorem</div>

Expected output

<div style="color:red;font-family:monospace;">Lorem</div>

Feature

I suppose we can convert kebab-case to camelCase and than convert it back, do you have any suggestion?

Thank you!

cure53 commented 7 months ago

Hey there, thanks for filing this. The demo hook is quite old and not actively maintained. But I had a look and indeed, now the style names do no longer match because if the reasons you described.

I suppose we can convert kebab-case to camelCase and than convert it back, do you have any suggestion?

This appears to be the right way to do, yes.

cure53 commented 7 months ago

This should, while likely this can be done more elegantly, kind of already do the trick, no? :sweat_smile:

if(styles[prop] && typeof styles[prop] === 'string'){
    var normalizedProp = prop.replace(/([A-Z])/g, '-$1').toLowerCase();
    if(allowed_properties.indexOf(normalizedProp) > -1) {
       if(allow_css_functions || !/\w+\(/.test(styles[prop])) {
            output.push(normalizedProp + ':' + styles[prop] +';');
        }
    }
}
cure53 commented 7 months ago

Or this, which uses JavaScript which is a bit less dusty :slightly_smiling_face:

function validateStyles(output, styles) {
    Object.keys(styles).forEach(prop => {
        const value = styles[prop];
        if (value && typeof value === 'string') {
            const normalizedProp = prop.replace(/([A-Z])/g, '-$1').toLowerCase();
            if (allowed_properties.includes(normalizedProp) && 
                (allow_css_functions || !/\w+\(/.test(value))) {
                output.push(`${normalizedProp}:${value};`);
            }
        }
    });
}
Ste35 commented 7 months ago

Yes, now it seems to work, thank you!

cure53 commented 7 months ago

Nice, closing this one then via https://github.com/cure53/DOMPurify/commit/84980c3504b7571e2fcbbbdd8f4f644b532d3992 :slightly_smiling_face: