Merge branch 'gitlab_popen_array' into 'master'

Change Gitlab::Popen to only accept arrays as commands

Merge branch 'gitlab_popen_array' into 'master'
Change Gitlab::Popen to only accept arrays as commands
Dmitriy Zaporozhets
2 parents 6c1af645 36c2c354
Showing 4 changed files with 31 additions and 7 deletions Show diff stats
app/models/key.rb
lib/gitlab/popen.rb
lib/gitlab/satellite/satellite.rb
spec/lib/gitlab/popen_spec.rb
@@ -53,7 +53,7 @@ class Key &lt; 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?
 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
@@ -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}")
@@ -10,7 +10,7 @@ describe &#39;Gitlab::Popen&#39;, 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 &#39;Gitlab::Popen&#39;, 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
...	...	@@ -53,7 +53,7 @@ class Key < ActiveRecord::Base
53	53	Tempfile.open('gitlab_key_file') do \|file\|
54	54	file.puts key
55	55	file.rewind
56		- cmd_output, cmd_status = popen("ssh-keygen -lf #{file.path}", '/tmp')
	56	+ cmd_output, cmd_status = popen(%W(ssh-keygen -lf #{file.path}), '/tmp')
57	57	end
58	58
59	59	if cmd_status.zero?
...	...
1	1	require 'fileutils'
	2	+require 'open3'
2	3
3	4	module Gitlab
4	5	module Popen
5		- def popen(cmd, path)
	6	+ extend self
	7	+
	8	+ def popen(cmd, path=nil)
	9	+ unless cmd.is_a?(Array)
	10	+ raise "System commands must be given as an array of strings"
	11	+ end
	12	+
	13	+ path \|\|= Dir.pwd
6	14	vars = { "PWD" => path }
7	15	options = { chdir: path }
8	16
...	...	@@ -12,10 +20,10 @@ module Gitlab
12	20
13	21	@cmd_output = ""
14	22	@cmd_status = 0
15		- Open3.popen3(vars, cmd, options) do \|stdin, stdout, stderr, wait_thr\|
16		- @cmd_status = wait_thr.value.exitstatus
	23	+ Open3.popen3(vars, *cmd, options) do \|stdin, stdout, stderr, wait_thr\|
17	24	@cmd_output << stdout.read
18	25	@cmd_output << stderr.read
	26	+ @cmd_status = wait_thr.value.exitstatus
19	27	end
20	28
21	29	return @cmd_output, @cmd_status
...	...
...	...	@@ -33,7 +33,7 @@ module Gitlab
33	33	end
34	34
35	35	def create
36		- output, status = popen("git clone #{project.repository.path_to_repo} #{path}",
	36	+ output, status = popen(%W(git clone -- #{project.repository.path_to_repo} #{path}),
37	37	Gitlab.config.satellites.path)
38	38
39	39	log("PID: #{project.id}: git clone #{project.repository.path_to_repo} #{path}")
...	...
...	...	@@ -10,7 +10,7 @@ describe 'Gitlab::Popen', no_db: true do
10	10
11	11	context 'zero status' do
12	12	before do
13		- @output, @status = @klass.new.popen('ls', path)
	13	+ @output, @status = @klass.new.popen(%W(ls), path)
14	14	end
15	15
16	16	it { @status.should be_zero }
...	...	@@ -19,11 +19,27 @@ describe 'Gitlab::Popen', no_db: true do
19	19
20	20	context 'non-zero status' do
21	21	before do
22		- @output, @status = @klass.new.popen('cat NOTHING', path)
	22	+ @output, @status = @klass.new.popen(%W(cat NOTHING), path)
23	23	end
24	24
25	25	it { @status.should == 1 }
26	26	it { @output.should include('No such file or directory') }
27	27	end
	28	+
	29	+ context 'unsafe string command' do
	30	+ it 'raises an error when it gets called with a string argument' do
	31	+ expect { @klass.new.popen('ls', path) }.to raise_error
	32	+ end
	33	+ end
	34	+
	35	+ context 'without a directory argument' do
	36	+ before do
	37	+ @output, @status = @klass.new.popen(%W(ls))
	38	+ end
	39	+
	40	+ it { @status.should be_zero }
	41	+ it { @output.should include('spec') }
	42	+ end
	43	+
28	44	end
29	45
...	...