diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 2eca5e70334..eb444192f06 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -302,14 +302,30 @@ class UsersController < ApplicationController file = params[:file] || params[:files].first - unless SiteSetting.authorized_image?(file) + # Only allow url uploading for API users + # TODO: Does not protect from huge uploads + # https://github.com/discourse/discourse/pull/1512 + if file.is_a?(String) && is_api? + adapted = ::UriAdapter.new(file) + file = adapted.build_uploaded_file + filesize = adapted.file_size + elsif file.is_a?(String) return render status: 422, text: I18n.t("upload.images.unknown_image_type") end # check the file size (note: this might also be done in the web server) - filesize = File.size(file.tempfile) + filesize ||= File.size(file.tempfile) max_size_kb = SiteSetting.max_image_size_kb * 1024 - return render status: 413, text: I18n.t("upload.images.too_large", max_size_kb: max_size_kb) if filesize > max_size_kb + + if filesize > max_size_kb + return render status: 413, + text: I18n.t("upload.images.too_large", + max_size_kb: max_size_kb) + end + + unless SiteSetting.authorized_image?(file) + return render status: 422, text: I18n.t("upload.images.unknown_image_type") + end upload = Upload.create_for(user.id, file, filesize) @@ -326,6 +342,8 @@ class UsersController < ApplicationController height: upload.height, } + rescue Discourse::InvalidParameters + render status: 422, text: I18n.t("upload.images.unknown_image_type") rescue FastImage::ImageFetchFailure render status: 422, text: I18n.t("upload.images.fetch_failure") rescue FastImage::UnknownImageType diff --git a/app/services/uri_adapter.rb b/app/services/uri_adapter.rb new file mode 100644 index 00000000000..23b48776959 --- /dev/null +++ b/app/services/uri_adapter.rb @@ -0,0 +1,64 @@ +# For converting urls to files +class UriAdapter + + attr_reader :target, :content, :tempfile, :original_filename + + def initialize(target) + raise Discourse::InvalidParameters unless target =~ /^https?:\/\// + + @target = URI(target) + @original_filename = ::File.basename(@target.path) + @content = download_content + @tempfile = TempfileFactory.new.generate(@original_filename) + end + + def download_content + open(target) + end + + def copy_to_tempfile(src) + while data = src.read(16*1024) + tempfile.write(data) + end + src.close + tempfile.rewind + tempfile + end + + def file_size + content.size + end + + def build_uploaded_file + return if (SiteSetting.max_image_size_kb * 1024) < file_size + + copy_to_tempfile(content) + content_type = content.content_type if content.respond_to?(:content_type) + content_type ||= "text/html" + + ActionDispatch::Http::UploadedFile.new( tempfile: tempfile, + filename: original_filename, + type: content_type + ) + end +end + +# From https://github.com/thoughtbot/paperclip/blob/master/lib/paperclip/tempfile_factory.rb +class TempfileFactory + ILLEGAL_FILENAME_CHARACTERS = /^~/ + + def generate(name) + @name = name + file = Tempfile.new([basename, extension]) + file.binmode + file + end + + def extension + File.extname(@name) + end + + def basename + File.basename(@name, extension).gsub(ILLEGAL_FILENAME_CHARACTERS, '_') + end +end \ No newline at end of file diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index d417327ba50..5fc9d91ad4c 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -954,42 +954,95 @@ describe UsersController do }) end - it 'raises an error when you don\'t have permission to upload an avatar' do - Guardian.any_instance.expects(:can_edit?).with(user).returns(false) - xhr :post, :upload_avatar, username: user.username - response.should be_forbidden + describe "with uploaded file" do + + it 'raises an error when you don\'t have permission to upload an avatar' do + Guardian.any_instance.expects(:can_edit?).with(user).returns(false) + xhr :post, :upload_avatar, username: user.username + response.should be_forbidden + end + + it 'rejects large images' do + SiteSetting.stubs(:max_image_size_kb).returns(1) + xhr :post, :upload_avatar, username: user.username, file: avatar + response.status.should eq 413 + end + + it 'rejects unauthorized images' do + SiteSetting.stubs(:authorized_image?).returns(false) + xhr :post, :upload_avatar, username: user.username, file: avatar + response.status.should eq 422 + end + + it 'is successful' do + upload = Fabricate(:upload) + Upload.expects(:create_for).returns(upload) + # enqueues the avatar generator job + Jobs.expects(:enqueue).with(:generate_avatars, { user_id: user.id, upload_id: upload.id }) + xhr :post, :upload_avatar, username: user.username, file: avatar + user.reload + # erase the previous template + user.uploaded_avatar_template.should == nil + # link to the right upload + user.uploaded_avatar.id.should == upload.id + # automatically set "use_uploaded_avatar" + user.use_uploaded_avatar.should == true + # returns the url, width and height of the uploaded image + json = JSON.parse(response.body) + json['url'].should == "/uploads/default/1/1234567890123456.jpg" + json['width'].should == 100 + json['height'].should == 200 + end end - it 'rejects large images' do - SiteSetting.stubs(:max_image_size_kb).returns(1) - xhr :post, :upload_avatar, username: user.username, file: avatar - response.status.should eq 413 - end + describe "with url" do + let(:avatar_url) { "http://cdn.discourse.org/assets/logo.png" } - it 'rejects unauthorized images' do - SiteSetting.stubs(:authorized_image?).returns(false) - xhr :post, :upload_avatar, username: user.username, file: avatar - response.status.should eq 422 - end + before :each do + UsersController.any_instance.stubs(:is_api?).returns(true) + end + + describe "correct urls" do + before :each do + UriAdapter.any_instance.stubs(:open).returns StringIO.new(fixture_file("images/logo.png")) + end + + it 'rejects large images' do + SiteSetting.stubs(:max_image_size_kb).returns(1) + xhr :post, :upload_avatar, username: user.username, file: avatar_url + response.status.should eq 413 + end + + it 'rejects unauthorized images' do + SiteSetting.stubs(:authorized_image?).returns(false) + xhr :post, :upload_avatar, username: user.username, file: avatar_url + response.status.should eq 422 + end + + it 'is successful' do + upload = Fabricate(:upload) + Upload.expects(:create_for).returns(upload) + # enqueues the avatar generator job + Jobs.expects(:enqueue).with(:generate_avatars, { user_id: user.id, upload_id: upload.id }) + xhr :post, :upload_avatar, username: user.username, file: avatar_url + user.reload + user.uploaded_avatar_template.should == nil + user.uploaded_avatar.id.should == upload.id + user.use_uploaded_avatar.should == true + + # returns the url, width and height of the uploaded image + json = JSON.parse(response.body) + json['url'].should == "/uploads/default/1/1234567890123456.jpg" + json['width'].should == 100 + json['height'].should == 200 + end + end + + it "should handle malformed urls" do + xhr :post, :upload_avatar, username: user.username, file: "foobar" + response.status.should eq 422 + end - it 'is successful' do - upload = Fabricate(:upload) - Upload.expects(:create_for).returns(upload) - # enqueues the avatar generator job - Jobs.expects(:enqueue).with(:generate_avatars, { user_id: user.id, upload_id: upload.id }) - xhr :post, :upload_avatar, username: user.username, file: avatar - user.reload - # erase the previous template - user.uploaded_avatar_template.should == nil - # link to the right upload - user.uploaded_avatar.id.should == upload.id - # automatically set "use_uploaded_avatar" - user.use_uploaded_avatar.should == true - # returns the url, width and height of the uploaded image - json = JSON.parse(response.body) - json['url'].should == "/uploads/default/1/1234567890123456.jpg" - json['width'].should == 100 - json['height'].should == 200 end end diff --git a/spec/services/uri_adapter_spec.rb b/spec/services/uri_adapter_spec.rb new file mode 100644 index 00000000000..7bbde1722d2 --- /dev/null +++ b/spec/services/uri_adapter_spec.rb @@ -0,0 +1,51 @@ +require 'spec_helper' + +describe UriAdapter do + let(:target) { "http://cdn.discourse.org/assets/logo.png" } + let(:response) { StringIO.new(fixture_file("images/logo.png")) } + + before :each do + response.stubs(:content_type).returns("image/png") + UriAdapter.any_instance.stubs(:open).returns(response) + end + + subject { UriAdapter.new(target) } + + describe "#initialize" do + + it "has a target" do + subject.target.should be_instance_of(URI::HTTP) + end + + it "has content" do + subject.content.should == response + end + + it "has an original_filename" do + subject.original_filename.should == "logo.png" + end + + it "has a tempfile" do + subject.tempfile.should be_instance_of Tempfile + end + + end + + describe "#copy_to_tempfile" do + it "does not allow files bigger then max_image_size_kb" do + SiteSetting.stubs(:max_image_size_kb).returns(1) + subject.build_uploaded_file.should == nil + end + end + + describe "#build_uploaded_file" do + it "returns an uploaded file" do + file = subject.build_uploaded_file + file.should be_instance_of(ActionDispatch::Http::UploadedFile) + file.content_type.should == "image/png" + file.original_filename.should == "logo.png" + file.tempfile.should be_instance_of Tempfile + end + end + +end \ No newline at end of file