[ARVADOS] updated: 7c04fe3e0e93d22b9c884934dbbd80fed6072ee3

[git at public.curoverse.com]

Summary of changes:
 services/api/app/models/commit.rb                  |   10 ++--
 .../arvados/v1/commits_controller_test.rb          |   50 +++++++++++++-------
 2 files changed, 37 insertions(+), 23 deletions(-)

       via  7c04fe3e0e93d22b9c884934dbbd80fed6072ee3 (commit)
      from  56c13e4c3af29ec274a8c3c5564ff3dbec2505fc (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 7c04fe3e0e93d22b9c884934dbbd80fed6072ee3
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Wed Mar 26 08:57:00 2014 -0400

    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 f8ef99e..4954124 100644
--- 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 f74833e..955b1f1 100644
--- 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
 

-----------------------------------------------------------------------


hooks/post-receive
--