From 3e389808456e4a27d86419d3dab7738d6bc15aab Mon Sep 17 00:00:00 2001 From: Steven Harman Date: Fri, 1 Mar 2024 22:55:13 -0500 Subject: [PATCH 1/2] Add binstubs Moved dev/test dependencies into proper Bundler groups. I also added binstubs for rspec and rubocop - this ensures we're running the right versions of those tools locally. --- .github/workflows/main.yml | 6 +++--- .github/workflows/release-gem.yml | 2 +- .rubocop_todo.yml | 1 + Gemfile | 17 ++++++++++++----- bin/rake | 27 +++++++++++++++++++++++++++ bin/rspec | 27 +++++++++++++++++++++++++++ bin/rubocop | 27 +++++++++++++++++++++++++++ 7 files changed, 98 insertions(+), 9 deletions(-) create mode 100755 bin/rake create mode 100755 bin/rspec create mode 100755 bin/rubocop diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 1447c1e..560d20c 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -33,10 +33,10 @@ jobs: run: bundle install - name: Lint with Rubocop - run: bundle exec rubocop + run: bin/rubocop - name: Run tests - run: bundle exec rspec + run: bin/rspec - name: Build gem - run: bundle exec rake build + run: bin/rake build diff --git a/.github/workflows/release-gem.yml b/.github/workflows/release-gem.yml index 6fd8043..70f35f5 100644 --- a/.github/workflows/release-gem.yml +++ b/.github/workflows/release-gem.yml @@ -26,7 +26,7 @@ jobs: touch $HOME/.gem/credentials chmod 0600 $HOME/.gem/credentials printf -- "---\n:rubygems_api_key: ${GEM_HOST_API_KEY}\n" > $HOME/.gem/credentials - bundle exec rake build + bin/rake build gem push pkg/*.gem env: GEM_HOST_API_KEY: "${{secrets.RUBYGEMS_AUTH_TOKEN}}" diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 33fb012..934ae78 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -12,6 +12,7 @@ Style/Documentation: Exclude: - 'spec/**/*' - 'test/**/*' + - 'app/controllers/letter_opener_web/application_controller.rb' - 'app/controllers/letter_opener_web/letters_controller.rb' - 'app/controllers/letter_opener_web/application_controller.rb' - 'app/models/letter_opener_web/letter.rb' diff --git a/Gemfile b/Gemfile index 046f1e4..5a0f7eb 100644 --- a/Gemfile +++ b/Gemfile @@ -7,8 +7,15 @@ source 'http://rubygems.org' # development dependencies will be added by default to the :development group. gemspec -gem 'rails', '~> 6.1' -gem 'rspec-rails', '~> 5.0' -gem 'rubocop', '~> 1.22' -gem 'rubocop-rails', '~> 2.12' -gem 'rubocop-rspec', '~> 2.5' +group :development do + gem 'rails', '~> 6.1' + gem 'rspec-rails', '~> 5.0' + gem 'rubocop', '~> 1.22' + gem 'rubocop-rails', '~> 2.12' + gem 'rubocop-rspec', '~> 2.5' +end + +group :development, :test do + gem 'pry-byebug' + gem 'pry-rails' +end diff --git a/bin/rake b/bin/rake new file mode 100755 index 0000000..9646b91 --- /dev/null +++ b/bin/rake @@ -0,0 +1,27 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# +# This file was generated by Bundler. +# +# The application 'rake' is installed as part of a gem, and +# this file is here to facilitate running it. +# + +ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../Gemfile', __dir__) + +bundle_binstub = File.expand_path('bundle', __dir__) + +if File.file?(bundle_binstub) + if File.read(bundle_binstub, 300).include?('This file was generated by Bundler') + load(bundle_binstub) + else + abort("Your `bin/bundle` was not generated by Bundler, so this binstub cannot run. +Replace `bin/bundle` by running `bundle binstubs bundler --force`, then run this command again.") + end +end + +require 'rubygems' +require 'bundler/setup' + +load Gem.bin_path('rake', 'rake') diff --git a/bin/rspec b/bin/rspec new file mode 100755 index 0000000..b79c4d1 --- /dev/null +++ b/bin/rspec @@ -0,0 +1,27 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# +# This file was generated by Bundler. +# +# The application 'rspec' is installed as part of a gem, and +# this file is here to facilitate running it. +# + +ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../Gemfile', __dir__) + +bundle_binstub = File.expand_path('bundle', __dir__) + +if File.file?(bundle_binstub) + if File.read(bundle_binstub, 300).include?('This file was generated by Bundler') + load(bundle_binstub) + else + abort("Your `bin/bundle` was not generated by Bundler, so this binstub cannot run. +Replace `bin/bundle` by running `bundle binstubs bundler --force`, then run this command again.") + end +end + +require 'rubygems' +require 'bundler/setup' + +load Gem.bin_path('rspec-core', 'rspec') diff --git a/bin/rubocop b/bin/rubocop new file mode 100755 index 0000000..786dcf0 --- /dev/null +++ b/bin/rubocop @@ -0,0 +1,27 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# +# This file was generated by Bundler. +# +# The application 'rubocop' is installed as part of a gem, and +# this file is here to facilitate running it. +# + +ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../Gemfile', __dir__) + +bundle_binstub = File.expand_path('bundle', __dir__) + +if File.file?(bundle_binstub) + if File.read(bundle_binstub, 300).include?('This file was generated by Bundler') + load(bundle_binstub) + else + abort("Your `bin/bundle` was not generated by Bundler, so this binstub cannot run. +Replace `bin/bundle` by running `bundle binstubs bundler --force`, then run this command again.") + end +end + +require 'rubygems' +require 'bundler/setup' + +load Gem.bin_path('rubocop', 'rubocop') From af997990ab55812fae849c9c00b8cbddfccba547 Mon Sep 17 00:00:00 2001 From: Steven Harman Date: Fri, 1 Mar 2024 22:57:03 -0500 Subject: [PATCH 2/2] 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