allow target="_blank"?

[Deekor]

I'm looking for an example to allow target="_blank" on links.

[fisher-]

@Deekor Try something like this:

DOMPurify.sanitize('your content', { ADD_ATTR: ['target'] });

But I don't know if this is the most appropriate solution.

[cure53]

Looks good to me :)

[Deekor]

Pretty sure thats what i ended up doing.

[ilanlal]

### Hook to open all links in a new window

// Add a hook to make all links open a new window
DOMPurify.addHook('afterSanitizeAttributes', function (node) {
  
// set all elements owning target to target=_blank
  if ('target' in node) { node.setAttribute('target', '_blank'); }
  // set non-HTML/MathML links to xlink:show=new
  if (!node.hasAttribute('target') && (node.hasAttribute('xlink:href') || node.hasAttribute('href'))) {
     node.setAttribute('xlink:show', 'new');
  }
});

// Clean HTML string and write into our DIV
var clean = DOMPurify.sanitize(dirty);

https://github.com/cure53/DOMPurify/tree/main/demos#hook-to-open-all-links-in-a-new-window-link

[ydaniv]

Note that simply adding target="_blank" to your links may expose your site to security issues, see here.

[mparpaillon]

Thanks @ydaniv and @ilanlal
To keep it secure I used the hook but added the rel noopener

DOMPurify.addHook('afterSanitizeAttributes', function (node) {
  // set all elements owning target to target=_blank
  if ('target' in node) {
    node.setAttribute('target', '_blank');
    node.setAttribute('rel', 'noopener');
  }
});

[dudeful]

Thanks @ydaniv and @ilanlal
To keep it secure I used the hook but added the rel noopener

DOMPurify.addHook('afterSanitizeAttributes', function (node) {
  // set all elements owning target to target=_blank
  if ('target' in node) {
    node.setAttribute('target', '_blank');
    node.setAttribute('rel', 'noopener');
  }
});

Looks good, I'm using this on a client's website.
Can anyone confirm if it is bulletproof?
btw, thank you @mparpaillon

[kckst8]

not sure why this is closed? All of the solutions use custom hooks to workaround the issue. This package should allow target="_blank" provided rel="noopener noreferrer" also exists on the element

[cure53]

Nope, things that can be solved with a config option or a hook need to change to the core library.

[kckst8]

With this logic, all users of all libraries who can find workarounds to bugs in said libraries should do just that and not fix bugs?

[cure53]

It is not a bug, it is a feature request. So, yes - if a feature can be enabled by using the available tools, the feature doesn't have to be added to the core library.

[kckst8]

I disagree....Incorrectly flagging secure html as insecure in a library whose purpose is to remove insecure html sure seems like a bug to me.

[cure53]

So, your expectation is that a feature request (that is long solved with three lines of custom code by using the exact API we have for that) should cause the core library to be updated and add the same code for everyone who uses it?

Because if so, your expectation is wrong :) You can easily get what you want with bit of code so there is no issue here.

I don't wanna sound unfriendly but I am uncertain what you want to achieve here, re-solve an already solved problem in a different way that affects everyone?

[kckst8]

My argument is that it is NOT a feature request, but rather a bug that essentially disallows any user of the library from adding anchor tags to their site that open in a new window, even if they've taken steps to make the links secure per common web standards.

I understand your rationale behind adding the custom hooks, but IMO this should not be a case where that is warranted, if it is indeed a bug in the core.

Even if it is deemed a feature request, custom hooks should be a temporary workaround, not a closure of the issue IMO. My issue, which I do not feel would be uncommon, is that I am consuming this library indirectly. For example, let's say you consume a package whose purpose is to allow for generating modal dialogs given html. That package (correctly) ensures that your html is safe for you prior to rendering the modal dialog through the use of this library. Now consumers of the modal library essentially cannot add links to their modals which open a new window. The only option is to file a bug/PR with the modal library to add a custom hook to that library....at which point the maintainer of that library would likely come back to you before wanting to add said hook anyways.

So, it's not always as simple as "add a hook", particularly in the case where it is being consumed indirectly.

[cure53]

You want have a certain combination of attributes allow-listed which is being stripped right now. This is not a bug, this is a feature request (in my eyes at least) - you want some extra capabilities from the library that it doesn't have yet.

For making the library more powerful and avoiding countless changes to the core based on individual requests, we added the Hooks API. Imagine how many miniature-changes we would have added to the core and how long it would have taken until they actually conflict with each other? We would be sitting on a pile of angry users, tons of changes per release, and ultimately a situation in which the library collapses from all the added bloat.

You now say "I want this and it is safe!" and the next user says "omg what did you do! this broke our application". We had this before, won't have it again.

So, either way - the core change for allowing a certain (rather complex to check) thing you or anyone else deems safe just so is not going to happen and a hook is the answer. The only answer, unless we really have a bug. Then we will fix it.

[cure53]

To give some insight on the possible bloat this adds vs. the 3 lines for a hook:
Let's assume we implement what you proposed.

• We allow target by default, fine - easy to implement
• But only if it uses _blank as value and if the anchor has a rel attribute with proper values, okay
• This already means that we need to do some string matching operations to check...
  1. if the rel attribute is there and not clobbered in somehow and
  2. if it contains the right values and no extra stuff maybe invalidating those
  3. if there is no mutation potential making us think they are proper but then they mutate when used
• We also need to consider the order of keywords might differ or people stuffing it with more keywords
• We also need to assume that frameworks do crazy things or add their own logic, maybe XSS via rel
• We need to then potentially filter the content of the rel attribute
• Some people might say we need noopener and noreferer, other might say noreferer is enough
• We also need to make sure that this works for HTML anchors, SVG, MathML, areas etc.
• We also need to make sure that someone disallowing target or using any other config setting won't be affected by this
• We further need to make sure that anyone messing with rel attributes being allowed or not doesn't break the new logic

This just doesn't scale and this is why we have the Hooks API. Three lines versus the bloat I scribbled up above... makes sense no?

[tvaliasek]

Another possible workaround:

const TEMPORARY_ATTRIBUTE = 'data-temp-href-target'

DOMPurify.addHook('beforeSanitizeAttributes', function (node) {
    if (node.tagName === 'A') {
        if (!node.hasAttribute('target')) {
            node.setAttribute('target', '_self')
        }

        if (node.hasAttribute('target')) {
            node.setAttribute(TEMPORARY_ATTRIBUTE, node.getAttribute('target'))
        }
    }
})

DOMPurify.addHook('afterSanitizeAttributes', function (node) {
    if (node.tagName === 'A' && node.hasAttribute(TEMPORARY_ATTRIBUTE)) {
        node.setAttribute('target', node.getAttribute(TEMPORARY_ATTRIBUTE))
        node.removeAttribute(TEMPORARY_ATTRIBUTE)
        if (node.getAttribute('target') === '_blank') {
            node.setAttribute('rel', 'noopener')
        }
    }
})