Skip to content

Commit

Permalink
Allow a dot (.) in the attachment file name
Browse files Browse the repository at this point in the history
The default constraint on segments does not allow dot (.) characters,
so requests for attachments with a dot in the file name wouldn't find a
route, resulting in a 404 out of the Rails router.
  • Loading branch information
stevenharman committed May 13, 2024
1 parent 3e38980 commit af99799
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 11 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## [Unreleased](https://github.com/fgrehm/letter_opener_web/compare/v2.0.0...master)

- Reliably strip Attachment links from the sidebar. [#132](https://github.com/fgrehm/letter_opener_web/pull/132)
- Allow dot (`.`) character in attachment file names. [#131](https://github.com/fgrehm/letter_opener_web/pull/131)
- Reliably strip Attachment links from the sidebar. [#132](https://github.com/fgrehm/letter_opener_web/pull/134)

## [v2.0.0](https://github.com/fgrehm/letter_opener_web/compare/v1.4.1...v2.0.0)

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/letter_opener_web/letters_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def show
end

def attachment
filename = "#{params[:file]}.#{params[:format]}"
filename = params[:file]
file = @letter.attachments[filename]

return render plain: 'Attachment not found!', status: 404 unless file.present?
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
post 'clear' => 'letters#clear', as: :clear_letters
get ':id(/:style)' => 'letters#show', as: :letter
post ':id/delete' => 'letters#destroy', as: :delete_letter
get ':id/attachments/:file' => 'letters#attachment'
get ':id/attachments/:file' => 'letters#attachment', constraints: { file: %r{[^/]+} }
end
34 changes: 26 additions & 8 deletions spec/controllers/letter_opener_web/letters_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,17 +87,35 @@
allow(letter).to receive(:valid?).and_return(true)
end

it 'sends the file as an inline attachment' do
allow(controller).to receive(:send_file) { controller.head :ok }
get :attachment, params: { id: id, file: file_name.gsub(/\.\w+/, ''), format: File.extname(file_name)[1..] }
context 'when the file exists' do
before do
# Stub a 200 response b/c `send_file` will 204 when the file doesn't actually exist
allow(controller).to receive(:send_file) { controller.head :ok }
end

expect(response.status).to eq(200)
expect(controller).to have_received(:send_file)
.with(attachment_path, filename: file_name, disposition: 'inline')
it 'sends the file as an inline attachment' do
get :attachment, params: { id: id, file: file_name }

expect(response.status).to eq(200)
expect(controller).to have_received(:send_file)
.with(attachment_path, filename: file_name, disposition: 'inline')
end

context 'when file name includes a dot' do
let(:file_name) { 'image.dot.jpg' }

it 'sends the file as an inline attachment' do
expect(controller).to receive(:send_file).with(attachment_path, filename: file_name,
disposition: 'inline')
get :attachment, params: { id: id, file: file_name }
expect(response.status).to eq(200)
end
end
end

it "throws a 404 if attachment file can't be found" do
get :attachment, params: { id: id, file: 'unknown', format: 'woot' }
it "returns a 404 if attachment file can't be found" do
get :attachment, params: { id: id, file: 'unknown.woot' }

expect(response.status).to eq(404)
end
end
Expand Down

0 comments on commit af99799

Please sign in to comment.