fix: Ponyfill should not tamper with globals #47

kriskowal · 2020-08-31T21:22:27Z

@benlesh For your consideration,

Prior to this change, the ponyfill module would also polyfill Symbol.observable. With this change, the ponyfill can be used in environments with immutable primordials.

Please let me know if this change is acceptable, or if I can adjust it to be consistent with this project’s style.

Prior to this change, the ponyfill module would also polyfill Symbol.observable. With this change, the ponyfill can be used in environments with immutable primordials.

erights · 2020-08-31T22:19:56Z

es/index.js

+root.Symbol = root.Symbol || function Symbol(description) {
+  return '@@' + description;
+};
+root.Symbol.observable = result;


This will still fail if Symbol (a primordial) is frozen.

There are two entry points. This is the polyfill (mutates globals) and the other is a ponyfill (the other module) is the subset that exports its API and does not mutate globals.

erights · 2020-08-31T22:21:15Z

es/index.js

@@ -16,4 +16,10 @@ if (typeof self !== 'undefined') {
 }


Probably not for this PR, but I suggest adding a globalThis branch to the above if-else sequence.

erights · 2020-08-31T22:27:39Z

es/ponyfill.js

@@ -1,17 +1,23 @@
+
+const observableSymbols = new WeakMap();


Does the following simpler module accomplish the same thing without reference to root or use of WeakMap?

export default Symbol('observable');

where index.js line 18 would just be

import result from 'ponyfill.js';

If not, I don't understand what we're trying to accomplish.

The WeakMap was only necessary to make this work with the unit test, which runs the ponyfill multiple times in the same execution environment. If I were to reframe the tests such that they run the ponyfill only once per process, it would be sufficient to capture the result in a module scope singleton.

If we want the same reference to a symbol, then Symbol(description) won't do that, but we don't need the WeakMap. We could use Symbol.for('symbol-observable-ponyfill') or the like, and have a unique, but shared symbol. (So long as Symbol.for exists, which it likely does if Symbol exists).

I know that two evaluations of Symbol('observable') will give two separate symbols, which is not what we want. But (under current conditions) a module has one instance, so all importers should get the same symbol.

Yes, using Symbol.for('observable') would be a more robust way to ensure that there is only one such symbol.

I’ll take another look at this with an eye for removing the WeakMap and using Symbol.for to ensure idempotence instead.

benlesh · 2020-08-31T23:22:05Z

es/index.js

@@ -16,4 +16,10 @@ if (typeof self !== 'undefined') {
 }

 var result = ponyfill(root);
