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

Prefer using a platform package over postinstalls #1217

Open
arcanis opened this issue Jun 17, 2023 · 3 comments
Open

Prefer using a platform package over postinstalls #1217

arcanis opened this issue Jun 17, 2023 · 3 comments

Comments

@arcanis
Copy link

arcanis commented Jun 17, 2023

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 [BUG] Config environment variable forwarding breaks with false config values npm/cli#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: evanw/esbuild#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:

  • Create a set of @supabase/cli-<platform> package, each one with the relevant bin for the given platform
  • Each of those packages should set the os, cpu, and possibly libc fields from the package.json accordingly
  • In the supabase package, remove the postinstall script and instead list the platform packages in the optionalDependencies field.
  • Finally, the bin field of supabase should point to a JS script that would check which platform package is installed, and execute its accompanying native bin using child_process.spawn.

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
Copy link
Contributor

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
Copy link
Author

arcanis commented Jun 17, 2023

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:

  • Your postinstall script of course can't respect the networkEnabled configuration
  • It's also hardcoded to GitHub, so cannot be pulled from an internal mirror
  • More generally, installing the project requires a network access (bad for building docker images)

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
Copy link

noppa commented Sep 30, 2023

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.

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

No branches or pull requests

3 participants