[Bug] HTML encoded self closing tags in attribute values leads to the removal of the attribute

[AndreVirtimo]

When I use html encoded content in an attribute self closing tags leads to the removal of the whole attribute.

Background & Context

Self closing tags like <br /> or <img /> are usually converted to single tags (<br>,<img>), which is totally fine.

Bug

Input

<p data-qtip="&lt;br/&gt;">foo</p>

Given output

<p>foo</p>

Expected output

<p data-qtip="<br>">foo</p>

Working example with single tag

When I'm using single tags, the output is as expected:

Input

<p data-qtip="&lt;br&gt;">foo</p>

Given output

<p data-qtip="<br>">foo</p>

[AndreVirtimo]

Here is another input which leads to removal of the attribute. It seems that the HTML Encoding is not necessary to reproduce the issue.

Input

<input type="text" value="foo <br/>">

Given output

<input type="text" >

Expected output

<input type="text" value="foo <br/>">

[cure53]

Heya, thanks for filing the issue. However, it's not a bug but a protection against an mXSS issue that has been plaguing browsers for a while. Allowing this will cause XSS.

You can likely work around that using a hook, but then you'd have to accept the mXSS risk, in case the attribute content is user-controlled.

[AndreVirtimo]

I don't know why the whole attribute is removed. Shouldn't it be enough to change the <br/> to <br> at that point?

When I'm sanitizing <br/> not as attribute value it also get transformed to <br> and won't be removed.

[cure53]

The default is of course removal as we offer a sanitizer library, so scrubbing is the way to go 😄

But, you can override that easily with a hook and then do what works in your context.

[edg2s]

Downstream issue affecting Wikipedia: https://phabricator.wikimedia.org/T279215#8525105

Per the inline comment, this appears to be fixing an issue in an old version of jQuery, and previously could be disabled with a flag.

But, you can override that easily with a hook and then do what works in your context.

To work around this with a hook would require re-implementing all the transforms and checks that are done to see if the attribute is about to be removed (SAFE_FOR_TEMPLATES, isValidAttribute), unless I have missed something?

[cure53]

I understand that this is annoying, but I am not sure what should best be done here. We see it like this:

• jQuery introduces a new bug that causes XSS, found by our team (by @masatokinugawa )
• We fix around that bug so to make sure that folks using buggy jQuery are safe too
• Three years later that causes an issue for people not even using affected jQuery versions

That has me wonder, is this really our bug when our mission is to prevent XSS? :)

Not wanting to sound dismissive though, if there is a satisfying solution, we'd be happy to go the extra mile and implement it, but right now I feel we are deadlocked. PRs are super welcome, any other input as well.

[edg2s]

Could we add back in a renamed flag to disable the feature, e.g. ALLOW_SELF_CLOSE_IN_ATTR (default is false)?

[cure53]

Not a huge fan of the idea but if that saves the day, a PR is welcome 😄
Please check for which branch you need it for, 2.x (MSIE is supported) or 3.x (MSIE is no longer supported).

[AndreVirtimo]

For my case I have now added the following hook. But I would prefer @edg2s solution when it is merged.

DOMPurify.addHook(
  'uponSanitizeAttribute',
  function (currentNode, hookEvent, config) {

    if(hookEvent.attrName === "value"){
        hookEvent.forceKeepAttr = true;
    }

    return currentNode;
  }
);

[cure53]

Alright, I added the flag plus test to the 2.x branch. Does that do the trick for you @edg2s ?

5f766bc

If so, we will also add this to main, release 3.0.0 and 2.4.4 @AndreVirtimo .

[AndreVirtimo]

Thank you @edg2s for the PR.
Thank you @cure53. Do you have an estimated release date?

I will give feedback when we can test the next release.

[cure53]

Hey all, we would like to release but need some confirmation that the change works as expected 😄

Could you confirm it works as expected? @AndreVirtimo @edg2s

[AndreVirtimo]

@cure53 I have tested 5f766bc with the flag ALLOW_SELF_CLOSE_IN_ATTR set to true. This worked for me and my previously postet workaround isn't needed any more.

[cure53]

This should be fixed now in all latest releases