Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove built-in pollution #1217

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

Jack-Works
Copy link

close #1216

@amark
Copy link
Owner

amark commented Mar 8, 2022

@Jack-Works

I appreciate the work.

This has irony.

I believed the same as you, for 8 years. All utilities were under GUN.

For the 1st time in 8 years, 6 months ago I changed. Global "pollution" is a feature, not a bug...

UNLESS you can show or prove to me where my code is causing your (or somebody else's) code to crash? You win right away, if true (easy to verify).

However, if it is ONLY on the pretense of "code priests say 'pollution' is a sin" then (these) edits will be rejected. Again, I was of the same belief for 8 years, so I sympathize with it, but I was wrong.


MDN polyfill you can argue with me more on, there are 3 sub-categories to that, 1 of which I'm wrong, the others I'm right or neither. Dependingly, this may be accepted.


isSecureContext I'm assuming MUST need to be accepted. Altho won't this break Mask in iframes, etc. (?)

@Jack-Works
Copy link
Author

I'm sorry for the wording "pollution" if it does not sound right.

Maybe I should introduce some background about why I need this change.

Secure ECMAScript is a JavaScript proposal, that is intended to enhance the security of the JavaScript running environment. For an ideal secure JavaScript environment, the primordials (objects that existed before any of your code runs, like Function, Number, String, ...etc) will be frozen.

This means trying to change or adding new items on it is not allowed.

Today we have a shim for it, called SES, you can try it in the browser. Open a new blank page, then run the following code in F12.

import('https://cdn.jsdelivr.net/npm/ses@latest/dist/ses.umd.js').then(function () {
    lockdown({ errorTaming: 'unsafe' })
    console.log('Now this is a SES-like environment')
})

If you try to load Gun in this environment

import('https://cdn.jsdelivr.net/npm/gun@0.2020.1235/gun.js')

You will find it cannot be loaded.
image

Introduce SES and lockdown the environment is in our security milestone, in DimensionDev/Maskbook#5759

Now I have two libraries not working in the SES, one of it is Gun therefore I opened this PR.

If you still think adding those functions are a good idea, it's cool, I can open another PR to do the MDN polyfill and the isSecureContext change, but I have to patch Gun inside node_modules to let it continous working in our environment.

@amark
Copy link
Owner

amark commented Mar 9, 2022

@Jack-Works ah yes, Aaron at MetaMask has mentioned SES too.

This is a valid reason. Sigh... so much for the hopes I had :P lol.

I'm a bit particular about naming, so I'll probably rename everything again.

SES allows window.libName = ... tho right? (I don't do this in NodeJS obviously, the code checks to be isomorphic)

Super swamped the next couple days with some announcement and Rust testing 40K+ socket connections.

When do you need this merged by? Immediately? Next month? Cause several pieces I'll need to pick&choose / deal with some merge conflicts in the CPU scheduling parts that deal with JSON. I want to make sure you get the git credit tho, even if I alter back some stuff right after.

Thanks for explaining the reasoning! Very important.

@Jack-Works
Copy link
Author

Jack-Works commented Mar 9, 2022

SES allows window.libName = ... tho right? (I don't do this in NodeJS obviously, the code checks to be isomorphic)

Yes. The global object is mutable. You can add window.random ...etc, but I hope Gun doesn't add too many things on the global object, it will be cool if those utils stay on the Gun.utils or some other namespace.

When do you need this merged by? Immediately? Next month?

This month will be good, if you have enough time. because another library has not responded to my PR yet so I'm not too hurry on this. (If I'm hurrying, I will choose to patch the package).

Thanks for your understanding!

@amark
Copy link
Owner

amark commented Mar 25, 2022

Ok I think I'm ready for this since I got the version out... did the other project you're waiting on get back to you how long they'll have it finished by?

I did a test, it looks like SES locksdown most, but setTimeout is available which is the only thing that works in both Browser & NodeJS & React-Native environments consistently, so can I please keep using that since SES didn't error? It saves a ton of boilerplate code that wastes bytes...

<script src="https://cdn.jsdelivr.net/npm/ses/dist/ses.umd.min.js" charset="utf-8"></script>
<script>
lockdown();
console.log("hello!", window, setTimeout);
Object.keys(window).forEach(function(k){
try{
	window[k].lol = 1;
	if(window[k].lol){
		console.log("found it!", k, window[k].lol, '---', window[k]);
	}
}catch(e){}
})
setTimeout(function(){
	console.log("test?", setTimeout.lol);
}, 100);
</script>

@Jack-Works
Copy link
Author

did the other project you're waiting on get back to you how long they'll have it finished by?

Not yet 😂

I did a test, it looks like SES locksdown most, but setTimeout is available which is the only thing that works in both Browser & NodeJS & React-Native environments consistently

Yeah, because setTimeout isn't in the JavaScript language itself therefore is not protected by lockdown.

@amark
Copy link
Owner

amark commented Apr 2, 2022

In my test ses just errors null... no description ... how do I determine whether it likes or dislikes something?

@Jack-Works
Copy link
Author

    lockdown({ errorTaming: 'unsafe' })

You need to use errorTaming: 'unsafe' because error.stack is not standard, SES will remove it by default.

@Jack-Works
Copy link
Author

And you need to use 'use strict' strict mode for every code to make it throw explicitly

@kumavis
Copy link
Contributor

kumavis commented Apr 4, 2022

👍 for ses support

@nicocha30
Copy link

This PR solved an issue when running GunJS with React + ParcelJS.

We were getting "setTimeout.each is not a function" errors.

@amark
Copy link
Owner

amark commented May 26, 2022

I'm getting closer to the """breaking""" version bump which I intend to include these (or a variation of) fixes in.

@nicocha30 I'm getting conflicting messages on what are safe globals. SES says setTimeout is fine (which kinda surprises me). However I'm hearing from you, that Parcel may not? I just tested React, and it has no issue.

  • global not safe in browser
  • window not safe in nodejs/native
  • export not safe in JS < 2015
  • require module not safe because webpack/etc. changes their rules every 6 months
  • Duktape, QuickJS, Espruino, Rhino, Hermes, Boa, etc. all I want to take into consideration, not just browsers JS engines.

I need a universal way to attach the few polyfills I use. Historically I attached them to GUN, but they're reused by other non-GUN-dependent modules in GUN, so I (in the last recent version bump) attached them to where the should/would belong (Object.keys on Object for instance).

It seems that some engines don't support setTimeout (Boa, Quick, yet embedded ones like Espruino do!), oddly. GUN needs a way to manage async time operations. Does anyone know how one would support events or future states in those systems? They internally have to have access to CPU tick cycles somehow, else how do they manage epoll/etc.?

I try to not even depend upon JSON (I've always hated it), altho I regretfully choose it as the default serialization format... 😔 even for the CRDT, because I foolishly bought into the "everyone/language supports JSON!" and ubiquity/compatibility was more important to me in 2014 than my hatred for it. I'm forcibly changing this slowly tho, since the edge cases requires sub-microsecond level conflicts on universally unique non-owned/multi-owned non-string values. CRDT divergence here is not capable of resulting in a glitch parallel universe's civilization being developed, so I think its safe - or at least the universe where JSON is still used will probably look like a hunger games hell compared to the solarutopia of the non-JSON one.

@amark
Copy link
Owner

amark commented May 27, 2022

Note to self: Float32Array is liked in most places.

@Jack-Works
Copy link
Author

global not safe in browser
window not safe in nodejs/native

Try globalThis first (the modern standard way), then the common fallback, global on Node and self on Web.

I need a universal way to attach the few polyfills I use.

You should not polyfill for the user as a library. And adding non-standard features isn't a "polyfill".

It seems that some engines don't support setTimeout (Boa, Quick, yet embedded ones like Espruino do!), oddly. GUN needs a way to manage async time operations. Does anyone know how one would support events or future states in those systems?

Yes. setTimeout isn't in the JavaScript itself. It's possible that some JS engine doesn't have that. They may have their own mechanism to add a timer (in this case you should let users to add support by themself), like Gun.sys.timer = theirPlatformTimer.
If they don't have any async time mechanism, I'm afraid you should disable some feature of gun, or even give up the support on those platforms.

If you have the ambition to support all JS engine, I have written a blog about this topic: Write library for any ECMAScript environment

@Jack-Works
Copy link
Author

export not safe in JS < 2015
require module not safe because webpack/etc. changes their rules every 6 months

I have to say it's a mission too difficult if you do not want any build tool and you want to support both those environments.

@amark
Copy link
Owner

amark commented May 27, 2022

@Jack-Works excellent article.

I'm fine with unbuild tools. But the only build tools I'm OK with are Rust/C++/etc.

"Polyfills" it is not fair to the user if they have to get 10 copies of uuid & string random functions, especially when all languages use nearly the same code for it. I don't want these attached to GUN because then you can't use SEA (core), RAD (core), etc. without a GUN dependency. So I'm looking for a generic place to put them, like a JS std::* without having to create yet-another global (and in my case: all the bytes required to lookup the "correct" global for every freaking environment... in every freaking library).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not mutating global variables
4 participants