Validator should detect redefinition of base objects

RESOLVED FIXED in 5.12.2

Status

addons.mozilla.org Graveyard
Admin/Editor Tools
P4
normal
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: Dave Garrett, Assigned: basta)

Tracking

unspecified
5.12.2

Details

(Reporter)

Description

8 years ago
The validator currently detects extensions of some base objects with 'prototype' in the unsafe JavaScript test. It should also detect the more serious case of redefining base objects altogether. Things such as "Number = something" can really muck up things. I actually tripped over another extension doing this that broke Flagfox and some other extensions a year or so ago (they've fixed it since then).

Suggested regexp:
/\b(Object|Array|String|Number|Boolean|Math|RegExp|Date)\s*=[^=]/
(Reporter)

Comment 1

8 years ago
It's annoying that JavaScript lets you make this kind of an error in the first place. You could probably also add all the built in JS functions:

/\b(encodeURI|decodeURI|encodeURIComponent|decodeURIComponent|escape|unescape|eval|isFinite|isNaN|parseFloat|parseInt)\s*=[^=]/

and even add a line for global properties:

/\b(Infinity|NaN|undefined)\s*=[^=]/

(bug 537863 should stop that for ES5, at least)

Might even be wise to check for redefining other things like 'Components' or any other objects global in chrome:

/\bComponents\s*=[^=]/

Unlike comment 0 I'm not sure how needed all of these are, but JS does let you do stupid things with them so it might be a good idea. Also not sure if you'd want separate regexp or one giant line for all of these.
Priority: -- → P4
Target Milestone: --- → 4.x (triaged)
We should find out how to find this with the static analyzer.  It'd be a good unit test.
Assignee: nobody → mbasta
(Assignee)

Comment 3

7 years ago
Last night, I checked in code to the validator's github that should detect this. I'll be writing unit tests for it soon.
Target Milestone: 4.x (triaged) → 5.12.2
(Assignee)

Comment 4

7 years ago
I added the entities to the predefined entities file. When JS support is finished, this will be covered.

As it stands, predefined globals are automatically treated as read-only variables (with very few exceptions). The validator also blocks the user from declaring a local variable that overrides a predefined global variable of the same name.

My changes are in commit bced1b8:

http://github.com/mattbasta/amo-validator/commit/bced1b8d4d632a458ef9e48ddf7dd87325aada72
(Reporter)

Comment 5

7 years ago
Wonderful work, thanks.

In this JSON format, does {"dangerous": True} automatically imply {"readonly": True}? If not, then could you please add a readonly flag for eval so it has them both?

WRT the base objects (Object through Date) I see that they are all set to:
{"value": {"prototype": {"dangerous": True}}}
which I read as setting its prototype is dangerous (in a global scope). Just to clarify from:

> As it stands, predefined globals are automatically treated as read-only

You mean that these base objects are considered to have the readonly flag and don't need it explicitly set here to have that effect? Just making sure, because as I said in comment 0 I was once burned by another extension doing "Number = something" in an overlay onto the main browser window.
(Assignee)

Comment 6

7 years ago
@Dave: All objects defined there should be implicitly read-only. "Dangerous" entities throw messages when they are accessed in any way (assigned, retrieved, called, etc.). The only reason I added the "readonly" attribute was so there's no dangling code (a la `{"Math":{}}`)

Also, the "readonly" attribute is particularly nice because it allows for expansion down the road. For instance, we've got localStorage. I'm not terribly familiar with it in a XUL context, but the user can assign values to it ("readonly":False}. On the same note, we might want to state that certain child properties or methods are banned. This way, we can allow the object to be overwritten/reassigned, but still have rules associated with it.

Thanks for looking it over!
(Assignee)

Comment 7

7 years ago
This has been completed in the latest commit (ac60594):

http://github.com/mattbasta/amo-validator/commit/ac60594b157d97497663076663ebd8f55a62f9d8

In the next few commits, I'll be pushing unit tests that prove that this is working. For now, you can create a JS file like so:

Number = {};

and run the JS engine on it:

python jsmain.py testfiles/js/redefinition_of_base_objs.js

It will report some very stern errors telling you that everything you are doing is bad. WOOHOO!
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.