Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Make Nuclide play well with projects using different versions of Flow #570

Open
nmote opened this issue Jul 1, 2016 · 10 comments
Open

Make Nuclide play well with projects using different versions of Flow #570

nmote opened this issue Jul 1, 2016 · 10 comments
Assignees

Comments

@nmote
Copy link
Contributor

nmote commented Jul 1, 2016

There have been lots of questions about this recently so I wanted to take the time to explain where we are.

The Problem

Many projects specify a Flow version in their .flowconfig file. This is useful because Flow upgrades usually introduce breaking changes, so projects can upgrade their Flow version at their own pace. Flow will refuse to start if its version does not match the version specified in the .flowconfig (Note: Nuclide will currently fail silently if this is the case -- I intend to have it pop up a message but I haven't gotten around to it yet).

This works fairly well within Facebook because we deploy a bunch of recent Flow binaries and call them through a wrapper script that first inspects the .flowconfig. So, you change the version in your .flowconfig and everything Just Works™️, even if you are working on projects that have different Flow versions. However, currently folks external to Facebook are expected to manage their own Flow versions. Without this wrapper script, there will typically be just one system-wide Flow version installed, so you are out of luck if you want to use different versions of Flow with different projects.

Potential Solutions

flow-bin

The Flow team publishes the flow-bin npm package every time they do a Flow release. You can use this to install Flow system-wide, or individual packages can add it as a devDependency to have their own version of Flow set. Then, you can just run the binary inside node_modules.

It would be trivial, from a technical perspective, to have Nuclide look inside node_modules for the copy of Flow installed by flow-bin.

Unfortunately, this is a glaring security vulnerability since Nuclide runs Flow all the time while you are browsing. So, just by downloading and browsing a malicious project, your machine will get completely owned. They could just have a repository with their code in node_modules/bin/flow or whatever the path is, and bam. Generally, people don't have the expectation that just browsing source code requires trust in the project they are looking at.

I'd be willing to implement a setting to enable this (with a scary description so people know the risk they are taking on), if there is demand, but that only works for people who know to ask for it. It would be better to fix this issue for everyone.

Distribute something like our internal wrapper script

When you install Flow, we could include a bunch of versions and have a central script that looks at the .flowconfig and decides which binary to actually run. Then, Flow externally would behave exactly like Flow internally and things would basically just work. No modifications to Nuclide would be necessary, and this also has the advantage (over the option below) that you can run Flow from outside of Nuclide, from the command line (useful for a variety of reasons, including debugging Nuclide/Flow issues). However it has the disadvantage that you still have to manually install Flow in addition to Nuclide

Bundle Flow with Nuclide

A few points about this:

  • We probably don't want to actually bundle the binaries with a Nuclide install, so it would probably download them on-demand
  • We also probably don't want to muck with your system when you install Nuclide, so this wouldn't make Flow available from the command line

Other than that, it's basically the same as above. Nuclide checks the .flowconfig for the right Flow version and then just runs it.

cc @zertosh @jeffmo @peterhal @koba04 @satya164

Twitter exchange that prompted this writeup: https://twitter.com/natmote/status/749009001329807361

@nmote nmote self-assigned this Jul 1, 2016
@koba04
Copy link

koba04 commented Jul 2, 2016

Thank you!

Unfortunately, this is a glaring security vulnerability since Nuclide runs Flow all the time while you are browsing. So, just by downloading and browsing a malicious project, your machine will get completely owned. They could just have a repository with their code in node_modules/bin/flow or whatever the path is, and bam.

I understand what you are concerned about.

I'd like to use flow with flow-bin installed as a devDependencies because of being dependencies explicitly.
I like the following style:

  • npm install
  • npm start
  • start development

But I understand a security issue for Nuclide supports this style.
It's a very difficult problem.

Also, I think some other editor plugins(ESLint etc..) have the same issue.

Distribute something like our internal wrapper script

👍 for this. It would be nice that the script is able to install(uninstall) flow easily.

We generally use different versions of languages and libraries by each projects.
So I think any solutions for supporting multiple flow versions which don't depend on Nuclide are important.

@satya164
Copy link

satya164 commented Jul 2, 2016

Thanks for the write up.

I'd be willing to implement a setting to enable this (with a scary description so people know the risk they are taking on), if there is demand, but that only works for people who know to ask for it. It would be better to fix this issue for everyone.

This is already how other plugins like ESLint work. So I think it should be fine to expect people to enable this explicitly.

Another solution is to show a banner before starting flow, and the user can enable flow from node_modules for that project knowingly.

Distribute something like our internal wrapper script

That sounds interesting. How does installing multiple flow versions work with the wrapper?

Bundle Flow with Nuclide

How will this work? Will Nuclide has to bundle every flow version available?

@zertosh
Copy link
Contributor

zertosh commented Jul 2, 2016

flow-bin and the wrapper script solve different problems, and we should support both - as in they work with Nuclide and the Flow team maintains them.

flow-bin

The security problem is easy to overcome. Nuclide can have a list of blessed checksums, that we regularly update as flow versions come out. Before running flow-bin/flow, we verify the checksum, and either refuse to run it if it doesn't match, or warn the user and ask for permission.

My problem with flow-bin is that it downloads the flow binary instead of shipping with it. This has has problems:

  • it doesn't use npm's proxy settings (so it complicates installs in envs like Sandcastle),
  • it doesn't install if you have ignore-scripts=true,
  • the deps it needs to download flow are larger than the flow bins themselves, so installs are fat and take longer than necessary.

IMHO, flow-bin should just package all the platform bins, and skip the download.

The wrapper script

I wanted to open source it but legal issues. I have it sitting in a private repo ready to be published to npm. It's more-or-less the script we used internally (before it supported semver), and a publishing script that downloads all the flow versions for all platforms before packaging. The idea was that you'd run npm -g install flow-versions, it would be a (currently) ~250mb install, but then you'd have a script in /usr/local/bin/flow that works in any setup.

The wrapper script is not for everyone (given how large it is). I see it as a compliment to flow-bin. Akin to having a global eslint install.

@satya164
Copy link

satya164 commented Jul 2, 2016

the deps it needs to download flow are larger than the flow bins themselves

sounds like bundling the binaries will be much better :)

