From 49b2f7c0a2aa41b1da77b652415078e19fcdcad8 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 4 Jan 2020 01:54:07 +0100 Subject: [PATCH] Fix base64-encoded file uploads not being possible (#12748) Fix #3804, Fix #5776 --- .../admin/custom_emojis_controller.rb | 4 --- app/controllers/api/v1/media_controller.rb | 3 -- app/controllers/application_controller.rb | 1 + .../concerns/obfuscate_filename.rb | 16 ---------- .../settings/profiles_controller.rb | 5 ---- app/models/concerns/attachmentable.rb | 11 +++++++ app/models/media_attachment.rb | 3 ++ config/initializers/paperclip.rb | 2 ++ .../controllers/api/proofs_controller_spec.rb | 5 +--- .../concerns/obfuscate_filename_spec.rb | 30 ------------------- spec/models/account_spec.rb | 1 + spec/models/media_attachment_spec.rb | 18 +++++++++++ .../models/concerns/account_avatar.rb | 20 +++++++++++++ .../models/concerns/account_header.rb | 23 ++++++++++++++ 14 files changed, 80 insertions(+), 62 deletions(-) delete mode 100644 app/controllers/concerns/obfuscate_filename.rb delete mode 100644 spec/controllers/concerns/obfuscate_filename_spec.rb create mode 100644 spec/support/examples/models/concerns/account_header.rb diff --git a/app/controllers/admin/custom_emojis_controller.rb b/app/controllers/admin/custom_emojis_controller.rb index 2af90f0513c..a446465c93f 100644 --- a/app/controllers/admin/custom_emojis_controller.rb +++ b/app/controllers/admin/custom_emojis_controller.rb @@ -2,10 +2,6 @@ module Admin class CustomEmojisController < BaseController - include ObfuscateFilename - - obfuscate_filename [:custom_emoji, :image] - def index authorize :custom_emoji, :index? diff --git a/app/controllers/api/v1/media_controller.rb b/app/controllers/api/v1/media_controller.rb index aaa93b61583..81825db1552 100644 --- a/app/controllers/api/v1/media_controller.rb +++ b/app/controllers/api/v1/media_controller.rb @@ -4,9 +4,6 @@ class Api::V1::MediaController < Api::BaseController before_action -> { doorkeeper_authorize! :write, :'write:media' } before_action :require_user! - include ObfuscateFilename - obfuscate_filename :file - respond_to :json def create diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3f92053813f..0cfa2b38606 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -24,6 +24,7 @@ class ApplicationController < ActionController::Base rescue_from ActionController::InvalidAuthenticityToken, with: :unprocessable_entity rescue_from ActionController::UnknownFormat, with: :not_acceptable rescue_from ActionController::ParameterMissing, with: :bad_request + rescue_from Paperclip::AdapterRegistry::NoHandlerError, with: :bad_request rescue_from ActiveRecord::RecordNotFound, with: :not_found rescue_from Mastodon::NotPermittedError, with: :forbidden rescue_from HTTP::Error, OpenSSL::SSL::SSLError, with: :internal_server_error diff --git a/app/controllers/concerns/obfuscate_filename.rb b/app/controllers/concerns/obfuscate_filename.rb deleted file mode 100644 index 22736ec3abf..00000000000 --- a/app/controllers/concerns/obfuscate_filename.rb +++ /dev/null @@ -1,16 +0,0 @@ -# frozen_string_literal: true - -module ObfuscateFilename - extend ActiveSupport::Concern - - class_methods do - def obfuscate_filename(path) - before_action do - file = params.dig(*path) - next if file.nil? - - file.original_filename = SecureRandom.hex(8) + File.extname(file.original_filename) - end - end - end -end diff --git a/app/controllers/settings/profiles_controller.rb b/app/controllers/settings/profiles_controller.rb index 8b640cdca1c..19a7ce157f0 100644 --- a/app/controllers/settings/profiles_controller.rb +++ b/app/controllers/settings/profiles_controller.rb @@ -1,16 +1,11 @@ # frozen_string_literal: true class Settings::ProfilesController < Settings::BaseController - include ObfuscateFilename - layout 'admin' before_action :authenticate_user! before_action :set_account - obfuscate_filename [:account, :avatar] - obfuscate_filename [:account, :header] - def show @account.build_fields end diff --git a/app/models/concerns/attachmentable.rb b/app/models/concerns/attachmentable.rb index 3bbc6453c7f..1e8c4806f67 100644 --- a/app/models/concerns/attachmentable.rb +++ b/app/models/concerns/attachmentable.rb @@ -9,6 +9,7 @@ module Attachmentable GIF_MATRIX_LIMIT = 921_600 # 1280x720px included do + before_post_process :obfuscate_file_name before_post_process :set_file_extensions before_post_process :check_image_dimensions before_post_process :set_file_content_type @@ -68,4 +69,14 @@ module Attachmentable rescue Terrapin::CommandLineError '' end + + def obfuscate_file_name + self.class.attachment_definitions.each_key do |attachment_name| + attachment = send(attachment_name) + + next if attachment.blank? + + attachment.instance_write :file_name, SecureRandom.hex(8) + File.extname(attachment.instance_read(:file_name)) + end + end end diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index 573ef5dfc18..1fd0adfd02b 100644 --- a/app/models/media_attachment.rb +++ b/app/models/media_attachment.rb @@ -202,9 +202,12 @@ class MediaAttachment < ApplicationRecord end after_commit :reset_parent_cache, on: :update + before_create :prepare_description, unless: :local? before_create :set_shortcode + before_post_process :set_type_and_extension + before_save :set_meta class << self diff --git a/config/initializers/paperclip.rb b/config/initializers/paperclip.rb index 5109baff701..8909678d653 100644 --- a/config/initializers/paperclip.rb +++ b/config/initializers/paperclip.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +Paperclip::DataUriAdapter.register + Paperclip.interpolates :filename do |attachment, style| if style == :original attachment.original_filename diff --git a/spec/controllers/api/proofs_controller_spec.rb b/spec/controllers/api/proofs_controller_spec.rb index dbde4927f14..2fe6150052c 100644 --- a/spec/controllers/api/proofs_controller_spec.rb +++ b/spec/controllers/api/proofs_controller_spec.rb @@ -85,10 +85,7 @@ describe Api::ProofsController do end it 'has the correct avatar url' do - first_part = 'https://cb6e6126.ngrok.io/system/accounts/avatars/' - last_part = 'original/avatar.gif' - - expect(body_as_json[:avatar]).to match /#{Regexp.quote(first_part)}(?:\d{3,5}\/){3}#{Regexp.quote(last_part)}/ + expect(body_as_json[:avatar]).to match "https://cb6e6126.ngrok.io#{alice.avatar.url}" end end end diff --git a/spec/controllers/concerns/obfuscate_filename_spec.rb b/spec/controllers/concerns/obfuscate_filename_spec.rb deleted file mode 100644 index e06d53c0385..00000000000 --- a/spec/controllers/concerns/obfuscate_filename_spec.rb +++ /dev/null @@ -1,30 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe ApplicationController, type: :controller do - controller do - include ObfuscateFilename - - obfuscate_filename :file - - def file - render plain: params[:file]&.original_filename - end - end - - before do - routes.draw { get 'file' => 'anonymous#file' } - end - - it 'obfusticates filename if the given parameter is specified' do - file = fixture_file_upload('files/imports.txt', 'text/plain') - post 'file', params: { file: file } - expect(response.body).to end_with '.txt' - expect(response.body).not_to include 'imports' - end - - it 'does nothing if the given parameter is not specified' do - post 'file' - end -end diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index b2f6234cb9f..3cca9b3439a 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -823,4 +823,5 @@ RSpec.describe Account, type: :model do end include_examples 'AccountAvatar', :account + include_examples 'AccountHeader', :account end diff --git a/spec/models/media_attachment_spec.rb b/spec/models/media_attachment_spec.rb index 7ddfba7ed9b..a275621a134 100644 --- a/spec/models/media_attachment_spec.rb +++ b/spec/models/media_attachment_spec.rb @@ -133,6 +133,24 @@ RSpec.describe MediaAttachment, type: :model do expect(media.file.meta["small"]["height"]).to eq 327 expect(media.file.meta["small"]["aspect"]).to eq 490.0 / 327 end + + it 'gives the file a random name' do + expect(media.file_file_name).to_not eq 'attachment.jpg' + end + end + + describe 'base64-encoded jpeg' do + let(:base64_attachment) { "data:image/jpeg;base64,#{Base64.encode64(attachment_fixture('attachment.jpg').read)}" } + let(:media) { MediaAttachment.create(account: Fabricate(:account), file: base64_attachment) } + + it 'saves media attachment' do + expect(media.persisted?).to be true + expect(media.file).to_not be_nil + end + + it 'gives the file a file name' do + expect(media.file_file_name).to_not be_blank + end end describe 'descriptions for remote attachments' do diff --git a/spec/support/examples/models/concerns/account_avatar.rb b/spec/support/examples/models/concerns/account_avatar.rb index f2a8a2459c5..2180f52739f 100644 --- a/spec/support/examples/models/concerns/account_avatar.rb +++ b/spec/support/examples/models/concerns/account_avatar.rb @@ -16,4 +16,24 @@ shared_examples 'AccountAvatar' do |fabricator| end end end + + describe 'base64-encoded files' do + let(:base64_attachment) { "data:image/jpeg;base64,#{Base64.encode64(attachment_fixture('attachment.jpg').read)}" } + let(:account) { Fabricate(fabricator, avatar: base64_attachment) } + + it 'saves avatar' do + expect(account.persisted?).to be true + expect(account.avatar).to_not be_nil + end + + it 'gives the avatar a file name' do + expect(account.avatar_file_name).to_not be_blank + end + + it 'saves a new avatar under a different file name' do + previous_file_name = account.avatar_file_name + account.update(avatar: base64_attachment) + expect(account.avatar_file_name).to_not eq previous_file_name + end + end end diff --git a/spec/support/examples/models/concerns/account_header.rb b/spec/support/examples/models/concerns/account_header.rb new file mode 100644 index 00000000000..77ee0e62901 --- /dev/null +++ b/spec/support/examples/models/concerns/account_header.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +shared_examples 'AccountHeader' do |fabricator| + describe 'base64-encoded files' do + let(:base64_attachment) { "data:image/jpeg;base64,#{Base64.encode64(attachment_fixture('attachment.jpg').read)}" } + let(:account) { Fabricate(fabricator, header: base64_attachment) } + + it 'saves header' do + expect(account.persisted?).to be true + expect(account.header).to_not be_nil + end + + it 'gives the header a file name' do + expect(account.header_file_name).to_not be_blank + end + + it 'saves a new header under a different file name' do + previous_file_name = account.header_file_name + account.update(header: base64_attachment) + expect(account.header_file_name).to_not eq previous_file_name + end + end +end