Sanitizing data injected into the DOM

Add-ons Development
#1

Hi,

I am attempting to port my Chrome extension over to Firefox but it has been rejected because the data I am injecting into the DOM is not sanitized. The reviewer recommended an article however, the article says it is deprecated and should not be followed when writing a WebExtension.

I replied asking for a reference that was not deprecated and was told “since you are already using jQuery, you could easily build jQuery objects using JSON notation and append those”.

Is anyone able to explain what the reviewer means by “jQuery objects” or provide some documentation on the topic?

Thanks!

#2

Data can be injected into DOM as data ie as text (with some reservations).

Remote data can NOT be converted to DOM and injected as it may can be a security risk.
Remote data can NOT be injected as executable script eg as JavaScript or a string that can be executed like href, src, on*** etc

Inserting text data as text in a non-executable form is fine.

Example of Remote data:

var obj = {
  "employees":[
      {"firstName":"John", "lastName":"Doe"},
      {"firstName":"Anna", "lastName":"Smith"},
      {"firstName":"Peter", "lastName":"Jones"}
  ]
}

var p = document.createElement('p');
p.textContent = 'Helllo ' + obj.employees[0].firstName + ' ' + 
                            obj.employees[0].lastName;

document.body.appendChild(p);
#3

Thanks very much for your reply.

My extension makes ajax requests to one remote domain - fixer.io - which is a JSON API for foreign exchange rates and currency conversion. It returns JSON, as you might expect, which I then use to allow the user to perform currency conversions with an on-screen converter. Does this data need to be sanitized? It is already in JSON format much like your obj example.

All other ajax requests made by my extension go to the same domain that the extension is designed for - discogs.com (a massive database/marketplace of music releases).

For example, one feature the extension offers is a feedback notifier for when another user leaves buyer/seller feedback on your profile. The extension periodically polls your feedback page, extracts data from the returned markup and stores that in an object in localStorage. It performs a comparison of the current data with the data stored in localStorage. If the data has changed since the last poll it inserts a notification badge into the header of the site which the user can click and be taken to the proper feedback page.

Does this need to be sanitized? It is coming from the same domain and is basically screen-scraped data. This seems safe to me but I am by no means a security expert.

#4

JSON data has to be made safe. It is not that straightforward to sanitize the JSON data.Therefore the best action is to inject the data in a format that would be safe.

Using eval(), innerHTML (or similar eg JQuery append) etc on remote content is a security risk and thus cause for rejection.

Which addon is it? If it is on AMO, I will try to have a quick look at the code.

1 Like
#5

Thanks again for your feedback!

Here is the link to the disabled AMO listing: https://addons.mozilla.org/en-US/developers/addon/discogs-enhancer

And here is the link to the source on my GitHub profile: https://github.com/salcido/Discogs-Enhancer-Firefox

#6

There are too many files to check for a quick check. Give me an example of AJAX JSON insertion and I will look at it for you.

#7

I’m sorry about that. I should have been more specific.

Here is my code for the currency converter as an example:

https://github.com/salcido/Discogs-Enhancer-Firefox/blob/master/js/currency-converter.js

There is a GET request on line 142 to api.fixer.io.

JSON (representing various currencies) is returned and then stored in localStorage for later reference.

The markup for the converter itself is a string defined on line 21 which is later injected into the DOM with jQuery’s append method on line 208.

The converter will take the value supplied by the user, convert it to the specified currency using the JSON data that was previously saved to localStorage, and then set the value of the span with the newly converted value.

#8

The JSON response if fine on its own but at the time of insertion it has to be made safe.

markup is a sting which is then converted to DOM via $('body').append(markup);
While above is not a security risk as it is, it is bad practice and has performance implications.
In general, converting strings to DOM is not a good idea. Additionally, whenever strings are converted to DOM (through whatever method) it has to be checked thoroughly to make sure they are safe.

As you are converting values to integers, the result is safe eg:
result = (input.val() * rates.rates[thatSelectedCurrency]).toFixed(2);

If you change the process, it should be safe.
Avoid converting strings to DOM ie markup or the following etc:
output.html('¯\\\_(ツ)_/¯');

JQuery.html() is similar to innerHTML and converts strings to DOM. While above is safe, the better way is to use textContent or JQuery.text().

Instead of the HTML entities, you can use their Unicode which works fine with textContent.

Finally, another problem of converting strings to DOM is that reviewer has to track each variable through many files to find out where they have come from and if they are safe. That significantly complicates the review process and adds to your waiting time.

#9

Thank you for taking the time to read through my code. Based on your comments it sounds like while I am using an approach that is frowned upon, the code is not unsafe.

While above is not a security risk as it is, it is bad practice and has performance implications. In general, converting strings to DOM is not a good idea.

I understand that it is less performant but the trade off for me is that it is significantly easier to maintain. The markup that is needed is relatively complex and it’s much easier to maintain a string that reads like html than it is to write a pure Javascript representation of each node and their attributes and then inject them accordingly.

By doing things this way, will my extension ever pass review? Because of the complexity of my extension I get the impression that the reviewer does not want to take the time to actually read through my code.

#10

I only looked at one instance of remote data injection. I have not checked the rest of the files/code.

#11

This is the only call to an external domain that my extension uses. All the other GET requests are made to the site the extension is designed to work with.

#12

Form the perspective of Firefox/amo, all web requests are external (except to localhost, maybe).

When you submit an add-on the reviewers can only verify that the code you are submitting is ok.
They cant say anything about the code you may be serving from you website, and especially can’t know how that will change in the future.
If you are using strings that are not sanatized incorrectly, they are potentially code, and that code is untrusted no matter which domain it comes from.