haskell/vscode-haskell

Allow using different versions of HLS for different workspaces with `serverExecutablePath` at machine scope

anka-213 opened this issue · 13 comments

anka-213 commented

This is especially important when writing patches for HLS itself, since I would want to use a stable version for developing and my unstable version for testing.

It was possible prior to 1.3.0, but the ability was removed (10676ee) for security reasons.

Is there any way to get back that ability without reintroducing those security issues?

jneira commented

mmm I was afraid of breaking some workflow when removing it and that is the case
the solution I can think of for this concrete case is add an additional machine path (dev version or something alike) and then choose per workspace if you want the normal or dev version.

anka-213 commented

Yeah, that would work for my use-case.

If we want more flexibility, one option could be to specify a list of allowed paths as a machine variable and then the workspace can only use paths from that list.

This is somewhat similar to what vs code did for their terminal option, but with a fancier UI (and using workspace local _storageService instead of a setting): microsoft/vscode#19758 (comment)

anka-213 commented

How does your workflow work? Maybe I could switch workflow as a workaround?

jneira commented

I use always a dev version on PATH (maybe not the latest one) , only switch to release when trying to reproduce issues.
I think the test suite uses the executable build locally inside dist-newstyle as part of cabal test (cause hls is listed in tool-build-depends)

jneira commented

If we want more flexibility, one option could be to specify a list of allowed paths as a machine variable and then the workspace can only use paths from that list.

