While these are not security issues per se, I'd recommend not using innerHTML at all. I think it can be avoided in all cases by using createElement, setAttribute and textContent. I have spotted several instance of this problem and will put them all in this single bug, since they require minor fixes after all. List follows (. is the contacts dir): ./js/contacts_list.js:95: title.innerHTML = '<abbr title="Contacts listed ' + group + '">'; ./js/contacts_list.js:96: title.innerHTML += letter + '</abbr>'; I suggest using createElement, setAttribute and textContent. It's a little more to write but makes it future proof. ./js/contacts.js:574: loading.querySelector('[data-l10n-id="loadingContacts"]').innerHTML = text; Just use a non-HTML attribute and you're fine. ./js/contacts_settings.js:207: span.innerHTML = message; Same, just use textContent

Quick comment: The current code does *not* contain XSS in these parts, since these functions are not reachable with user data. Future code changes might introduce this and I would like us to be have a safe foundation.

This is something we have discussed in the past amongst the security team, and it applies to Gaia more broadly. However, I remember a counter argument that innerHTML was faster - I thought it was from a discussion on the mailing list but I can't find it right now. A quick search says there are 360 instances of "innerHTML" in Gaia at the moment, and I would doubt that all, or even many, need to use innerHTML for performance or other reasons. It's probably worth raising on the dev-gaia mailing list for broader discussion, so I will do that.

Just as a reminder: Andreas Gal posted to dev-gaia last week, that textContent should be preferred to innerHTML because it's faster. Makes sense, since text doesn't need any rendering ;)

Removal of all innerHTML calls within the contacts app. Open for debate: clearing elements happens through innerHTML = '', I've changed it to do this via DOM. Perhaps gaia already has taken a stand on that. For that sake I have most of those changes in a separate commit, so please first review before I squash everything into one single commit.

As an informal review (please let alberto look at it again), I'd say this is very good. Thanks for taking a shot at it! I don't know if removing childNodes in a while-loop is a better idea than using innerHTML="" though. I mostly wanted to remove innerHTML assignments with possible user input because of their attack surface. But then, you should always measure the gain before doing something unusual for performance reasons.

Wow, it seems removeChild is quite faster. I'll take a look to the changes in order to land it as soon as possible. Thanks! http://jsperf.com/innerhtml-vs-removechild

I have updated the PR to reflect the comments in this issue and the GH PR.