diff --git a/app/models/key.rb b/app/models/key.rb index 79f7bbd25901e8334750839545a9bd021f0e4c83..29a76f53f3d1ef0062ed8771d56871d89bedd8f2 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -53,7 +53,7 @@ class Key < ActiveRecord::Base Tempfile.open('gitlab_key_file') do |file| file.puts key file.rewind - cmd_output, cmd_status = popen("ssh-keygen -lf #{file.path}", '/tmp') + cmd_output, cmd_status = popen(%W(ssh-keygen -lf #{file.path}), '/tmp') end if cmd_status.zero? diff --git a/lib/gitlab/popen.rb b/lib/gitlab/popen.rb index 5283cf0b82151bf8667ffb076b1bc0bb6914bd65..e2fbafb389911ef68e4da2da7da917f6acb9e86a 100644 --- a/lib/gitlab/popen.rb +++ b/lib/gitlab/popen.rb @@ -1,8 +1,16 @@ require 'fileutils' +require 'open3' module Gitlab module Popen - def popen(cmd, path) + extend self + + def popen(cmd, path=nil) + unless cmd.is_a?(Array) + raise "System commands must be given as an array of strings" + end + + path ||= Dir.pwd vars = { "PWD" => path } options = { chdir: path } @@ -12,10 +20,10 @@ module Gitlab @cmd_output = "" @cmd_status = 0 - Open3.popen3(vars, cmd, options) do |stdin, stdout, stderr, wait_thr| - @cmd_status = wait_thr.value.exitstatus + Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| @cmd_output << stdout.read @cmd_output << stderr.read + @cmd_status = wait_thr.value.exitstatus end return @cmd_output, @cmd_status diff --git a/lib/gitlab/satellite/satellite.rb b/lib/gitlab/satellite/satellite.rb index 353c3024aad4d6cae77db40030fd8d3d7ebc4b68..bcf3012bd9242db7e060c1c3bfac69956d2aabc8 100644 --- a/lib/gitlab/satellite/satellite.rb +++ b/lib/gitlab/satellite/satellite.rb @@ -33,7 +33,7 @@ module Gitlab end def create - output, status = popen("git clone #{project.repository.path_to_repo} #{path}", + output, status = popen(%W(git clone -- #{project.repository.path_to_repo} #{path}), Gitlab.config.satellites.path) log("PID: #{project.id}: git clone #{project.repository.path_to_repo} #{path}") diff --git a/spec/lib/gitlab/popen_spec.rb b/spec/lib/gitlab/popen_spec.rb index 4791be4161373ea39b44ea192fdc21c18cc0d155..76d506eb3c0bf385fbba06d42e3f935f90e9bef1 100644 --- a/spec/lib/gitlab/popen_spec.rb +++ b/spec/lib/gitlab/popen_spec.rb @@ -10,7 +10,7 @@ describe 'Gitlab::Popen', no_db: true do context 'zero status' do before do - @output, @status = @klass.new.popen('ls', path) + @output, @status = @klass.new.popen(%W(ls), path) end it { @status.should be_zero } @@ -19,11 +19,27 @@ describe 'Gitlab::Popen', no_db: true do context 'non-zero status' do before do - @output, @status = @klass.new.popen('cat NOTHING', path) + @output, @status = @klass.new.popen(%W(cat NOTHING), path) end it { @status.should == 1 } it { @output.should include('No such file or directory') } end + + context 'unsafe string command' do + it 'raises an error when it gets called with a string argument' do + expect { @klass.new.popen('ls', path) }.to raise_error + end + end + + context 'without a directory argument' do + before do + @output, @status = @klass.new.popen(%W(ls)) + end + + it { @status.should be_zero } + it { @output.should include('spec') } + end + end