diff options
| -rw-r--r-- | app/controllers/snippets_controller.rb | 2 | ||||
| -rw-r--r-- | app/controllers/uploads_controller.rb | 20 | ||||
| -rw-r--r-- | app/helpers/snippets_helper.rb | 10 | ||||
| -rw-r--r-- | app/uploaders/file_mover.rb | 82 | ||||
| -rw-r--r-- | app/views/layouts/snippets.html.haml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/osw-persist-tmp-snippet-uploads.yml | 5 | ||||
| -rw-r--r-- | config/routes/uploads.rb | 4 | ||||
| -rw-r--r-- | spec/controllers/snippets_controller_spec.rb | 4 | ||||
| -rw-r--r-- | spec/controllers/uploads_controller_spec.rb | 177 | ||||
| -rw-r--r-- | spec/features/snippets/user_creates_snippet_spec.rb | 2 | ||||
| -rw-r--r-- | spec/routing/uploads_routing_spec.rb | 13 | ||||
| -rw-r--r-- | spec/uploaders/file_mover_spec.rb | 133 |
12 files changed, 313 insertions, 144 deletions
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 8ea5450b4e8..fad036b8df8 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -137,7 +137,7 @@ class SnippetsController < ApplicationController def move_temporary_files params[:files].each do |file| - FileMover.new(file, @snippet).execute + FileMover.new(file, from_model: current_user, to_model: @snippet).execute end end end diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 5d28635232b..94bd18f70d4 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -41,7 +41,11 @@ class UploadsController < ApplicationController when Note can?(current_user, :read_project, model.project) when User - true + # We validate the current user has enough (writing) + # access to itself when a secret is given. + # For instance, user avatars are readable by anyone, + # while temporary, user snippet uploads are not. + !secret? || can?(current_user, :update_user, model) when Appearance true else @@ -56,9 +60,13 @@ class UploadsController < ApplicationController def authorize_create_access! return unless model - # for now we support only personal snippets comments. Only personal_snippet - # is allowed as a model to #create through routing. - authorized = can?(current_user, :create_note, model) + authorized = + case model + when User + can?(current_user, :update_user, model) + else + can?(current_user, :create_note, model) + end render_unauthorized unless authorized end @@ -75,6 +83,10 @@ class UploadsController < ApplicationController User === model || Appearance === model end + def secret? + params[:secret].present? + end + def upload_model_class MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError) end diff --git a/app/helpers/snippets_helper.rb b/app/helpers/snippets_helper.rb index ecb2b2d707b..6ccc1fb2ed1 100644 --- a/app/helpers/snippets_helper.rb +++ b/app/helpers/snippets_helper.rb @@ -1,6 +1,16 @@ # frozen_string_literal: true module SnippetsHelper + def snippets_upload_path(snippet, user) + return unless user + + if snippet&.persisted? + upload_path('personal_snippet', id: snippet.id) + else + upload_path('user', id: user.id) + end + end + def reliable_snippet_path(snippet, opts = nil) if snippet.project_id? project_snippet_path(snippet.project, snippet, opts) diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb index 236b7ed2b3d..12be1e2bb22 100644 --- a/app/uploaders/file_mover.rb +++ b/app/uploaders/file_mover.rb @@ -1,22 +1,29 @@ # frozen_string_literal: true class FileMover - attr_reader :secret, :file_name, :model, :update_field + include Gitlab::Utils::StrongMemoize - def initialize(file_path, model, update_field = :description) + attr_reader :secret, :file_name, :from_model, :to_model, :update_field + + def initialize(file_path, update_field = :description, from_model:, to_model:) @secret = File.split(File.dirname(file_path)).last @file_name = File.basename(file_path) - @model = model + @from_model = from_model + @to_model = to_model @update_field = update_field end def execute + temp_file_uploader.retrieve_from_store!(file_name) + return unless valid? + uploader.retrieve_from_store!(file_name) + move if update_markdown - uploader.record_upload + update_upload_model uploader.schedule_background_upload end end @@ -24,52 +31,77 @@ class FileMover private def valid? - Pathname.new(temp_file_path).realpath.to_path.start_with?( - (Pathname(temp_file_uploader.root) + temp_file_uploader.base_dir).to_path - ) + if temp_file_uploader.file_storage? + Pathname.new(temp_file_path).realpath.to_path.start_with?( + (Pathname(temp_file_uploader.root) + temp_file_uploader.base_dir).to_path + ) + else + temp_file_uploader.exists? + end end def move - FileUtils.mkdir_p(File.dirname(file_path)) - FileUtils.move(temp_file_path, file_path) + if temp_file_uploader.file_storage? + FileUtils.mkdir_p(File.dirname(file_path)) + FileUtils.move(temp_file_path, file_path) + else + uploader.copy_file(temp_file_uploader.file) + temp_file_uploader.upload.destroy! + end end def update_markdown - updated_text = model.read_attribute(update_field) - .gsub(temp_file_uploader.markdown_link, uploader.markdown_link) - model.update_attribute(update_field, updated_text) + updated_text = to_model.read_attribute(update_field) + .gsub(temp_file_uploader.markdown_link, uploader.markdown_link) + to_model.update_attribute(update_field, updated_text) rescue revert false end - def temp_file_path - return @temp_file_path if @temp_file_path + def update_upload_model + return unless upload = temp_file_uploader.upload + return if upload.destroyed? - temp_file_uploader.retrieve_from_store!(file_name) + upload.update!(model: to_model) + end - @temp_file_path = temp_file_uploader.file.path + def temp_file_path + strong_memoize(:temp_file_path) do + temp_file_uploader.file.path + end end def file_path - return @file_path if @file_path - - uploader.retrieve_from_store!(file_name) - - @file_path = uploader.file.path + strong_memoize(:file_path) do + uploader.file.path + end end def uploader - @uploader ||= PersonalFileUploader.new(model, secret: secret) + @uploader ||= + begin + uploader = PersonalFileUploader.new(to_model, secret: secret) + + # Enforcing a REMOTE object storage given FileUploader#retrieve_from_store! won't do it + # (there's no upload at the target yet). + if uploader.class.object_store_enabled? + uploader.object_store = ::ObjectStorage::Store::REMOTE + end + + uploader + end end def temp_file_uploader - @temp_file_uploader ||= PersonalFileUploader.new(nil, secret: secret) + @temp_file_uploader ||= PersonalFileUploader.new(from_model, secret: secret) end def revert - Rails.logger.warn("Markdown not updated, file move reverted for #{model}") + Rails.logger.warn("Markdown not updated, file move reverted for #{to_model}") - FileUtils.move(file_path, temp_file_path) + if temp_file_uploader.file_storage? + FileUtils.move(file_path, temp_file_path) + end end end diff --git a/app/views/layouts/snippets.html.haml b/app/views/layouts/snippets.html.haml index 5f986c81ff4..841b2a5e79c 100644 --- a/app/views/layouts/snippets.html.haml +++ b/app/views/layouts/snippets.html.haml @@ -1,9 +1,10 @@ - header_title _("Snippets"), snippets_path +- snippets_upload_path = snippets_upload_path(@snippet, current_user) - content_for :page_specific_javascripts do - - if @snippet && current_user + - if snippets_upload_path -# haml-lint:disable InlineJavaScript :javascript - window.uploads_path = "#{upload_path('personal_snippet', id: @snippet.id)}"; + window.uploads_path = "#{snippets_upload_path}"; = render template: "layouts/application" diff --git a/changelogs/unreleased/osw-persist-tmp-snippet-uploads.yml b/changelogs/unreleased/osw-persist-tmp-snippet-uploads.yml new file mode 100644 index 00000000000..9348626c41d --- /dev/null +++ b/changelogs/unreleased/osw-persist-tmp-snippet-uploads.yml @@ -0,0 +1,5 @@ +--- +title: Persist tmp snippet uploads at users +merge_request: +author: +type: security diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb index b594f55f8a0..920f8454ce2 100644 --- a/config/routes/uploads.rb +++ b/config/routes/uploads.rb @@ -7,7 +7,7 @@ scope path: :uploads do # show uploads for models, snippets (notes) available for now get '-/system/:model/:id/:secret/:filename', to: 'uploads#show', - constraints: { model: /personal_snippet/, id: /\d+/, filename: %r{[^/]+} } + constraints: { model: /personal_snippet|user/, id: /\d+/, filename: %r{[^/]+} } # show temporary uploads get '-/system/temp/:secret/:filename', @@ -28,7 +28,7 @@ scope path: :uploads do # create uploads for models, snippets (notes) available for now post ':model', to: 'uploads#create', - constraints: { model: /personal_snippet/, id: /\d+/ }, + constraints: { model: /personal_snippet|user/, id: /\d+/ }, as: 'upload' end diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index f8666a1986f..3aba02bf3ff 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -209,8 +209,8 @@ describe SnippetsController do context 'when the snippet description contains a file' do include FileMoverHelpers - let(:picture_file) { '/-/system/temp/secret56/picture.jpg' } - let(:text_file) { '/-/system/temp/secret78/text.txt' } + let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" } + let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" } let(:description) do "Description with picture:  and "\ "text: [text.txt](/uploads#{text_file})" diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index d27658e02cb..0876502a899 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -24,121 +24,160 @@ describe UploadsController do let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) } describe 'POST create' do - let(:model) { 'personal_snippet' } - let(:snippet) { create(:personal_snippet, :public) } let(:jpg) { fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg') } let(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } - context 'when a user does not have permissions to upload a file' do - it "returns 401 when the user is not logged in" do - post :create, params: { model: model, id: snippet.id }, format: :json + context 'snippet uploads' do + let(:model) { 'personal_snippet' } + let(:snippet) { create(:personal_snippet, :public) } - expect(response).to have_gitlab_http_status(401) - end + context 'when a user does not have permissions to upload a file' do + it "returns 401 when the user is not logged in" do + post :create, params: { model: model, id: snippet.id }, format: :json - it "returns 404 when user can't comment on a snippet" do - private_snippet = create(:personal_snippet, :private) + expect(response).to have_gitlab_http_status(401) + end - sign_in(user) - post :create, params: { model: model, id: private_snippet.id }, format: :json + it "returns 404 when user can't comment on a snippet" do + private_snippet = create(:personal_snippet, :private) - expect(response).to have_gitlab_http_status(404) - end - end + sign_in(user) + post :create, params: { model: model, id: private_snippet.id }, format: :json - context 'when a user is logged in' do - before do - sign_in(user) + expect(response).to have_gitlab_http_status(404) + end end - it "returns an error without file" do - post :create, params: { model: model, id: snippet.id }, format: :json + context 'when a user is logged in' do + before do + sign_in(user) + end - expect(response).to have_gitlab_http_status(422) - end + it "returns an error without file" do + post :create, params: { model: model, id: snippet.id }, format: :json - it "returns an error with invalid model" do - expect { post :create, params: { model: 'invalid', id: snippet.id }, format: :json } - .to raise_error(ActionController::UrlGenerationError) - end + expect(response).to have_gitlab_http_status(422) + end - it "returns 404 status when object not found" do - post :create, params: { model: model, id: 9999 }, format: :json + it "returns an error with invalid model" do + expect { post :create, params: { model: 'invalid', id: snippet.id }, format: :json } + .to raise_error(ActionController::UrlGenerationError) + end - expect(response).to have_gitlab_http_status(404) - end + it "returns 404 status when object not found" do + post :create, params: { model: model, id: 9999 }, format: :json - context 'with valid image' do - before do - post :create, params: { model: 'personal_snippet', id: snippet.id, file: jpg }, format: :json + expect(response).to have_gitlab_http_status(404) end - it 'returns a content with original filename, new link, and correct type.' do - expect(response.body).to match '\"alt\":\"rails_sample\"' - expect(response.body).to match "\"url\":\"/uploads" + context 'with valid image' do + before do + post :create, params: { model: 'personal_snippet', id: snippet.id, file: jpg }, format: :json + end + + it 'returns a content with original filename, new link, and correct type.' do + expect(response.body).to match '\"alt\":\"rails_sample\"' + expect(response.body).to match "\"url\":\"/uploads" + end + + it 'creates a corresponding Upload record' do + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq snippet + end + end end - it 'creates a corresponding Upload record' do - upload = Upload.last + context 'with valid non-image file' do + before do + post :create, params: { model: 'personal_snippet', id: snippet.id, file: txt }, format: :json + end - aggregate_failures do - expect(upload).to exist - expect(upload.model).to eq snippet + it 'returns a content with original filename, new link, and correct type.' do + expect(response.body).to match '\"alt\":\"doc_sample.txt\"' + expect(response.body).to match "\"url\":\"/uploads" + end + + it 'creates a corresponding Upload record' do + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq snippet + end end end end + end + + context 'user uploads' do + let(:model) { 'user' } + + it 'returns 401 when the user has no access' do + post :create, params: { model: 'user', id: user.id }, format: :json - context 'with valid non-image file' do + expect(response).to have_gitlab_http_status(401) + end + + context 'when user is logged in' do before do - post :create, params: { model: 'personal_snippet', id: snippet.id, file: txt }, format: :json + sign_in(user) + end + + subject do + post :create, params: { model: model, id: user.id, file: jpg }, format: :json end it 'returns a content with original filename, new link, and correct type.' do - expect(response.body).to match '\"alt\":\"doc_sample.txt\"' - expect(response.body).to match "\"url\":\"/uploads" + subject + + expect(response.body).to match '\"alt\":\"rails_sample\"' + expect(response.body).to match "\"url\":\"/uploads/-/system/user/#{user.id}/" end it 'creates a corresponding Upload record' do + expect { subject }.to change { Upload.count } + upload = Upload.last aggregate_failures do expect(upload).to exist - expect(upload.model).to eq snippet + expect(upload.model).to eq user end end - end - context 'temporal with valid image' do - subject do - post :create, params: { model: 'personal_snippet', file: jpg }, format: :json - end + context 'with valid non-image file' do + subject do + post :create, params: { model: model, id: user.id, file: txt }, format: :json + end - it 'returns a content with original filename, new link, and correct type.' do - subject + it 'returns a content with original filename, new link, and correct type.' do + subject - expect(response.body).to match '\"alt\":\"rails_sample\"' - expect(response.body).to match "\"url\":\"/uploads/-/system/temp" - end + expect(response.body).to match '\"alt\":\"doc_sample.txt\"' + expect(response.body).to match "\"url\":\"/uploads/-/system/user/#{user.id}/" + end - it 'does not create an Upload record' do - expect { subject }.not_to change { Upload.count } - end - end + it 'creates a corresponding Upload record' do + expect { subject }.to change { Upload.count } - context 'temporal with valid non-image file' do - subject do - post :create, params: { model: 'personal_snippet', file: txt }, format: :json + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq user + end + end end - it 'returns a content with original filename, new link, and correct type.' do - subject + it 'returns 404 when given user is not the logged in one' do + another_user = create(:user) - expect(response.body).to match '\"alt\":\"doc_sample.txt\"' - expect(response.body).to match "\"url\":\"/uploads/-/system/temp" - end + post :create, params: { model: model, id: another_user.id, file: txt }, format: :json - it 'does not create an Upload record' do - expect { subject }.not_to change { Upload.count } + expect(response).to have_gitlab_http_status(404) end end end diff --git a/spec/features/snippets/user_creates_snippet_spec.rb b/spec/features/snippets/user_creates_snippet_spec.rb index 1c97d5ec5b4..93d77d5b5ce 100644 --- a/spec/features/snippets/user_creates_snippet_spec.rb +++ b/spec/features/snippets/user_creates_snippet_spec.rb @@ -41,7 +41,7 @@ describe 'User creates snippet', :js do expect(page).to have_content('My Snippet') link = find('a.no-attachment-icon img[alt="banana_sample"]')['src'] - expect(link).to match(%r{/uploads/-/system/temp/\h{32}/banana_sample\.gif\z}) + expect(link).to match(%r{/uploads/-/system/user/#{user.id}/\h{32}/banana_sample\.gif\z}) reqs = inspect_requests { visit(link) } expect(reqs.first.status_code).to eq(200) diff --git a/spec/routing/uploads_routing_spec.rb b/spec/routing/uploads_routing_spec.rb index 6a041ffdd6c..42e84774088 100644 --- a/spec/routing/uploads_routing_spec.rb +++ b/spec/routing/uploads_routing_spec.rb @@ -12,10 +12,19 @@ describe 'Uploads', 'routing' do ) end + it 'allows creating uploads for users' do + expect(post('/uploads/user?id=1')).to route_to( + controller: 'uploads', + action: 'create', + model: 'user', + id: '1' + ) + end + it 'does not allow creating uploads for other models' do - UploadsController::MODEL_CLASSES.keys.compact.each do |model| - next if model == 'personal_snippet' + unroutable_models = UploadsController::MODEL_CLASSES.keys.compact - %w(personal_snippet user) + unroutable_models.each do |model| expect(post("/uploads/#{model}?id=1")).not_to be_routable end end diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index e474a714b10..a9e03f3d4e5 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -3,79 +3,140 @@ require 'spec_helper' describe FileMover do include FileMoverHelpers + let(:user) { create(:user) } let(:filename) { 'banana_sample.gif' } - let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) } + let(:secret) { 'secret55' } + let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", secret, filename) } let(:temp_description) do "test  "\ "same  " end - let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) } + let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, secret, filename) } let(:snippet) { create(:personal_snippet, description: temp_description) } - subject { described_class.new(temp_file_path, snippet).execute } + let(:tmp_uploader) do + PersonalFileUploader.new(user, secret: secret) + end + + let(:file) { fixture_file_upload('spec/fixtures/banana_sample.gif') } + subject { described_class.new(temp_file_path, from_model: user, to_model: snippet).execute } describe '#execute' do - before do - expect(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path))) - expect(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path)) - allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true) - allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10) + let(:tmp_upload) { tmp_uploader.upload } - stub_file_mover(temp_file_path) + before do + tmp_uploader.store!(file) end - context 'when move and field update successful' do - it 'updates the description correctly' do - subject + context 'local storage' do + before do + allow(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path))) + allow(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path)) + allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true) + allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10) - expect(snippet.reload.description) - .to eq( - "test  "\ - "same  " - ) + stub_file_mover(temp_file_path) end - it 'creates a new update record' do - expect { subject }.to change { Upload.count }.by(1) + context 'when move and field update successful' do + it 'updates the description correctly' do + subject + + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + end + + it 'updates existing upload record' do + expect { subject } + .to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') } + .from([user.id, 'User']).to([snippet.id, 'Snippet']) + end + + it 'schedules a background migration' do + expect_any_instance_of(PersonalFileUploader).to receive(:schedule_background_upload).once + + subject + end end - it 'schedules a background migration' do - expect_any_instance_of(PersonalFileUploader).to receive(:schedule_background_upload).once + context 'when update_markdown fails' do + before do + expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path)) + end - subject + subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute } + + it 'does not update the description' do + subject + + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + end + + it 'does not change the upload record' do + expect { subject } + .not_to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') } + end end end - context 'when update_markdown fails' do + context 'when tmp uploader is not local storage' do before do - expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path)) + allow(PersonalFileUploader).to receive(:object_store_enabled?) { true } + tmp_uploader.object_store = ObjectStorage::Store::REMOTE + allow_any_instance_of(PersonalFileUploader).to receive(:file_storage?) { false } end - subject { described_class.new(file_path, snippet, :non_existing_field).execute } + after do + FileUtils.rm_f(File.join('personal_snippet', snippet.id.to_s, secret, filename)) + end - it 'does not update the description' do - subject + context 'when move and field update successful' do + it 'updates the description correctly' do + subject - expect(snippet.reload.description) - .to eq( - "test  "\ - "same  " - ) + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + end + + it 'creates new target upload record an delete the old upload' do + expect { subject } + .to change { Upload.last.attributes.values_at('model_id', 'model_type') } + .from([user.id, 'User']).to([snippet.id, 'Snippet']) + + expect(Upload.count).to eq(1) + end end - it 'does not create a new update record' do - expect { subject }.not_to change { Upload.count } + context 'when update_markdown fails' do + subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute } + + it 'does not update the description' do + subject + + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + end + + it 'does not change the upload record' do + expect { subject } + .to change { Upload.last.attributes.values_at('model_id', 'model_type') }.from([user.id, 'User']) + end end end end context 'security' do context 'when relative path is involved' do - let(:temp_file_path) { File.join('uploads/-/system/temp', '..', 'another_subdir_of_temp') } + let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", '..', 'another_subdir_of_temp') } it 'does not trigger move if path is outside designated directory' do - stub_file_mover('uploads/-/system/another_subdir_of_temp') + stub_file_mover("uploads/-/system/user/#{user.id}/another_subdir_of_temp") expect(FileUtils).not_to receive(:move) subject |