From f1cf96baab41c051a35a7fbcec5d5cba9587326e Mon Sep 17 00:00:00 2001
From: Alexander Machehin <alexander.machehin@gmail.com>
Date: Wed, 25 Apr 2012 00:03:30 +0600
Subject: [PATCH] [refs #385] fixed security bug & added some tests for project
 archive

---
 app/controllers/git/trees_controller.rb       |  3 ++
 spec/controllers/git_trees_controller_spec.rb | 53 +++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 spec/controllers/git_trees_controller_spec.rb

diff --git a/app/controllers/git/trees_controller.rb b/app/controllers/git/trees_controller.rb
index c60650ae7..e84079f88 100644
--- a/app/controllers/git/trees_controller.rb
+++ b/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'
diff --git a/spec/controllers/git_trees_controller_spec.rb b/spec/controllers/git_trees_controller_spec.rb
new file mode 100644
index 000000000..ca92662f5
--- /dev/null
+++ b/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