Skip to content
This repository has been archived by the owner on Nov 4, 2021. It is now read-only.

Allow whitelisted imports in babel #284

Merged
merged 7 commits into from
Mar 29, 2021
Merged

Allow whitelisted imports in babel #284

merged 7 commits into from
Mar 29, 2021

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Mar 25, 2021

Changes

  • PRs over issues, so here's a babel plugin to replace import anything from 'anywhere' with const anything = __pluginHostImports['anywhere']['anything']

Checklist

  • Updated Settings section in README.md, if settings are affected
  • Jest tests

@mariusandra mariusandra requested review from macobo and Twixes March 25, 2021 11:40
@mariusandra mariusandra mentioned this pull request Mar 25, 2021
1 task
@macobo
Copy link
Contributor

macobo commented Mar 25, 2021

This approach has its own set of versioning issues - basically it's guaranteed someone will be caught out by us using a different library version from what they have specified in package.json.


Looking at the implementation: So if I did import * from 'random-library' this would silently succeed until used?

Might a better approach be to throw during initialization as this should make loadPlugins fail.

Also we should have tests for running this style of plugin.

@mariusandra
Copy link
Collaborator Author

Addressing the feedback:

  • Just like segment, we need to publish the list of versions that are installed. We'd have to do this anyway if the package is in a global or in meta.
  • This will throw unless you import from a real library
  • This does throw when initializing, there's even a test to catch this.
  • For running tests with this type of plugin, I just added to vm.test.ts one test that imports and then uses fetch

@@ -15,7 +16,11 @@ export async function createPluginConfigVM(
pluginConfig: PluginConfig, // NB! might have team_id = 0
indexJs: string
): Promise<PluginConfigVMReponse> {
const transformedCode = transformCode(indexJs, server)
const imports = {
'node-fetch': { default: fetch },
Copy link
Member

Choose a reason for hiding this comment

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

Possibly dumb question: by providing fetch outright, will nobody be able to access some properties in a way that affects other VMs (e.g. redefine fetch.isRedirect)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely not a dumb question. VM2 should wrap everything in proxies. Worth testing it out though. This PR won't really change anything security-wise though, so might be worth tackling after.

Copy link
Contributor

@macobo macobo left a comment

Choose a reason for hiding this comment

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

Implementation lgtm! :)

@mariusandra
Copy link
Collaborator Author

Thanks! Just some flaky tests being flaky here. I'll run them one more time (have to -> merged with master) and if it's just the pisinca.destroy line that fails, I'll merge this in.

@mariusandra mariusandra merged commit d568d30 into master Mar 29, 2021
@mariusandra mariusandra deleted the babel-imports branch March 29, 2021 09:25
fuziontech pushed a commit to PostHog/posthog that referenced this pull request Oct 12, 2021
…rver#284)

* Allow whitelisted imports in babel

* add test for fetching via `import`

* Reword whitelisted → provided

* also support "require" and handle defaults nodejs style

* support requires

* fix test

Co-authored-by: Michael Matloka <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants