Allow custom tags and attributes

[smasam]

I think the DOMPurify custom tags and attributes, not just by adding a list of allowed attributes to config, but there should be a provision to allow any custom tag or attribute

[cure53]

Would that only cover custom tags that are created like <prefix-suffix>?

[smasam]

functional custom tags will definitely use <prefix-suffix>, but in some place we might even end up using <custom> this way, may not be functional but could used to drive some rules in templates or any other places

[cure53]

I have doubts that this will be useful as a core library feature - and I fear it might introduce bypasses as we would migrate from a white-list to a black-list methodology.

However, this should be very easy to implement in a custom hook. Wanna give it a try?

[smasam]

which hook might help this, I see once sanitization is done the custom tags/attributes will be removed

[cure53]

You have to write a custom hook that, upon sanitizing elements, permits all those that contain a dash in their tag-name. That should do the trick.

[smasam]

Thanks for the suggestion.
So the hook should keep adding the permitted element/attributes, But wouldn't that hamper the performance?

[cure53]

Yes, the hook could just on-the fly add the spotted custom element to the existing white-list.

I doubt this will affect performance too much - but if it does, we can think about something to fix that.

[smasam]

Ok, Let me give a try, measure the performance as well.

Thanks and Regards
Srinivas

[cure53]

Okay :)

[smasam]

I tried with the custom hooks, once sanitize is called there is no way I can add my custom tags to ADD_TAGS, if I have to do it then I have to call it twice which means calls sanitize and then in the hooks put the custom tags in to ADD_TAGS list inspecting the tagName and then call sanitize again with the white listed tags, I think we need to give one more method or at least supply a config parameter to the hooks so that the hooks can add the required config data and the purifier will consider this config data going forward with its sanitization.

One more thing is we need to even support template tags if passed as standalone element, right now it is only supported as part of WHOLE_DOCUMENT, I understand that this is added to the head tag once parsed, but we should be supporting this as shadow_dom

[cure53]

So, here is a proposal for a change:

Your hook could look like this....

// Add a hook to convert all text to capitals
DOMPurify.addHook('uponSanitizeElement', function(node, data){
    // Set text node content to uppercase
    if(node.nodeName && node.nodeName.match(/^\w+-\w+$/)) {
        data.allowedTags[data.tagName] = true;
    }
});

... if we modified the core like this

/* Execute a hook if present */
_executeHook('uponSanitizeElement', currentNode, {
    tagName: tagName,
    allowedTags: ALLOWED_TAGS
});

Does this cover your use-case in full?
About the template tag issue, this is a different thing imho. Can you open a separate ticket please?

[smasam]

Yes, I modified the core code in the same fashion but I merged the provided tags post hook execution, I think this way the core only will add to the whitelisting instead of the custom hook, and this will be central code. But if we do this way, we need to even provide the custom what are my white listed tags, so that the hook does not end up adding duplicates. Any how your suggested code snippet does that, but in case you want to provide a different kind of solution, I am hinting on existing whitelisted tags,
also can you please do the same for uponSanitizeAttribute.

I will raise a seprate jira for templates.

Regards

[cure53]

Well, you can always ask in the hook if the tag is already allowed or not - that's a one-liner. For us, it's important to have only minimal changes to the core for exotic feature requests.

If you can work with that proposed change, I would start running tests, adding a unit test and then do a commit.

[smasam]

ok, I will do that right away.

meanwhile, ticket for adding template support #185

[cure53]

So the proposed change is okay?

[smasam]

Yes it is fine. Are you doing the same change for attributes

[cure53]

Yep, that's the plan. I will move the extra data from upon to before though to avoid introducing clobbering vulnerabilities.

[smasam]

one quick question, why doesn't the purifier allow actions like "onClick" I understand about the callback event handler like onError and onSuccuss.. but why are safe event handlers or any other attributes not added to the standard whitelist?

[cure53]

What do you mean by "safe event handler"?

[cure53]

Either way, tests are working well, commit coming soon. I actually stuck with upon and not before - no DOM-clobbering risk as far as I can see and smaller change w/o API breakage.

[cure53]

Here is the unit test, showing how to implement the new feature:

https://github.com/cure53/DOMPurify/blob/master/test/test-suite.js#L266

[smasam]

Thank You.

When I said safe, it means onClick event handlers on the element requires user to click on the node to exexute, where as onError and onSuccess let say on image may not be safe as it executes script on load, without users knowledge, so my question why are we not allowing all the safe attributes

[cure53]

We don't differentiate between XSS via UI or XSS w/o UI :) Imagine a transparent element with onClick that is positioned over the entire page using a CSS injection. Not different from onLoad because at some point the user will click - without even seeing the element.

[smasam]

True,

This means we need to apply to only those sources of data which are unreliable