supabase / cli

Supabase CLI. Manage postgres migrations, run Supabase locally, deploy edge functions. Postgres backups. Generating types from your database schema.
https://supabase.com/docs/reference/cli/about
MIT License
1.07k stars 209 forks source link

Prefer using a platform package over postinstalls #1217

Open arcanis opened 1 year ago

arcanis commented 1 year ago

Is your feature request related to a problem? Please describe. The Supabase package currently runs a postinstall script to download the binary for the current platform. This strategy should be avoided: postinstall scripts are dangerous, but also very inconvenient (they are unstable, slow, don't work well with caches, disable optimizations in package managers).

  • Not all of npm's settings can be forwarded due to npm bugs such as https://github.com/npm/cli/issues/2284, and npm has said these bugs will never be fixed.
  • Some people have configured their network environments such that downloading from https://registry.npmjs.org/ will hang instead of either succeeding or failing.
  • The installed package was broken if you used npm --ignore-scripts because then the post-install script wasn't run. Some people enable this option so that malicious packages must be run first before being able to do malicious stuff.

For more details, see this thread, which is fairly detailed: https://github.com/evanw/esbuild/pull/1621.

Describe the solution you'd like Instead, you should use the same strategy as tools like esbuild or swc. It's been documented in detail in the thread I mentioned, but to give you an idea, you'd have to do the following:

As a result, package managers will only download the single package

Additional context I wonder if perhaps Yarn could implement something to make publishing such packages easier. You don't use Yarn though, so I don't know if that would help you.

sweatybridge commented 1 year ago

Not all of npm's settings can be forwarded

Since the logic of postinstall script is kept as simple as possible, we will probably never need to forward these buggy config values via npm.

Some people have configured their network environments

In our case, the CLI binaries are hosted on GitHub releases instead of npm registry. This means the postinstall script doesn't depend on installing another nested OS specific package, hence avoiding the network configuration issue.

The installed package was broken if you used npm --ignore-scripts

Unfortunately there's no workaround to this as long as we rely on postinstall script. However, I would argue this is a minor issue because our postinstall script is open source, version controlled, and very readable due to its simplicity. There are other popular installation methods which execute a bash script from a https source which seems way more suspicious.

I wonder if perhaps Yarn could implement something to make publishing such packages easier. You don't use Yarn though, so I don't know if that would help you.

We started using postinstall script precisely to support yarn because it's very popular among our users. Since node itself is supposed to provide cross platform compatibilities, I doubt it will be a high priority for package managers in the node ecosystem to support these os, cpu, and libc fields uniformly. A perfect alternative would be OS package managers like brew or scoop.

Also, we have introduced CI checks for these installation scripts to ensure they don't break. It would be unnecessarily complex to publish OS specific npm packages and maintain the same level of integrity via CI.

arcanis commented 1 year ago

In our case, the CLI binaries are hosted on GitHub releases instead of npm registry [...] hence avoiding the network configuration issue.

You'd think that, but that's not quite true. For example:

I would argue this is a minor issue because our postinstall script is open source, version controlled, and very readable due to its simplicity.

That's the case of all the other projects who used to do the same (a lot) / still do (rarer). The security problem isn't really your script in itself, but that it makes it more difficult for projects to adopt safer practices. In other words, users of the supabase package cannot use --ignore-scripts to secure their projects - that's unfortunate.

Since node itself is supposed to provide cross platform compatibilities, I doubt it will be a high priority for package managers in the node ecosystem to support these os, cpu, and libc fields uniformly.

They already do. I should know it, I maintain Yarn! 🙂

As I mentioned, both Esbuild and SWC, and as such Next.js, already follow this pattern - we thus made sure that all package managers would support this pattern. Both npm, pnpm, & Yarn do (and given how widespread are those packages in the ecosystem, we have tests to prevent regressions).

Also, we have introduced CI checks for these installation scripts to ensure they don't break. It would be unnecessarily complex to publish OS specific npm packages and maintain the same level of integrity via CI.

I understand it'd be a change compared to how you currently publish things. In my opinion, it's still something I'd recommend you to consider: while it usually works, I never saw postinstall scripts that didn't lead to frustration for users.

noppa commented 1 year ago

It would be nice if supabase would at least be installed with --ignore-scripts in such a way that running npx supabase would then give something a bit more descriptive than the current error, sh: line 1: supabase: command not found.

Ignoring scripts in a project or user-level config is a good default to use. These days it breaks so few packages that I had forgotten the whole thing, and that error came to be a bit of a head-scratcher.