Concerned about having add-on disabled with the next update

Add-ons addons.mozilla.org
#1

For almost 3 months I have waited for my add-on to be reviewed, which finally happened yesterday. On the same day I had to push an update which fortunately has already been approved, however now I have received the following from the reviewer:

Reviewer:
erosman

Comments:
Your preliminary review request has
been approved.

Please note the following for the next update:

  1. Please do not store large
    CSS sytlesheets in executable JS files. Please include them as separate files
    within your XPI.
  1. I’m sorry, but we can’t allow the inclusion of
    resources as data: URIs in static JavaScript strings, as doing so considerably
    complicates the review process. Please include them as separate files within
    your XPI.
  1. Due to both security and performance reasons, innerHTML (or
    similar method e.g. insertAdjacentHTML, JQuery
    append/prepend/appendTo/html/before/after/insertBefore/insertAfter) should not
    be set using dynamic values. This can lead to security issues or fairly serious
    performance degradation.

For inserting text, textContent (or JQuery text)
or createTextNode() should be used instead of innerHTML.
For inserting HTML,
the safer method is to use createElement(), textContent, appendChild() instead
of innerHTML.
Removing the innerHTML will improve add-on’s prospect of
approval.

First of all, my add-on is literally an adaptation of its userscript version, there is nothing unique to the add-on that is different than the userscript except the method of loading it in the add-on environment. I have created it in a way so that it could be used without any changes for Firefox, Chrome and both userscript managers: Greasemonkey and Tampermonkey.

Now in regards to each guideline:

1 - The current method I use to load CSS files is the only one I can use that works across all platforms. I have no way of loading external CSS files with userscripts so the current method used is the only one that works.

2 - The URIs are necessary and cannot be avoided, I do not host a dedicated service to host the image files and I do not wish to add a load on GitHub servers by hosting all of the images on their servers. I also cannot include them as separate files since this is not possible for the userscript version. If the issue is to check what images are being used it is very simple to open them, I can even create a simple JS tool which can open any data URI in the browser for inspection, I do not see how this is a valid reason to reject an add-on. I have only used icon URIs which don’t exceed 15kb

3 - I only use insertBefore because there is no other method of adding elements to the DOM, especially for creating UI elements and the settings menu, at the position that needs to be inserted, before certain elements. If there is an accepted different method of inserting elements before others I’d appreciate if you could tell me, otherwise this remark is unwarranted. To add more, none of the elements I add to the DOM are dynamic, every single element I create is first converted into a string before adding it to the DOM, all of them are static elements.

4 - I do not understand where this one comes from, in my entire software not once have I used innerHTML.

I do not intend to make a completely different version just for the add-on version of my software, I worked for months to make it work for all 3 platforms without having to change anything whenever I wanted to push an update.

If I will be forced to do anything different that might compromise the ability to have a common payload (userscript) just for Firefox AMO then I am sorry but you give me no choice but to completely drop the add-on version of my software. The review queues were a nightmare, now I am being greeted with these ultimatum warning guidelines, this is a very aggressive environment which discourages any further contribuition for the AMO store.

Will my add-on be disabled if I do not comply with the reviewers’ guidelines when I push the next update? Why did I receive them now and not when the add-on was first approved? Do I have to add to the notes for reviewers all of the above every single time I need to update the add-on?

These look like warnings from a Full Review, is it necessary for these to be sent for every update of a preliminarily reviewed add-on?

1 Like
#2

Addressing point nr. 3:
I think the e-mail is referring to the JQuery versions of insertBefore/append/prepend/etc specifically.
And this is most likely because the JQuery functions are able to interpret strings, posing a security concern.

The paragraph you numbered 4, seems to be clarification of point 3.

#3

Thanks MartijnJ, but I don’t even use jQuery in my software, in fact I don’t use any library, I only use native javascript.

If what you said is true then that is yet another remark that is unwarranted.

#4

Please reply to the review email to discuss any reviews.

#5

Here’s something helps a ton with the last point, when I got that review my reviewer recommended this to me and I’ve been using it since that day, its awesome: https://developer.mozilla.org/en-US/Add-ons/Overlay_Extensions/XUL_School/DOM_Building_and_HTML_Insertion

