MLFlexer / resurrect.wezterm

A plugin to restore windows, tabs and panes for Wezterm inspired by tmux-resurrect
MIT License
74 stars 2 forks source link

32 encryption is limited by size of state #34

Closed MLFlexer closed 2 months ago

MLFlexer commented 3 months ago

This replaces wezterm.run_child_process with luas buildin io.popen such that it is possible to write to stdin, i.e. omitting the need for temp files and hopefully making it possible to encrypt large states

MLFlexer commented 3 months ago

I tested it on linux with having 5+ panes with a filled scrollback by using the following command:

for i in {1..1000}; do
  ls
done

It worked fine on linux, but loading and saving the state was very slow, so I think it would be great to add this suggestion of having a user defined limit on the number of lines saved @joncrangle Are you able to test Mac and Windows?

MLFlexer commented 3 months ago

Will update the readme if this approach works on Mac and Windows

joncrangle commented 3 months ago

I tested it on linux with having 5+ panes with a filled scrollback by using the following command:

for i in {1..1000}; do
  ls
done

It worked fine on linux, but loading and saving the state was very slow, so I think it would be great to add this suggestion of having a user defined limit on the number of lines saved @joncrangle Are you able to test Mac and Windows?

I've tested this new approach and it's working on Windows. In PR - 35 I'm wrapping io.popen in a pcall so that I can emit events and return success from encrypt. In decrypt, I'm emitting events and returning the result of the pcall (json_data) if it is successful. I Will test on Mac later.

joncrangle commented 2 months ago

On Mac, I experienced some weird issues.

The good news is that the popen function you provided works for the encrypt function, but I could not get it to work for the decrypt function. It's mixed, but I think good news, because I never experienced this bug on decrypt. I don't think the OS's treat stdout as part of the command length, and they probably just create a reader for it (just a guess).

When I dug further, I found that when Wezterm uses popen to read to stdout it would not resolve the correct PATH environment variables from my system no matter what I did. It couldn't find the age binary without me explicitly providing its full path in the cmd. I've got age installed with homebrew, and I've got /opt/homebew/bin in my system path. I was able to decrypt default.json from the terminal using the identical cmd, but just not from within the Wezterm process.

I confirmed in multiple terminals and different shells (native Mac zsh, bash, Homebrew-installed zsh) that my PATH was correctly setup. I also confirmed in my Lua interpreter that Lua was able to use the correct system PATH vars with print(os.getenv 'PATH').

I also followed the Wezterm docs to explicitly set the PATH var using:

config.set_environment_variables = {
  PATH = wezterm.home_dir .. '/opt/homebrew/bin:' .. os.getenv 'PATH',
  },

But this didn't work either. Logging of PATH still wouldn't resolve the correct values. I ended up reverting the decrypt function to use wezterm.run_child_process using your execute_shell_cmd function and it's working well. I plan to test it again in Windows tomorrow to confirm.

It's very strange, and a bit confusing since the encrypt/decrypt functions are pretty much mirror images of each other, but that's what I found 😕

MLFlexer commented 2 months ago

Again thank you for your thorough investigation, i really appreciate your work! The plugin would not have gotten all these issues resolved without your help! That sounds really strange that it worked for the encryption but not for the decryption. Did you have a successful encryption and an unsuccessful decryption within the same Wezterm process instance?

If possible, then I would like to use popen for both decryption and encryption, as it is more "clean" compared to the sh -c hack that is used with run_child_process. I have pushed a commit where i include the path in the decryption, hopefully that would fix the issue. I have tested on linux and it works just fine as before

MLFlexer commented 2 months ago

I have also ran a limited test on windows by encrypting state using the periodic encryption and then decrypting using wezterm.plugin.require("https://github.com/MLFlexer/resurrect.wezterm").load_state("~", "workspace") in the debug overlay and it returned the decrypted json.

MLFlexer commented 2 months ago

I have however notices that a window pops up when the encryption runs on Windows, is that something you have noticed aswell? @joncrangle

Reading about it online it seems to only affect certain windows versions and it does not seem to be something that can be solved trivially without adding a new lua API in wezterm which allows writing/reading to stdin/out/err... here is why it does not happend for the run_child_process.

