fix #966 by using fixed folders #1362
Conversation
fidgetingbits
requested review from
rntz,
pokey,
AndreasArvidsson and
knausj85
|
please don't merge until I do security review |
| home_dir = Path(os.path.expanduser("~")) | ||
| if app.platform == "linux" or app.platform == "mac": | ||
| return ( | ||
| Path(home_dir) / f".talon/.comms/{actions.user.command_server_directory()}" |
There was a problem hiding this comment.
I thought accessing a hidden folder was also a problem for snaps, no? Cc/ @auscompgeek
There was a problem hiding this comment.
This is true for most snaps by default iirc, but afaik vscode uses something called classic confinement, which has unrestricted access to files. This makes sense since it's an IDE and users would expected to be able to access anything as their user anyway. So I think we should be okay here (though I haven't tested with the snap yet).
|
Once everything is reviewed and ready to merge, we should do one final test on each platform, given the potential repercussions of this code breaking 😅 |
nriley
changed the title
| if app.platform == "linux" or app.platform == "mac": | ||
| return home_dir / ".talon" / " .comms" / directory_name | ||
| elif app.platform == "windows": | ||
| # subprocess.run(["attrib","+H","myfile.txt"],check=True) |
There was a problem hiding this comment.
This seems like it may have been some testing code?
There was a problem hiding this comment.
I left this in for someone with access to Windows in case they want to test using a hidden folder, but didn't uncomment it because I can't test if it works.
There was a problem hiding this comment.
@knausj85 any thoughts about that part? I'm happy to just delete the commented out bit if windows users don't care to add it
There was a problem hiding this comment.
I have no concerns with leaving this file unhidden. Thoughts on this @AndreasArvidsson ?
There was a problem hiding this comment.
fyi I'm not sure I want plugins making top-level directories under .talon, hidden or not
There was a problem hiding this comment.
Sounds good. Should we continue that discussion in the issue or here?
Personally I would prefer to keep it in the temp directory and agree with @rntz arguments in the issue that the extension should respect TMPDIR.
If that doesn't work for some technical reason we should probably just create a new directory in user home. eg ~/.rpc.
There was a problem hiding this comment.
I believe TMPDIR doesn't work as a solution because it can be different path in talon vs vscode. This can definitely be the case on nix, as noted here #966 (comment) and also unrelated to that plugin which I've run into (which I already forget where as I've been using this PR for ~ 1 year). I originally hardcoded /tmp in the PR, but then eventually we opted for a /home-based path, which unfortunately I now can't find the reason/comment for it seems.
But I think because of all the potential snap/flatpak/nix related issues mentioned it's likely just best to avoid hoping one of those tmpdir/runtime path solutions work everywhere? Pretty sure atm we don't have the capacity to actually test it.
Something like ~/.rpc is fine with me if that's what people like (I just want it fixed at this point :D), but imo it does seem a bit opaque and potentially collision prone vs some name that associates it with talon, which was why I chose to drop the folder in ~/.talon/... ~/.talon-rpc or similar maybe makes more sense?
There was a problem hiding this comment.
If someone on purpose has set a different environment variable in Talon and vscode they kind off asking for trouble. As long as we check the sources in the same order we should get the same result.
Year that is a real problem because we don't want to break it for existing users.
~/.talon-rpc would be fine with me, but I'm not sure the home directory is any more stable than the temp directory since the home directory can also be moved with environment variables.
There was a problem hiding this comment.
FWIW I agree if it were actually the user just wantonly changing environment variables, it shouldn't something that needs catering to. However, this isn't a case of a user choosing to change TMPDIR, it's being done somewhere behind the scenes, so it's not something they can just choose not to do.
In addition to the original bug filer, there were numerous (#966 (comment)) cases of it differing in practice, as well as in my systems and for some other Nix users. When it started breaking for me (which prompted finally creating the PR) it's certainly not from me explicitly changing TMPDIR myself. This comment #966 (comment) also implies this type of thing happens on windows.
In light of that, I don't see how keeping the order checking consistent is enough, unless you mean you have some other mechanism to ensure the paths they are checking in the same order are actually matching? Maybe I'm missing something.
There was a problem hiding this comment.
If we can't make the temp directory work reliably then we can't of course use that. I assume the home directory is more reliable?
|
Aside from the Path.home() bit which will be quick to fix, this is still mostly waiting on someone who uses Windows to test that it actually works as intended. Anyone have time to try? |
|
@fidgetingbits I will hopefully have time (about 2-6h) to try it on windows on Saturday and I can find more time after that. |
Hey @DonnerWolfBach thanks! Reminds me @saidelike recently said they could also test this on Windows. Easiest is to merge the PR into some temporary branch you want to build from: git clone https://github.com/pokey/command-server
cd command-server
git switch -c dev-build
git fetch origin pull/21/head:static-tmp-dir
git merge static-tmp-dir -m "Merge branch 'use-static-tmp-dir' into dev"Then you need to build and install. I do it on nix on both macOS and linux, so merge yet another PR so I can just run nix build. I do it like this: git fetch origin pull/22/head:nix-flake
git merge nix-flake -m "Merge branch 'nix-flake' into dev"
nix build
code --force --install-extension result/*.vsixIf you have a nix box and can do the above, I think the .vsix file will actually work on windows too. But, if you have to build the extension on windows, presumably you need to run the same yarn commands as what the flake PR does. But note that I also had to update Then the manual build will be something like this: yarn run compile
yarn vsce package --yarn -o ./command-server.vsix
code --force --install-extension ./command-server.vsix@pokey might be worth adding a small note to the command-server README about how to "officially" build it. I'll file an issue. |
| home_dir = Path(os.path.expanduser("~")) | ||
| directory_name = actions.user.command_server_directory() | ||
| if app.platform == "linux" or app.platform == "mac": | ||
| return home_dir / ".talon" / " .comms" / directory_name |
There was a problem hiding this comment.
Don't snaps prevent access to ~/.*?
There was a problem hiding this comment.
In the default containment profiles yes (I believe you need to request personal confinement to access dotfiles), but from what I read vscode should be okay due to it using classic confinement (which is effectively no confinement). See here from this same PR where I said similar: #1362 (comment)
@DonnerWolfBach said he was using VSCodium. I suspect since it's just an open source build of VSCode (?) and an IDE when packaged as a snap it would use the same classic confinement. Maybe if he's using the snap, he could confirm though.
From what I've seen they tend to grant classic confinement to all IDEs under the assumption they will be used to edit any files. For instance neovim snap also has classic confinement.
I think the only other option if we want to assume we need to communicate with a snaps with restricted confinement is /tmp, and the discussion on #966 people couldn't agree, and it seemed some (pokey and rntz at least iirc) were leaning towards using $HOME over /tmp, so I just used $HOME for the sake of getting anything going.
There was a problem hiding this comment.
I don't use VSCodium in a snap unfortunately. I use the nix package
The origin in there is a typo isn't it? With it it doesn't work, but without it does, at least on my machine that branch was created locally from your PR edit: @fidgetingbits (sorry, I am unfamiliar with how github commenting works, not sure whether this is the right way to point out such a small thing)^^' |
I modified the commands from a script that uses my local branches rather than the PRs, so likely why. I'll edit the comment in case anyone else tries to use it. |
Here I am running into troubles once again: Due to lacking knowledge of yarn etc. I don't think I am able to troubleshoot that myself farther than this. Version info: (cartago delenda est - I mean I think cursorless-dev/command-server#26 really makes sense) |
|
Any chance the instructions saidelike posted on that other issue would help building on windows? He implies 'pnpm compile' is enough. I don't even remember why I used yarn anymore.. |
Fwiw i used "pnpm" for neovim's command-server because cursorless used "pnpm" and i didn't realise command-server was using "yarn" by default (as indicated in the package.json). Maybe @pokey can indicate how he builds the vscode's command-server? Btw, I am happy to test these command-client's changes along with corresponding changes in my neovim command-server extension. |
|
@fidgetingbits managed to test it on windows 10 with the newest version of both branches. @saidelike thanks for the hint, changing yarn to pnpm in the package.json solved the problem Interestingly it also worked if I just changed the command server and not the talon user files. there was still a command server in the temporary directory and it was working... |
|
I'm going through some of the older PRs. This looks like it's ready to be merged to me? Something I'm missing? |
I think so, with the caveat I think the @pokey command server vscode extension will have to merge the PR and release an updated version first. Otherwise it will break RPC for users. There was the snap concern about home access, which I've noted I don't think is valid for editors. There is also the commented code for setting a hidden file attribute for windows, which I don't know if windows people want and couldn't test, but happy to just have that deleted before merge. |
|
Also @lunixbochs had commented they wanted to do a security review before this gets merged, so will need to take a look. |
This is a fix for #966. I've got three different systems where I run into this issue, so want to finally address it for everyone.
There is a corresponding PR for command-server at cursorless-dev/command-server#21
I haven't actually tested this on windows, as I don't have it, so I'm not sure if the path I used is sufficient. On this side it is TALON_HOME, and on command-server side it is hardcoded which I think is the same but not positive. Also in the discussion there was a suggestion to make the folder hidden. I left a commented out cmd for setting the folder as hidden, but again not sure if it is right and can't test it, so feedback on if that's the right approach is welcome. Will be good if someone can test/verify the windows side (I added a few windows users as requested reviewers).
Its also worth noting that in #966 discussion, @auscompgeek suggested to use
XDG_RUNTIME_DIRon Linux, which I had implemented and tested originally. However, when looking into snaps I ran across https://forum.snapcraft.io/t/rethinking-how-we-handle-xdg-runtime-dir/22223 which suggests that they may want to change snaps to no longer have access to the sameXDG_RUNTIME_DIRfolder. If they actually ever implemented that then this would break, so in the end I opted not to use it.There's also per user temp directories on mac, which you can query using
getconf DARWIN_USER_TEMP_DIR, but I decided not to keep that code in the end since it keeps the mac/linux case simpler both using/tmp, especially in light of not usingXDG_RUNTIME_DIRin the end.ATM I cache the result in a global, and this was mostly because originally I had code that was calling
getconf DARWIN_USER_TEMP_DIR, but I chose to just leave the global afterwards anyway.I've tested the changes using /tmp on both Linux and Mac and it works fine.