666066 - Show warnings about compressed/obfuscated/minified code

Status: RESOLVED INCOMPLETE

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

Description

[Kris Maglione [:kmag]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110513 Firefox/6.0a1
Build Identifier: 

There should be a fairly simple heuristic to detect compressed/obfuscated/minified code. Density of space vs. non-space characters or lack of indentation should be sufficient. Minified code in hosted add-ons should trigger the following warnings:

• Code compression or minification serves little purpose in add-ons as XPIs are deflate compressed and downloaded rarely. Please consider submitting your code in a human readable form.

• If you choose to submit your code in a non-human-readable form, it must be reviewed by an administrator and may incur a significant delay. Please leave instructions on where to obtain a human readable form in your review notes, even if you've already sent it to [e-mail]

I'm hoping that this will cut down on some needless minification and otherwise make the binary review process somewhat smoother.

Reproducible: Always

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

We already have bug 540028 in the pipeline, so I think this warning would be more along the lines of "send us the source using the new submission form".

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

I'd also like to point out that some authors have said that minifying their code actually reduced their startup time, so that may be something worth investigating. I wouldn't be so keen to saying it's pointless unless we're sure.

Comment 3

[Kris Maglione [:kmag]]

I've done these tests, both with a huge codebase (Pentadactyl) as well as with simpler test add-ons. Aside from being absurd in principle (especially given that most methods of loading code use the startup cache), I haven't been able to measure any difference whatsoever.

Comment 4

[Wil Clouser [:clouserw]]

So, Jorge, is this something we want?

Comment 5

[Matt Basta [:basta]]

If we do this, I'd like to base it on the statistics that I'm going to be gathering with Diggy. I'm planning a Diggy feature that will log add-on properties (metadata, message counts, source files, etc.) of all of AMO's add-ons into a map/reduceable form.

Based on this data, I could come up with some statistics and plot some graphs which would let us come up with a sensible threshold. If nobody has any objections, I can have something ready fairly soon.

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to comment #4)
> So, Jorge, is this something we want?

I think it's useful, yes, but I think we should fix bug 540028 first and then have this as a warning so developers know the next step they need to follow. Adding it as a dependency.

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...

Comment 8

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Jorge Villalobos [:jorgev] from comment #6)
> (In reply to comment #4)
> > So, Jorge, is this something we want?
> 
> I think it's useful, yes, but I think we should fix bug 540028 first and
> then have this as a warning so developers know the next step they need to
> follow. Adding it as a dependency.

Since bug 540028 isn't going to be fixed soon, I think we can look into this again as an independent bug. Having a warning for this sort of code is definitely useful.

Comment 9

[Matt Basta [:basta]]

This is going to be important for the feature profile stuff that's coming up. Moving to the Marketplace since it'll get more love that way. I'll put the code in valcom so that the love gets spread back to amo-validator.

Comment 10

[Wil Clouser [:clouserw]]

I'm moving this back to AMO.  The Marketplace validator is being rewritten in JS currently which will distance it from AMO's validator.

Comment 11

[Yohan Boniface [:ybon]]

@jorge: can you summarize the needs now that bug 540028 is fixed? I'd prefer to have them clear before starting working on it, thanks :)

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

We need the validator to detect minified and obfuscated code, and show a warning for it. The warning should be the same we show for binaries. The one I suggested on IRC is:

"Compiled binaries, as well as minified or obfuscated scripts (excluding known libraries) need to have their sources submitted separately for review. Make sure that you use the source code upload field to avoid having your submission rejected."

The binary detection warning should change to this new text as well.

Comment 13

[Kris Maglione [:kmag]]

I'm not entirely sure what should trigger this warning. The simplest thing would be to detect extremely long lines of code. Unfortunately, these are often strings which, though they should ideally not be that long, usually do not qualify as obfuscated code.

Ideally, we'd detect, e.g., more than a dozen statements which occur on the same line.

We should also, given the landing of bug 540028, ask for confirmation when attempting to submit a new version/add-on without an accompanying source archive when these warnings, or binary file warnings, have been triggered.

Comment 14

[David Larlet [:davidbgk]]

Counting the number of ";" per line would give you a good idea of how minified the code is I guess. Obfuscated code is way more difficult to detect though. How will you compare sources to minified/obfuscated files to make sure that it's the same code?

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to David Larlet [:davidbgk] from comment #14)
> How will you compare sources to minified/obfuscated files to
> make sure that it's the same code?

We require developers to send us instructions on how they generate the minified/obfuscated files so we can reproduce the process.

Comment 16

[Kris Maglione [:kmag]]

I think that long lines with large numbers of semicolons or commas would be an acceptable way to detect minified code.

For obfuscated code, I can think of a few things:

• Lack of indentation.
• High percentages of extremely short variable names.
• Certain variable name patterns, which I can try to track down.
• A few patterns for specific obfuscation methods, which js-beautify uses for specific deobfuscation techniques. Examples: "eval(function(p,a,c,k,e,r)" (or similar), '^var _0x[a-f0-9]+ ?\= ?\['

Comment 17

[u442650]

I'm just curious, if a developer were to bypass these detections, for example:  

Step1: Obfuscate and Minify
Step2: Obfuscate and Unminify (Using different applications of course) 

Would the code still be reviewable? and would the code still be detected as Minified or Obfuscated? 

-Null

Comment 18

[Kris Maglione [:kmag]]

It would not be reviewable without sources. If machine reformatting were sufficient, sources would never be necessary.

It probably would not be automatically detected, but such add-ons are not a major part of the problem we're trying to solve here.

Comment 19

[u442650]

Correction to my previous comment In "Step2" i meant: *Unobfuscate and Unminify*. 

I'm not sure i understand which add-on's you are referring to, but as i understand it, the new warning(s) would help determine the readability of sections of source, which is sometimes easy to determine, while other times it can be more difficult.

Comment 20

[Mathieu Agopian [:magopian]]

PR: https://github.com/mozilla/amo-validator/pull/277

Comment 21

[Mathieu Agopian [:magopian]]

Attachment: Screen Shot 2015-06-17 at 14.30.15.png

This might be a stupid solution, but why not simply displaying the warning in the "add-on submission checklist"?

There's also a mention in the label for the "upload sources".

I'm not very comfortable with the added complexity that detecting obfuscation and minification would add to the validator codebase, with a very limited gain it seems (only hoping to detect the obfuscation/minification before a reviewer does).

Also, this detection would have to not be run on the included libraries (jquery, underscore...), because it would be a false positive.

What I'm saying is: let's not complexify the codebase if the gain is low.