sravioli commented 2 months ago

I have however notices that a window pops up when the encryption runs on Windows, is that something you have noticed aswell? @joncrangle

Reading about it online it seems to only affect certain windows versions and it does not seem to be something that can be solved trivially without adding a new lua API in wezterm which allows writing/reading to stdin/out/err... here is why it does not happend for the run_child_process.

I've had the exact same issue on my config on windows sravioli/wezterm#8. I solved the issue by caching values in a temp file. This has of course its downsides but it fixed the popup windows on Windows 10 and 11.

MLFlexer commented 2 months ago

I've had the exact same issue on my config on windows sravioli/wezterm#8. I solved the issue by caching values in a temp file. This has of course its downsides but it fixed the popup windows on Windows 10 and 11.

Thanks for your suggestion, but i unfortunetly don't Think it will work as for this usecase (encrypting/decrypting files) as the output should ideally change at every Call, so caching it would not be viable. Another issue is that the output should ideally never become a plaintext file on the users machine as that can be a security concern.

joncrangle commented 2 months ago

I have however notices that a window pops up when the encryption runs on Windows, is that something you have noticed aswell? @joncrangle

Reading about it online it seems to only affect certain windows versions and it does not seem to be something that can be solved trivially without adding a new lua API in wezterm which allows writing/reading to stdin/out/err... here is why it does not happend for the run_child_process.

Yes - I've also noticed a quick popup on Windows that closes after the process finishes.

On Mac, I had tried adding PATH to the cmd without success. Later when I have some time I will try symlinking age to /usr/local/bin to see if that can resolve the issue on Mac.

joncrangle commented 2 months ago

Again thank you for your thorough investigation, i really appreciate your work! The plugin would not have gotten all these issues resolved without your help! That sounds really strange that it worked for the encryption but not for the decryption. Did you have a successful encryption and an unsuccessful decryption within the same Wezterm process instance?

If possible, then I would like to use popen for both decryption and encryption, as it is more "clean" compared to the sh -c hack that is used with run_child_process. I have pushed a commit where i include the path in the decryption, hopefully that would fix the issue. I have tested on linux and it works just fine as before

I'm finding with popen that the error checks aren't reliable because it doesn't return stderr. Even if you append 2>&1 to the cmd, it's hit or miss and I need to be very verbose about what I'm looking for, because the command sort of silently fails but will seem successful.

joncrangle commented 2 months ago

I've had the exact same issue on my config on windows sravioli/wezterm#8. I solved the issue by caching values in a temp file. This has of course its downsides but it fixed the popup windows on Windows 10 and 11.

Thanks for your suggestion, but i unfortunetly don't Think it will work as for this usecase (encrypting/decrypting files) as the output should ideally change at every Call, so caching it would not be viable. Another issue is that the output should ideally never become a plaintext file on the users machine as that can be a security concern.

What are your thoughts on using wezterm.run_child_process() first, and popen as a fallback on stderr? It has a downside of making the functions longer and more complex though.

sravioli commented 2 months ago

I've had the exact same issue on my config on windows sravioli/wezterm#8. I solved the issue by caching values in a temp file. This has of course its downsides but it fixed the popup windows on Windows 10 and 11.

Thanks for your suggestion, but i unfortunetly don't Think it will work as for this usecase (encrypting/decrypting files) as the output should ideally change at every Call, so caching it would not be viable. Another issue is that the output should ideally never become a plaintext file on the users machine as that can be a security concern.

What are your thoughts on using wezterm.run_child_process() first, and popen as a fallback on stderr? It has a downside of making the functions longer and more complex though.

I had to use io.popen() since wezterm.run_child_process() if I recall correctly it still caused popups on Windows, but I'm not 100% sure

MLFlexer commented 2 months ago

I've had the exact same issue on my config on windows sravioli/wezterm#8. I solved the issue by caching values in a temp file. This has of course its downsides but it fixed the popup windows on Windows 10 and 11.

