From f2675d4fa9b21e2404e4c03e5deec2e595d29503 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Tue, 27 Jun 2017 10:53:06 +0000
Subject: [PATCH] Merge branch '33359-pers-snippet-files-location' into
'security-9-3'
Use uploads/system directory for personal snippets
See merge request !2123
---
app/uploaders/personal_file_uploader.rb | 4 +
.../33359-pers-snippet-files-location.yml | 4 +
config/routes/uploads.rb | 4 +-
...0612071012_move_personal_snippets_files.rb | 91 +++++++++
spec/controllers/snippets_controller_spec.rb | 8 +-
spec/controllers/uploads_controller_spec.rb | 4 +-
.../snippets/user_creates_snippet_spec.rb | 6 +-
.../snippets/user_edits_snippet_spec.rb | 2 +-
.../move_personal_snippets_files_spec.rb | 180 ++++++++++++++++++
spec/uploaders/file_mover_spec.rb | 14 +-
spec/uploaders/personal_file_uploader_spec.rb | 4 +-
11 files changed, 300 insertions(+), 21 deletions(-)
create mode 100644 changelogs/unreleased/33359-pers-snippet-files-location.yml
create mode 100644 db/post_migrate/20170612071012_move_personal_snippets_files.rb
create mode 100644 spec/migrations/move_personal_snippets_files_spec.rb
diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb
index 7f857765fbfd..ef70871624b7 100644
--- a/app/uploaders/personal_file_uploader.rb
+++ b/app/uploaders/personal_file_uploader.rb
@@ -3,6 +3,10 @@ def self.dynamic_path_segment(model)
File.join(CarrierWave.root, model_path(model))
end
+ def self.base_dir
+ File.join(root_dir, 'system')
+ end
+
private
def secure_url
diff --git a/changelogs/unreleased/33359-pers-snippet-files-location.yml b/changelogs/unreleased/33359-pers-snippet-files-location.yml
new file mode 100644
index 000000000000..22fa301cb5e4
--- /dev/null
+++ b/changelogs/unreleased/33359-pers-snippet-files-location.yml
@@ -0,0 +1,4 @@
+---
+title: Use uploads/system directory for personal snippets
+merge_request:
+author:
diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb
index a49e244af1a2..54a95c006d03 100644
--- a/config/routes/uploads.rb
+++ b/config/routes/uploads.rb
@@ -5,12 +5,12 @@
constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /[^\/]+/ }
# show uploads for models, snippets (notes) available for now
- get ':model/:id/:secret/:filename',
+ get 'system/:model/:id/:secret/:filename',
to: 'uploads#show',
constraints: { model: /personal_snippet/, id: /\d+/, filename: /[^\/]+/ }
# show temporary uploads
- get 'temp/:secret/:filename',
+ get 'system/temp/:secret/:filename',
to: 'uploads#show',
constraints: { filename: /[^\/]+/ }
diff --git a/db/post_migrate/20170612071012_move_personal_snippets_files.rb b/db/post_migrate/20170612071012_move_personal_snippets_files.rb
new file mode 100644
index 000000000000..33043364bdeb
--- /dev/null
+++ b/db/post_migrate/20170612071012_move_personal_snippets_files.rb
@@ -0,0 +1,91 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+class MovePersonalSnippetsFiles < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+ disable_ddl_transaction!
+
+ DOWNTIME = false
+
+ def up
+ return unless file_storage?
+
+ @source_relative_location = File.join('/uploads', 'personal_snippet')
+ @destination_relative_location = File.join('/uploads', 'system', 'personal_snippet')
+
+ move_personal_snippet_files
+ end
+
+ def down
+ return unless file_storage?
+
+ @source_relative_location = File.join('/uploads', 'system', 'personal_snippet')
+ @destination_relative_location = File.join('/uploads', 'personal_snippet')
+
+ move_personal_snippet_files
+ end
+
+ def move_personal_snippet_files
+ query = "SELECT uploads.path, uploads.model_id, snippets.description FROM uploads "\
+ "INNER JOIN snippets ON snippets.id = uploads.model_id WHERE uploader = 'PersonalFileUploader'"
+ select_all(query).each do |upload|
+ secret = upload['path'].split('/')[0]
+ file_name = upload['path'].split('/')[1]
+
+ next unless move_file(upload['model_id'], secret, file_name)
+ update_markdown(upload['model_id'], secret, file_name, upload['description'])
+ end
+ end
+
+ def move_file(snippet_id, secret, file_name)
+ source_dir = File.join(base_directory, @source_relative_location, snippet_id.to_s, secret)
+ destination_dir = File.join(base_directory, @destination_relative_location, snippet_id.to_s, secret)
+
+ source_file_path = File.join(source_dir, file_name)
+ destination_file_path = File.join(destination_dir, file_name)
+
+ unless File.exist?(source_file_path)
+ say "Source file `#{source_file_path}` doesn't exist. Skipping."
+ return
+ end
+
+ say "Moving file #{source_file_path} -> #{destination_file_path}"
+
+ FileUtils.mkdir_p(destination_dir)
+ FileUtils.move(source_file_path, destination_file_path)
+
+ true
+ end
+
+ def update_markdown(snippet_id, secret, file_name, description)
+ source_markdown_path = File.join(@source_relative_location, snippet_id.to_s, secret, file_name)
+ destination_markdown_path = File.join(@destination_relative_location, snippet_id.to_s, secret, file_name)
+
+ source_markdown = "](#{source_markdown_path})"
+ destination_markdown = "](#{destination_markdown_path})"
+
+ if description.present?
+ description = description.gsub(source_markdown, destination_markdown)
+ quoted_description = quote_string(description)
+
+ execute("UPDATE snippets SET description = '#{quoted_description}', description_html = NULL "\
+ "WHERE id = #{snippet_id}")
+ end
+
+ query = "SELECT id, note FROM notes WHERE noteable_id = #{snippet_id} "\
+ "AND noteable_type = 'Snippet' AND note IS NOT NULL"
+ select_all(query).each do |note|
+ text = note['note'].gsub(source_markdown, destination_markdown)
+ quoted_text = quote_string(text)
+
+ execute("UPDATE notes SET note = '#{quoted_text}', note_html = NULL WHERE id = #{note['id']}")
+ end
+ end
+
+ def base_directory
+ File.join(Rails.root, 'public')
+ end
+
+ def file_storage?
+ CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File
+ end
+end
diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb
index 15416a89017f..475ceda11feb 100644
--- a/spec/controllers/snippets_controller_spec.rb
+++ b/spec/controllers/snippets_controller_spec.rb
@@ -186,8 +186,8 @@ def create_snippet(snippet_params = {}, additional_params = {})
end
context 'when the snippet description contains a file' do
- let(:picture_file) { '/temp/secret56/picture.jpg' }
- let(:text_file) { '/temp/secret78/text.txt' }
+ let(:picture_file) { '/system/temp/secret56/picture.jpg' }
+ let(:text_file) { '/system/temp/secret78/text.txt' }
let(:description) do
"Description with picture:  and "\
"text: [text.txt](/uploads#{text_file})"
@@ -208,8 +208,8 @@ def create_snippet(snippet_params = {}, additional_params = {})
snippet = subject
expected_description = "Description with picture: "\
- " and "\
- "text: [text.txt](/uploads/personal_snippet/#{snippet.id}/secret78/text.txt)"
+ " and "\
+ "text: [text.txt](/uploads/system/personal_snippet/#{snippet.id}/secret78/text.txt)"
expect(snippet.description).to eq(expected_description)
end
diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb
index 01a0659479b2..96f719e2b82e 100644
--- a/spec/controllers/uploads_controller_spec.rb
+++ b/spec/controllers/uploads_controller_spec.rb
@@ -102,7 +102,7 @@
subject
expect(response.body).to match '\"alt\":\"rails_sample\"'
- expect(response.body).to match "\"url\":\"/uploads/temp"
+ expect(response.body).to match "\"url\":\"/uploads/system/temp"
end
it 'does not create an Upload record' do
@@ -119,7 +119,7 @@
subject
expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
- expect(response.body).to match "\"url\":\"/uploads/temp"
+ expect(response.body).to match "\"url\":\"/uploads/system/temp"
end
it 'does not create an Upload record' do
diff --git a/spec/features/snippets/user_creates_snippet_spec.rb b/spec/features/snippets/user_creates_snippet_spec.rb
index 57dec14b480d..698d3b5d3e32 100644
--- a/spec/features/snippets/user_creates_snippet_spec.rb
+++ b/spec/features/snippets/user_creates_snippet_spec.rb
@@ -41,7 +41,7 @@ def fill_form
expect(page).to have_content('My Snippet')
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
- expect(link).to match(%r{/uploads/temp/\h{32}/banana_sample\.gif\z})
+ expect(link).to match(%r{/uploads/system/temp/\h{32}/banana_sample\.gif\z})
visit(link)
expect(page.status_code).to eq(200)
@@ -59,7 +59,7 @@ def fill_form
wait_for_requests
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
- expect(link).to match(%r{/uploads/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
+ expect(link).to match(%r{/uploads/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
visit(link)
expect(page.status_code).to eq(200)
@@ -84,7 +84,7 @@ def fill_form
end
expect(page).to have_content('Hello World!')
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
- expect(link).to match(%r{/uploads/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
+ expect(link).to match(%r{/uploads/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
visit(link)
expect(page.status_code).to eq(200)
diff --git a/spec/features/snippets/user_edits_snippet_spec.rb b/spec/features/snippets/user_edits_snippet_spec.rb
index cff644238739..c9f9741b4bbe 100644
--- a/spec/features/snippets/user_edits_snippet_spec.rb
+++ b/spec/features/snippets/user_edits_snippet_spec.rb
@@ -33,7 +33,7 @@
wait_for_requests
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
- expect(link).to match(%r{/uploads/personal_snippet/#{snippet.id}/\h{32}/banana_sample\.gif\z})
+ expect(link).to match(%r{/uploads/system/personal_snippet/#{snippet.id}/\h{32}/banana_sample\.gif\z})
end
it 'updates the snippet to make it internal' do
diff --git a/spec/migrations/move_personal_snippets_files_spec.rb b/spec/migrations/move_personal_snippets_files_spec.rb
new file mode 100644
index 000000000000..8505c7bf3e38
--- /dev/null
+++ b/spec/migrations/move_personal_snippets_files_spec.rb
@@ -0,0 +1,180 @@
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20170612071012_move_personal_snippets_files.rb')
+
+describe MovePersonalSnippetsFiles do
+ let(:migration) { described_class.new }
+ let(:test_dir) { File.join(Rails.root, "tmp", "tests", "move_snippet_files_test") }
+ let(:uploads_dir) { File.join(test_dir, 'uploads') }
+ let(:new_uploads_dir) { File.join(uploads_dir, 'system') }
+
+ before do
+ allow(CarrierWave).to receive(:root).and_return(test_dir)
+ allow(migration).to receive(:base_directory).and_return(test_dir)
+ FileUtils.remove_dir(test_dir) if File.directory?(test_dir)
+ allow(migration).to receive(:say)
+ end
+
+ describe "#up" do
+ let(:snippet) do
+ snippet = create(:personal_snippet)
+ create_upload('picture.jpg', snippet)
+ snippet.update(description: markdown_linking_file('picture.jpg', snippet))
+ snippet
+ end
+
+ let(:snippet_with_missing_file) do
+ snippet = create(:snippet)
+ create_upload('picture.jpg', snippet, create_file: false)
+ snippet.update(description: markdown_linking_file('picture.jpg', snippet))
+ snippet
+ end
+
+ it 'moves the files' do
+ source_path = File.join(uploads_dir, model_file_path('picture.jpg', snippet))
+ destination_path = File.join(new_uploads_dir, model_file_path('picture.jpg', snippet))
+
+ migration.up
+
+ expect(File.exist?(source_path)).to be_falsy
+ expect(File.exist?(destination_path)).to be_truthy
+ end
+
+ describe 'updating the markdown' do
+ it 'includes the new path when the file exists' do
+ secret = "secret#{snippet.id}"
+ file_location = "/uploads/system/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
+
+ migration.up
+
+ expect(snippet.reload.description).to include(file_location)
+ end
+
+ it 'does not update the markdown when the file is missing' do
+ secret = "secret#{snippet_with_missing_file.id}"
+ file_location = "/uploads/personal_snippet/#{snippet_with_missing_file.id}/#{secret}/picture.jpg"
+
+ migration.up
+
+ expect(snippet_with_missing_file.reload.description).to include(file_location)
+ end
+
+ it 'updates the note markdown' do
+ secret = "secret#{snippet.id}"
+ file_location = "/uploads/system/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
+ markdown = markdown_linking_file('picture.jpg', snippet)
+ note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown}")
+
+ migration.up
+
+ expect(note.reload.note).to include(file_location)
+ end
+ end
+ end
+
+ describe "#down" do
+ let(:snippet) do
+ snippet = create(:personal_snippet)
+ create_upload('picture.jpg', snippet, in_new_path: true)
+ snippet.update(description: markdown_linking_file('picture.jpg', snippet, in_new_path: true))
+ snippet
+ end
+
+ let(:snippet_with_missing_file) do
+ snippet = create(:personal_snippet)
+ create_upload('picture.jpg', snippet, create_file: false, in_new_path: true)
+ snippet.update(description: markdown_linking_file('picture.jpg', snippet, in_new_path: true))
+ snippet
+ end
+
+ it 'moves the files' do
+ source_path = File.join(new_uploads_dir, model_file_path('picture.jpg', snippet))
+ destination_path = File.join(uploads_dir, model_file_path('picture.jpg', snippet))
+
+ migration.down
+
+ expect(File.exist?(source_path)).to be_falsey
+ expect(File.exist?(destination_path)).to be_truthy
+ end
+
+ describe 'updating the markdown' do
+ it 'includes the new path when the file exists' do
+ secret = "secret#{snippet.id}"
+ file_location = "/uploads/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
+
+ migration.down
+
+ expect(snippet.reload.description).to include(file_location)
+ end
+
+ it 'keeps the markdown as is when the file is missing' do
+ secret = "secret#{snippet_with_missing_file.id}"
+ file_location = "/uploads/system/personal_snippet/#{snippet_with_missing_file.id}/#{secret}/picture.jpg"
+
+ migration.down
+
+ expect(snippet_with_missing_file.reload.description).to include(file_location)
+ end
+
+ it 'updates the note markdown' do
+ markdown = markdown_linking_file('picture.jpg', snippet, in_new_path: true)
+ secret = "secret#{snippet.id}"
+ file_location = "/uploads/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
+ note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown}")
+
+ migration.down
+
+ expect(note.reload.note).to include(file_location)
+ end
+ end
+ end
+
+ describe '#update_markdown' do
+ it 'escapes sql in the snippet description' do
+ migration.instance_variable_set('@source_relative_location', '/uploads/personal_snippet')
+ migration.instance_variable_set('@destination_relative_location', '/uploads/system/personal_snippet')
+
+ secret = '123456789'
+ filename = 'hello.jpg'
+ snippet = create(:personal_snippet)
+
+ path_before = "/uploads/personal_snippet/#{snippet.id}/#{secret}/#{filename}"
+ path_after = "/uploads/system/personal_snippet/#{snippet.id}/#{secret}/#{filename}"
+ description_before = "Hello world; '; select * from users;"
+ description_after = "Hello world; '; select * from users;"
+
+ migration.update_markdown(snippet.id, secret, filename, description_before)
+
+ expect(snippet.reload.description).to eq(description_after)
+ end
+ end
+
+ def create_upload(filename, snippet, create_file: true, in_new_path: false)
+ secret = "secret#{snippet.id}"
+ absolute_path = if in_new_path
+ File.join(new_uploads_dir, model_file_path(filename, snippet))
+ else
+ File.join(uploads_dir, model_file_path(filename, snippet))
+ end
+
+ if create_file
+ FileUtils.mkdir_p(File.dirname(absolute_path))
+ FileUtils.touch(absolute_path)
+ end
+
+ create(:upload, model: snippet, path: "#{secret}/#{filename}", uploader: PersonalFileUploader)
+ end
+
+ def markdown_linking_file(filename, snippet, in_new_path: false)
+ markdown = "![#{filename.split('.')[0]}]"
+ markdown += '(/uploads'
+ markdown += '/system' if in_new_path
+ markdown += "/#{model_file_path(filename, snippet)})"
+ markdown
+ end
+
+ def model_file_path(filename, snippet)
+ secret = "secret#{snippet.id}"
+
+ File.join('personal_snippet', snippet.id.to_s, secret, filename)
+ end
+end
diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb
index 896cb410ed56..d7c1b390f9ae 100644
--- a/spec/uploaders/file_mover_spec.rb
+++ b/spec/uploaders/file_mover_spec.rb
@@ -4,11 +4,11 @@
let(:filename) { 'banana_sample.gif' }
let(:file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', filename)) }
let(:temp_description) do
- 'test  same ![banana_sample]'\
- '(/uploads/temp/secret55/banana_sample.gif)'
+ 'test  same ![banana_sample]'\
+ '(/uploads/system/temp/secret55/banana_sample.gif)'
end
let(:temp_file_path) { File.join('secret55', filename).to_s }
- let(:file_path) { File.join('uploads', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s }
+ let(:file_path) { File.join('uploads', 'system', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s }
let(:snippet) { create(:personal_snippet, description: temp_description) }
@@ -28,8 +28,8 @@
expect(snippet.reload.description)
.to eq(
- "test "\
- " same "
+ "test "\
+ " same "
)
end
@@ -50,8 +50,8 @@
expect(snippet.reload.description)
.to eq(
- "test "\
- " same "
+ "test "\
+ " same "
)
end
diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb
index fb92f2ae3abe..eb55e8ebd240 100644
--- a/spec/uploaders/personal_file_uploader_spec.rb
+++ b/spec/uploaders/personal_file_uploader_spec.rb
@@ -10,7 +10,7 @@
dynamic_segment = "personal_snippet/#{snippet.id}"
- expect(described_class.absolute_path(upload)).to end_with("#{dynamic_segment}/secret/foo.jpg")
+ expect(described_class.absolute_path(upload)).to end_with("/system/#{dynamic_segment}/secret/foo.jpg")
end
end
@@ -19,7 +19,7 @@
uploader = described_class.new(snippet, 'secret')
allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
- expected_url = "/uploads/personal_snippet/#{snippet.id}/secret/file_name"
+ expected_url = "/uploads/system/personal_snippet/#{snippet.id}/secret/file_name"
expect(uploader.to_h).to eq(
alt: 'file_name',
--
GitLab