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

chore(deps): Upgrade symbol-observable to version 2 #315

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

kriskowal
Copy link
Contributor

This upgrade brings xstream into the set of npm packages that can be safely run without any mutation of primordial prototypes. Such packages can be used in applications that freeze the prototypes to mitigate prototype pollution supply chain attacks. For example, xstream is in the supply chain for the CosmJS financial instruments project and would benefit from such safety measures.

This is considered a breaking change on account of the unlikely possibility that a platform exists that does not support both Symbol and Symbol.for. This hypothetical platform would no longer be supported.

Closes #312

@mightyiam
Copy link
Contributor

symbol-observable instead of symbol-observer in the commit message.

@kriskowal kriskowal changed the title chore(deps)!: Upgrade symbol-observer to version 2 chore(deps)!: Upgrade symbol-observable to version 2 Sep 21, 2020
@kriskowal
Copy link
Contributor Author

symbol-observable instead of symbol-observer in the commit message.

Done.

@kriskowal
Copy link
Contributor Author

Ah, I missed an important detail. xstream would need to use the ponyfill instead of the polyfill. Working.

@kriskowal kriskowal marked this pull request as draft September 21, 2020 04:37
@kriskowal
Copy link
Contributor Author

Well, evidently only being passingly familiar with TypeScript has proven a hindrance in converting xstream to use the ponyfill instead of the polyfill. It feels like we need a minor version bump on symbol-observable to add more type information, specifically about the polyfill module, and maybe even some work to reveal the polyfill at the root of the package instead of under es or lib. I’ve yet to figure out where to put the types to make it work.

@staltz
Copy link
Owner

staltz commented Sep 21, 2020

Hey @kriskowal, thanks for the PR. It looks all good to me, I don't understand what you meant with ponyfill instead of polyfill, it seems like symbol-observable is already a ponyfill. And I think I would call this a non-breaking change because xstream was released in 2016, two years after the browsers you mentioned here were updated.

The only change I would make is fixing the commit message to not have an exclamation mark, it's not a convention in this repo. But I can do this as a "squash and merge".

@kriskowal
Copy link
Contributor Author

@staltz symbol-observable’s documentation is a bit misleading in this regard. It may have drifted from its original form. There are two entry points: index.js and ponyfill.js. The index.js modifies the global environment and returns the symbol. The ponyfill.js creates and memoizes the symbol. By definition, the “ponyfill” shouldn’t alter the environment under any circumstances, but in practice, it just tolerates a frozen environment by catching an error. This is not as lucid as would be ideal, but for the purposes of compatibility with a frozen environment, the ponyfill entry point should be sufficient. I’m still trying to make this work. I have so far verified that importing symbol-observable with a locked down environment does cause an exception during load.

@staltz
Copy link
Owner

staltz commented Sep 21, 2020

@kriskowal Alright, but can you help me understand why change xstream from index.js to ponyfill.js? Is it so that symbol-observable's index.js is not "safe" in a frozen environment while ponyfill.js is?

@kriskowal
Copy link
Contributor Author

@staltz That is correct. To be fully pedantic, ponyfill.js can be used in an environment that is not vulnerable to prototype pollution attacks, so it is the environment that is safe and the ponyfill cooperates. Whereas, running index.js in a safe environment throws an exception.

This upgrade brings xstream into the set of npm packages that can be safely run without any mutation of primordial prototypes. Such packages can be used in applications that freeze the prototypes to mitigate prototype pollution supply chain attacks.  For example, `xstream` is in the supply chain for the CosmJS financial instruments project and would benefit from such safety measures.

This is considered a breaking change on account of the unlikely possibility that a platform exists that does not support both Symbol and Symbol.for. This hypothetical platform would no longer be supported.

Closes staltz#312
@kriskowal
Copy link
Contributor Author

This is ready for review again. What’s changed?

  • Addressed feedback about the commit message.
  • symbol-observable is up to 2.0.3 with all the necessary TypeScript definitions exported properly.
  • most is upgraded to 1.9.0, which in turn depends on symbol-observable and required a similar change.
  • globalthis added as a dependency to satisfy the contract of symbol-observable/ponyfill with a globalThis shim.
  • tsc upgraded and necessary changes applied to get tests passing, particularly null check made explicitly boolean.

@kriskowal kriskowal changed the title chore(deps)!: Upgrade symbol-observable to version 2 chore(deps): Upgrade symbol-observable to version 2 Oct 8, 2020
@kriskowal
Copy link
Contributor Author

@staltz Thank you for your feedback. I’ve addressed that and upgraded the necessary dependencies. I’ve not made a change that freezes the the primordials before running tests, but can do that in a follow-up if that’s desirable. For now, the posted changes are sufficient for anyone using xstream with frozen primordials.

@staltz
Copy link
Owner

staltz commented Oct 12, 2020

Thanks @kriskowal. One question about the globalThis shim: I'm not in full support of that TC39 proposal, so I'd like to know if it's supported in SES and if it's the only way how SES could use the root object? If there's another way, I'd prefer to not have the globalthis dependency.

@kriskowal
Copy link
Contributor Author

Interesting. globalThis is stage 4 (finished) and rolled out to every browser except IE. Regardless, the globalthis shim captures the concern of getting the global from whatever environment you are in, even in no-eval CSP environments. Otherwise, the most reliable way to get the global object is new Function("return this")(), but that does not work under a no-eval CSP.

Otherwise, some JS environments, particularly the XS engine, only realizes the global object by the name globalThis.

SES does not require that you use the globalthis package. The globalthis package just conveniently produces the global object regardless of environment. It could effectively be inlined, though the globalthis package goes through the trouble of ensuring that Node.js only sees module.exports = global, whereas all other environments see a long cascade of typedef checks.

@staltz
Copy link
Owner

staltz commented Oct 12, 2020

@kriskowal Okay, bummer that globalThis proposal is official and deployed, I'll merge this then. Thanks for the efforts!

@staltz staltz merged commit 81bbaff into staltz:master Oct 12, 2020
@staltz
Copy link
Owner

staltz commented Oct 12, 2020

Released xstream@11.14.0

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.

Upgrade symbol-observable to ^2
4 participants