660421 - Validator warning about global variable when it is really local

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[Dave Garrett]

Take a look at the Flagfox 4.1.3 validator output:
https://addons.mozilla.org/en-US/developers/addon/flagfox/file/120833/validation

Under "Extension Tests" most of the warnings are false-positives due to the validator not knowing about JSM (bug 531311). However, I just noticed one buried in there that is not the same; a different kind of false-positive.

The warning given:
============================================================
Global overwrite

Warning: An attempt to overwrite a global variable was made in some JS code.
chrome/flagfox/modules/flagfox.jsm
1015  parameter = token.slice(1,-1);
1016  maybeEncode = function(str) { return String(str).replace(/\\
1017  break;
============================================================

For each warning the middle line of the 3 line block is the one in question therefore "maybeEncode" is the one it's complaining about.

A wider view of the JS in question:
============================================================
function getParameterValue(token,template,encoding)
{
    var parameter, maybeEncode;
    switch (encoding)
    {
        default:
        case "none":
            parameter = token.slice(1,-1);  // Cut off { & }
            maybeEncode = function(a) { return a; };
            break;

        case "url":
            parameter = token.slice(3,-3);  // Cut off %7B & %7D
            maybeEncode = encodeURIComponent;
            break;

        case "escapequotes":
            parameter = token.slice(1,-1);
            maybeEncode = function(str) { return String(str).replace(/\\/g,"\\\\").replace(/\'/g,"\\\'").replace(/\"/g,"\\\""); };
            break;
    }
...
...
============================================================

There are two variables being assigned in each case block, "parameter" and "maybeEncode", both declared but not defined together on the first line and are local to this function (inside another function, if it matters).

After a couple quick test uploads, it seems if I remove the "maybeEncode = encodeURIComponent;" line in the "url" case then the warning goes away even though it's a few lines down where the warning is suppose to be about. My best guess is this is because "encodeURIComponent" is a built-in JavaScript function reference and not anything anywhere in this file. Somehow this has tripped up the validator and exposed that it's handling variables somehow wrong here. You seem to also get into the same situation if you use "undefined" instead.

This is admittedly an edge-case, but it could mean that whatever is mishandling the variable could cause other false-positives (or false-negatives) somewhere else, and seeing as I don't know what exactly is going wrong I thought I might as well file a bug here with what I learned when trying to figure out why it was giving me this warning.

Comment 1

[Matt Basta [:basta]]

Whoa, this is a biggie. Lots of explanation. Let me begin by explaining why you're getting the error (besides the fact that JSM isn't accounted for).

The validator can't possibly account for every scenario. The most ideal method to scan JS for vulnerabilities would be to emulate a copy of Firefox and fire off every possible event in every possible order with every possible set of arguments, passing every range of random values for the random function and waiting out every setTimeout. The problem is that this isn't practical: even if we were able to simulate FF 100% accurately, the test could take up to centuries to complete, which we obviously don't have.

The second most ideal method is to do an iteration of all of the code (what we currently do). This also has its problems, though. Firstly, branching poses a serious issue. If we were to evaluate all of the code, what behavior should we take at an if statement? Ideally, we would fork the process, keeping two separate scopes and merging after each branch of the flow where possible. While this may be something we'll look at in the future, the amount of resources that it requires is pretty huge. Instead of taking branches and loops into account, we simply run straight through. This means that the following code:

if(a = b + 1) {
    foo();
} else {
    bar();
}

is run as

a = b + 1;
foo();
bar();

The same goes for switch statements, loops, etc. We do some special sauce to make functions work properly, and we always obey the laws of scope, so that enables us to accurately find certain problems.

Where we run into issues is when there are branches of code that conflict with one another. For instance:

if ( ... )
    x = String.prototype;
else
    x.foo = "bar";

In this case, the code is executed as

String.prototype.foo = "bar"

which should (and does) throw an error. This is almost exactly 

At present, without serious resource-intensive scanning techniques, there's no a whole lot we can do to remedy the false positives, so that's likely going to be around for a while.

In the mean time, however, there are two issues that need correcting. The first is the JSM nonsense. I've just written a quick patch that should take care of all of the errors. I'm doing a little research just to make sure that it's not going to expose any vulnerabilities. The second issue is that a local variable containing a global object cannot be reassigned. I know we had a similar issue to this with XPCOM (I think) a while back, and I believe I added a special property to the JSWrapper class to handle it. I'll look into what was done, but I've added a unit test to handle it until that gets fixed.

Comment 2

[Matt Basta [:basta]]

> hich should (and does) throw an error. This is almost exactly 

Whoops. BRBed and never finished my thought. I meant to say:

This is almost exactly what the problem with the flagfox.jsm file is.

Comment 3

[Dave Garrett]

(In reply to comment #1)
> Where we run into issues is when there are branches of code that conflict
> with one another. For instance:
> 
> if ( ... )
>     x = String.prototype;
> else
>     x.foo = "bar";
> 
> In this case, the code is executed as
> 
> String.prototype.foo = "bar"
> 
> which should (and does) throw an error.

This makes sense, however in my code sample each case merely assigns a new reference. If the switch were flattened as your example by dropping the "case" and "break" lines, then the result would just assign the two variables 3 times each. "maybeEncode" would be a reference to a global function then be re-assigned to a reference to the locally declared function. This isn't invalid; it's just a reference. Yes, if it were to modify a property of that reference then that would be that problem, but that's not the case even in the flattened test scenario.

> In the mean time, however, there are two issues that need correcting. The
> first is the JSM nonsense. I've just written a quick patch that should take
> care of all of the errors.

Fixing bug 531311 would be great. It'll be nice to get rid of most of those warnings so people using JSM can actually see the relevant ones.

> The second issue is that
> a local variable containing a global object cannot be reassigned.

To be precise, this is a local variable containing a reference to a global function. In any case, why can't this be reassigned? Is this a limitation in the validator or something to do with the JS engine? In the following code in getParameterValue() it only uses maybeEncode() as needed and only assigns it the one time in the switch (though as you stated part of the problem is the validator can't see that).

Comment 4

[Matt Basta [:basta]]

> Is this a limitation in the validator or something to do with the JS engine?

It's just a bug. I'm working on it now. Basically, the validator (at the moment) tries to re-use JSWrappers whenever they're present because it fetches identifiers as an expression (even as an l-value). It's a pretty easy fix, once I find where it needs to be done.

Comment 5

[Matt Basta [:basta]]

Fixed:

https://github.com/mattbasta/amo-validator/commit/13926c3fccd5b2dfe0f61dc0fa69ccfe4a9f53fa

Comment 6

[Matt Basta [:basta]]

Merged:

https://github.com/mozilla/amo-validator/commit/b77ffa6905da7f62483ebcde79c3451471f35e19