Fix base64-encoded file uploads not being possible (#12748)

Fix #3804, Fix #5776
This commit is contained in:
Eugen Rochko 2020-01-04 01:54:07 +01:00 committed by GitHub
parent 500276c99b
commit 49b2f7c0a2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 80 additions and 62 deletions

View file

@ -2,10 +2,6 @@
module Admin
class CustomEmojisController < BaseController
include ObfuscateFilename
obfuscate_filename [:custom_emoji, :image]
def index
authorize :custom_emoji, :index?

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -1,5 +1,7 @@
# frozen_string_literal: true
Paperclip::DataUriAdapter.register
Paperclip.interpolates :filename do |attachment, style|
if style == :original
attachment.original_filename

View file

@ -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

View file

@ -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

View file

@ -823,4 +823,5 @@ RSpec.describe Account, type: :model do
end
include_examples 'AccountAvatar', :account
include_examples 'AccountHeader', :account
end

View file

@ -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

View file

@ -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

View file

@ -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