+
+root.Symbol = root.Symbol || function Symbol(description) {
+  return '@@' + description;


The only problem I see with this is that it doesn't align with what Symbol does.

With Symbol(description) the description doesn't affect the equality of the result. In other words: Symbol('test') !== Symbol('test')... This is different than Symbol.for, which does what you're expecting this to do.

The result of Symbol must be a completely unique value. The @@ helps, and is an accepted practice. But really, I guess you'd need to do something like:

let _symbolNumber = 0; function Symbol(description) { return `@@_${_symbolNumber++}_${description}` }

The other depressing bit about this, is you'd have to make sure that description was converted into a valid set of characters for a property name. OR you'd have to just attach the description to the string you returned somehow (although I haven't thought much about how).

Since we're polyfilling Symbol in this case, I guess I'd expect it to behave appropriately (and even possibly implement Symbol.for)

Ah, and the code I’m modifying presumes the existence of Symbol or a Symbol shim as you describe. That being the case, I think it would be sufficient for parity to simply remove this line. Does that track?

We could remove that line yeah... However, I'm not sure if there's a runtime that exists where WeakMap exists, but Symbol does not. I'm trying to research that now.

So I have two questions:

Can you elaborate on the problem you're trying to solve?

Can we move the creation of the WeakMap inside of the check for existence of Symbol?

I guess, if I'm understanding right, in a runtime you're using calling Symbol.observable = ??? will throw an error because Symbol is frozen. Is that correct?

If that's the case, could we just wrap it in a try-catch?

We could remove the WeakMap but I would have to make a more invasive change to the test, either by exporting a clearForRetest function or running each test in a fresh process.

Capturing the failed assignment to Symbol.observable is an option. Checking the property descriptor for writability would be another. This is still my recommendation since it clearly separates the polyfill from the ponyfill. I’ve really just moved the assignment to Symbol.observable from the ponyfill to the polyfill. I’ll write up whatever solution you prefer.

if there's a runtime that exists where WeakMap exists, but Symbol does not. I'm trying to research that now.

They were both introduced in ES6, so I would be surprised. Please let us know what you find. I'm curious.

I guess, if I'm understanding right, in a runtime you're using calling Symbol.observable = ??? will throw an error because Symbol is frozen. Is that correct?

Yes.

If that's the case, could we just wrap it in a try-catch?

And do what in the catch clause? Whatever that is, if it works in that case, why not just do that all the time?

benlesh · 2020-08-31T23:34:04Z

test/ponyfill.js

@@ -33,7 +33,6 @@ describe('ponyfill unit tests', function () {

        var result = ponyfill(root);

-        expect(Symbol.observable).to.equal('Symbol(observable)');


I don't think we should remove this, as if Symbol exists, we're expecting to have polyfilled it, but if it doesn't exist, we at least have the returned value.

benlesh · 2020-08-31T23:34:42Z

es/ponyfill.js

 	if (typeof Symbol === 'function') {
 		if (Symbol.observable) {
 			result = Symbol.observable;
 		} else {
 			result = Symbol('observable');
-			Symbol.observable = result;


Again, I'm not sure if I understand the goal of removing the polyfilling from this?

Is it that it causes an error in some runtimes?

Yes. With the XS runtime and with SES, which both protect client code from prototype pollution, Symbol is a frozen object. My understanding is that the useful distinction between a ponyfill and a polyfill is that the former is useful in such environments and doesn’t require the special treatment of shims.

kriskowal · 2020-08-31T23:45:17Z

I should provide more context: I’m coming with this change request because there is a cryptoeconomic project that would benefit from protection against supply chain attacks. That project largely works under SES, but three of its modules depend on this ponyfill.

benlesh · 2020-08-31T23:52:59Z

Okay, so I think the "ponyfill" aspect of this is maybe a little bit off... when I inherited this repo from @sindresorhus, the ponyfill would also polyfill if it could: https://github.com/benlesh/symbol-observable/blob/1738bfc9fefef23d310dba3d68e0d83d44253265/index.js

So perhaps that is the source of some of the confusion here. BUT... I think we can solve this in this PR.

We can check for the existence of Symbol.for (which TMK exists in all runtimes that support Symbol) and use that to create symbol that is the same every time this ponyfill is called.
If Symbol (and Symbol.for) does not exist, it returns "@@observable", which because it's a string, is always equal no matter how many times this is imported.
If Symbol is frozen with Object.freeze, I think trying to assign to Symbol.observable does nothing at all, not even an error, so that should be fine, but we could wrap it in a try/catch if we don't think that is safe enough.

Do you think that will solve your issues?

erights · 2020-09-01T00:00:16Z

Okay, so I think the "ponyfill" aspect of this is maybe a little bit off... when I inherited this repo from @sindresorhus, the ponyfill would also polyfill if it could: https://github.com/benlesh/symbol-observable/blob/1738bfc9fefef23d310dba3d68e0d83d44253265/index.js

So perhaps that is the source of some of the confusion here. BUT... I think we can solve this in this PR.

We can check for the existence of Symbol.for (which TMK exists in all runtimes that support Symbol)

I think so as well, since Symbol.for came in with ES6. Again, please let me know what you find. Symbol.for is indeed the most robust solution to ensuring you're talking about the same symbol.

and use that to create symbol that is the same every time this ponyfill is called.
2. If Symbol (and Symbol.for) does not exist, it returns "@@observable", which because it's a string, is always equal no matter how many times this is imported.

yes.

If Symbol is frozen with Object.freeze, I think trying to assign to Symbol.observable does nothing at all, not even an error,

Assigning to a non-writable property does throw in strict-mode code, and therefore in module code. It only does not throw in sloppy mode code.

so that should be fine, but we could wrap it in a try/catch if we don't think that is safe enough.

The try/catch would work, leaving a system in which the correct symbol is importable but not on Symbol, which is good.

Do you think that will solve your issues?

AFAICT, yes.

benlesh · 2020-09-01T00:20:46Z

@kriskowal, @erights, please review the following for me?

#48

kriskowal · 2020-09-01T01:00:55Z

I believe the changes in #48 do the job. Thank you!

fix: Ponyfill should not tamper with globals

7e31b84

Prior to this change, the ponyfill module would also polyfill Symbol.observable. With this change, the ponyfill can be used in environments with immutable primordials.

erights reviewed Aug 31, 2020

View reviewed changes

benlesh requested changes Aug 31, 2020

View reviewed changes

benlesh reviewed Aug 31, 2020

View reviewed changes

benlesh mentioned this pull request Sep 1, 2020

fix: Resolve issues in environments with frozen Symbol #48

Merged

kriskowal closed this Sep 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Ponyfill should not tamper with globals #47

fix: Ponyfill should not tamper with globals #47

kriskowal commented Aug 31, 2020

erights Aug 31, 2020

kriskowal Aug 31, 2020

erights Aug 31, 2020

erights Aug 31, 2020

kriskowal Aug 31, 2020

benlesh Aug 31, 2020 •

edited

Loading

erights Aug 31, 2020

kriskowal Aug 31, 2020

benlesh Aug 31, 2020

kriskowal Aug 31, 2020

benlesh Aug 31, 2020

benlesh Aug 31, 2020

kriskowal Aug 31, 2020

erights Aug 31, 2020

erights Aug 31, 2020

benlesh Aug 31, 2020

benlesh Aug 31, 2020

kriskowal Aug 31, 2020

kriskowal commented Aug 31, 2020

benlesh commented Aug 31, 2020

erights commented Sep 1, 2020

benlesh commented Sep 1, 2020

kriskowal commented Sep 1, 2020

		@@ -33,7 +33,6 @@ describe('ponyfill unit tests', function () {

		var result = ponyfill(root);

		expect(Symbol.observable).to.equal('Symbol(observable)');

fix: Ponyfill should not tamper with globals #47

fix: Ponyfill should not tamper with globals #47

Conversation

kriskowal commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Aug 31, 2020

benlesh commented Aug 31, 2020

erights commented Sep 1, 2020

benlesh commented Sep 1, 2020

kriskowal commented Sep 1, 2020

benlesh Aug 31, 2020 •

edited

Loading