Commit e34d6219 authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

Merge branch 'security-persist-tmp-snippet-uploads-11-11' into '11-11-stable'

Persist tmp snippet uploads at users

See merge request gitlab/gitlabhq!3165
parents c563d481 63d52d83
...@@ -137,7 +137,7 @@ class SnippetsController < ApplicationController ...@@ -137,7 +137,7 @@ class SnippetsController < ApplicationController
def move_temporary_files def move_temporary_files
params[:files].each do |file| params[:files].each do |file|
FileMover.new(file, @snippet).execute FileMover.new(file, from_model: current_user, to_model: @snippet).execute
end end
end end
end end
...@@ -41,7 +41,11 @@ class UploadsController < ApplicationController ...@@ -41,7 +41,11 @@ class UploadsController < ApplicationController
when Note when Note
can?(current_user, :read_project, model.project) can?(current_user, :read_project, model.project)
when User 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 when Appearance
true true
else else
...@@ -56,9 +60,13 @@ class UploadsController < ApplicationController ...@@ -56,9 +60,13 @@ class UploadsController < ApplicationController
def authorize_create_access! def authorize_create_access!
return unless model return unless model
# for now we support only personal snippets comments. Only personal_snippet authorized =
# is allowed as a model to #create through routing. case model
authorized = can?(current_user, :create_note, model) when User
can?(current_user, :update_user, model)
else
can?(current_user, :create_note, model)
end
render_unauthorized unless authorized render_unauthorized unless authorized
end end
...@@ -75,6 +83,10 @@ class UploadsController < ApplicationController ...@@ -75,6 +83,10 @@ class UploadsController < ApplicationController
User === model || Appearance === model User === model || Appearance === model
end end
def secret?
params[:secret].present?
end
def upload_model_class def upload_model_class
MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError) MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError)
end end
......
# frozen_string_literal: true # frozen_string_literal: true
module SnippetsHelper 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) def reliable_snippet_path(snippet, opts = nil)
if snippet.project_id? if snippet.project_id?
project_snippet_path(snippet.project, snippet, opts) project_snippet_path(snippet.project, snippet, opts)
......
# frozen_string_literal: true # frozen_string_literal: true
class FileMover class FileMover
attr_reader :secret, :file_name, :model, :update_field attr_reader :secret, :file_name, :from_model, :to_model, :update_field
def initialize(file_path, model, update_field = :description) def initialize(file_path, update_field = :description, from_model:, to_model:)
@secret = File.split(File.dirname(file_path)).last @secret = File.split(File.dirname(file_path)).last
@file_name = File.basename(file_path) @file_name = File.basename(file_path)
@model = model @from_model = from_model
@to_model = to_model
@update_field = update_field @update_field = update_field
end end
...@@ -16,7 +17,7 @@ class FileMover ...@@ -16,7 +17,7 @@ class FileMover
move move
if update_markdown if update_markdown
uploader.record_upload update_upload_model
uploader.schedule_background_upload uploader.schedule_background_upload
end end
end end
...@@ -35,14 +36,20 @@ class FileMover ...@@ -35,14 +36,20 @@ class FileMover
end end
def update_markdown def update_markdown
updated_text = model.read_attribute(update_field) updated_text = to_model.read_attribute(update_field)
.gsub(temp_file_uploader.markdown_link, uploader.markdown_link) .gsub(temp_file_uploader.markdown_link, uploader.markdown_link)
model.update_attribute(update_field, updated_text) to_model.update_attribute(update_field, updated_text)
rescue rescue
revert revert
false false
end end
def update_upload_model
return unless upload = temp_file_uploader.upload
upload.update!(model_id: to_model.id, model_type: to_model.type)
end
def temp_file_path def temp_file_path
return @temp_file_path if @temp_file_path return @temp_file_path if @temp_file_path
...@@ -60,15 +67,15 @@ class FileMover ...@@ -60,15 +67,15 @@ class FileMover
end end
def uploader def uploader
@uploader ||= PersonalFileUploader.new(model, secret: secret) @uploader ||= PersonalFileUploader.new(to_model, secret: secret)
end end
def temp_file_uploader def temp_file_uploader
@temp_file_uploader ||= PersonalFileUploader.new(nil, secret: secret) @temp_file_uploader ||= PersonalFileUploader.new(from_model, secret: secret)
end end
def revert 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) FileUtils.move(file_path, temp_file_path)
end end
......
- header_title _("Snippets"), snippets_path - header_title _("Snippets"), snippets_path
- snippets_upload_path = snippets_upload_path(@snippet, current_user)
- content_for :page_specific_javascripts do - content_for :page_specific_javascripts do
- if @snippet && current_user - if snippets_upload_path
-# haml-lint:disable InlineJavaScript -# haml-lint:disable InlineJavaScript
:javascript :javascript
window.uploads_path = "#{upload_path('personal_snippet', id: @snippet.id)}"; window.uploads_path = "#{snippets_upload_path}";
= render template: "layouts/application" = render template: "layouts/application"
---
title: Persist tmp snippet uploads at users
merge_request:
author:
type: security
...@@ -7,7 +7,7 @@ scope path: :uploads do ...@@ -7,7 +7,7 @@ scope path: :uploads do
# show uploads for models, snippets (notes) available for now # show uploads for models, snippets (notes) available for now
get '-/system/:model/:id/:secret/:filename', get '-/system/:model/:id/:secret/:filename',
to: 'uploads#show', to: 'uploads#show',
constraints: { model: /personal_snippet/, id: /\d+/, filename: %r{[^/]+} } constraints: { model: /personal_snippet|user/, id: /\d+/, filename: %r{[^/]+} }
# show temporary uploads # show temporary uploads
get '-/system/temp/:secret/:filename', get '-/system/temp/:secret/:filename',
...@@ -28,7 +28,7 @@ scope path: :uploads do ...@@ -28,7 +28,7 @@ scope path: :uploads do
# create uploads for models, snippets (notes) available for now # create uploads for models, snippets (notes) available for now
post ':model', post ':model',
to: 'uploads#create', to: 'uploads#create',
constraints: { model: /personal_snippet/, id: /\d+/ }, constraints: { model: /personal_snippet|user/, id: /\d+/ },
as: 'upload' as: 'upload'
end end
......
...@@ -209,8 +209,8 @@ describe SnippetsController do ...@@ -209,8 +209,8 @@ describe SnippetsController do
context 'when the snippet description contains a file' do context 'when the snippet description contains a file' do
include FileMoverHelpers include FileMoverHelpers
let(:picture_file) { '/-/system/temp/secret56/picture.jpg' } let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" }
let(:text_file) { '/-/system/temp/secret78/text.txt' } let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" }
let(:description) do let(:description) do
"Description with picture: ![picture](/uploads#{picture_file}) and "\ "Description with picture: ![picture](/uploads#{picture_file}) and "\
"text: [text.txt](/uploads#{text_file})" "text: [text.txt](/uploads#{text_file})"
......
...@@ -24,121 +24,160 @@ describe UploadsController do ...@@ -24,121 +24,160 @@ describe UploadsController do
let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) } let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) }
describe 'POST create' do 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(:jpg) { fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg') }
let(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } 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 context 'snippet uploads' do
it "returns 401 when the user is not logged in" do let(:model) { 'personal_snippet' }
post :create, params: { model: model, id: snippet.id }, format: :json let(:snippet) { create(:personal_snippet, :public) }
expect(response).to have_gitlab_http_status(401) context 'when a user does not have permissions to upload a file' do
end 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 expect(response).to have_gitlab_http_status(401)
private_snippet = create(:personal_snippet, :private) end
sign_in(user) it "returns 404 when user can't comment on a snippet" do
post :create, params: { model: model, id: private_snippet.id }, format: :json private_snippet = create(:personal_snippet, :private)
expect(response).to have_gitlab_http_status(404) sign_in(user)
end post :create, params: { model: model, id: private_snippet.id }, format: :json
end
context 'when a user is logged in' do expect(response).to have_gitlab_http_status(404)
before do end
sign_in(user)
end end
it "returns an error without file" do context 'when a user is logged in' do
post :create, params: { model: model, id: snippet.id }, format: :json before do
sign_in(user)
end
expect(response).to have_gitlab_http_status(422) it "returns an error without file" do
end post :create, params: { model: model, id: snippet.id }, format: :json
it "returns an error with invalid model" do expect(response).to have_gitlab_http_status(422)
expect { post :create, params: { model: 'invalid', id: snippet.id }, format: :json } end
.to raise_error(ActionController::UrlGenerationError)
end
it "returns 404 status when object not found" do it "returns an error with invalid model" do
post :create, params: { model: model, id: 9999 }, format: :json 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) it "returns 404 status when object not found" do
end post :create, params: { model: model, id: 9999 }, format: :json
context 'with valid image' do expect(response).to have_gitlab_http_status(404)
before do
post :create, params: { model: 'personal_snippet', id: snippet.id, file: jpg }, format: :json
end end
it 'returns a content with original filename, new link, and correct type.' do context 'with valid image' do
expect(response.body).to match '\"alt\":\"rails_sample\"' before do
expect(response.body).to match "\"url\":\"/uploads" 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 end
it 'creates a corresponding Upload record' do context 'with valid non-image file' do
upload = Upload.last before do
post :create, params: { model: 'personal_snippet', id: snippet.id, file: txt }, format: :json
end
aggregate_failures do it 'returns a content with original filename, new link, and correct type.' do
expect(upload).to exist expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
expect(upload.model).to eq snippet 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
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 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 end
it 'returns a content with original filename, new link, and correct type.' do it 'returns a content with original filename, new link, and correct type.' do
expect(response.body).to match '\"alt\":\"doc_sample.txt\"' subject
expect(response.body).to match "\"url\":\"/uploads"
expect(response.body).to match '\"alt\":\"rails_sample\"'
expect(response.body).to match "\"url\":\"/uploads/-/system/user/#{user.id}/"
end end
it 'creates a corresponding Upload record' do it 'creates a corresponding Upload record' do
expect { subject }.to change { Upload.count }
upload = Upload.last upload = Upload.last
aggregate_failures do aggregate_failures do
expect(upload).to exist expect(upload).to exist
expect(upload.model).to eq snippet expect(upload.model).to eq user
end end
end end
end
context 'temporal with valid image' do context 'with valid non-image file' do
subject do subject do
post :create, params: { model: 'personal_snippet', file: jpg }, format: :json post :create, params: { model: model, id: user.id, file: txt }, format: :json
end end
it 'returns a content with original filename, new link, and correct type.' do it 'returns a content with original filename, new link, and correct type.' do
subject subject
expect(response.body).to match '\"alt\":\"rails_sample\"' expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
expect(response.body).to match "\"url\":\"/uploads/-/system/temp" expect(response.body).to match "\"url\":\"/uploads/-/system/user/#{user.id}/"
end end
it 'does not create an Upload record' do it 'creates a corresponding Upload record' do
expect { subject }.not_to change { Upload.count } expect { subject }.to change { Upload.count }
end
end
context 'temporal with valid non-image file' do upload = Upload.last
subject do
post :create, params: { model: 'personal_snippet', file: txt }, format: :json aggregate_failures do
expect(upload).to exist
expect(upload.model).to eq user
end
end
end end
it 'returns a content with original filename, new link, and correct type.' do it 'returns 404 when given user is not the logged in one' do
subject another_user = create(:user)
expect(response.body).to match '\"alt\":\"doc_sample.txt\"' post :create, params: { model: model, id: another_user.id, file: txt }, format: :json
expect(response.body).to match "\"url\":\"/uploads/-/system/temp"
end
it 'does not create an Upload record' do expect(response).to have_gitlab_http_status(404)
expect { subject }.not_to change { Upload.count }
end end
end end
end end
......
...@@ -41,7 +41,7 @@ describe 'User creates snippet', :js do ...@@ -41,7 +41,7 @@ describe 'User creates snippet', :js do
expect(page).to have_content('My Snippet') expect(page).to have_content('My Snippet')
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src'] 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) } reqs = inspect_requests { visit(link) }
expect(reqs.first.status_code).to eq(200) expect(reqs.first.status_code).to eq(200)
......
...@@ -12,10 +12,19 @@ describe 'Uploads', 'routing' do ...@@ -12,10 +12,19 @@ describe 'Uploads', 'routing' do
) )
end 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 it 'does not allow creating uploads for other models' do
UploadsController::MODEL_CLASSES.keys.compact.each do |model| unroutable_models = UploadsController::MODEL_CLASSES.keys.compact - %w(personal_snippet user)
next if model == 'personal_snippet'
unroutable_models.each do |model|
expect(post("/uploads/#{model}?id=1")).not_to be_routable expect(post("/uploads/#{model}?id=1")).not_to be_routable
end end
end end
......
...@@ -3,20 +3,29 @@ require 'spec_helper' ...@@ -3,20 +3,29 @@ require 'spec_helper'
describe FileMover do describe FileMover do
include FileMoverHelpers include FileMoverHelpers
let(:user) { create(:user) }
let(:filename) { 'banana_sample.gif' } 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 let(:temp_description) do
"test ![banana_sample](/#{temp_file_path}) "\ "test ![banana_sample](/#{temp_file_path}) "\
"same ![banana_sample](/#{temp_file_path}) " "same ![banana_sample](/#{temp_file_path}) "
end 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) } 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 describe '#execute' do
before do before do
tmp_uploader.store!(file)
expect(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path))) 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)) 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(:exists?).and_return(true)
...@@ -25,6 +34,8 @@ describe FileMover do ...@@ -25,6 +34,8 @@ describe FileMover do
stub_file_mover(temp_file_path) stub_file_mover(temp_file_path)
end end
let(:tmp_upload) { tmp_uploader.upload }
context 'when move and field update successful' do context 'when move and field update successful' do
it 'updates the description correctly' do it 'updates the description correctly' do
subject subject
...@@ -36,8 +47,10 @@ describe FileMover do ...@@ -36,8 +47,10 @@ describe FileMover do
) )
end end
it 'creates a new update record' do it 'updates existing upload record' do
expect { subject }.to change { Upload.count }.by(1) expect { subject }
.to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
.from([user.id, 'User']).to([snippet.id, 'PersonalSnippet'])
end end
it 'schedules a background migration' do it 'schedules a background migration' do
...@@ -52,30 +65,31 @@ describe FileMover do ...@@ -52,30 +65,31 @@ describe FileMover do
expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path)) expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path))
end end
subject { described_class.new(file_path, snippet, :non_existing_field).execute } subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute }
it 'does not update the description' do it 'does not update the description' do
subject subject
expect(snippet.reload.description) expect(snippet.reload.description)
.to eq( .to eq(
"test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "\ "test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) " "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "
) )
end end
it 'does not create a new update record' do it 'does not change the upload record' do
expect { subject }.not_to change { Upload.count } expect { subject }
.not_to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
end end
end end
end end
context 'security' do context 'security' do
context 'when relative path is involved' 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 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) expect(FileUtils).not_to receive(:move)
subject subject
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment