From 0a70e51f741621710f96011a8905c4e8ba9611f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Wed, 22 Apr 2015 19:50:25 +0200 Subject: [PATCH 1/2] Add a token the filename for exported user data Also redirect to it for download, for Amazon S3 compatibility. Prior to this patch an attacker could obtain an users export by guessing the filename with a high chance of success. Fully authenticating the download request is a lot harder due to our diverse deployment scenarios. This brings the used method in line with the photo export feature. Thanks to @tomekr for the report. --- app/controllers/users_controller.rb | 2 +- app/uploaders/exported_photos.rb | 8 ++------ app/uploaders/exported_user.rb | 4 ++-- app/uploaders/secure_uploader.rb | 7 +++++++ spec/controllers/users_controller_spec.rb | 7 +++---- 5 files changed, 15 insertions(+), 13 deletions(-) create mode 100644 app/uploaders/secure_uploader.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index eff00e138..00932312a 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -140,7 +140,7 @@ class UsersController < ApplicationController end def download_profile - send_data File.open(current_user.export.path).read, type: :json, filename: current_user.export.filename + redirect_to current_user.export.url end def export_photos diff --git a/app/uploaders/exported_photos.rb b/app/uploaders/exported_photos.rb index d68f94e0e..0e4465cdd 100644 --- a/app/uploaders/exported_photos.rb +++ b/app/uploaders/exported_photos.rb @@ -2,7 +2,7 @@ # licensed under the Affero General Public License version 3 or later. See # the COPYRIGHT file. -class ExportedPhotos < CarrierWave::Uploader::Base +class ExportedPhotos < SecureUploader def store_dir "uploads/users" @@ -12,10 +12,6 @@ class ExportedPhotos < CarrierWave::Uploader::Base "#{model.username}_photos_#{secure_token}.zip" if original_filename.present? end - protected - def secure_token(bytes = 16) - var = :"@#{mounted_as}_secure_token" - model.instance_variable_get(var) or model.instance_variable_set(var, SecureRandom.urlsafe_base64(bytes)) - end + end diff --git a/app/uploaders/exported_user.rb b/app/uploaders/exported_user.rb index ff323ecce..a7c4d5a2b 100644 --- a/app/uploaders/exported_user.rb +++ b/app/uploaders/exported_user.rb @@ -2,7 +2,7 @@ # licensed under the Affero General Public License version 3 or later. See # the COPYRIGHT file. -class ExportedUser < CarrierWave::Uploader::Base +class ExportedUser < SecureUploader def store_dir "uploads/users" @@ -13,7 +13,7 @@ class ExportedUser < CarrierWave::Uploader::Base end def filename - "#{model.username}_diaspora_data.json.gz" + "#{model.username}_diaspora_data_#{secure_token}.json.gz" end end diff --git a/app/uploaders/secure_uploader.rb b/app/uploaders/secure_uploader.rb new file mode 100644 index 000000000..08bbed1c7 --- /dev/null +++ b/app/uploaders/secure_uploader.rb @@ -0,0 +1,7 @@ +class SecureUploader < CarrierWave::Uploader::Base + protected + def secure_token(bytes = 16) + var = :"@#{mounted_as}_secure_token" + model.instance_variable_get(var) or model.instance_variable_set(var, SecureRandom.urlsafe_base64(bytes)) + end +end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 19a3819b9..032160cec 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -24,8 +24,7 @@ describe UsersController, :type => :controller do it "downloads a user's export file" do @user.perform_export! get :download_profile - parsed = JSON.parse(ActiveSupport::Gzip.decompress(response.body)) - expect(parsed['user']['username']).to eq @user.username + expect(response).to redirect_to(@user.export.url) end end @@ -37,7 +36,7 @@ describe UsersController, :type => :controller do expect(response).to redirect_to(edit_user_path) end end - + describe '#download_photos' do it "redirects to user's photos zip file" do @user.perform_export_photos! @@ -74,7 +73,7 @@ describe UsersController, :type => :controller do get :public, :username => @user.username, :format => :atom expect(response.body).to include('a href') end - + it 'includes reshares in the atom feed' do reshare = FactoryGirl.create(:reshare, :author => @user.person) get :public, :username => @user.username, :format => :atom From 6e546ff2bfc3ac0fc749617c804f3fba156ad050 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Wed, 22 Apr 2015 20:14:58 +0200 Subject: [PATCH 2/2] Trigger exports through a POST request GET requests don't get any CSRF protection by Rails, thus these sensitive actions should be better protected. Thanks to @tomekr for the report. --- app/views/users/edit.html.haml | 8 ++++---- config/routes.rb | 4 ++-- spec/controllers/users_controller_spec.rb | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/views/users/edit.html.haml b/app/views/users/edit.html.haml index b722e5a93..d94681dbc 100644 --- a/app/views/users/edit.html.haml +++ b/app/views/users/edit.html.haml @@ -179,9 +179,9 @@ = link_to t('.download_export'), download_profile_user_path, class: "btn btn-success" %h6 = t('.last_exported_at', timestamp: current_user.exported_at) - = link_to t('.request_export_update'), export_profile_user_path, class: "btn" + = link_to t(".request_export_update"), export_profile_user_path, method: :post, class: "btn" - else - = link_to t('.request_export'), export_profile_user_path, :class => "btn" + = link_to t(".request_export"), export_profile_user_path, method: :post, class: "btn" - if current_user.exporting_photos .small-horizontal-spacer @@ -191,10 +191,10 @@ = link_to t('.download_export_photos'), download_photos_user_path, class: "btn btn-success" %h6 = t('.last_exported_at', timestamp: current_user.exported_photos_at) - = link_to t('.request_export_photos_update'), export_photos_user_path, class: "btn" + = link_to t(".request_export_photos_update"), export_photos_user_path, method: :post, class: "btn" - else .small-horizontal-spacer - = link_to t('.request_export_photos'), export_photos_user_path, :class => "btn" + = link_to t(".request_export_photos"), export_photos_user_path, method: :post, class: "btn" .span6 %h3 diff --git a/config/routes.rb b/config/routes.rb index bce7e3e6a..c7f924bae 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -101,9 +101,9 @@ Diaspora::Application.routes.draw do resource :user, :only => [:edit, :update, :destroy], :shallow => true do get :getting_started_completed - get :export_profile + post :export_profile get :download_profile - get :export_photos + post :export_photos get :download_photos end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 032160cec..40006a3af 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -14,7 +14,7 @@ describe UsersController, :type => :controller do describe '#export_profile' do it 'queues an export job' do expect(@user).to receive :queue_export - get :export_profile + post :export_profile expect(request.flash[:notice]).to eql(I18n.t('users.edit.export_in_progress')) expect(response).to redirect_to(edit_user_path) end @@ -31,7 +31,7 @@ describe UsersController, :type => :controller do describe '#export_photos' do it 'queues an export photos job' do expect(@user).to receive :queue_export_photos - get :export_photos + post :export_photos expect(request.flash[:notice]).to eql(I18n.t('users.edit.export_photos_in_progress')) expect(response).to redirect_to(edit_user_path) end