Merge pull request #110 from ashmckenzie/ashmckenzie/validate-file-path

Ensure letter is within letters base path
fgrehm · Oct 6, 2021 · ae7e362 · ae7e362
2 parents 968d1a0 + 817982a
commit ae7e362
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 10 deletions.
diff --git 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
@@ -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 <html> 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
@@ -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
@@ -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