diff --git a/app/controllers/letter_opener_web/letters_controller.rb b/app/controllers/letter_opener_web/letters_controller.rb index f5aa7c6..b23920c 100644 --- a/app/controllers/letter_opener_web/letters_controller.rb +++ b/app/controllers/letter_opener_web/letters_controller.rb @@ -48,7 +48,8 @@ def check_style def load_letter @letter = Letter.find(params[:id]) - head :not_found unless @letter.exists? + + head :not_found unless @letter.valid? end def routes diff --git a/app/models/letter_opener_web/letter.rb b/app/models/letter_opener_web/letter.rb index f0c343d..3072441 100644 --- a/app/models/letter_opener_web/letter.rb +++ b/app/models/letter_opener_web/letter.rb @@ -56,17 +56,19 @@ def attachments end def delete - FileUtils.rm_rf("#{LetterOpenerWeb.config.letters_location}/#{id}") + return unless valid? + + FileUtils.rm_rf(base_dir.to_s) end - def exists? - File.exist?(base_dir) + def valid? + exists? && base_dir_within_letters_location? end private def base_dir - "#{LetterOpenerWeb.config.letters_location}/#{id}" + LetterOpenerWeb.config.letters_location.join(id).cleanpath end def read_file(style) @@ -77,6 +79,14 @@ def style_exists?(style) File.exist?("#{base_dir}/#{style}.html") end + def exists? + File.exist?(base_dir) + end + + def base_dir_within_letters_location? + base_dir.to_s.start_with?(LetterOpenerWeb.config.letters_location.to_s) + end + def adjust_link_targets(contents) # We cannot feed the whole file to an XML parser as some mails are # "complete" (as in they have the whole structure) and letter_opener diff --git a/spec/controllers/letter_opener_web/letters_controller_spec.rb b/spec/controllers/letter_opener_web/letters_controller_spec.rb index 01ec7a8..553f21e 100644 --- a/spec/controllers/letter_opener_web/letters_controller_spec.rb +++ b/spec/controllers/letter_opener_web/letters_controller_spec.rb @@ -32,7 +32,7 @@ shared_examples 'found letter examples' do |letter_style| before(:each) do expect(LetterOpenerWeb::Letter).to receive(:find).with(id).and_return(letter) - expect(letter).to receive(:exists?).and_return(true) + expect(letter).to receive(:valid?).and_return(true) get :show, params: { id: id, style: letter_style } end @@ -84,7 +84,7 @@ before do allow(LetterOpenerWeb::Letter).to receive(:find).with(id).and_return(letter) - allow(letter).to receive(:exists?).and_return(true) + allow(letter).to receive(:valid?).and_return(true) end it 'sends the file as an inline attachment' do @@ -118,9 +118,20 @@ let(:id) { 'an-id' } it 'removes the selected letter' do - allow_any_instance_of(LetterOpenerWeb::Letter).to receive(:exists?).and_return(true) + allow_any_instance_of(LetterOpenerWeb::Letter).to receive(:valid?).and_return(true) expect_any_instance_of(LetterOpenerWeb::Letter).to receive(:delete) delete :destroy, params: { id: id } end + + it 'throws a 404 if attachment is outside of the letters base path' do + bad_id = '../an-id' + + allow_any_instance_of(LetterOpenerWeb::Letter).to receive(:valid?).and_return(false) + expect_any_instance_of(LetterOpenerWeb::Letter).not_to receive(:delete) + + delete :destroy, params: { id: bad_id } + + expect(response.status).to eq(404) + end end end diff --git a/spec/models/letter_opener_web/letter_spec.rb b/spec/models/letter_opener_web/letter_spec.rb index df9ade0..cbe318d 100644 --- a/spec/models/letter_opener_web/letter_spec.rb +++ b/spec/models/letter_opener_web/letter_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true describe LetterOpenerWeb::Letter do - let(:location) { File.expand_path('../../tmp', __dir__) } + let(:location) { Pathname.new(__dir__).join('..', '..', 'tmp').cleanpath } def rich_text(mail_id) <<-MAIL @@ -128,13 +128,24 @@ def rich_text(mail_id) describe '#delete' do let(:id) { '1111_1111' } + subject { described_class.new(id: id).delete } - it'removes the letter with given id' do + it 'removes the letter with given id' do subject directories = Dir["#{location}/*"] expect(directories.count).to eql(1) expect(directories.first).not_to match(id) end + + context 'when the id is outside of the letters base path' do + let(:id) { '../3333_3333' } + + it 'does not remove the letter' do + expect(FileUtils).not_to receive(:rm_rf).with(location.join(id).cleanpath.to_s) + + expect(subject).to be_nil + end + end end end