Disable loading of workspace configs by default and make it configurable

[the-dipsy]

Makes loading of workspace-local config.toml and languages.toml files optional and disables it by default to protect against vulnerability to arbitrary code execution when running helix in an untrusted directory. Should fix #9514 and alleviate #2697.

[archseer]

This is a quick fix but not the one I'm intending.

• The editor should hold an allow list file that contains a list of allowed directories followed by a hash of the file.
• We cross reference this list when trying to load a local config. If a config is detected but not on the list a status message should appear
• :allow would append the file + it's hash to the allowlist

[the-mikedavis]

I'm not sure if we will want this behavior once we have workspace trust. Instead I think we will want the opposite: the ability to say that you never want to be prompted about workspace trust and never use workspace config. (So the default would be the opposite: that you do merge workspace configs but only when you trust the workspace.)

[the-mikedavis]

Let's not have separate configurations for workspace-config for general config and language config, it's just confusing. Let's only add it to Config and use that value to determine whether we should load workspace configs.

[the-dipsy]

How would you implement that with the current code structure? Loading the entire config a second time seems wasteful but the current control flow doesn't seem to make it trivial to pass a parameter loaded from the config.toml to the code that loads the languages.toml.

[the-mikedavis]

I think the only option is passing down a boolean parameter. IIRC we always load normal config before language config so it should be possible to read this only from config.toml

[archseer]

Additionally, workspace-config should not be in languages.toml.

[the-dipsy]

@the-mikedavis I'd be fine with your workspace trust concept once it is implemented and if the prompt is absolutely clear about the risks. I'd really like the ability to activate language servers and loading of workspace configs separately though.

[pascalkuthe]

I'd really like the ability to activate language servers and loading of workspace configs separately though.

that ssesm like security theater to me. VSCode doesn't do that either. If you trust the LSP you trust the config. The other way around you can disable LSP in your workspace config.

[the-dipsy]

@archseer There is one workspace-config = true for config.toml and one for languages.toml. The current code structure makes it way easier to implement this way and I think it makes sense too 🤷

[the-dipsy]

@pascalkuthe It seems entirely reasonable to me to have language servers statically analyze code, provide snippets etc. without opening up the gates for ACE 😳 I'm not a heavy user of language servers but this is what GPT-4 says about it:

User: How common is unprotected execution of code from currently edited code repositories in language servers used in IDEs?
GPT-4: Unprotected execution of code from currently edited repositories in language servers is not common and is generally considered a security risk. Language servers typically provide features like code analysis, linting, autocomplete, and refactoring. They parse the code and understand its structure, but they do not execute the actual code being edited. When execution is necessary, for example, during debugging sessions, it is done in a controlled environment, often with explicit user consent and understanding of the risks.

I'd assume that a lot of people who switch to a small-code-base editor that doesn't rely on plugins, advertises to be usable over SSH, and is implemented in a security-driven language expect it to follow a security-conscious approach and configurability.

[the-dipsy]

Can other users of VSCode confirm that enabling language servers is coupled to loading project local configurations and executing code from them? I've never really used the thing but that'd seem like a no-go to me.

[pascalkuthe]

gpt is not a reliable source for anything and as usual provides a wordsoup without substance.

The majority of languageserver will execute arbitrary code which is why vscode disables them by default. For example rust-analyzer will execute all "build.rs" files and procedural macros (yes without any sandboxing). They often even have their own config files which they read from the project from which they can and do execute commands.

[the-dipsy]

@pascalkuthe Certainly no proof and unreliable but often decent at reiterating and summarizing information. I know that rust does this but it therefore is also pretty much the first thing that comes up when putting "language server" and "arbitrary code execution" into a search engine. I don't have a reliable source that says most LSPs don't do this. Do you have a source for the opposite? Jedi apparently is one explicit counter-example. In any case, the fact that there are plenty of ways to use the language server protocol without arbitrary code execution warrants an option to use them that way IMO.

[the-mikedavis]

I'm not keen to accept this as-is: we usually do not accept "bandaid" fixes that work around missing features, especially when we would have to remove them or make breaking changes to them later as we would with workspace trust. IMO this is not a very serious threat and seems mostly theoretical so it would be an overreaction to hot-fix it that leaves us with annoying defaults, a lot like git's safe.directory response to cve-2022-24765 (reiterating what I was saying here).

But I think this PR could be a really good base for the changes we need for workspace trust and could be merged before that is introduced with a small change: let's switch the workspace_config: bool option out for an enum

#[derive(Default)]
pub enum LoadWorkspaceConfig {
	Always,
    #[default]
    Never,
}

which you would write in config as "always" or "never". Workspace trust would add a Trust variant that should become the default. That way you can decide what behavior you want: always or never load workspace configs, and the meaning of these options doesn't change at all once workspace trust is added.

[archseer]

I pushed my set of changes based on some comments above here: 5ed223f

[the-dipsy]

@archseer Oh, shoot. I refactored a bunch already. Is my version okay for you?