Thanks for your suggestion, but i unfortunetly don't Think it will work as for this usecase (encrypting/decrypting files) as the output should ideally change at every Call, so caching it would not be viable. Another issue is that the output should ideally never become a plaintext file on the users machine as that can be a security concern.

What are your thoughts on using wezterm.run_child_process() first, and popen as a fallback on stderr? It has a downside of making the functions longer and more complex though.

Yea, this might be the best option, the logic could be placed in another function to reduce complexity

Does it reliably produce a stderr or should there be a check to see if the json+cmd is larger than the limit? Then based on the check it would use popen or run_child_process?

MLFlexer commented 2 months ago

I have however notices that a window pops up when the encryption runs on Windows, is that something you have noticed aswell? @joncrangle

Reading about it online it seems to only affect certain windows versions and it does not seem to be something that can be solved trivially without adding a new lua API in wezterm which allows writing/reading to stdin/out/err... here is why it does not happend for the run_child_process.

Yes - I've also noticed a quick popup on Windows that closes after the process finishes.

On Mac, I had tried adding PATH to the cmd without success. Later when I have some time I will try symlinking age to /usr/local/bin to see if that can resolve the issue on Mac.

This is super strange, does this also happen if you open wezterm from your user shell? I.e. open it by typing "wezterm" in iterm?

joncrangle commented 2 months ago

I have however notices that a window pops up when the encryption runs on Windows, is that something you have noticed aswell? @joncrangle

Reading about it online it seems to only affect certain windows versions and it does not seem to be something that can be solved trivially without adding a new lua API in wezterm which allows writing/reading to stdin/out/err... here is why it does not happend for the run_child_process.

Yes - I've also noticed a quick popup on Windows that closes after the process finishes. On Mac, I had tried adding PATH to the cmd without success. Later when I have some time I will try symlinking age to /usr/local/bin to see if that can resolve the issue on Mac.

This is super strange, does this also happen if you open wezterm from your user shell? I.e. open it by typing "wezterm" in iterm?

This is certainly super strange. If I launch WezTerm using the binary in /Applications, it has limited PATH var access. If I launch it from another terminal (including the instance of WezTerm itself with limited access) it now has full access to PATH vars.

In my other branch, I'm working on an option to let the user supply resurrect.set_encryption.method (thinking about letting the user select "age" "rage" or "gpg") but the same feature would also allow me to supply an absolute path to "age".

MLFlexer commented 2 months ago

Latest commit adds execute_cmd_with_stdin which will use run_child_process if the command is less than 32k chars and will use io.popen otherwise for encrypting the state. Decryption will always use run_child_process as there is no need to write to stdin.

Tested on Linux and works as expected

MLFlexer commented 2 months ago

I have however notices that a window pops up when the encryption runs on Windows, is that something you have noticed aswell? @joncrangle

Reading about it online it seems to only affect certain windows versions and it does not seem to be something that can be solved trivially without adding a new lua API in wezterm which allows writing/reading to stdin/out/err... here is why it does not happend for the run_child_process.

Yes - I've also noticed a quick popup on Windows that closes after the process finishes. On Mac, I had tried adding PATH to the cmd without success. Later when I have some time I will try symlinking age to /usr/local/bin to see if that can resolve the issue on Mac.

This is super strange, does this also happen if you open wezterm from your user shell? I.e. open it by typing "wezterm" in iterm?

This is certainly super strange. If I launch WezTerm using the binary in /Applications, it has limited PATH var access. If I launch it from another terminal (including the instance of WezTerm itself with limited access) it now has full access to PATH vars.

In my other branch, I'm working on an option to let the user supply resurrect.set_encryption.method (thinking about letting the user select "age" "rage" or "gpg") but the same feature would also allow me to supply an absolute path to "age".

Yea, it is probably not the same env the program is launched in, hence different paths. I think the best solution is to assume that the user has age in the path and the provide an easy way to switch it, like using another encryption provider or defining the path. I dont think this should be tackled in this issue as it is outside the scope of fixing the encryption length limit

MLFlexer commented 2 months ago

Also tested on Windows and works as expected

joncrangle commented 2 months ago

Tested working well on Mac.