From af997990ab55812fae849c9c00b8cbddfccba547 Mon Sep 17 00:00:00 2001
From: Steven Harman <steven@harmanly.com>
Date: Fri, 1 Mar 2024 22:57:03 -0500
Subject: [PATCH] Allow a dot (.) in the attachment file name

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.
---
 CHANGELOG.md                                  |  3 +-
 .../letter_opener_web/letters_controller.rb   |  2 +-
 config/routes.rb                              |  2 +-
 .../letters_controller_spec.rb                | 34 ++++++++++++++-----
 4 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 06c3a7a..8ae8411 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -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)
 
diff --git a/app/controllers/letter_opener_web/letters_controller.rb b/app/controllers/letter_opener_web/letters_controller.rb
index 7fa6ff3..a008664 100644
--- a/app/controllers/letter_opener_web/letters_controller.rb
+++ b/app/controllers/letter_opener_web/letters_controller.rb
@@ -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?
diff --git a/config/routes.rb b/config/routes.rb
index 275dbd8..6b6daff 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -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
diff --git a/spec/controllers/letter_opener_web/letters_controller_spec.rb b/spec/controllers/letter_opener_web/letters_controller_spec.rb
index ce0c772..cd96744 100644
--- a/spec/controllers/letter_opener_web/letters_controller_spec.rb
+++ b/spec/controllers/letter_opener_web/letters_controller_spec.rb
@@ -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