Fix broken access control and refactor avatar upload

This commit moves the note folder from
/public/uploads/note
to
/uploads/note
and changes the uploader accordingly.
Now it's no longer possible to avoid the access control by modifing the url.
The Avatar upload has been refactored to use an own uploader as well
to cleanly seperate the two upload types.

CHANGELOG

 v 7.8.0 (unreleased)
+  - Fix broken access control for note attachments (Hannes Rosenögger)
   - Replace highlight.js with rouge-fork rugments (Stefan Tatschner)
   - Make project search case insensitive (Hannes Rosenögger)
   - Include issue/mr participants in list of recipients for reassign/close/reopen emails

app/controllers/files_controller.rb

@@ -6,7 +6,9 @@ class FilesController < ApplicationController
     if uploader.file_storage?
       if can?(current_user, :read_project, note.project)
         disposition = uploader.image? ? 'inline' : 'attachment'
-        send_file uploader.file.path, disposition: disposition
+        # Replace old notes location in /public with the new one in / and send the file
+        path = uploader.file.path.gsub("#{Rails.root}/public",Rails.root.to_s)
+        send_file path, disposition: disposition
       else
         not_found!
       end

app/models/group.rb

@@ -23,7 +23,7 @@ class Group < Namespace
   validate :avatar_type, if: ->(user) { user.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
-  mount_uploader :avatar, AttachmentUploader
+  mount_uploader :avatar, AvatarUploader
 
   after_create :post_create_hook
   after_destroy :post_destroy_hook

app/models/project.rb

@@ -138,7 +138,7 @@ class Project < ActiveRecord::Base
     if: ->(project) { project.avatar && project.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
-  mount_uploader :avatar, AttachmentUploader
+  mount_uploader :avatar, AvatarUploader
 
   # Scopes
   scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) }

app/models/user.rb

@@ -177,7 +177,7 @@ class User < ActiveRecord::Base
     end
   end
 
-  mount_uploader :avatar, AttachmentUploader
+  mount_uploader :avatar, AvatarUploader
 
   # Scopes
   scope :admins, -> { where(admin:  true) }

app/uploaders/attachment_uploader.rb

@@ -3,10 +3,8 @@
 class AttachmentUploader < CarrierWave::Uploader::Base
   storage :file
 
-  after :store, :reset_events_cache
-
   def store_dir
-    "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
+    "#{Rails.root}/uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
   end
 
   def image?
@@ -29,8 +27,4 @@ class AttachmentUploader < CarrierWave::Uploader::Base
   def file_storage?
     self.class.storage == CarrierWave::Storage::File
   end
-
-  def reset_events_cache(file)
-    model.reset_events_cache if model.is_a?(User)
-  end
 end

app/uploaders/avatar_uploader.rb

+# encoding: utf-8
+
+class AvatarUploader < CarrierWave::Uploader::Base
+  storage :file
+
+  after :store, :reset_events_cache
+
+  def store_dir
+    "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
+  end
+
+  def image?
+    img_ext = %w(png jpg jpeg gif bmp tiff)
+    if file.respond_to?(:extension)
+      img_ext.include?(file.extension.downcase)
+    else
+      # Not all CarrierWave storages respond to :extension
+      ext = file.path.split('.').last.downcase
+      img_ext.include?(ext)
+    end
+  rescue
+    false
+  end
+
+  def file_storage?
+    self.class.storage == CarrierWave::Storage::File
+  end
+
+  def reset_events_cache(file)
+    model.reset_events_cache if model.is_a?(User)
+  end
+end

db/migrate/20150213111727_move_note_folder.rb

