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

Use Symbol.observable “ponyfill” #541

Merged
merged 6 commits into from
Oct 5, 2020

Conversation

kriskowal
Copy link
Contributor

This change upgrades symbol-observable and changes use to symbol-observable/ponyfill so that MostJS can be used in JS environments where the primordials are frozen to mitigate prototype pollution attacks.

I have not added unit tests to verify this, but with your consent, I can add a development dependency on SES and use that to lock down the primordials during tests.

This is not a user-visible change, but does enable MostJS to be used in more environments. I do not see a change log to mention this in.

@kriskowal
Copy link
Contributor Author

Good to see you again, @briancavalier 😊

src/index.js Outdated Show resolved Hide resolved
@kriskowal kriskowal marked this pull request as draft September 30, 2020 01:44
@briancavalier
Copy link
Member

Hey @kriskowal 👋 Thanks for this, and for the note about globalThis. I'll take a closer look soon.

@kriskowal kriskowal marked this pull request as ready for review September 30, 2020 02:10
@kriskowal
Copy link
Contributor Author

Posted follow-up changes to use a globalthis polyfill so this can still work with a no-eval CSP.

@kriskowal
Copy link
Contributor Author

Hey @kriskowal 👋 Thanks for this, and for the note about globalThis. I'll take a closer look soon.

Awesome. I took care of globalThis so this should work with a no-eval CSP now. Took this back out of draft.

Copy link
Member

@briancavalier briancavalier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a question about using globalthis in a couple more places. Let's stick with package-lock.json, unless there's a compelling reason to have a yarn lockfile also. Thanks!


// eslint-disable-next-line no-new-func
const globalThis = new Function('return this')()
const symbolObservable = provideSymbolObservable(globalThis)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also use the globalthis module here, or is there a reason to use the new Function approach here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this one! Good catch.


// eslint-disable-next-line no-new-func
const globalThis = new Function('return this')()
const symbolObservable = provideSymbolObservable(globalThis)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

package.json Outdated
@@ -68,7 +68,8 @@
"dependencies": {
"@most/multicast": "^1.2.5",
"@most/prelude": "^1.4.0",
"symbol-observable": "^1.0.2"
"globalthis": "^1.0.1",
"symbol-observable": "^2.0.2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

This change brings in the next major version of a shim for Symbol.observable, such that MoSt can take advantage of the Ponyfill and avoid mutating the environment.  This allows MoSt to run in an environment where the primordials have been frozen to thwart prototype pollution attacks.
@briancavalier
Copy link
Member

I have not added unit tests to verify this, but with your consent, I can add a development dependency on SES and use that to lock down the primordials during tests.

Oops, I just noticed this! Can you say a bit more about what's involved in adding the SES dep and modifying the tests?

@kriskowal
Copy link
Contributor Author

I’ve pushed fixes for the remaining symbol-observables, and removed the commit that added a yarn.lock unintentionally.

@kriskowal
Copy link
Contributor Author

Oops, I just noticed this! Can you say a bit more about what's involved in adding the SES dep and modifying the tests?

I could do this in a follow-up. The purpose would be to block future changes that would introduce prototype pollution, either directly or through an upgraded third-party.

This would involve adding import "ses" and calling lockdown() or lockdown({errorTaming: "unsafe"}) at the head of your test runner, or creating a second entry point to your tests that locks down and runs the tests again under those conditions.

@kriskowal
Copy link
Contributor Author

@briancavalier I believe this is once again ready for review. Please let me know if this is a satisfactory change.

Copy link
Collaborator

@Frikki Frikki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@briancavalier briancavalier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, @kriskowal. Thank you!

@briancavalier
Copy link
Member

@kriskowal

This would involve adding import "ses" and calling lockdown() or lockdown({errorTaming: "unsafe"}) at the head of your test runner, or creating a second entry point to your tests that locks down and runs the tests again under those conditions.

I like the idea of a second test entrypoint if that's relatively simple. Would you be up for opening a PR for that so we can see what it looks like?

BTW, have you looked at @most/core? It's the new version of the core ideas of most, with a primarily functions-only API. If we're able to add SES testing in a simple way here, I could imagine adding it there as well if the other maintainers are open to it.

Thanks again!

@briancavalier briancavalier merged commit a398425 into cujojs:master Oct 5, 2020
@kriskowal
Copy link
Contributor Author

Thank you! I will watch for the next release.

@briancavalier
Copy link
Member

@kriskowal We just published v1.9.0 with this change. Sorry for the delay. We had to deal with some quirks of most's antiquated build setup.

@kriskowal
Copy link
Contributor Author

Thank you @briancavalier. You have my sympathies. I have a number of projects with a crusty CI setup that hasn’t run in years. I’ll let you know if I run into integration trouble rolling this up.

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.

None yet

3 participants