From 55c684f8a032f264a2077d1d8c7c665d6d8b10a2 Mon Sep 17 00:00:00 2001 From: Matt Brictson Date: Thu, 9 Feb 2017 20:35:50 -0800 Subject: [PATCH] Properly shell escape git:wrapper steps The `upload!` method does escaping of its own internally, so we don't need to do anything special there. Likewise environment variables are already handled. The only place we need to explicitly `shellescape` is when executing the `chmod` command. Verify this all works as expected by changing the user in the Cucumber features to be `(GitHub Web Flow) via ShipIt`. This user is now used when exercising the `git:check` task. Fixes #1842. --- CHANGELOG.md | 1 + features/step_definitions/assertions.rb | 6 ++++-- lib/capistrano/scm/git.rb | 2 +- lib/capistrano/scm/tasks/git.rake | 4 ++-- spec/lib/capistrano/scm/git_spec.rb | 4 ++-- spec/support/test_app.rb | 3 ++- 6 files changed, 12 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67bbe478..dabbad80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ https://github.com/capistrano/capistrano/compare/v3.7.2...HEAD * [#1835](https://github.com/capistrano/capistrano/pull/1835): Stopped printing parenthesis in ask prompt if no default or nil was passed as argument [(@chamini2)](https://github.com/chamini2) * [#1840](https://github.com/capistrano/capistrano/pull/1840): Git plugin: shellescape git_wrapper_path [(@olleolleolle)](https://github.com/olleolleolle) +* [#1843](https://github.com/capistrano/capistrano/pull/1843): Properly shell escape git:wrapper steps - [@mattbrictson](https://github.com/mattbrictson) * Your contribution here! ## `3.7.2` (2017-01-27) diff --git a/features/step_definitions/assertions.rb b/features/step_definitions/assertions.rb index f5a8db2f..6926407c 100644 --- a/features/step_definitions/assertions.rb +++ b/features/step_definitions/assertions.rb @@ -1,10 +1,12 @@ +require "shellwords" + Then(/^references in the remote repo are listed$/) do expect(@output).to include("refs/heads/master") end Then(/^git wrapper permissions are 0700$/) do - permissions_test = %Q([ $(stat -c "%a" #{TestApp.git_wrapper_path}) == "700" ]) - expect(vagrant_cli_command("ssh -c '#{permissions_test}'")).to be_success + permissions_test = %Q([ $(stat -c "%a" #{TestApp.git_wrapper_path.shellescape}) == "700" ]) + expect(vagrant_cli_command("ssh -c #{permissions_test.shellescape}")).to be_success end Then(/^the shared path is created$/) do diff --git a/lib/capistrano/scm/git.rb b/lib/capistrano/scm/git.rb index 8b53c55f..3191d0a0 100644 --- a/lib/capistrano/scm/git.rb +++ b/lib/capistrano/scm/git.rb @@ -7,7 +7,7 @@ class Capistrano::SCM::Git < Capistrano::SCM::Plugin set_if_empty :git_wrapper_path, lambda { # Try to avoid permissions issues when multiple users deploy the same app # by using different file names in the same dir for each deployer and stage. - suffix = [:application, :stage, :local_user].map { |key| fetch(key).to_s }.join("-").shellescape + suffix = [:application, :stage, :local_user].map { |key| fetch(key).to_s }.join("-") "#{fetch(:tmp_dir)}/git-ssh-#{suffix}.sh" } set_if_empty :git_environmental_variables, lambda { diff --git a/lib/capistrano/scm/tasks/git.rake b/lib/capistrano/scm/tasks/git.rake index 9160aff1..22892bdc 100644 --- a/lib/capistrano/scm/tasks/git.rake +++ b/lib/capistrano/scm/tasks/git.rake @@ -5,9 +5,9 @@ namespace :git do desc "Upload the git wrapper script, this script guarantees that we can script git without getting an interactive prompt" task :wrapper do on release_roles :all do - execute :mkdir, "-p", File.dirname(fetch(:git_wrapper_path)) + execute :mkdir, "-p", File.dirname(fetch(:git_wrapper_path)).shellescape upload! StringIO.new("#!/bin/sh -e\nexec /usr/bin/ssh -o PasswordAuthentication=no -o StrictHostKeyChecking=no \"$@\"\n"), fetch(:git_wrapper_path) - execute :chmod, "700", fetch(:git_wrapper_path) + execute :chmod, "700", fetch(:git_wrapper_path).shellescape end end diff --git a/spec/lib/capistrano/scm/git_spec.rb b/spec/lib/capistrano/scm/git_spec.rb index 2d29548a..9d03dad3 100644 --- a/spec/lib/capistrano/scm/git_spec.rb +++ b/spec/lib/capistrano/scm/git_spec.rb @@ -28,13 +28,13 @@ module Capistrano end describe "#set_defaults" do - it "sets ::git_wrapper_path in a shell-safe way" do + it "makes git_wrapper_path using application, stage, and local_user" do env.set(:tmp_dir, "/tmp") env.set(:application, "my_app") env.set(:stage, "staging") env.set(:local_user, "(Git Web User) via ShipIt") subject.set_defaults - expect(env.fetch(:git_wrapper_path)).to eq('/tmp/git-ssh-my_app-staging-\(Git\ Web\ User\)\ via\ ShipIt.sh') + expect(env.fetch(:git_wrapper_path)).to eq("/tmp/git-ssh-my_app-staging-(Git Web User) via ShipIt.sh") end end diff --git a/spec/support/test_app.rb b/spec/support/test_app.rb index 299fe501..77edb116 100644 --- a/spec/support/test_app.rb +++ b/spec/support/test_app.rb @@ -19,6 +19,7 @@ module TestApp set :linked_files, #{linked_files} set :linked_dirs, #{linked_dirs} set :format_options, log_file: nil + set :local_user, #{current_user.inspect} CONFIG end @@ -154,7 +155,7 @@ module TestApp end def current_user - `whoami`.chomp + "(GitHub Web Flow) via ShipIt" end def task_dir