basecamp / kamal

Deploy web apps anywhere.
https://kamal-deploy.org
MIT License
10.6k stars 408 forks source link

SSHKit does not escape command arguments, resulting in failure to clone a path with spaces #1036

Open davidstosik opened 1 day ago

davidstosik commented 1 day ago

I understand this might be unconventional and frowned upon in some circles, but I store my whole source code folder in iCloud and use a symlink for convenience. I don't really have to justify, but that helps me with instant backups and when working with multiple computers. (I expect "use Git" comments, and that's okay. 😬 )

$ cd ~/src/my-project && pwd -P
/Users/sto/Library/Mobile Documents/com~apple~CloudDocs/src/my-project

So far, I have had very little issues with this quirk in my setup.

Enters Kamal. Since the announcements and talks at RailsWorld 2024, I've been very excited about trying it out on some pet projects.

I've ignored the issue at first and managed to deploy a pet project to DigitalOcean when my project's path didn't include spaces, but I thought it's now time to report this issue.

When running kamal deploy in a project which path includes spaces, Kamal fails to clone the repository:

$ kamal deploy
...                                                                                                                                                       unescaped space here!
                                                                                                                                                                    ⬇️
  INFO [a007f48f] Running /usr/bin/env git -C /var/folders/1q/mcsb93312b73zl6fm0887wzc0000gn/T/kamal-clones/my-project-a97a49be96920 clone /Users/sto/Library/Mobile Documents/com~apple~CloudDocs/src/my-project --recurse-submodules as sto@localhost
  Finished all in 255.7 seconds
  ERROR (SSHKit::Command::Failed): git exit status: 32768
git stdout: Nothing written
git stderr: fatal: repository '/Users/sto/Library/Mobile' does not exist

I dug a little bit and it seems related to how SSHKit sometimes constructs commands:

Here's a simple script to illustrate this:

require "sshkit"
SSHKit::Backend::Local.new.execute(:ls, "/Users/sto/Library/Mobile Documents/com~apple~CloudDocs/src/my-project")
# /Users/sto/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/sshkit-1.23.1/lib/sshkit/command.rb:97:in `exit_status=': ls exit status: 256 (SSHKit::Command::Failed)
# ls stdout: Nothing written
# ls stderr: ls: /Users/sto/Library/Mobile: No such file or directory
# ls: Documents/com~apple~CloudDocs/src/my-project: No such file or directory

If this can't get fixed in SSHKit, I wonder if that's something that needs to get fixed in Kamal. 🤔

I tried "fixing" this problem in Kamal like this:

diff --git c/lib/kamal/commands/builder/clone.rb i/lib/kamal/commands/builder/clone.rb
index 17d9c93..466b523 100644
--- c/lib/kamal/commands/builder/clone.rb
+++ i/lib/kamal/commands/builder/clone.rb
@@ -6,7 +6,7 @@ module Kamal::Commands::Builder::Clone
   end

   def clone
-    git :clone, Kamal::Git.root, "--recurse-submodules", path: clone_directory
+    git :clone, Kamal::Git.root.shellescape, "--recurse-submodules", path: clone_directory
   end

   def clone_reset_steps

It looks like I managed to deploy my app, but I'm sure this is not the right level to do this and there would be more locations where a path needs to be escaped, so I'm not opening a PR yet, as I would rather discuss with people who know more about the project first.

davidstosik commented 1 day ago

I just found this 8-year-old issue: Issue: Trusted strings, shell escaping and backwards compatibility. Unfortunately it seems to have rapidly lost any traction...