I usually use a local npm caching tool like npm_lazy due to the long npm install times, and since flow-bin always downloads the binary, I can't use the cache for it.

@jeffmo
Copy link

jeffmo commented Jul 7, 2016

What if there were a published list of checksums for all the Flow binaries somewhere that Nuclide could trust?

The biggest downside would be that Nuclide would have to checksum the binary each time, but maybe it's worth seeing how much that actually matters in practice?

@jeffmo
Copy link

jeffmo commented Jul 7, 2016

(I just noticed that ^ this is basically what @zertosh already said... :p )

@nmote
Copy link
Contributor Author

nmote commented Jul 7, 2016

Nuclide could copy it to a temp directory and checksum it once. But yes, it's necessary to have flow-bin download everything up front for this to be possible. Otherwise we have to run flow-bin code in order to even download the binary, which means we have to checksum the entire contents of the node_modules/flow-bin directory. Which I guess is doable, though not palatable.

@satya164 regarding bundling Flow with Nuclide, we could either ship binaries for all platforms of all supported Flow versions (probably not doable), or download them on the fly. Since this code lives inside of Nuclide and not in the project being opened, there are not security concerns if Nuclide itself downloads it. However that does have the problems that @zertosh pointed out, such as not using npm's proxies.

Anyway I will start by implementing a setting that allows Nuclide to look for flow-bin and use the Flow from it. Unfortunately it cannot be the default, but it sounds like it will still add value. Right now I'm in the middle of upgrading Nuclide's own code to use Flow v0.28, but I will implement this setting after that.

ghost pushed a commit that referenced this issue Jul 19, 2016
Summary:
This makes the various parts easier to reason about and paves the way for the
setting to use the `flow-bin` copy of Flow (#570), since we can have a separate
path to Flow for each root.

One unpleasant side effect of this change is that we are now caching the
pathToFlow setting, when we weren't before. That means that after you update the
setting, it can take up to 30 seconds to take effect. I will explore options to
improve this but I don't think it should block this diff.

This kills a lot of code which I will remove in a separate diff.

Reviewed By: mostafaeweda

Differential Revision: D3555803

fbshipit-source-id: e56cae562ae8f5f522aba568fe867fbba751b80c
ghost pushed a commit that referenced this issue Jul 20, 2016
Summary:
As noted in the setting description and in #570, enabling this setting is a
security risk unless you fully trust all of the projects that you open. However,
it can be very useful if you are willing to only open projects you trust. It
can't be the default, but there has been enough demand for such a setting that
it was worth implementing.

Reviewed By: zertosh

Differential Revision: D3590454

fbshipit-source-id: e9b391459ccded64b98d976bf22b43ba28033910
ghost pushed a commit that referenced this issue Jul 20, 2016
Summary:
We want to support Flow outline view outside of Flow root, and even for untitled
files. Obviously, we have to use the system-wide Flow for that. But we also want
to support Flow outline view if the only Flow you have is a local `flow-bin`
installation (assuming you have the setting to use `flow-bin` checked).

See #570

Reviewed By: zertosh

Differential Revision: D3593787

fbshipit-source-id: 8e11b3156e9db379026b66650c904d61ddd7f493
@elyobo
Copy link

elyobo commented Nov 15, 2017

Is there any workaround available for this? It seems to be a show stopper for actually using this unless everything you work on uses the same global version of flow.

@mrienstra
Copy link

mrienstra commented Jan 16, 2018

Related: the current documentation regarding this topic is a little confusing.

E.g.
https://nuclide.io/docs/languages/flow/#installing-flow
doesn't explicitly discuss installing Flow globally, and links to
https://flow.org/en/docs/getting-started/#installing-flow
(should be https://flow.org/en/docs/install/ )
which says "Flow works best when installed per-project with explicit versioning rather than globally."

https://github.com/facebook/flow#installing-flow
also discourages global use.

As it is, it's easy for new users to end up going in circles for a bit.

At the very least maybe something should be added to https://nuclide.io/docs/help/troubleshooting/#flow-issues ?

@divergio
Copy link

divergio commented Dec 9, 2018

Bumping this issue because this documentation loop (mentioned by @mrienstra) still exists:
flow suggests installing locally, nuclide wants a global flow. Pretty confusing for new users.

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

No branches or pull requests

8 participants