[refs #385] fixed security bug & added some tests for project archive

app/controllers/git/trees_controller.rb

@@ -20,6 +20,9 @@ class Git::TreesController < Git::BaseController
     treeish = params[:treeish].presence || @project.default_branch
     format = params[:format] || 'tar'
     commit = @project.git_repository.log(treeish, nil, :max_count => 1).first
+    if !commit or !['tar', 'zip'].include?(format)
+      raise ActiveRecord::RecordNotFound#("Couldn't send Project archive with id=#{@project.id}, treeish=#{treeish} and format=#{format}")
+    end
     name = "#{@project.owner.uname}-#{@project.name}#{@project.tags.include?(treeish) ? "-#{treeish}" : ''}-#{commit.id[0..19]}"
     fullname = "#{name}.#{format == 'tar' ? 'tar.gz' : 'zip'}"
     file = Tempfile.new fullname, 'tmp'

spec/controllers/git_trees_controller_spec.rb

@@ -0,0 +1,53 @@
+# -*- encoding : utf-8 -*-
+require 'spec_helper'
+
+describe Git::TreesController do
+
+  def fill_project
+    %x(cp -Rf #{Rails.root}/spec/tests.git/* #{@project.git_repository.path}) # maybe FIXME ?
+  end
+
+  before(:each) do
+    stub_rsync_methods
+
+    @project = FactoryGirl.create(:project)
+    @another_user = FactoryGirl.create(:user)
+    @params = {:project_id => @project.id, :format => 'tar'}
+  end
+
+  context 'for guest' do
+    it 'should be able to perform archive action' do
+      fill_project
+      get :archive, @params
+      response.should be_success
+    end
+  end
+
+  context 'for other user' do
+    it 'should not be able to archive empty project' do
+      @user = FactoryGirl.create(:user)
+      set_session_for(@user)
+      expect { get :archive, @params }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    it 'should not be able to injection code with format' do
+      @user = FactoryGirl.create(:user)
+      set_session_for(@user)
+      fill_project
+      expect { get :archive, @params.merge(:format => "tar master > /dev/null; echo 'I am hacker!';\#") }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    it 'should not be able to injection code with treeish' do
+      @user = FactoryGirl.create(:user)
+      set_session_for(@user)
+      fill_project
+      expect { get :archive, @params.merge(:treeish => "master > /dev/null; echo 'I am hacker!';\#") }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    it 'should be able to perform archive action' do
+      fill_project
+      get :archive, @params
+      response.should be_success
+    end
+  end
+end