Security: don't use an install script to download the binary

[mjroeleveld]

The usage of an install script is a vulnerability issue. It downloads an unsigned binary to the executing machine, which opens the way for potentially malicious code to be unintendedly downloaded. The NPM package should contain the binary such that it's there when installed and the install script can be omitted. This will make sure that an install of a locked version of the package will always result in the same artifact, which also helps with caching.

[kaiyoma]

We've also run into issues with the install script. The binary fails to download sometimes, which either causes our CI to fail during yarn install or causes our webpack build to fail with this:

ERROR in Sentry CLI Plugin: spawn .../node_modules/@sentry/cli/sentry-cli ENOENT

(It's a bit troubling how many npm packages connect to a non-npm URL directly and download mysterious things during package installation.)

[kaiyoma]

Another issue, if I'm reading scripts/install.js correctly, is that the download is not verified against a checksum. Why this is important: we're dealing with some network issues that are potentially causing corrupted/truncated downloads, but this sentry-cli install script isn't properly detecting those situations, leading to the error above (a missing file that should be there).

[sehcheese]

@kaiyoma We're having a similar issue with our CI where we get intermittent errors of

ERROR in Sentry CLI Plugin: spawn .../node_modules/@sentry/cli/sentry-cli ENOENT

It does seem that the ideal would be a prebuilt binary, both from a security and convenience standpoint. In the meantime, how have you worked around the CI error?

[kaiyoma]

@sehcheese We ended up creating a local binary/library cache on one of our internal file servers and populating it with the versions that we use. Sentry provides a mechanism to specify a custom URL to the sentry-cli binary:

https://docs.sentry.io/product/cli/installation/#downloading-from-a-custom-source

In our CI, our package installation looks like this now:

SENTRYCLI_CDNURL="http://cache/sentry-cli" yarn install --frozen-lockfile

(where cache is the network name of our file server)

[lobsterkatie]

I've been looking into this, and I believe the reason we've gone for the download-in-postinstall-script approach is in order to be able to match the installed binary to the architecture on which it's running.

It turns out esbuild had the exact same problem, which they ended up solving using optional dependencies. If you read that whole thread, you'll see that there are pros and cons to this approach. The biggest one of those cons (yarn and npm both downloading every binary, for every platform) has been fixed in the latest versions of both (see yarn's fix and npm's fix), and so I think it's a worthwhile strategy to for us to think about pursuing.

(That said, in the interests of full disclosure, with the holidays coming up it's unlikely this will come up for consideration before the new year.)

[github-actions]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[mjroeleveld]

This issue should remain open.

[kamilogorek]

Agree, just added the bot, so it went through everything :)

[kamilogorek]

I went through all the linked issues of other repositories, and I believe there is indeed not a single perfect solution for this problem.

The main issue is that this CLI is used across various other projects indirectly, be it Android, Xcode, Fastlane, Maven, Gradle to name a few. And only recent versions of npm/yarn are doing everything correctly, no noisy logs, no fetching of unnecessary platforms data, etc.

Also migrating to optionalDependencies solution is making the whole releasing process quite more complex.
We'd need to release N packages (for every platform, 9 of them currently, more to come) every single time, update all GH actions, transform this rust repo into js monorepo, so on and so forth. And our internal registry would need to keep track of these additional releases, which is not ideal.

For now, I'd opt for adding checksum validation (security measure) and upgrading our install script to allow for a better and faster diagnosis of what went wrong and how to possibly fix the given problem (ux measure).
The checksum validation is implemented in a way, where hashes are generated during the GH action run, and they are stored directly inside the published npm package. This means, that in order for the binary to be invalid, yet installed, both npm and our CDN would need to be compromised.

Installing 1.73.0 in verbose mode, will now prompt you with:

Checksum verification has been already added in the latest version - #1123
And I'll work on adding more diagnostics for the install script next.

I'm as always open for the feedback :)

[kamilogorek]

After adding #1129 it's now also possible to use the local binary, which is installed in any way users choose to. This, and checksum validation should be enough of an upgrade, for now, so I'm going to close this issue.
Again, if anyone has any feedback and ideas around improving this, feel free to ping me.