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.67k stars 698 forks source link

Documentation issue and question for custom elements #854

Closed Judekeyser closed 1 year ago

Judekeyser commented 1 year ago

I was trying to use the examples written in https://github.com/cure53/DOMPurify#control-behavior-relating-to-custom-elements and there is something I think is wrong documentation. The two last configuration look roughly the same, up to regex/predicate difference, yet they do not seem to return the same result (the is attribute is removed in the last snippet).

I was wondering if it shouldn't be instead

const clean = DOMPurify.sanitize(
    '<foo-bar baz="foobar" forbidden="true"></foo-bar><div is="foo-baz"></div>',
    {
        CUSTOM_ELEMENT_HANDLING: {
            tagNameCheck: /^foo-/, // allow all tags starting with "foo-"
            attributeNameCheck: /baz/, // allow all attributes containing "baz"
            allowCustomizedBuiltInElements: false, // customized built-ins are *not* allowed <<<<< HERE
        },
    }
); // <foo-bar baz="foobar"></foo-bar><div is=""></div>

const clean = DOMPurify.sanitize(
    '<foo-bar baz="foobar" forbidden="true"></foo-bar><div is="foo-baz"></div>',
    {
        CUSTOM_ELEMENT_HANDLING: {
            tagNameCheck: (tagName) => tagName.match(/^foo-/), // allow all tags starting with "foo-"
            attributeNameCheck: (attr) => attr.match(/baz/), // allow all containing "baz"
            allowCustomizedBuiltInElements: true, // allow customized built-ins
        },
    }
); // <foo-bar baz="foobar"></foo-bar><div is="foo-baz"></div>

I also notice that if the element foo-bar actually writes something (for example, it does this.textContent = "Hello" when an attribute changes), even in strict default mode, then the value of the text content is still in the DOM, but not the custom element. Is this behavior expected, or is it simply undefined behavior ?

cure53 commented 1 year ago

Hey there, thanks for filing this. I think you are right, either the result be different or the value should be set to false. From what I can see, this line needs changing, correct?

https://github.com/cure53/DOMPurify/blob/main/README.md?plain=1#L243

It should be this, correct?

<foo-bar baz="foobar"></foo-bar><div is="foo-baz"></div>

Judekeyser commented 1 year ago

This is my guess too, based on what I observed.

For the second point, I couldn't find anywhere in the documentation if it is expected that the DOM content of the custom element will be removed, or only the custom element markup. From what I test, it feels like only the markup is gone and the content is still there (might it be a content generated by the element itself! for example, after getting triggered by an attribute change), but I do not know if this is a specified behavior, or if I cannot count on it.

cure53 commented 1 year ago

This is my guess too, based on what I observed.

Cool, will fix then, thanks.

but I do not know if this is a specified behavior, or if I cannot count on it.

I think this is currently expected behavior: https://github.com/cure53/DOMPurify/blob/main/src/purify.js#L291

Judekeyser commented 1 year ago

Excellent, thank you for the reference! I missed it previously.