diff options
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/snippets_controller_spec.rb | 10 | ||||
-rw-r--r-- | spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb | 30 | ||||
-rw-r--r-- | spec/uploaders/file_mover_spec.rb | 24 | ||||
-rw-r--r-- | spec/uploaders/file_uploader_spec.rb | 44 | ||||
-rw-r--r-- | spec/uploaders/personal_file_uploader_spec.rb | 7 |
5 files changed, 86 insertions, 29 deletions
diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index daa560649f0..66fb1ef8129 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -242,8 +242,10 @@ describe SnippetsController do context 'when the snippet description contains a file' do include FileMoverHelpers - let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" } - let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" } + let(:picture_secret) { SecureRandom.hex } + let(:text_secret) { SecureRandom.hex } + let(:picture_file) { "/-/system/user/#{user.id}/#{picture_secret}/picture.jpg" } + let(:text_file) { "/-/system/user/#{user.id}/#{text_secret}/text.txt" } let(:description) do "Description with picture: ![picture](/uploads#{picture_file}) and "\ "text: [text.txt](/uploads#{text_file})" @@ -266,8 +268,8 @@ describe SnippetsController do snippet = subject expected_description = "Description with picture: "\ - "![picture](/uploads/-/system/personal_snippet/#{snippet.id}/secret56/picture.jpg) and "\ - "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/secret78/text.txt)" + "![picture](/uploads/-/system/personal_snippet/#{snippet.id}/#{picture_secret}/picture.jpg) and "\ + "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/#{text_secret}/text.txt)" expect(snippet.description).to eq(expected_description) end diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 73087befad2..662c64647d6 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -69,13 +69,39 @@ RSpec.shared_examples 'handle uploads' do end describe "GET #show" do + let(:filename) { "rails_sample.jpg" } + + let(:upload_service) do + UploadService.new(model, jpg, uploader_class).execute + end + let(:show_upload) do - get :show, params: params.merge(secret: secret, filename: "rails_sample.jpg") + get :show, params: params.merge(secret: secret, filename: filename) end before do allow(FileUploader).to receive(:generate_secret).and_return(secret) - UploadService.new(model, jpg, uploader_class).execute + upload_service + end + + context 'when the secret is invalid' do + let(:secret) { "../../../../../../../../" } + let(:filename) { "Gemfile.lock" } + let(:upload_service) { nil } + + it 'responds with status 404' do + show_upload + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'is a working exploit without the validation' do + allow_any_instance_of(FileUploader).to receive(:secret) { secret } + + show_upload + + expect(response).to have_gitlab_http_status(:ok) + end end context 'when accessing a specific upload via different model' do diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index 9f053f03b0e..474515b537c 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -7,7 +7,7 @@ describe FileMover do let(:user) { create(:user) } let(:filename) { 'banana_sample.gif' } - let(:secret) { 'secret55' } + let(:secret) { SecureRandom.hex } let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", secret, filename) } let(:temp_description) do @@ -47,8 +47,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ") + .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ") end it 'updates existing upload record' do @@ -75,8 +75,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ") + .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ") end it 'does not change the upload record' do @@ -101,8 +101,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ") + .to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ") end it 'creates new target upload record an delete the old upload' do @@ -121,8 +121,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ") + .to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ") end it 'does not change the upload record' do @@ -138,12 +138,8 @@ describe FileMover do 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/user/#{user.id}/another_subdir_of_temp") expect(FileUtils).not_to receive(:move) - - subject - - expect(snippet.reload.description).to eq(temp_description) + expect { subject }.to raise_error(FileUploader::InvalidSecret) end end diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index beb7aa3cf2c..efdbea85d4a 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -6,7 +6,8 @@ describe FileUploader do let(:group) { create(:group, name: 'awesome') } let(:project) { create(:project, :legacy_storage, namespace: group, name: 'project') } let(:uploader) { described_class.new(project, :avatar) } - let(:upload) { double(model: project, path: 'secret/foo.jpg') } + let(:upload) { double(model: project, path: "#{secret}/foo.jpg") } + let(:secret) { "55dc16aa0edd05693fd98b5051e83321" } # this would be nicer as SecureRandom.hex, but the shared_examples breaks subject { uploader } @@ -14,7 +15,7 @@ describe FileUploader do include_examples 'builds correct paths', store_dir: %r{awesome/project/\h+}, upload_path: %r{\h+/<filename>}, - absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg} + absolute_path: %r{#{described_class.root}/awesome/project/55dc16aa0edd05693fd98b5051e83321/foo.jpg} end context 'legacy storage' do @@ -51,11 +52,11 @@ describe FileUploader do end describe 'initialize' do - let(:uploader) { described_class.new(double, secret: 'secret') } + let(:uploader) { described_class.new(double, secret: secret) } it 'accepts a secret parameter' do expect(described_class).not_to receive(:generate_secret) - expect(uploader.secret).to eq('secret') + expect(uploader.secret).to eq(secret) end end @@ -154,8 +155,39 @@ describe FileUploader do describe '#secret' do it 'generates a secret if none is provided' do - expect(described_class).to receive(:generate_secret).and_return('secret') - expect(uploader.secret).to eq('secret') + expect(described_class).to receive(:generate_secret).and_return(secret) + expect(uploader.secret).to eq(secret) + expect(uploader.secret.size).to eq(32) + end + + context "validation" do + before do + uploader.instance_variable_set(:@secret, secret) + end + + context "32-byte hexadecimal" do + let(:secret) { SecureRandom.hex } + + it "returns the secret" do + expect(uploader.secret).to eq(secret) + end + end + + context "10-byte hexadecimal" do + let(:secret) { SecureRandom.hex(10) } + + it "returns the secret" do + expect(uploader.secret).to eq(secret) + end + end + + context "invalid secret supplied" do + let(:secret) { "%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2Fgrafana%2Fconf%2F" } + + it "raises an exception" do + expect { uploader.secret }.to raise_error(described_class::InvalidSecret) + end + end end end diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb index ec652af222d..c211ec3607c 100644 --- a/spec/uploaders/personal_file_uploader_spec.rb +++ b/spec/uploaders/personal_file_uploader_spec.rb @@ -6,12 +6,13 @@ describe PersonalFileUploader do let(:model) { create(:personal_snippet) } let(:uploader) { described_class.new(model) } let(:upload) { create(:upload, :personal_snippet_upload) } + let(:secret) { SecureRandom.hex } subject { uploader } shared_examples '#base_dir' do before do - subject.instance_variable_set(:@secret, 'secret') + subject.instance_variable_set(:@secret, secret) end it 'is prefixed with uploads/-/system' do @@ -23,12 +24,12 @@ describe PersonalFileUploader do shared_examples '#to_h' do before do - subject.instance_variable_set(:@secret, 'secret') + subject.instance_variable_set(:@secret, secret) end it 'is correct' do allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name')) - expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name" + expected_url = "/uploads/-/system/personal_snippet/#{model.id}/#{secret}/file_name" expect(uploader.to_h).to eq( alt: 'file_name', |