Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure letter is within letters base path #110

Merged
merged 2 commits into from
Oct 6, 2021
Merged

Ensure letter is within letters base path #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
88cb8b2
Use base_dir to DRY things up
ashmckenzie Sep 29, 2021
817982a
Ensure letter is within letters base path
ashmckenzie Sep 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/controllers/letter_opener_web/letters_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 14 additions & 4 deletions app/models/letter_opener_web/letter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@ashmckenzie ashmckenzie Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: LetterOpenerWeb.config.letters_location is actually an instance of Pathname so we can leverage its powers 🙂.

#cleanpath is key here because it removes dots and slashes.

end

def read_file(style)
Expand All @@ -77,6 +79,14 @@ def style_exists?(style)
File.exist?("#{base_dir}/#{style}.html")
end

def exists?
File.exist?(base_dir)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: As #base_dir is actually an instance of Pathname, we could also change this to:

Suggested change
File.exist?(base_dir)
base_dir.exist?

But I wanted to keep the diff as small as possible.

end

def base_dir_within_letters_location?
base_dir.to_s.start_with?(LetterOpenerWeb.config.letters_location.to_s)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: This is the main change to ensure base_dir starts with LetterOpenerWeb.config.letters_location. It leans on #base_dir returning a 'clean path'.

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
Expand Down
17 changes: 14 additions & 3 deletions spec/controllers/letter_opener_web/letters_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
15 changes: 13 additions & 2 deletions spec/models/letter_opener_web/letter_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: LetterOpenerWeb.config.letters_location is actually an instance of Pathname so we can leverage its powers 🙂


def rich_text(mail_id)
<<-MAIL
Expand Down Expand Up @@ -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