[ARVADOS] updated: 56c13e4c3af29ec274a8c3c5564ff3dbec2505fc
git at public.curoverse.com
git at public.curoverse.com
Tue Mar 25 16:52:03 EDT 2014
Summary of changes:
doc/user/topics/tutorial-job1.html.textile.liquid | 5 ++-
services/api/app/models/commit.rb | 34 ++++++++++---------
.../arvados/v1/commits_controller_test.rb | 21 +++++++++++-
3 files changed, 41 insertions(+), 19 deletions(-)
via 56c13e4c3af29ec274a8c3c5564ff3dbec2505fc (commit)
from 0ce7d041f53bea47f98f509ccf7c196b8ad393b0 (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 56c13e4c3af29ec274a8c3c5564ff3dbec2505fc
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Tue Mar 25 16:51:57 2014 -0400
Reworked input validation on find_commit_range (for real this time!) and added tests.
Fixed descriptive text in tutorial-job1.
Removed dead code.
diff --git a/doc/user/topics/tutorial-job1.html.textile.liquid b/doc/user/topics/tutorial-job1.html.textile.liquid
index c100d6b..58fe329 100644
--- a/doc/user/topics/tutorial-job1.html.textile.liquid
+++ b/doc/user/topics/tutorial-job1.html.textile.liquid
@@ -34,7 +34,8 @@ EOF
* @<<EOF@ tells the shell to direct the following lines into the standard input for @cat@ up until it sees the line @EOF@
* @>the_job@ redirects standard output to a file called @the_job@
* @"script"@ specifies the name of the script to run. The script is searched for in the "crunch_scripts/" subdirectory of the @git@ checkout specified by @"script_version"@.
-* @"script_version"@ specifies the version of the script that you wish to run. This can be in the form of an explicit @git@ revision hash, or in the form "repository:branch" (in which case it will take the HEAD of the specified branch). Arvados logs the script version that was used in the run, enabling you to go back and re-run any past job with the guarantee that the exact same code will be used as was used in the previous run. You can access a list of available @git@ repositories on the Arvados workbench under "Compute %(rarr)→% Code repositories":https://{{site.arvados_workbench_host}}/repositories .
+* @"repository"@ is the git repository to search for the script version. You can access a list of available @git@ repositories on the Arvados workbench under "Compute %(rarr)→% Code repositories":https://{{site.arvados_workbench_host}}//repositories .
+* @"script_version"@ specifies the version of the script that you wish to run. This can be in the form of an explicit @git@ revision hash, a tag, or a branch (in which case it will take the HEAD of the specified branch). Arvados logs the script version that was used in the run, enabling you to go back and re-run any past job with the guarantee that the exact same code will be used as was used in the previous run.
* @"script_parameters"@ are provided to the script. In this case, the input is the locator for the collection that we inspected in the previous section.
Use @arv job create@ to actually submit the job. It should print out a JSON object which describes the newly created job:
@@ -199,7 +200,7 @@ The log collection consists of one log file named with the job id. You can acce
2013-12-16_20:44:38 qr1hi-8i9sb-1pm1t02dezhupss 7575 status: 0 done, 1 running, 0 todo
2013-12-16_20:44:39 qr1hi-8i9sb-1pm1t02dezhupss 7575 0 child 7681 on compute13.1 exit 0 signal 0 success=true
2013-12-16_20:44:39 qr1hi-8i9sb-1pm1t02dezhupss 7575 0 success in 1 seconds
-2013-12-16_20:44:39 qr1hi-8i9sb-1pm1t02dezhupss 7575 0 output
+2013-12-16_20:44:39 qr1hi-8i9sb-1pm1t02dezhupss 7575 0 output
2013-12-16_20:44:39 qr1hi-8i9sb-1pm1t02dezhupss 7575 wait for last 0 children to finish
2013-12-16_20:44:39 qr1hi-8i9sb-1pm1t02dezhupss 7575 status: 1 done, 0 running, 1 todo
2013-12-16_20:44:39 qr1hi-8i9sb-1pm1t02dezhupss 7575 start level 1
diff --git a/services/api/app/models/commit.rb b/services/api/app/models/commit.rb
index 0e9ae11..f8ef99e 100644
--- a/services/api/app/models/commit.rb
+++ b/services/api/app/models/commit.rb
@@ -1,11 +1,19 @@
class Commit < ActiveRecord::Base
require 'shellwords'
+ def self.git_check_ref_format(e)
+ if !e or e.empty? or e[0] == '-'
+ # definitely not valid
+ false
+ else
+ `git check-ref-format --allow-onelevel #{e.shellescape}`
+ $?.success?
+ end
+ end
+
def self.find_commit_range(current_user, repository, minimum, maximum, exclude)
- # disallow starting with '-' so verision strings can't be interpreted as command line options
- valid_pattern = /[A-Za-z0-9_][A-Za-z0-9_-]/
- if (minimum and !minimum.match valid_pattern) or !maximum.match valid_pattern
- logger.warn "find_commit_range called with string containing invalid characters: '#{minimum}', '#{maximum}'"
+ if (minimum and !git_check_ref_format(minimum)) or !git_check_ref_format(maximum)
+ logger.warn "find_commit_range called with invalid minimum or maximum: '#{minimum}', '#{maximum}'"
return nil
end
@@ -28,22 +36,14 @@ class Commit < ActiveRecord::Base
readable = readable.where(name: repository)
end
- #puts "min #{minimum}"
- #puts "max #{maximum}"
- #puts "rep #{repository}"
-
commits = []
readable.each do |r|
if on_disk_repos[r.name]
ENV['GIT_DIR'] = on_disk_repos[r.name][:git_dir]
- #puts "dir #{on_disk_repos[r.name][:git_dir]}"
-
# We've filtered for invalid characters, so we can pass the contents of
# minimum and maximum safely on the command line
- #puts "git rev-list --max-count=1 #{maximum}"
-
# Get the commit hash for the upper bound
max_hash = nil
IO.foreach("|git rev-list --max-count=1 #{maximum}") do |line|
@@ -51,16 +51,19 @@ class Commit < ActiveRecord::Base
end
# If not found or string is invalid, nothing else to do
- next if !max_hash or !max_hash.match valid_pattern
+ next if !max_hash or !git_check_ref_format(max_hash)
resolved_exclude = nil
if exclude
resolved_exclude = []
exclude.each do |e|
- if e.match valid_pattern
+ if git_check_ref_format(e)
IO.foreach("|git rev-list --max-count=1 #{e}") do |line|
resolved_exclude.push(line.strip)
end
+ else
+ logger.warn "find_commit_range called with invalid exclude invalid characters: '#{exclude}'"
+ return nil
end
end
end
@@ -73,10 +76,9 @@ class Commit < ActiveRecord::Base
end
# If not found or string is invalid, nothing else to do
- next if !min_hash or !min_hash.match valid_pattern
+ next if !min_hash or !git_check_ref_format(min_hash)
# Now find all commits between them
- #puts "git rev-list #{min_hash}..#{max_hash}"
IO.foreach("|git rev-list #{min_hash}..#{max_hash}") do |line|
hash = line.strip
commits.push(hash) if !resolved_exclude or !resolved_exclude.include? hash
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 70e5276..f74833e 100644
--- a/services/api/test/functional/arvados/v1/commits_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/commits_controller_test.rb
@@ -6,7 +6,7 @@ class Arvados::V1::CommitsControllerTest < ActionController::TestCase
# See git_setup.rb for the commit log for test.git.tar
include GitSetup
-
+
test "test_find_commit_range" do
authorize_with :active
@@ -48,6 +48,25 @@ 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
+
end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list