Ensure letter is within letters base path #110
Conversation
| end | ||
|
|
||
| private | ||
|
|
||
| def base_dir | ||
| "#{LetterOpenerWeb.config.letters_location}/#{id}" | ||
| LetterOpenerWeb.config.letters_location.join(id).cleanpath |
There was a problem hiding this comment.
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.
| @@ -77,6 +79,14 @@ def style_exists?(style) | |||
| File.exist?("#{base_dir}/#{style}.html") | |||
| end | |||
|
|
|||
| def exists? | |||
| File.exist?(base_dir) | |||
There was a problem hiding this comment.
thought: As #base_dir is actually an instance of Pathname, we could also change this to:
| File.exist?(base_dir) | |
| base_dir.exist? |
But I wanted to keep the diff as small as possible.
| @@ -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 } | |||
There was a problem hiding this comment.
thought: LetterOpenerWeb.config.letters_location is actually an instance of Pathname so we can leverage its powers 🙂
| end | ||
|
|
||
| def base_dir_within_letters_location? | ||
| base_dir.to_s.start_with?(LetterOpenerWeb.config.letters_location.to_s) |
There was a problem hiding this comment.
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'.
|
@fgrehm for your review, please 🙂 🤞 |
|
Thank you! |
This PR ensures that when attempting to view or delete a letter, the provided
:idis within the letters base path.