#6

Thanks, noitidart, but as I said in my post I do not use the innerHTML method at all.

If you are referring to insertBefore then your suggestion is not appropriate since it would remove elements that contain event listeners independent from my software which I cannot restore.

#7

Whats the link to your addon? Set the source publicly view-able Ill take a look.

#8

https://addons.mozilla.org/en-US/firefox/addon/youtube-plus/

#9

Your issue is things like theese:

https://github.com/ParticleCore/Particle/blob/master/dev/Firefox/data/YouTubePlus.user.js#L1882-L1889

This code:

notification.remove();
notification = [
    '<div id="appbar-main-guide-notification-container">\n',
    '    <div class="appbar-guide-notification" role="alert">\n',
    '        <span class="appbar-guide-notification-content-wrapper yt-valign">\n',
    '            <span class="appbar-guide-notification-icon yt-sprite"></span>\n',
    '            <span class="appbar-guide-notification-text-content"></span>\n',
    '        </span>\n',
    '    </div>\n',
    '</div>'
].join('');
notification = string2HTML(notification).querySelector('#appbar-main-guide-notification-container');
document.getElementsByClassName('yt-masthead-logo-container')[0].appendChild(notification);

Would be rewritten with jsonToDOM like this:

notificationJson = ['html:div', {id:'appbar-main-guide-notification-container'},
                            ['html:div', {class:'appbar-guide-notification', role:'alert'},
                                ['html:span', {class:'appbar-guide-notification-content-wrapper yt-valign'},
                                    ['html:span', {class:'appbar-guide-notification-icon yt-sprite'}],
                                    ['html:span', {class:'appbar-guide-notification-text-content'}]
                                ]
                            ]
                    ];
document.getElementsByClassName('yt-masthead-logo-container')[0].appendChild(jsonToDOM(notificationJson, document, {}));
#10

Noitidart I can see no reason to use a different method for building static elements, your solution does not contribute with anything to solve what was noted by the reviewer.

Furthermore I specifically noted that this is a userscript ported to an add-on and I did not want to change anything that would make the core software be different across the platforms, as far as I know jsonToDOM is restricted to the Firefox add-on environemnt so I wouldn’t be able to use it with Chrome or with userscript managers.

Perhaps you are incorrectly thinking that I am adding dynamic elements to the DOM, but I am not. All elements that I generate are converted into a string before adding them to the DOM and I never user innerHTML, only appendChild.

#11

I noticed you used DOMParser.parseString. I suspect that’s the issue. Add-ons run at a much more elevated privleage. Therefore are susceptible to more things.

So in response to your “I can see no reason”, I don’t understand it either, but the reviewers obviously know what they are talking about. Ask them if DOMParser.parseString is vulnerable too, please share what they say I’m curious.

jsonToDOM docs clearly state it works across all browsers. It’s not limitied to the elevated priv scope either. Here is the jsonToDOM function:

function jsonToDOM(json, doc, nodes) {

    var namespaces = {
        html: 'http://www.w3.org/1999/xhtml',
        xul: 'http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'
    };
    var defaultNamespace = namespaces.html;

    function namespace(name) {
        var m = /^(?:(.*):)?(.*)$/.exec(name);        
        return [namespaces[m[1]], m[2]];
    }

    function tag(name, attr) {
        if (Array.isArray(name)) {
            var frag = doc.createDocumentFragment();
            Array.forEach(arguments, function (arg) {
                if (!Array.isArray(arg[0]))
                    frag.appendChild(tag.apply(null, arg));
                else
                    arg.forEach(function (arg) {
                        frag.appendChild(tag.apply(null, arg));
                    });
            });
            return frag;
        }

        var args = Array.slice(arguments, 2);
        var vals = namespace(name);
        var elem = doc.createElementNS(vals[0] || defaultNamespace, vals[1]);

        for (var key in attr) {
            var val = attr[key];
            if (nodes && key == 'id')
                nodes[val] = elem;

            vals = namespace(key);
            if (typeof val == 'function')
                elem.addEventListener(key.replace(/^on/, ''), val, false);
            else
                elem.setAttributeNS(vals[0] || '', vals[1], val);
        }
        args.forEach(function(e) {
            try {
                elem.appendChild(
                                    Object.prototype.toString.call(e) == '[object Array]'
                                    ?
                                        tag.apply(null, e)
                                    :
                                        e instanceof doc.defaultView.Node
                                        ?
                                            e
                                        :
                                            doc.createTextNode(e)
                                );
            } catch (ex) {
                elem.appendChild(doc.createTextNode(ex));
            }
        });
        return elem;
    }
    return tag.apply(null, json);
}

