Added more tests and more aggressive input checking.

diff --git a/services/api/app/models/commit.rb b/services/api/app/models/commit.rb
index f8ef99e9334fe38e62ed4e36f110d1649fa6a165..49541248f28cf5b4f677710aa30f03991a2fa6af 100644 (file)
--- a/services/api/app/models/commit.rb
+++ b/services/api/app/models/commit.rb
@@ -2,7 +2,7 @@ class Commit < ActiveRecord::Base
   require 'shellwords'
 
   def self.git_check_ref_format(e)
-    if !e or e.empty? or e[0] == '-'
+    if !e or e.empty? or e[0] == '-' or e[0] == '$'
       # definitely not valid
       false
     else
@@ -46,7 +46,7 @@ class Commit < ActiveRecord::Base
 
         # Get the commit hash for the upper bound
         max_hash = nil
-        IO.foreach("|git rev-list --max-count=1 #{maximum}") do |line|
+        IO.foreach("|git rev-list --max-count=1 #{maximum.shellescape}") do |line|
           max_hash = line.strip
         end
 
@@ -58,7 +58,7 @@ class Commit < ActiveRecord::Base
           resolved_exclude = []
           exclude.each do |e|
             if git_check_ref_format(e)
-              IO.foreach("|git rev-list --max-count=1 #{e}") do |line|
+              IO.foreach("|git rev-list --max-count=1 #{e.shellescape}") do |line|
                 resolved_exclude.push(line.strip)
               end
             else
@@ -71,7 +71,7 @@ class Commit < ActiveRecord::Base
         if minimum
           # Get the commit hash for the lower bound
           min_hash = nil
-          IO.foreach("|git rev-list --max-count=1 #{minimum}") do |line|
+          IO.foreach("|git rev-list --max-count=1 #{minimum.shellescape}") do |line|
             min_hash = line.strip
           end
 
@@ -79,7 +79,7 @@ class Commit < ActiveRecord::Base
           next if !min_hash or !git_check_ref_format(min_hash)
 
           # Now find all commits between them
-          IO.foreach("|git rev-list #{min_hash}..#{max_hash}") do |line|
+          IO.foreach("|git rev-list #{min_hash.shellescape}..#{max_hash.shellescape}") do |line|
             hash = line.strip
             commits.push(hash) if !resolved_exclude or !resolved_exclude.include? hash
           end

diff --git a/services/api/test/functional/arvados/v1/commits_controller_test.rb b/services/api/test/functional/arvados/v1/commits_controller_test.rb
index f74833e66cd5de48501b208fff7ec5a5e455b2bf..955b1f12a531779cd651e97457833096d9763868 100644 (file)
--- a/services/api/test/functional/arvados/v1/commits_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/commits_controller_test.rb
@@ -48,24 +48,38 @@ class Arvados::V1::CommitsControllerTest < ActionController::TestCase
     a = Commit.find_commit_range(users(:active), nil, '31ce37fe365b3dc204300a3e4c396ad333ed0556', '077ba2ad3ea24a929091a9e6ce545c93199b8e57', ['tag1'])
     assert_equal ['077ba2ad3ea24a929091a9e6ce545c93199b8e57', '31ce37fe365b3dc204300a3e4c396ad333ed0556'], a
 
-    touchdir = Dir.mktmpdir()
-
-  # invalid input
-    a = Commit.find_commit_range(users(:active), nil, nil, "31ce37fe365b3dc204300a3e4c396ad333ed0556 ; touch #{touchdir}/uh_oh", nil)
-    assert !File.exists?("#{touchdir}/uh_oh"), "#{touchdir}/uh_oh should not exist, 'maximum' parameter of find_commit_range is exploitable"
-    assert_equal nil, a
-
-  # invalid input
-    a = Commit.find_commit_range(users(:active), nil, "31ce37fe365b3dc204300a3e4c396ad333ed0556 ; touch #{touchdir}/uh_oh", "31ce37fe365b3dc204300a3e4c396ad333ed0556", nil)
-    assert !File.exists?("#{touchdir}/uh_oh"), "#{touchdir}/uh_oh should not exist, 'minimum' parameter of find_commit_range is exploitable"
-    assert_equal nil, a
-
-  # invalid input
-    a = Commit.find_commit_range(users(:active), nil, "31ce37fe365b3dc204300a3e4c396ad333ed0556", "077ba2ad3ea24a929091a9e6ce545c93199b8e57", ["4fe459abe02d9b365932b8f5dc419439ab4e2577 ; touch #{touchdir}/uh_oh"])
-    assert !File.exists?("#{touchdir}/uh_oh"), "#{touchdir}/uh_oh should not exist, 'excludes' parameter of find_commit_range is exploitable"
-    assert_equal nil, a
-
-    FileUtils.remove_entry touchdir
+    Dir.mktmpdir do |touchdir|
+      # invalid input to maximum
+      a = Commit.find_commit_range(users(:active), nil, nil, "31ce37fe365b3dc204300a3e4c396ad333ed0556 ; touch #{touchdir}/uh_oh", nil)
+      assert !File.exists?("#{touchdir}/uh_oh"), "#{touchdir}/uh_oh should not exist, 'maximum' parameter of find_commit_range is exploitable"
+      assert_equal nil, a
+
+      # invalid input to maximum
+      a = Commit.find_commit_range(users(:active), nil, nil, "$(uname>#{touchdir}/uh_oh)", nil)
+      assert !File.exists?("#{touchdir}/uh_oh"), "#{touchdir}/uh_oh should not exist, 'maximum' parameter of find_commit_range is exploitable"
+      assert_equal nil, a
+
+      # invalid input to minimum
+      a = Commit.find_commit_range(users(:active), nil, "31ce37fe365b3dc204300a3e4c396ad333ed0556 ; touch #{touchdir}/uh_oh", "31ce37fe365b3dc204300a3e4c396ad333ed0556", nil)
+      assert !File.exists?("#{touchdir}/uh_oh"), "#{touchdir}/uh_oh should not exist, 'minimum' parameter of find_commit_range is exploitable"
+      assert_equal nil, a
+
+      # invalid input to minimum
+      a = Commit.find_commit_range(users(:active), nil, "$(uname>#{touchdir}/uh_oh)", "31ce37fe365b3dc204300a3e4c396ad333ed0556", nil)
+      assert !File.exists?("#{touchdir}/uh_oh"), "#{touchdir}/uh_oh should not exist, 'minimum' parameter of find_commit_range is exploitable"
+      assert_equal nil, a
+
+      # invalid input to 'excludes'
+      a = Commit.find_commit_range(users(:active), nil, "31ce37fe365b3dc204300a3e4c396ad333ed0556", "077ba2ad3ea24a929091a9e6ce545c93199b8e57", ["4fe459abe02d9b365932b8f5dc419439ab4e2577 ; touch #{touchdir}/uh_oh"])
+      assert !File.exists?("#{touchdir}/uh_oh"), "#{touchdir}/uh_oh should not exist, 'excludes' parameter of find_commit_range is exploitable"
+      assert_equal nil, a
+
+      # invalid input to 'excludes'
+      a = Commit.find_commit_range(users(:active), nil, "31ce37fe365b3dc204300a3e4c396ad333ed0556", "077ba2ad3ea24a929091a9e6ce545c93199b8e57", ["$(uname>#{touchdir}/uh_oh)"])
+      assert !File.exists?("#{touchdir}/uh_oh"), "#{touchdir}/uh_oh should not exist, 'excludes' parameter of find_commit_range is exploitable"
+      assert_equal nil, a
+
+    end
 
   end