workspace trust: v2#15857
Conversation
Simplifies the previous implementation: user is prompted to either trust or deny, esc skips the prompt for now. Less choice fatigue and matches other implementations. Unlike other editors, we follow direnv implementation: .helix/ contents are hashed and a change to the config requires the user to re-approve. This avoids attack vectors where the user trusts the workspace but pulls in an untrusted PR branch. To make the hashing safe I had to pull in sha2, we could in practice use sha1 that's already in the dependency tree via gix, but that seems like a bad cryptographic choice in 2026. The on-disk file layout changed to use a directory with a file per filepath, this avoids concurrent conflicts if you have multiple different editor instances writing to the trust config. If the workspace is running in untrusted mode and trusting would make a meaningful change (local config exists, or in level=none a language server would start), then we show an indicator on the bottom corner of the editor. There's still a problem with git config: smudge filters still get executed and disabling that in untrusted mode would break autocrlf diffs. Since we only leverage gix for diffing, I'm hoping upstream could consider a patch for this.
| deliberate action than dismiss a dialog. | ||
|
|
||
| > [!WARNING] | ||
| > `level = "all"` is highly discouraged. It implicitly trusts every |
There was a problem hiding this comment.
I still think insecure is better naming so users are less tempted to set it
There was a problem hiding this comment.
Yeah I think we can just replace all with insecure, that seems clearer to me
| // Get the actual data that git would make out of the git object. | ||
| // This will apply the user's git config or attributes like crlf conversions. | ||
| // | ||
| // SECURITY: `filter.*.clean` / `filter.*.smudge` drivers in the repo `.git/config` are external |
There was a problem hiding this comment.
@Byron is there any config that could be added to gix to support this? We basically only use gix for diffing right now and the only reason we enable the filter pipeline is to support autocrlf. Ideally if we don't trust the workspace we'd still run the pipeline, but only with the builtin autocrlf filter.
Note, there is also #13133 |
Should get the lints CI green
|
This looks much better that my PR in every way, but it's a shame that 'Trust once' and setup of trust from config have to go. They do have their uses, and some people wanted them quite bad... |
| prompt = true | ||
| ``` | ||
|
|
||
| Language servers and debug adapters start automatically in every |
There was a problem hiding this comment.
| Language servers and debug adapters start automatically in every | |
| Language servers start automatically in every |
I think we need to manually kick off debug adapter servers, right? They're never started automatically iirc
| deliberate action than dismiss a dialog. | ||
|
|
||
| > [!WARNING] | ||
| > `level = "all"` is highly discouraged. It implicitly trusts every |
There was a problem hiding this comment.
Yeah I think we can just replace all with insecure, that seems clearer to me
|
I couldn't think of a case where I'd want trust once: either I trust the workspace or I don't. I can always trust then untrust at the end of the session if I have to. It's not a feature I could find on any other editor and I found having more choices in the prompt just made it more difficult when deciding between options. Most editors don't even allow dismissing the prompt to only temporarily ignore/deny. I feel similar about globbing, it seemed like a workaround when we were over-prompting -- per project .helix/ folders should be fairly rare. Globbing makes it easy to accidentally execute vulnerable code because you allowed ~/src, forgot about it and cloned a malicious repo in there. It also doesn't work well with the config hashing: it'd sidestep it altogether. Basically the idea is that the defaults should be reasonably secure and we should make it as hard as possible for someone to shoot themselves in the foot. |
You're making an assumption about other people's workflows, which I don't think is correct. I have many repos and many workstations. I also like to setup my workstations from scratch regularly, to make sure my setup scripts and backups are working smoothly. Without the ability to trust a set of repos via configuration, this workflow is a huge pain. I would be prompted all the time for my own repos.
Helix is a niche editor for power users. Let's not take that power away in order to optimize for people who will never use Helix anyway. If somebody trusts I would like to trust |
| | Key | Description | Default | | ||
| | --- | --- | --- | | ||
| | `level` | `"none"`: prompt for every workspace. `"servers"`: trust LSP and DAP launches but still gate local config and git. `"all"`: trust everything. | `"servers"` | | ||
| | `prompt` | Whether opening a file in an untrusted workspace pops a modal. The statusline `[⚠]` indicator is always shown either way. | `true` | | ||
|
|
||
| Example: | ||
|
|
||
| ```toml | ||
| [editor.workspace-trust] | ||
| # Start language servers automatically; still require :workspace-trust for | ||
| # .helix/config.toml and .helix/languages.toml. | ||
| level = "servers" | ||
| ``` |
There was a problem hiding this comment.
There is a lot packed into the descriptions, I wonder if moving the details to the example section would be better?
| | Key | Description | Default | | |
| | --- | --- | --- | | |
| | `level` | `"none"`: prompt for every workspace. `"servers"`: trust LSP and DAP launches but still gate local config and git. `"all"`: trust everything. | `"servers"` | | |
| | `prompt` | Whether opening a file in an untrusted workspace pops a modal. The statusline `[⚠]` indicator is always shown either way. | `true` | | |
| Example: | |
| ```toml | |
| [editor.workspace-trust] | |
| # Start language servers automatically; still require :workspace-trust for | |
| # .helix/config.toml and .helix/languages.toml. | |
| level = "servers" | |
| ``` | |
| | Key | Description | Default | | |
| | -------- | ---------------------------------------------------------------------- | ----------- | | |
| | `level` | The default level of trust for all workspace's. | `"servers"` | | |
| | `prompt` | Whether to show a modal when opening a file in an untrusted workspace. | `true` | | |
| Example: | |
| ```toml | |
| [editor.workspace-trust] | |
| # Even if `false`, the statusline `[⚠]` indicator is always shown. | |
| prompt = false | |
| # `"none"`: prompt for every workspace. | |
| # `"servers"`: trust LSP and DAP launches but still gate local config and git. | |
| # - still requires `:workspace-trust` for .helix/config.toml, .helix/languages.toml, etc. | |
| # `"insecure"`: trust everything. | |
| level = "none|servers|insecure" |
| let workspace = doc.workspace_root(); | ||
| let trust = self.workspace_trust.query(workspace, TrustQuery::Lsp); | ||
| if !trust.is_trusted() { |
There was a problem hiding this comment.
This reads a bit weird (trust.is_trusted). I wonder if we can have:
let workspace = self.workspace_trust.query(doc.workspace_root(), TrustQuery::Lsp);
if !workspace.is_trusted() {
return;
}Or maybe there is a way to encapsulate a lot of the logic in a Workspace struct which encapsulates these kinds of operations better?
Like Workspace::is_trusted_for(WorkspaceCapability::Lsp).
if !workspace.is_trusted_for(WorkspaceCapability::Lsp) {
return;
}| let trust_full = self | ||
| .workspace_trust | ||
| .query(doc.workspace_root(), TrustQuery::Git) | ||
| .is_trusted(); | ||
| if let Some(diff_base) = self.diff_providers.get_diff_base(&path, trust_full) { | ||
| doc.set_diff_base(diff_base); | ||
| } | ||
| doc.set_version_control_head(self.diff_providers.get_current_head_name(&path)); | ||
| doc.set_version_control_head( | ||
| self.diff_providers.get_current_head_name(&path, trust_full), | ||
| ); |
There was a problem hiding this comment.
Same as above, trust_full reads a bit weird. Looking at the call chain, this trust_full bool is pretty viral as well. Not sure if it would be easy to encapsulate these kinds of operations, but given that we want to treat workspace trust as first class, it might be good to try as much as possible?
Workspace::diff_base(/* path could be a field in the Workspace */) -> Option<_>, Workspace::current_head_name() -> Option<_>, etc.
If nothing else, I think a better name/switching from a bool to an enum would help readability.
| // We need to build a transient `WorkspaceTrust` just to ask whether the workspace is | ||
| // trusted enough to load its `.helix/config.toml`. The persisted-trust file on disk is the | ||
| // source of truth either way; this transient instance has an empty cache and is dropped | ||
| // after the check. |
There was a problem hiding this comment.
Hmm, I think this was already a potential issue, but if user opens up a document from workspace A, which has a .helix/config.toml, which has custom commands for better working for that workspace, like a :make, and also opens up a document from workspace B which might also have a :make custom command in its .helix/config.toml, considering that these are two different workspaces for two different documents, I think the expectation could be that :make would behave differently for the two workspaces. Throwing in trust levels with a flat merger could complicate how this happens.
I wonder if its time to try to wire up per workspace/document configs? This was already something I was thinking on, like if I am in a rust file and use :build I want to use cargo build, where if I am in a go file, I would want go build. This kind of granularity already kind of exists with the :format command, but I think we need to support this in general so that we can deal with the above case better.
Wonder if there are any other thoughts on this?
There was a problem hiding this comment.
I guess this is similar to task capability too:
If this endeavor was to go forward, then it should at least be kept extendable to something like this.
|
There is a UX detail I wanted to bring up. The muscle memory of not having it is so strong that the prompt, by me, is almost always clicked/cancelled accidentally. Having the prompt out of focus would entirely prevent it. But that seem like a solution for gui. Not sure if it can be translated here. |
|
I ran into a panic while using this. You can reproduce by running cd $(mktemp -d)
git init
GIT_DIR=.git hx newfile.txtor VISUAL=hx git --git-dir=.git commit --allow-emptyThis panics at https://github.com/GitoxideLabs/gitoxide/blob/eac50e1207e2549b23302c9faf595a420b9919fc/gix/src/open/repository.rs#L184 with the following backtrace: For context, I use the |
|
Looks like a bug in In other news, is the trust even working for git anymore? Now I have smudge filters apply with Edit: huh, looks like this is intentional (for now, at least) Lines 49 to 55 in 7fa938e |
|
I might be really stupid, but I can't figure out what you mean..... In my PR, it works pretty much how it should: CRLF filters do apply whether or not workspace is trusted, but smudge filters are executed only when trust is granted, isn't that what we want? Am I testing it wrong? I just can't find the problem in practice. I am not sure that you're right about how gix applies trust. Looking at |
|
Maybe I'm also misunderstanding the code but I think your PR only set gix trust level: that part is retained. But from what I understand, the autocrlf setting requires trusting the config and running the pipeline which also ends up running smudge filters. |
That’s correct. Everything in the filter pipeline runs, but it will ignore filters from untrusted configuration to avoid executing untrusted programs. |
Except it doesn't work. It certainly panics when GIT_DIR is set, but in my testing it just plainly doesn't work: smudge filters are executed whether or not workspace is trusted. I think this doesn't work because of this: this function ( We just need to use our own discovery function and that fixes both panic and execution of smudge filters. |
|
Just now I had some time to catch up on this PR properly. And without having read anything, it seems there are a couple of issues:
I'd want to avoid that and rather make |
Yes, it was ;), so this will be fixed with the next release. |
And this one has a fix as well. |
the-mikedavis
left a comment
There was a problem hiding this comment.
Overall looks great to me now. I've been using a few revisions behind this for a little while and mainly I like not being prompted for trust when it's not necessary. I have a blocking comment about additional dead code we can delete, but then I'd prefer merging this to iterating further in this PR. (Looks like there are lints to fix up too though.)
I think you mentioned it on #15319 but this is the main thing I'd consider blocking a long overdue (sorry 😓) release - I don't want to release a workspace trust feature that leads to decision fatigue. After driving nearly this change for a while I'm pretty comfortable with it.
There was a problem hiding this comment.
This reads well to me now 👍
|
@Byron always appreciate the quick patches in gix! Thank you! I'm going ahead and merging this (note that I restored the globbing config, but worded the docs to discourage use). We can land any further tweaks in follow ups, this already improves the current state. |
* workspace trust: v2 Simplifies the previous implementation: user is prompted to either trust or deny, esc skips the prompt for now. Less choice fatigue and matches other implementations. Unlike other editors, we follow direnv implementation: .helix/ contents are hashed and a change to the config requires the user to re-approve. This avoids attack vectors where the user trusts the workspace but pulls in an untrusted PR branch. To make the hashing safe I had to pull in sha2, we could in practice use sha1 that's already in the dependency tree via gix, but that seems like a bad cryptographic choice in 2026. The on-disk file layout changed to use a directory with a file per filepath, this avoids concurrent conflicts if you have multiple different editor instances writing to the trust config. If the workspace is running in untrusted mode and trusting would make a meaningful change (local config exists, or in level=none a language server would start), then we show an indicator on the bottom corner of the editor. There's still a problem with git config: smudge filters still get executed and disabling that in untrusted mode would break autocrlf diffs. Since we only leverage gix for diffing, I'm hoping upstream could consider a patch for this. * Address lints * Address clippy lint * Break doc links to private items Should get the lints CI green * fix: gix regression * Rename all to insecure, reintroduce globs * fix lint * Remove unused function --------- Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
* workspace trust: v2 Simplifies the previous implementation: user is prompted to either trust or deny, esc skips the prompt for now. Less choice fatigue and matches other implementations. Unlike other editors, we follow direnv implementation: .helix/ contents are hashed and a change to the config requires the user to re-approve. This avoids attack vectors where the user trusts the workspace but pulls in an untrusted PR branch. To make the hashing safe I had to pull in sha2, we could in practice use sha1 that's already in the dependency tree via gix, but that seems like a bad cryptographic choice in 2026. The on-disk file layout changed to use a directory with a file per filepath, this avoids concurrent conflicts if you have multiple different editor instances writing to the trust config. If the workspace is running in untrusted mode and trusting would make a meaningful change (local config exists, or in level=none a language server would start), then we show an indicator on the bottom corner of the editor. There's still a problem with git config: smudge filters still get executed and disabling that in untrusted mode would break autocrlf diffs. Since we only leverage gix for diffing, I'm hoping upstream could consider a patch for this. * Address lints * Address clippy lint * Break doc links to private items Should get the lints CI green * fix: gix regression * Rename all to insecure, reintroduce globs * fix lint * Remove unused function --------- Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
Simplifies the previous implementation: user is prompted to either trust or deny, esc skips the prompt for now. Less choice fatigue and matches other implementations.
Unlike other editors, we follow direnv implementation: .helix/ contents are hashed and a change to the config requires the user to re-approve. This avoids attack vectors where the user trusts the workspace but pulls in an untrusted PR branch. To make the hashing safe I had to pull in sha2, we could in practice use sha1 that's already in the dependency tree via gix, but that seems like a bad cryptographic choice in 2026.
The on-disk file layout changed to use a directory with a file per filepath, this avoids concurrent conflicts if you have multiple different editor instances writing to the trust config. (I also copied this from direnv.)
If the workspace is running in untrusted mode and trusting would make a meaningful change (local config exists, or in level=none a language server would start), then we show an indicator on the bottom corner of the editor.
There's still a problem with git config: smudge filters still get executed and disabling that in untrusted mode would break autocrlf diffs. Since we only leverage gix for diffing, I'm hoping upstream could consider a patch for this.