It’s very powerful, you can even do on* in the object for each element and it will add them as addEventListener, its very cool

#12

Things like Array.slice in that code should be Array.prototype.slice so it works in Chrome, I think. That’s all it takes. Please share if that fixes in Chrome so I can update.

#13

DOMParser.parseString does not run anything in its environment since I clearly set its document type as “text/html”. This is not vulnerable and I do not build any script elements with it. The only script element I create is the first one that adds the userscript to the page, no other script element is created after that or before.

Using jsonToDOM to create the exact same static elements that I already create with the current method is still not much different than the method I am using, again it doesn’t contribute for anything to solve either of the notes made by the reviewer.

Nevertheless I will keep that method in mind in case it performs better than my current method.

EDIT: I am sorry, but I thought jsonToDOM was a Javascript function, not a custom made one. I have no interest in such alternatives. I do appreciate the help.

#14

Cool man. As i mentioned I don’t know the inner workings, but the editor also mentioned “performance” issues. So it might not just be security. I really don’t know if it’s right or wrong. Please do share what the editor replies to you with (after you share this stuff with him), we can all learn from it.

The issue I see with parseFromString is that it works kind of like eval it looks like to me. Malicious software can modify the strings and it gets parsed and boom.

#15

The issue I see with parseFromString is that it works kind of like eval
it looks like to me. Malicious software can modify the strings and it
gets parsed and boom.

It doesn’t at all. If you try to parse executable code through the method I am using it will only return a non executable element, the code will never run before or after it is added to the DOM. It is for this exact reason that I chose this method over innerHTML.

Note

script elements get marked unexecutable and the contents of
noscript get parsed as markup.

http://stackoverflow.com/questions/28112807/why-script-elements-created-through-domparser-do-not-execute/28437685#28437685

1 Like
#16

Cool man, very interesting thanks for the learning.

Please do share the reply from the review team. I’m very interested.

#17

Maybe not a script element, but it is able to parse executable event attributes.
i.e.:

<div onclick="stealNakedPictures();"></div>

And I haven’t looked too closely at the jsonToDOM function, but it looks like it’s only able to have actual functions assigned though addEventListener. And not strings.

#18

Maybe not a script element, but it is able to parse executable event attributes.

None of which is able to inject itself via my code, there is no remote data being used for building the elements, all of them are built locally and with static data.

Your argument can also be used to reject all elements being added to the DOM via createElement + addAttribute, which would be completely illogical.

Also using jsonToDOM would force me to inflate my software just to accomodate having to add all element attributes after these are created instead of being created already with them, let alone the possible performance impact it can have.

Furthermore, the reviewer did not note any issue with the current method I am using for creating elements, so I’d appreciate if we wouldn’t continue focusing on something that wasn’t addressed in the post.

#19

Please reply to the review email to discuss any reviews.

I have replied, but received no response. Since the review queues have been very long I thought I’d make use of the forum as well, otherwise I might still be waiting for a reply from the reviewer when I need to push a new update, and without any of my concerns addressed I fear that I might get my add-on disabled because I did not comply with any of the demands made by the reviewer.

I am also still waiting for answers to my 3 questions posted at the end of my thread, which was the entire point of making this thread. Should I post in the old forums instead to see if my questions can be answered since posting here has not resulted in any of my questions being answered?

#20

Can do both, if you do onclick:function() it does addventListener if you do onclick:'stringFunction()' then it does that.

Only limitiation right now is it can only do one on*, so like you can only do one onclick. but thats an easy add in. I should probably hook it up.