Add workspace trust support#15177
Conversation
|
Now it does show a warning when a new file is open. I don't really like that the message disappears on a first keypress since that makes it too easy to miss if one is not familiar with the feature, but I think it is not hostile to unsuspecting user any more. |
|
Also see #14668. I haven't found the time lately but the |
|
Yeah, |
|
I thought about it a bit, and I think The way I see it, the default should be |
078a835 to
e2a8f8f
Compare
|
Sorry about big messy PR, but I think it does what it needs to now. Now, by default, it will prompt the user with There is a config option Notes:
But I think I am mostly happy with it now, so I'll wait for some input.
With |
This comment was marked as resolved.
This comment was marked as resolved.
b937531 to
5d85500
Compare
|
I've added an |
0869ce5 to
682a197
Compare
|
Alright, now both I have also added docs. I think this thing is really ready to review now |
| }, | ||
| }, | ||
| TypableCommand { | ||
| name: "workspace_trust", |
There was a problem hiding this comment.
- name: "workspace_trust",
+ name: "workspace-trust",typable commands in helix are kebab-case by convention and not snake_case.
There was a problem hiding this comment.
Thanks, I think it is fixed everywhere now
1f6a4c8 to
7d6a301
Compare
|
I finally applied the Select component to master so this PR can be rebased on those changes now, 76c23b3. It's nearly the same as it was in my branch but I added some horizontal padding to the message |
7d6a301 to
93a6bfa
Compare
|
Just FYI, but I see a warning when building current master with I only see this warning when building with nix, |
93a6bfa to
19c41bc
Compare
|
All right, I rebased it on top of |
|
Ah yeah, when building the package we use the latest stable if I remember correctly, so it shows warnings which aren't emitted by the MSRV. And the devShell is using the MSRV. I'll push some fixes for more recent rustc / clippy |
| match fs::read_to_string(workspace_trust_file()) { | ||
| Ok(workspace_trust_file) => { | ||
| for line in workspace_trust_file.split('\n') { | ||
| if !line.is_empty() { | ||
| let path = PathBuf::from(line); | ||
| trusted.insert(path); | ||
| } | ||
| } | ||
| } | ||
| Err(e) => log::error!("workspace file couldn't be read: {:?}", e), | ||
| }; |
There was a problem hiding this comment.
This pattern seems repeated multiple times
There was a problem hiding this comment.
I think it would be more efficient to read the file in a streaming way, returning a line iterator we can scan through
There was a problem hiding this comment.
Now load() is used only when there is a need to write changes to trust-related files. For checking the trust there are quick_query_workspace() and quick_query_workspace_with_explicit_untrust() functions now, they do basically what you've described.
There was a problem hiding this comment.
Fixed repeated pattern now too
There was a problem hiding this comment.
quick_query_workspace()andquick_query_workspace_with_explicit_untrust()functions now, they do basically what you've described.
Well, sort of. They both use read_to_string which blocks until the entire file has been loaded into memory. If the files were line-buffered (e.g. BufRead::lines) then the iterator can yield the lines immediately as they arrive from storage.
| } | ||
| } | ||
|
|
||
| fn write_untrust_to_file(&self) { |
There was a problem hiding this comment.
I think there should be just one trust file, and untrust only removes entries. Users should either opt to trust all workspaces, or selectively trust. Blacklisting specific entries is too error prone because if you forget you're vulnerable. Same with opening the workspace then calling :untrust on it: by that point it's already too late since the config got executed
There was a problem hiding this comment.
I went with only one trust file initially, and I added second blacklist file after I copied Select UI approach from Mike's branch since that has Not now and Never options, and for Never to work we need to parse an additional file.
When trust-selector = "simple" config option is set, it doesn't check that second file since it doesn't really need to ("simple" is basically implementation of your ideas from #2697 (comment)).
:workspace-untrust is more of a convenience feature that you would only use because it is easier than modifying trust file by hand. Every workspace is still not trusted until user says it is.
If we want to remove second file, we need to remove Never option from the default trust selector approach, and I am not sure yet that we want that
There was a problem hiding this comment.
I think there should just be two modes: insecure=true that trusts everything and the default where each workspace has to be explicitly appproved. This should match VSCode behaviour
There was a problem hiding this comment.
Yeah, you're right, I just changed it.
I wanted to have a middle ground between trusting everything and having to click through a pop-up, but Select is really good, I lived for a week with it and it doesn't get in the way.
b0ccafc to
ffb8195
Compare
|
I renamed |
c004ccb to
b84fe28
Compare
1bd9d84 to
07e4a44
Compare
52af224 to
20849a8
Compare
Tested - the path is the same on Linux and macOS
| @@ -0,0 +1,23 @@ | |||
| # Workspace trust | |||
|
|
|||
| Helix has a number of potentially dangerous features, namely LSP and ability to use local to workspace configurations. Those features can lead to unexpected code execution. To protect against code execution in dangerous contexts, Helix has a workspace trust protection, which will prevent these potentially dangerous features from running automatically. | |||
There was a problem hiding this comment.
Thank you so much for this PR!
I am not sure to understand the doc:
Will untrusted workspaces not have any LSP enabled (even if globally set) and not local config (which can contain LSP and language settings)?
Or are the global LSP always enabled?
I would suggest at the end:
Helix has a workspace trust protection, which will disable the following features:
- loading config the the workspace’s
.helixfolder (language matching, LSP…)- running the globally configured LSPs
There was a problem hiding this comment.
(Btw “ability to use local to workspace configurations” reads convoluted)
There was a problem hiding this comment.
Yeah, nothing can load local configs or start an LSP server/client until workspace is trusted. I will try to do something about the docs
There was a problem hiding this comment.
Yes, I find it clearer. Thanks!
Regarding “trust”, the child folders of trusted workspaces are also trusted, right?
So if I trust ~/work, every (sub-)subfolder in it will be automatically trusted, right?
There was a problem hiding this comment.
Probably not in the way you expect it to work.
This is the function that selects the workspace:
Line 265 in 253e619
If your ~/work is a git repository, then yeah. If not, then you'll have to trust each repository in it when needed.
Originally, I did exactly what you described: walking directory tree up and looking if any parent is trusted, but that is not only slower, but a giant footgun too: you can open a file in your /home, allow trust there, forget about it, and then basically everything you open will be trusted. IMO better design is to make prompt about trust as unobtrusive as possible, which I hope this PR achieves
There was a problem hiding this comment.
Ok, thanks for the detailed explanation!
I appreciate the vscode prompt, which offers a nice middle ground I think: the ability to trust one parent level (ie all direct children of ~/work could be marked as trusted, but sub-children must be trusted explicitly).
Note that I am already satisfied with this feature, and just want to give some (hopefully constructive) feedback ❤️
* Workspace trust: init * Apply suggestion from @archseer * Apply suggestion from @the-mikedavis Tested - the path is the same on Linux and macOS * Avoid logging when workspace trust files can't be read from NotFound --------- Co-authored-by: Blaž Hrastnik <blaz@mxxn.io> Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
|
I am making a PR to add "only once" trust option, does it makes sense? Just want to gauge the opinion before actually opening a PR. |
|
I think it would make sense to have an option that trusts the current config. We'd take a cryptographic hash of the config and store it in the trust file(s). That would prevent future In the wild I know https://github.com/direnv/direnv works like this. |
* Workspace trust: init * Apply suggestion from @archseer * Apply suggestion from @the-mikedavis Tested - the path is the same on Linux and macOS * Avoid logging when workspace trust files can't be read from NotFound --------- Co-authored-by: Blaž Hrastnik <blaz@mxxn.io> Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
Should fix #2697
This isn't quite ready to be pushed to main branch, but it does work (on my machine).
The biggest thing that it lacks is some kind of message that CWD is not in the list of trusted workspaces with an instruction on how to change that (I think that single prompt like "Current directory is not trusted; run ':workspace_trust' to enable all features' might be sufficient), but I couldn't figure out how to do that just yet.
Smaller flaws: