jro_apps/redmine Commit - r2600:15a14e55cdb9

Returns a 404 error when trying to view/download an attachment that can't be read from disk....

Jean-Philippe Lang -

r2600:15a14e55cdb9

parent child

Context file:

r2600:15a14e55cdb9

Collapse all files

app/controllers/attachments_controller.rb +6 -1

             # Redmine - project management software
             # Copyright (C) 2006-2008  Jean-Philippe Lang
             #
             # This program is free software; you can redistribute it and/or
             # modify it under the terms of the GNU General Public License
             # as published by the Free Software Foundation; either version 2
             # of the License, or (at your option) any later version.
             #
             # This program is distributed in the hope that it will be useful,
             # but WITHOUT ANY WARRANTY; without even the implied warranty of
             # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
             # GNU General Public License for more details.
             #
             # You should have received a copy of the GNU General Public License
             # along with this program; if not, write to the Free Software
             # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
             class AttachmentsController < ApplicationController
               before_filter :find_project
-              before_filter :read_authorize, :except => :destroy
+              before_filter :file_readable, :read_authorize, :except => :destroy
               before_filter :delete_authorize, :only => :destroy
               verify :method => :post, :only => :destroy
               def show
                 if @attachment.is_diff?
                   @diff = File.new(@attachment.diskfile, "rb").read
                   render :action => 'diff'
                 elsif @attachment.is_text? && @attachment.filesize <= Setting.file_max_size_displayed.to_i.kilobyte
                   @content = File.new(@attachment.diskfile, "rb").read
                   render :action => 'file'
                 else
                   download
                 end
               end
               def download
                 if @attachment.container.is_a?(Version) || @attachment.container.is_a?(Project)
                   @attachment.increment_download
                 end
                 # images are sent inline
                 send_file @attachment.diskfile, :filename => filename_for_content_disposition(@attachment.filename),
                                                 :type => @attachment.content_type,
                                                 :disposition => (@attachment.image? ? 'inline' : 'attachment')
               end
               def destroy
                 # Make sure association callbacks are called
                 @attachment.container.attachments.delete(@attachment)
                 redirect_to :back
               rescue ::ActionController::RedirectBackError
                 redirect_to :controller => 'projects', :action => 'show', :id => @project
               end
             private
               def find_project
                 @attachment = Attachment.find(params[:id])
                 # Show 404 if the filename in the url is wrong
                 raise ActiveRecord::RecordNotFound if params[:filename] && params[:filename] != @attachment.filename
                 @project = @attachment.project
               rescue ActiveRecord::RecordNotFound
                 render_404
               end
+              # Checks that the file exists and is readable
+              def file_readable
+                @attachment.readable? ? true : render_404
+              end
               def read_authorize
                 @attachment.visible? ? true : deny_access
               end
               def delete_authorize
                 @attachment.deletable? ? true : deny_access
               end
             end

app/models/attachment.rb +5 0

             # redMine - project management software
             # Copyright (C) 2006-2007  Jean-Philippe Lang
             #
             # This program is free software; you can redistribute it and/or
             # modify it under the terms of the GNU General Public License
             # as published by the Free Software Foundation; either version 2
             # of the License, or (at your option) any later version.
             #
             # This program is distributed in the hope that it will be useful,
             # but WITHOUT ANY WARRANTY; without even the implied warranty of
             # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
             # GNU General Public License for more details.
             #
             # You should have received a copy of the GNU General Public License
             # along with this program; if not, write to the Free Software
             # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
             require "digest/md5"
             class Attachment < ActiveRecord::Base
               belongs_to :container, :polymorphic => true
               belongs_to :author, :class_name => "User", :foreign_key => "author_id"
               validates_presence_of :container, :filename, :author
               validates_length_of :filename, :maximum => 255
               validates_length_of :disk_filename, :maximum => 255
               acts_as_event :title => :filename,
                             :url => Proc.new {|o| {:controller => 'attachments', :action => 'download', :id => o.id, :filename => o.filename}}
               acts_as_activity_provider :type => 'files',
                                         :permission => :view_files,
                                         :author_key => :author_id,
                                         :find_options => {:select => "#{Attachment.table_name}.*",
                                                           :joins => "LEFT JOIN #{Version.table_name} ON #{Attachment.table_name}.container_type='Version' AND #{Version.table_name}.id = #{Attachment.table_name}.container_id " +
                                                                     "LEFT JOIN #{Project.table_name} ON #{Version.table_name}.project_id = #{Project.table_name}.id OR ( #{Attachment.table_name}.container_type='Project' AND #{Attachment.table_name}.container_id = #{Project.table_name}.id )"}
               acts_as_activity_provider :type => 'documents',
                                         :permission => :view_documents,
                                         :author_key => :author_id,
                                         :find_options => {:select => "#{Attachment.table_name}.*",
                                                           :joins => "LEFT JOIN #{Document.table_name} ON #{Attachment.table_name}.container_type='Document' AND #{Document.table_name}.id = #{Attachment.table_name}.container_id " +
                                                                     "LEFT JOIN #{Project.table_name} ON #{Document.table_name}.project_id = #{Project.table_name}.id"}
               cattr_accessor :storage_path
               @@storage_path = "#{RAILS_ROOT}/files"
               def validate
                 if self.filesize > Setting.attachment_max_size.to_i.kilobytes
                   errors.add(:base, :too_long, :count => Setting.attachment_max_size.to_i.kilobytes)
                 end
               end
               def file=(incoming_file)
                 unless incoming_file.nil?
                   @temp_file = incoming_file
                   if @temp_file.size > 0
                     self.filename = sanitize_filename(@temp_file.original_filename)
                     self.disk_filename = Attachment.disk_filename(filename)
                     self.content_type = @temp_file.content_type.to_s.chomp
                     self.filesize = @temp_file.size
                   end
                 end
               end
               def file
                 nil
               end
               # Copies the temporary file to its final location
               # and computes its MD5 hash
               def before_save
                 if @temp_file && (@temp_file.size > 0)
                   logger.debug("saving '#{self.diskfile}'")
                   md5 = Digest::MD5.new
                   File.open(diskfile, "wb") do |f|
                     buffer = ""
                     while (buffer = @temp_file.read(8192))
                       f.write(buffer)
                       md5.update(buffer)
                     end
                   end
                   self.digest = md5.hexdigest
                 end
                 # Don't save the content type if it's longer than the authorized length
                 if self.content_type && self.content_type.length > 255
                   self.content_type = nil
                 end
               end
               # Deletes file on the disk
               def after_destroy
                 File.delete(diskfile) if !filename.blank? && File.exist?(diskfile)
               end
               # Returns file's location on disk
               def diskfile
                 "#{@@storage_path}/#{self.disk_filename}"
               end
               def increment_download
                 increment!(:downloads)
               end
               def project
                 container.project
               end
               def visible?(user=User.current)
                 container.attachments_visible?(user)
               end
               def deletable?(user=User.current)
                 container.attachments_deletable?(user)
               end
               def image?
                 self.filename =~ /\.(jpe?g|gif|png)$/i
               end
               def is_text?
                 Redmine::MimeType.is_type?('text', filename)
               end
               def is_diff?
                 self.filename =~ /\.(patch|diff)$/i
               end
+              # Returns true if the file is readable
+              def readable?
+                File.readable?(diskfile)
+              end
             private
               def sanitize_filename(value)
                 # get only the filename, not the whole path
                 just_filename = value.gsub(/^.*(\\|\/)/, '')
                 # NOTE: File.basename doesn't work right with Windows paths on Unix
                 # INCORRECT: just_filename = File.basename(value.gsub('\\\\', '/'))
                 # Finally, replace all non alphanumeric, hyphens or periods with underscore
                 @filename = just_filename.gsub(/[^\w\.\-]/,'_')
               end
               # Returns an ASCII or hashed filename
               def self.disk_filename(filename)
                 df = DateTime.now.strftime("%y%m%d%H%M%S") + "_"
                 if filename =~ %r{^[a-zA-Z0-9_\.\-]*$}
                   df << filename
                 else
                   df << Digest::MD5.hexdigest(filename)
                   # keep the extension if any
                   df << $1 if filename =~ %r{(\.[a-zA-Z0-9]+)$}
                 end
                 df
               end
             end

test/functional/attachments_controller_test.rb +7 -2

             # redMine - project management software
             # Copyright (C) 2006-2008  Jean-Philippe Lang
             #
             # This program is free software; you can redistribute it and/or
             # modify it under the terms of the GNU General Public License
             # as published by the Free Software Foundation; either version 2
             # of the License, or (at your option) any later version.
             #
             # This program is distributed in the hope that it will be useful,
             # but WITHOUT ANY WARRANTY; without even the implied warranty of
             # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
             # GNU General Public License for more details.
             #
             # You should have received a copy of the GNU General Public License
             # along with this program; if not, write to the Free Software
             # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
             require File.dirname(__FILE__) + '/../test_helper'
             require 'attachments_controller'
             # Re-raise errors caught by the controller.
             class AttachmentsController; def rescue_action(e) raise e end; end
             class AttachmentsControllerTest < Test::Unit::TestCase
