search

mishoo / UglifyJS

JavaScript parser / mangler / compressor / beautifier toolkit
http://lisperator.net/uglifyjs/
Other
13.16k stars 1.25k forks source link

IIFE a.k.a JavaScript Function Closures #2166

Closed Martii closed 7 years ago

Martii commented 7 years ago

Bug report or feature request? CONFIRMED SECURITY BUG

ES5 or ES6+ input? ES6+

Uglify version (uglifyjs -V) 3.0.20

JavaScript input Martii/UserScripts/blob/35fcac/src/RFC 2606§3/Hello, World/RFC 2606§3 - Hello, World.user.js

The uglifyjs CLI command executed or minify() options used.

API: OpenUserJs/OpenUserJS.org/blob/0220412/controllers/scriptStorage.js#L744-L755

JavaScript output or error produced.

IIFE is stripped completely out as compared to last stable tested of uglify-es@3.0.10

Refs:

Martii commented 7 years ago

Bug appeared between uglify-js@3.0.15 (PASS) and uglify-js@3.0.17 (FAIL) .

Ref:

https://github.com/mishoo/UglifyJS2/releases/tag/v3.0.16 appears to be not available on npmjs.com for release isolation testing:

npm ERR! notarget No compatible version found: uglify-es@3.0.16

EDIT: Breaking change detected at https://github.com/mishoo/UglifyJS2/commit/8af362ed57ab6d236f87abe76301f50928153661#diff-230c53e13e3ee367aec7d49470eb42ca

rvanvelzen commented 7 years ago

What is the actual issue? (function () { alert('Hello world'); }()) being transformed to alert('Hello world');?

(if so, please describe accurately how this is a security issue)

Martii commented 7 years ago

What is the actual issue? (function () { alert('Hello world'); }()) being transformed to alert('Hello world');?

Not quite (stripping out the comments and license since this is a three line unit test for the last 10ish years):

(function () {

  alert('Hello, World!');

})();

... resolves to (comments and license stripped yet again):

alert("Hello, World!");

... should be (again comments and license temporarily stripped) ...

!function(){
alert("Hello, World!")}();

See the references above for IIFE usage if there are any questions.

rvanvelzen commented 7 years ago

I know what IIFEs are, thank you.

Now what is the actual issue?

Martii commented 7 years ago

See section above with label of "JavaScript output or error produced."... and the 2nd reiteration in the other above post.

rvanvelzen commented 7 years ago

This transformation is safe. If it's not, please clearly explain what the actual issue is.

Martii commented 7 years ago

This transformation is safe.

No it's not.

I know what IIFEs are, thank you.

Think adding variables to the global namespace. So it is definitely not safe.

rvanvelzen commented 7 years ago

You must be confused over what this transformation does, since it does not introduce global variables:

echo '(function () { var foo = 5; alert(foo); }())' | ./bin/uglifyjs -c 'reduce_vars=0'
!function(){var foo=5;alert(foo)}();

vs.

echo '(function () { var foo = 5; alert(foo); }())' | ./bin/uglifyjs -c
alert(5);
Martii commented 7 years ago

@rvanvelzen

You are quite confused on how IIFE's work and the fact that you are using the CLI and not the API. Since it's been talked about with our OUJS organization we will probably be dropping uglify-js and issuing a node cert against this project for allowing this security breach.

I really wish that this project would get their recent heads out of their *****. People shouldn't be afraid of retaliation from maintainers. Way to alienate your users as well as your contributors again. You'll learn the hard way it seems.

Martii commented 7 years ago

Btw this is a breaking change... so it's been screwed up there as well.

stephengardner commented 5 years ago

I'm confused with this issue. IIFEs cannot be transformed into non-IIFEs... That defeats the purpose of using an IIFE.

Removing an IIFE is not safe. What is the solution to this @Martii ?

stephengardner commented 5 years ago

nevermind @Martii I found your issue here: https://github.com/OpenUserJS/OpenUserJS.org/commit/89566179afee5a8afd85cbc4f68609a3bb073626

To anyone who comes across this issue, it appears that @rvanvelzen is confused as to the purpose of an IIFE, and the fact that it is not safe to remove it. You should add: compress: { inline: false } to your options.