+class MoveNoteFolder < ActiveRecord::Migration
+  def up
+    system(
+      "if [ -d '#{Rails.root}/public/uploads/note' ];
+        then mv #{Rails.root}/public/uploads/note #{Rails.root}/uploads/note;
+        echo 'note folder has been moved successfully';
+      else
+        echo 'note folder has already been moved or does not exist yet. Nothing to do here.'; fi")
+  end
+
+  def down
+    system(
+      "if [ -d '#{Rails.root}/uploads/note' ];
+        then mv #{Rails.root}/uploads/note #{Rails.root}/public/uploads/note;
+        echo 'note folder has been moved successfully';
+      else
+        echo 'note folder has already been moved or does not exist yet. Nothing to do here.'; fi")
+  end
+end

features/steps/groups.rb

@@ -110,7 +110,7 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
   end
 
   step 'I should see new group "Owned" avatar' do
-    Group.find_by(name: "Owned").avatar.should be_instance_of AttachmentUploader
+    Group.find_by(name: "Owned").avatar.should be_instance_of AvatarUploader
     Group.find_by(name: "Owned").avatar.url.should == "/uploads/group/avatar/#{ Group.find_by(name:"Owned").id }/gitlab_logo.png"
   end
 

features/steps/profile/profile.rb

@@ -29,7 +29,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps
   end
 
   step 'I should see new avatar' do
-    @user.avatar.should be_instance_of AttachmentUploader
+    @user.avatar.should be_instance_of AvatarUploader
     @user.avatar.url.should == "/uploads/user/avatar/#{ @user.id }/gitlab_logo.png"
   end
 

features/steps/project/project.rb

@@ -35,7 +35,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps
   end
 
   step 'I should see new project avatar' do
-    @project.avatar.should be_instance_of AttachmentUploader
+    @project.avatar.should be_instance_of AvatarUploader
     url = @project.avatar.url
     url.should == "/uploads/project/avatar/#{ @project.id }/gitlab_logo.png"
   end

lib/backup/manager.rb

 module Backup
   class Manager
-    BACKUP_CONTENTS = %w{repositories/ db/ uploads/ backup_information.yml}
+    BACKUP_CONTENTS = %w{repositories/ db/ public/ uploads/ backup_information.yml}
 
     def pack
       # saving additional informations

lib/backup/uploads.rb

 module Backup
   class Uploads
-    attr_reader :app_uploads_dir, :backup_uploads_dir, :backup_dir
+    attr_reader :app_public_uploads_dir, :app_private_uploads_dir, :backup_public_uploads_dir,
+     :backup_private_uploads_dir, :backup_dir, :backup_public_dir
 
     def initialize
-      @app_uploads_dir = File.realpath(Rails.root.join('public', 'uploads'))
+      @app_public_uploads_dir = File.realpath(Rails.root.join('public', 'uploads'))
+      @app_private_uploads_dir = File.realpath(Rails.root.join('uploads'))
       @backup_dir = Gitlab.config.backup.path
-      @backup_uploads_dir = File.join(Gitlab.config.backup.path, 'uploads')
+      @backup_public_dir = File.join(backup_dir, 'public')
+      @backup_public_uploads_dir = File.join(backup_dir, 'public', 'uploads')
+      @backup_private_uploads_dir = File.join(backup_dir, 'uploads')
     end
 
-    # Copy uploads from public/uploads to backup/uploads
+    # Copy uploads from public/uploads to backup/public/uploads and from /uploads to backup/uploads
     def dump
-      FileUtils.mkdir_p(backup_uploads_dir)
-      FileUtils.cp_r(app_uploads_dir, backup_dir)
+      FileUtils.mkdir_p(backup_public_uploads_dir)
+      FileUtils.cp_r(app_public_uploads_dir, backup_public_dir)
+      
+      FileUtils.mkdir_p(backup_private_uploads_dir)
+      FileUtils.cp_r(app_private_uploads_dir, backup_dir)
     end
 
     def restore
-      backup_existing_uploads_dir
+      backup_existing_public_uploads_dir
+      backup_existing_private_uploads_dir
+
+      FileUtils.cp_r(backup_public_uploads_dir, app_public_uploads_dir)
+      FileUtils.cp_r(backup_private_uploads_dir, app_private_uploads_dir)
+    end
 
-      FileUtils.cp_r(backup_uploads_dir, app_uploads_dir)
+    def backup_existing_public_uploads_dir
+      timestamped_public_uploads_path = File.join(app_public_uploads_dir, '..', "uploads.#{Time.now.to_i}")
+      if File.exists?(app_public_uploads_dir)
+        FileUtils.mv(app_public_uploads_dir, timestamped_public_uploads_path)
+      end
     end
 
-    def backup_existing_uploads_dir
-      timestamped_uploads_path = File.join(app_uploads_dir, '..', "uploads.#{Time.now.to_i}")
-      if File.exists?(app_uploads_dir)
-        FileUtils.mv(app_uploads_dir, timestamped_uploads_path)
+    def backup_existing_private_uploads_dir
+      timestamped_private_uploads_path = File.join(app_private_uploads_dir, '..', "uploads.#{Time.now.to_i}")
+      if File.exists?(app_private_uploads_dir)
+        FileUtils.mv(app_private_uploads_dir, timestamped_private_uploads_path)
       end
     end
   end