That would be ideal, will take a look (pr's welcome, as always)

marcin-rzeznicki commented

This change is a total disaster - sorry for the harsh words, but I can't imagine how you could have come to the conclusion that it doesn't break any (real) workflow. Also, the security argument is very weak. No-one runs VS Code as root, so even if someone can sneakily introduce something to your workspace file (which is quite improbable in itself), what damage is to be expected? It is also solved on a completely wrong level - if someone is really afraid of such a scenario, he solves it by running the host process in a sandbox. Please revert this

Case 1 - with nix

We have several haskell projects at work and some of them work with NIX - each project has a slightly different layout and we use our own script like exec nix run -c haskell-language-server" $ @ " to set everything up. It relied on availability to replace it for each workspace: the path to the script may vary depending on the project + you only need it for some projects. This is broken now

Case 2 - STACK_YAML

You cannot start hls with various STACK_YAML right now - this causes all kinds of troubles and is very common (and you have to be able to override it per workspace). I believe there is no way to achieve this other than setting STACK_YAML=... /path/to/hls in a script

jneira commented

@marcin-rzeznicki
Sorry to hear that change would suppose so many problems for you.
Agree in revert the change, at least for the next release.

That said, i think it is a change that has to be done sooner or later:

  • Security i a delicate question and i think we have the responsability to make the software as safe as we can, without delegating it to the final user and letting the secure configuration be opt-in . Users can run vscode as root (i run it as admin in windows, an it is not uncommon!), dont run it in a sandbox and still our extension must be secure imho.

  • It seems to me that setting the executable path per workspace is a workaround to issues that should be fixed in the extension, the lsp server or even upstream. Maintaining them and using wrapper scripts must be cumbersome and error prone. But i understand that things has to be done and fix things upstream can be an endless effort.

cradle:
  stack:
    stackYaml: "./stack-8.8.3.yaml"

  • But it would be interesting know why you need an alternative stack.yaml, when ideally hls should work with the default one.

  • I am not a nix user and cant help much with the nix setup (maybe @fendor or @pepeiborra?)

  • Taking in account above considerations a plan could be:

    • Revert the change and set the scope of the config option to workspace again
    • Mark that config option as deprecated, warning about the security issue in the description of the option and the README. No hard deadline to remove it for now but warn about the fact it will be removed.
    • Try to recover the uses cases which will be broken changing the scope of the option. Some possible ways:
      • specify a list of allowed paths as a machine variable and then the workspace can only use paths from that list, as suggested by @anka-213 (to investigate)
      • let the paths be relative to workspace root
      • ???

what do you think about?

mpickering commented

I am almost always setting per-project wrapper scripts which pass additional options to hls. This change would be quite inconvenient for me as well.

marcin-rzeznicki commented

@jneira Thanks a lot for a detailed reply, much appreciated.

Sorry to hear that change would suppose so many problems for you.

No problem and sorry for the too harsh comment.

Agree in revert the change, at least for the next release.

That said, i think it is a change that has to be done sooner or later:

Many thanks for taking this into account, this is the verdict of Solomon IMO, which does not neglect the issues and at the same time gives everyone time to prepare a solution 👍

Security i a delicate question and i think we have the responsability to make the software as safe as we can, without delegating it to the final user and letting the secure configuration be opt-in . Users can run vscode as root (i run it as admin in windows, an it is not uncommon!), dont run it in a sandbox and still our extension must be secure imho.

Ah, okay, I didn't think about Windows at all - sorry, my bad, I was too focused on the typical Linux setup.

It seems to me that setting the executable path per workspace is a workaround to issues that should be fixed in the extension, the lsp server or even upstream. Maintaining them and using wrapper scripts must be cumbersome and error prone. But i understand that things has to be done and fix things upstream can be an endless effort.

That's my thinking as well. Overalll I think it all boils down to setting up the configuration for various tools that are needed to build the project (at least to the point where HLS can gather all the info it needs to work correctly - but in my experience HLS generally needs to have a building project). In this category we have my use case of STACK_YAML - that's not HLS concern per se, but I didn't know any other way to do it (before your great tip) so maybe it's just a matter of documenting what you can already do? Nix seems to be a bigger obstacle as it "controls" all the paths and components (GHC, stack and even HLS in my setup) - but I'm no expert on this - maybe some special "configured via nix" checkbox which has the effect of delegating everything to nix is enough?

Re using alternative stack.yaml: you can set the path to an alternative yaml file in the hie.yaml configuration: https://github.com/mpickering/hie-bios#stack

Great tip! Thank you, I didn't know that, I'll try it out as soon as possible

But it would be interesting know why you need an alternative stack.yaml, when ideally hls should work with the default one.

Long story short - I use it (perhaps wrongly) for a project that links to native libraries. I have one "default" config which just uses the system defaults for linking which is checked in git, and my own local machine-specific overrides with something like

extra-include-dirs:
- /usr/local/include
extra-lib-dirs:
- /usr/local/lib
ghc-options:
  "$locals": -optl=-Wl,-rpath,/usr/local/lib

This way I can have, say, a newer version of the library installed somewhere (/usr/local/lib in this case) on my system and link to it during development.

I am not a nix user and cant help much with the nix setup (maybe @fendor or @pepeiborra?)

I'll be happy to learn how to do it better. The thing with NIX is that I only use it at work, and all my Haskell colleagues have used this setup for ages - you could say it's a legacy, I am not saying it is perfectly logical, but it has worked fine for us so I didn't give it much thought. If there is a better way, I'll be happy to adapt.

Taking in account above considerations a plan could be:

  * Revert the change and set the scope of the config option to `workspace` again
  * Mark that config option as deprecated, warning about the security issue in the description of the option and the README. No hard deadline to remove it for now but warn about the fact it will be removed.
  * Try to recover the uses cases which will be broken changing the scope of the option. Some possible ways:
    
    * specify a list of allowed paths as a machine variable and then the workspace can only use paths from that list, as suggested by @anka-213 (to investigate)
    * let the paths be relative to workspace root
    * ???

what do you think about?

Thank you - I am more than happy with this outcome. For the use cases I can think of, everything you proposed ⬆️ seems to work. One more thing comes to mind - a kind of map workspace -> path (user-level) where the plugin can add a key that overrides the HLS path for the workspace identified by that key (absolute path to workspace - say). I think that's how plug-ins like GitHub PRs operate - this one has a user-level map host -> authorization token.

  • 👍1
jneira commented

A new version 1.4.0 with the change reverted has been published in the vscode marketplace. The publishing is already shown in the marketplace: https://marketplace.visualstudio.com/items?itemName=haskell.haskell
Sorry all for the inconveniences, any help to design or implement an alternative to the actual option scope will be very welcomed.
The option already takes in account variables like $HOME, ~ and more importantly (from the readme):

${workspaceFolder} and ${workspaceRoot} will expand into your current project root.

So one piece to help set a version for workspace already exists. You could set a common path for all workspaces: ${workspaceFolder}/ws-hls-wrapper and add different wrapper per workspace under the same name. Not ideal but workable.

I think that combined with some mechanism to being able to set several executable paths would cover most workflows.

jneira commented

Related: #302

fendor commented

I consider this to be resolved, scope is now machine-overridable in accordance to most other language server plugins.
It should still allow us to use overwrite settings per workspace.

If we overlooked something, feel free to reopen.

  • 👍1
marcin-rzeznicki commented

Yes, I think it is solved. Thanks!