-              fixtures :users, :projects, :roles, :members, :enabled_modules, :issues, :attachments,
+              fixtures :users, :projects, :roles, :members, :enabled_modules, :issues, :trackers, :attachments,
-                       :versions, :wiki_pages, :wikis
+                       :versions, :wiki_pages, :wikis, :documents
               def setup
                 @controller = AttachmentsController.new
                 @request    = ActionController::TestRequest.new
                 @response   = ActionController::TestResponse.new
                 Attachment.storage_path = "#{RAILS_ROOT}/test/fixtures/files"
                 User.current = nil
               end
               def test_routing
                 assert_routing('/attachments/1', :controller => 'attachments', :action => 'show', :id => '1')
                 assert_routing('/attachments/1/filename.ext', :controller => 'attachments', :action => 'show', :id => '1', :filename => 'filename.ext')
                 assert_routing('/attachments/download/1', :controller => 'attachments', :action => 'download', :id => '1')
                 assert_routing('/attachments/download/1/filename.ext', :controller => 'attachments', :action => 'download', :id => '1', :filename => 'filename.ext')
               end
               def test_recognizes
                 assert_recognizes({:controller => 'attachments', :action => 'show', :id => '1'}, '/attachments/1')
                 assert_recognizes({:controller => 'attachments', :action => 'show', :id => '1'}, '/attachments/show/1')
                 assert_recognizes({:controller => 'attachments', :action => 'show', :id => '1', :filename => 'filename.ext'}, '/attachments/1/filename.ext')
                 assert_recognizes({:controller => 'attachments', :action => 'download', :id => '1'}, '/attachments/download/1')
                 assert_recognizes({:controller => 'attachments', :action => 'download', :id => '1', :filename => 'filename.ext'},'/attachments/download/1/filename.ext')
               end
               def test_show_diff
                 get :show, :id => 5
                 assert_response :success
                 assert_template 'diff'
                 assert_equal 'text/html', @response.content_type
               end
               def test_show_text_file
                 get :show, :id => 4
                 assert_response :success
                 assert_template 'file'
                 assert_equal 'text/html', @response.content_type
               end
               def test_show_text_file_should_send_if_too_big
                 Setting.file_max_size_displayed = 512
                 Attachment.find(4).update_attribute :filesize, 754.kilobyte
                 get :show, :id => 4
                 assert_response :success
                 assert_equal 'application/x-ruby', @response.content_type
               end
               def test_show_other
                 get :show, :id => 6
                 assert_response :success
                 assert_equal 'application/octet-stream', @response.content_type
               end
               def test_download_text_file
                 get :download, :id => 4
                 assert_response :success
                 assert_equal 'application/x-ruby', @response.content_type
               end
+              def test_download_missing_file
+                get :download, :id => 2
+                assert_response 404
+              end
               def test_anonymous_on_private_private
                 get :download, :id => 7
                 assert_redirected_to '/login?back_url=http%3A%2F%2Ftest.host%2Fattachments%2Fdownload%2F7'
               end
               def test_destroy_issue_attachment
                 issue = Issue.find(3)
                 @request.session[:user_id] = 2
                 assert_difference 'issue.attachments.count', -1 do
                   post :destroy, :id => 1
                 end
                 # no referrer
                 assert_redirected_to 'projects/ecookbook'
                 assert_nil Attachment.find_by_id(1)
                 j = issue.journals.find(:first, :order => 'created_on DESC')
                 assert_equal 'attachment', j.details.first.property
                 assert_equal '1', j.details.first.prop_key
                 assert_equal 'error281.txt', j.details.first.old_value
               end
               def test_destroy_wiki_page_attachment
                 @request.session[:user_id] = 2
                 assert_difference 'Attachment.count', -1 do
                   post :destroy, :id => 3
                   assert_response 302
                 end
               end
               def test_destroy_project_attachment
                 @request.session[:user_id] = 2
                 assert_difference 'Attachment.count', -1 do
                   post :destroy, :id => 8
                   assert_response 302
                 end
               end
               def test_destroy_version_attachment
                 @request.session[:user_id] = 2
                 assert_difference 'Attachment.count', -1 do
                   post :destroy, :id => 9
                   assert_response 302
                 end
               end
               def test_destroy_without_permission
                 post :destroy, :id => 3
                 assert_redirected_to '/login?back_url=http%3A%2F%2Ftest.host%2Fattachments%2Fdestroy%2F3'
                 assert Attachment.find_by_id(3)
               end
             end

General Comments 0

Write
Preview

You need to be logged in to leave comments. Login now

No TODOs yet

	Site-wide shortcuts
/	Use quick search box
g h	Goto home page
g g	Goto my private gists page
g G	Goto my public gists page
g 0-9	Goto bookmarked items from 0-9
n r	New repository page
n g	New gist page

	Repositories
g s	Goto summary page
g c	Goto changelog page
g f	Goto files page
g F	Goto files page with file search activated
g p	Goto pull requests page
g o	Goto repository settings
g O	Goto repository access permissions settings
t s	Toggle sidebar on some pages