1176721 - Overzelous Un-listed Signing Severity High Warning: `setTimeout` called in potentially dangerous manner

Status: RESOLVED INCOMPLETE

(addons.mozilla.org Graveyard :: Add-on Validation, defect)

Description

[Steven Harris]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.124 Safari/537.36

Steps to reproduce:

Uploaded an un-listed add-on to the Validator with something like the following code:

(function(){
	function fun(){
		console.log('having fun');
		window.setTimeout(fun, 1000);
	};
	window.setTimeout(fun, 1000);
})();


Actual results:

Received the following warning - `setTimeout` called in potentially dangerous manner
Signing severity: high
Warning: In order to prevent vulnerabilities, the `setTimeout` and `setInterval` functions should be called only with function expressions as their first argument.


Expected results:

Uploaded add-on with something like the following code:
(function(){
	function fun(){
		console.log('having fun');
		window.setTimeout(fun, 1000);
	};
	window.setTimeout(fun, 1000);
})();

The Validator is treating the first argument as a generic lvalue, so it's flagging it as a an error since an lvalue could in fact be a string (and thus requiring an eval). This is not the case. It's a function, defined in a closure, so there is no way it could be changed into a string under normal circumstances.

Flagged as a HIGH Signing Severity Warning results in our add-on requiring manual review, and changes the time frames for us to get our unlisted add-on signed.

In addition to the above, it is a common practice for us to name the function and pass it's name as the first argument to setTimeout/setInterval. To address this warning, we will be forced to change the above to something like:

(function(){
	function fun(){
		console.log('having fun');
		window.setTimeout(function(){fun();}, 1000);
	};
	window.setTimeout(function(){fun();}, 1000);
})();

This creates less readable code only to address what I view as a shortcoming of the Validator.

I propose that the Validator instead ensure two things:
1) That the first argument is in fact the name of a function, or is an lvalue already assigned a function
2) That the name of the function, or the lvalue, are within a closure. This addresses the potential for a third party to assign the name/lvalue to a string value, thus introducing the concerns already documented in bug id 509500.

Comment 1

[Mathieu Agopian [:magopian]]

Kris, thoughts?

Comment 2

[Kris Maglione [:kmag]]

It's only the first setTimeout call that's causing this warning. I'm not entirely sure why `fun` is not being bound properly here, but I'll look into it.

In the mean time, you have several alternatives which won't trigger warnings, including:

setInterval(function() {
    console.log('Thing.');
}, 1000);

!function next() {
    setTimeout(function() {
        console.log('Thing.');
        next();
    }, 1000);
}();

Comment 3

[Wil Clouser [:clouserw]]

I think bug 1172044 will address this.  Kris says he is looking into this, so I'll cautiously assign to him.  Please unassign if you're not able to.