Closed
Bug 1176721
Opened 9 years ago
Closed 1 month ago
Overzelous Un-listed Signing Severity High Warning: `setTimeout` called in potentially dangerous manner
Categories
(addons.mozilla.org Graveyard :: Add-on Validation, defect)
addons.mozilla.org Graveyard
Add-on Validation
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: steven.harris, Unassigned)
Details
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 2•9 years ago
|
||
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); }();
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(kmaglione+bmo)
Comment 3•9 years ago
|
||
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.
Assignee: nobody → kmaglione+bmo
Assignee | ||
Updated•8 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
Updated•8 years ago
|
Assignee: kmaglione+bmo → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•