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

[WIP] Require extensions to request downloads #12703

Closed

Conversation

someone13574
Copy link
Contributor

@someone13574 someone13574 commented Jun 5, 2024

Closes #12589

This a WIP PR which will make extensions require user input before downloading binaries; with the aim of making Zed a bit more friendly for the security conscious. It still requires a fair bit of work to get working well though.

Current status:

  • Add async function called from the download function which creates a request notification.
  • Pass extension name to notification.
  • Add setting to dictate the behavior of this feature (ie. always ask (default), always allow, never allow).
  • Prevent repeat popup's due to lsp restart (not sure the best way to do with. Comments welcome)
  • Remember selection per-lsp.

Also not sure about the best way to handle when declines occur without changing the api. Currently it just gives an Err which causes the lsp to attempt a restart.

Release Notes:

  • Added notification for extensions requesting file downloads

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jun 5, 2024
@someone13574 someone13574 changed the title [WIP] Require extensions' to request downloads [WIP] Require extensions to request downloads Jun 5, 2024
@ConradIrwin
Copy link
Member

hey @someone13574, thanks for making a start on this!

@mikayla-maki and I were talking through how this should work and we think this approach puts too much friction in the way for users. Given that they just decided to install the Zig extension, we don't want to give them another "are you sure you want the Zig extension to work?" prompt.

It also seems most likely that you either want us to manage binaries or you don't.

We'd like to do this so that:

  • There's a single global setting for "allow_executable_downloads" that is respected by extensions (and by zed itself). This is true by default, but could be set to false by the user.
  • We ensure extensions to prefer the binary from $PATH by default (I'm not sure what's involved with this, cc @maxdeviant / @mrnugget – ideally we could do it by making our APIs enforce it, otherwise we can just do it by review policy).
  • We intercept any download requests from extensions when you have the setting set so we can show the user a notification like "For rust extension to work, you need to have rust-analyzer version X available in $PATH." when they try and edit a file in a broken state, so that we can help them make progress.

@ConradIrwin ConradIrwin closed this Jun 5, 2024
@ConradIrwin
Copy link
Member

(we're not planning to work on this immediately, so if you wanted to take this on; that would be appreciated!)

@someone13574
Copy link
Contributor Author

someone13574 commented Jun 6, 2024

  • We intercept any download requests from extensions when you have the setting set so we can show the user a notification like "For rust extension to work, you need to have rust-analyzer version X available in $PATH." when they try and edit a file in a broken state, so that we can help them make progress.

How open would you be to integrating package managers to this as well? One approach would be a env variable containing a search command, which runs whatever command that distro uses for their "provides" command, and then extracts the package name. Then another env variable could say for the format the command. So instead of saying you need in the path, it would say you need it in path and can install it with xyz. The other way would be putting the logic in rust, but then it needs to live in your repo instead of wherever the package is built from.

@ConradIrwin
Copy link
Member

I think it'd be nice to be as helpful as possible. Although I think a few end users will enable this setting, I imagine the primary use-case is distributors.

I think it'd be fine to use a static string for this for a v1 (like the ZED_UPDATE_EXPLANATION), but I'd be open to having some amount of custom logic.

Maybe the v1 notification ends up looking something like:

For the rust extension to work in this project you need to install rust-analyzer at version 2024-02-02 or greater.

You can try installing the needed tools through flatpak.

There's some tension here with rustup/pyenv/nvm/etc. which are used to manage development environments that require multiple versions to be installed simultaneously; so while it might be handy for packagers provide a "click here to install" button that spawns a task in the terminal, I want to be mindful that we're not guiding people down a path that won't actually solve their problem.

@someone13574
Copy link
Contributor Author

someone13574 commented Jun 6, 2024

@ConradIrwin How about this for an api? (Many breaking changes, but that would be made up for by being a lot simpler for extensions to use).

Essentially it would replace the github methods and the download method with one over-arching get_binary() function. The extension would optionally pass in a github repo, release tag (or latest), and asset name and the function would do the logic which most extensions do right now: check for a cached binary, check path, and download otherwise. If the file is downloaded, it returns an optional which the extension can then handle. Otherwise it returns none and uses the binary in PATH or whatever is cached. Additionally it would have proper handling for allow_executable_downloads and updating to newer releases.

It would standardize/enforce how binaries are gotten and cut down the amount of boilerplate each extension needs to implement.

extension_logic

@ConradIrwin
Copy link
Member

My gut sense is that it'd be better to have a slightly larger API surface that is easier to understand and debug, but I don't really know what extensions need or are using right now. @maxdeviant or @mrnugget do you have any input on the above?

@ConradIrwin
Copy link
Member

(also, worth a read through: #12418; seems like it's even less clear what we should do than I thought :D).

@mrnugget
Copy link
Member

mrnugget commented Jun 6, 2024

@maxdeviant or @mrnugget do you have any input on the above?

I wrote down quite a few usecases here:

#7902

Recommend reading at least the original issue. Also related: #7894 -- if we lean more on PATH we also need to put some more effort into how we handle process environments. Do have some ideas and happy to help, we just need to do it :)

Input on @someone13574's diagram:

  1. Users also want to explicitly configure the binary. That's already possible for rust-analyzer, gopls, and others.
  2. I think the PATH lookup has to happen before the Does a cached binary exist step, otherwise it won't work for many cases where people have a specific language setup in a specific project.

@mikayla-maki mikayla-maki mentioned this pull request Jul 9, 2024
6 tasks
@rowillia
Copy link
Contributor

@ConradIrwin / @mikayla-maki WDYT about supporting mirrors for binaries in addition to requiring permission / disabling their download?

Both nvm and pyenv have examples of supporting this:
https://github.com/nvm-sh/nvm/blob/master/README.md#use-a-mirror-of-node-binaries
https://github.com/pyenv/pyenv/blob/f17809c4b9db8d5c8108eae6b1890870fac481cf/plugins/python-build/README.md?plain=1#L156-L161

Similarly, while I totally understand why you wouldn't want to inherit the global npm configuration (https://github.com/zed-industries/zed/blob/main/crates/node_runtime/src/node_runtime.rs#L158), it'd be nice if we could provide registry-related settings like [always-auth](https://docs.npmjs.com/cli/v6/using-npm/config#always-auth) or [registry](https://docs.npmjs.com/cli/v6/using-npm/config#registry)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zed downloads NodeJS binary and npm packages from Internet without user’s consent
4 participants