Safe (sanitized) string and .innerHTML and AMO policy

Add-ons Development
#1

Hello.

I have read a few topics on https://discourse.mozilla.org/c/add-ons/addons-mozilla-org and I have a few questions about the policy behind injecting html into DOM.

I do understand the dangers and why we should use .textContent and I do use it on a daily basis. However I’m working on an extension where using full HTML template would be more effective.

Here is what I’m gonna use to sanitize the user input:
https://jsfiddle.net/kpion/wfzqvtmd/

In short, I’m replacing all the < and > and friends with entities. With this:

sanitize (str) {
  const replacements = {
      '"': '&quot;', '&': '&amp;', "'": '&apos;','<' : '&lt;', '>': '&gt;',
    };
  return str.toString().replace(/[\<\>\&\"\']/g, c => replacements[c]);  
}

And then something like:

element.innerHTML = '<p>' + sanitize(userValue) + '</p>';

And the question is - is this fine and enough? I want to ask it before I’ll go fully with the code to avoid this :

Quote:

(…) why is the addon unlisted on AMO?

(…) Because Mozilla policy of forbidding assignment of innerHTML and custom DOM library. Changing them is not an easy task as many sites are affected and some with Geo Lock or require account. It will be hard for me to ensure (…)

Also, aside from this - I’m wondering, how using jquery solves this problem? With jquery.html(stuff) we can do as much harm as with any other lib using .innerHTML under the hoods , right? But really, this question isn’t important :slight_smile: I’m just curious.

#2

how using jquery solves this problem

When has jQuery ever solved any problem in the past 10 years?
It certainly doesn’t help here.

I recently switched to something like this (adjusted for your path lookup):

const template = `
  <div>
    <p>Here is this guy, <b path=name></b></p> 
    <p>He tries to <b path=hack></b></p> 
    <p>He has a cat called <span path=animals.cat></span> and a dog - <span path=animals.dog></span></p> 
  </div>
`;

const person = {
  name: 'John Smith',
  hack: `hack <img src=x onerror="alert('XSS Attack')"> us`,
  animals: { cat:'Rambo', dog: 'Pinky', },
};

fillWithTemplate(document.querySelector('#host'), template, person);

function fillWithTemplate(host, html, data) {
  host.innerHTML = html;
  host.querySelectorAll('[path]').forEach(slot => {
    slot.textContent = getNestedProperty(data, slot.getAttribute('path'));
    slot.removeAttribute('path')
  });
}

function getNestedProperty(object, path) {
  for (const name of path) {
    object = object[name]; if (typeof object !== 'object') { break; }
  } return object;
}

The only place where this uses innerHTML is with a constant string, which is fine.
If you want to help the reviewer and your code permits it, do the assignment directly with the string literal (i.e. without a variable). That way, the linter won’t even warn about it.

Oh, and I haven’t tested the code, beware of typos … but the idea works.

1 Like
#3

Thank you very much!

The way you do this is quite interesting, would never think about that :slight_smile: Right now I already wrote quite a few lines of code using my way, but it’s good to know that if something happens I’ll be able to switch to yours with only changing a few characters here and there in templates.

Data objects can stay intact. And so 98% of the code I wrote :slight_smile:

So I’ll just see first how it goes with the review and possibly later make the changes.

Thank you very much once again!