Safe insertAdjacentHTML with user input? Or I should avoid it anyway?

Add-ons Development
#1

Hello. I do know that Mozilla doesn’t like innerHTML and family (like insertAdjacentHTML) - but is this review fully automatic (i.e. if only these functions are in use, then the extension is rejected) or does a human being takes a look at it?

I’m asking because I believe what I want to do is safe and I want to know if I may do it.

So - I want to display a “box” (new div) when a user clicks an image with some of the modifier keys (like ctrl), with this image on it, and a few details - here is a simplified version of the code:

        //"our" hardcoded string, not user input, just some styling
        let containerStyle = `
            position:absolute;z-index:1001;
            left:100px; top:100px;
        `;

        // here, srcElement is the element being clicked
        let imgSrc = srcElement.getAttribute('src');

        // making the whole html
        let html = `
            <div style = '${containerStyle}'>
                <img src = '${imgSrc}'>
            </div>
        `;
        document.querySelector('body').insertAdjacentHTML ('beforeend', html);

Note, that this is a content script, i.e. this will be injected on the site being opened.

It’s safe, right? A malicious site owner can’t do any harm, as far as I understand, yes? I’m using here only this one attr - ‘src’.

It’s different from the typical hack:

let html = "<img src='x' onerror='alert(1)'>";
someElement.innerHTML = html; // shows the alert

So, may I use it (the first code snippet :)) or will this be automatically rejected?

1 Like
#2

Hi,

I’m a volunteer reviewer for AMO.

Trying to use innerHTML safely can be really tricky. Unsafe usage can lead to a rejection and could create complications for you and your users later on.

If you want to be sure that your extension will be safe and pass review regarding all your innerHTML/insertAdjacentHTML/etc usages, please use the latest release tag of this library : https://github.com/cure53/DOMPurify/releases/tag/2.0.8

Documentation and usage : https://github.com/cure53/DOMPurify

Regards,

Sylvain

1 Like
#3

It is possible to get extensions using innerHTML and alike approved without using DOMPurify. But you will always have the risk that (another) reviewer looks at it, and decides to disable your extension in AMO. I had an extension that used innerHTML and insertAdjacentHTML a lot. But suddenly a day I got the message it was disabled because of unsafe use of those methods. I went through the code and replaced most occurrences with something else(*). But in a few place where they were difficult to replace and I felt I could argue their use being safe, I wrote a comment in the code arguing for the use being safe. I got the extension re-enabled in AMO after this. BUT it is still my plan to get rid of most/all the remaining use of these methods, because I guess there’s still a risk another reviewer don’t like it.

(*)Generally I don’t feel like including the big “DOMPurify” library or similar into my extension. So I went the way of constructing the “DOM structures” manually instead. I created this little utility method which I personally find very handy for that:

function createRichElement(tagName, attributes, ...content) {
  let element = document.createElement(tagName);
  if (attributes) {
    for (const [attr, value] of Object.entries(attributes)) {
      element.setAttribute(attr, value);
    }
  }
  if (content && content.length) {
    element.append(...content);
  }
  return element;
}

With this you could create your example html (container) with:

let imgElem = createRichElement('img', {src: imgSrc});
let container = createRichElement('div', {style: containerStyle}, imgElem);

You can insert your container by replacing your insertAdjacentHTML with insertAdjacentElement.

The utility method gets even more practical when you have several elements in your “container”, possibly a mix of elements and text strings. F.ex:

let container = createRichElement('div', {style: containerStyle}, imgElem, " this is some text ", linkElem, " this is some more text ");

Well, at least I find it handy :slight_smile:

BTW, Now when we have a reviewer in the thread… @sylvaing you don’t happen to have an opinion on if use of iframes in extensions are considered using “remote scripts”? : Iframes in web extension popover?
I’m a little sad there newer was a response on that from someone who might now better than me and the poster of that question…

1 Like
#4

Doc on how to safely inserting external content to your webextension can be read at

Usage of iframes is allowed, as long as it follow the policies : https://extensionworkshop.com/documentation/publish/add-on-policies/

1 Like
#5

Hi,
Thanks - I’ll think about it, although it’s pretty big library… and then it’s a content script. Personally, I usually have abut 20-30 tabs opened and injecting this into every single one seems weird to me :slight_smile:

But OK, I understand that I should avoid the insertHTML family, I will then try find a different solution, thanks again!

#6

Yeah, I used something similar in one of the projects, but I really hoped that I can use insertAdjacentHTML if I’ll be careful enough. After all, it’s much more readable.

Now I think I’ll go with this solution indeed…
Thank you!
Edit: yeah, after a bit of thinking I’ll go with your method :slight_smile: Thanks again!

BTW, we can’t respond to two (or more) ppl, in one post, here on this forum?