From af0caadb8d9781770399c1804976af4a71d1313b Mon Sep 17 00:00:00 2001 From: George Claghorn Date: Fri, 15 Dec 2017 19:26:53 -0500 Subject: Handle invalid signed blob IDs gracefully --- .../app/controllers/active_storage/blobs_controller.rb | 10 ++++------ .../controllers/active_storage/previews_controller.rb | 10 ++++------ .../controllers/active_storage/variants_controller.rb | 10 ++++------ .../app/controllers/concerns/active_storage/set_blob.rb | 16 ++++++++++++++++ activestorage/test/controllers/blobs_controller_test.rb | 5 +++++ .../test/controllers/previews_controller_test.rb | 9 +++++++++ .../test/controllers/variants_controller_test.rb | 9 +++++++++ 7 files changed, 51 insertions(+), 18 deletions(-) create mode 100644 activestorage/app/controllers/concerns/active_storage/set_blob.rb (limited to 'activestorage') diff --git a/activestorage/app/controllers/active_storage/blobs_controller.rb b/activestorage/app/controllers/active_storage/blobs_controller.rb index a17e3852f9..fa44131048 100644 --- a/activestorage/app/controllers/active_storage/blobs_controller.rb +++ b/activestorage/app/controllers/active_storage/blobs_controller.rb @@ -5,12 +5,10 @@ # security-through-obscurity factor of the signed blob references, you'll need to implement your own # authenticated redirection controller. class ActiveStorage::BlobsController < ActionController::Base + include ActiveStorage::SetBlob + def show - if blob = ActiveStorage::Blob.find_signed(params[:signed_id]) - expires_in ActiveStorage::Blob.service.url_expires_in - redirect_to blob.service_url(disposition: params[:disposition]) - else - head :not_found - end + expires_in ActiveStorage::Blob.service.url_expires_in + redirect_to @blob.service_url(disposition: params[:disposition]) end end diff --git a/activestorage/app/controllers/active_storage/previews_controller.rb b/activestorage/app/controllers/active_storage/previews_controller.rb index 9e8cf27b6e..aa7ef58ca4 100644 --- a/activestorage/app/controllers/active_storage/previews_controller.rb +++ b/activestorage/app/controllers/active_storage/previews_controller.rb @@ -1,12 +1,10 @@ # frozen_string_literal: true class ActiveStorage::PreviewsController < ActionController::Base + include ActiveStorage::SetBlob + def show - if blob = ActiveStorage::Blob.find_signed(params[:signed_blob_id]) - expires_in ActiveStorage::Blob.service.url_expires_in - redirect_to ActiveStorage::Preview.new(blob, params[:variation_key]).processed.service_url(disposition: params[:disposition]) - else - head :not_found - end + expires_in ActiveStorage::Blob.service.url_expires_in + redirect_to ActiveStorage::Preview.new(@blob, params[:variation_key]).processed.service_url(disposition: params[:disposition]) end end diff --git a/activestorage/app/controllers/active_storage/variants_controller.rb b/activestorage/app/controllers/active_storage/variants_controller.rb index dc5e78ecc0..e8f8dd592d 100644 --- a/activestorage/app/controllers/active_storage/variants_controller.rb +++ b/activestorage/app/controllers/active_storage/variants_controller.rb @@ -5,12 +5,10 @@ # security-through-obscurity factor of the signed blob and variation reference, you'll need to implement your own # authenticated redirection controller. class ActiveStorage::VariantsController < ActionController::Base + include ActiveStorage::SetBlob + def show - if blob = ActiveStorage::Blob.find_signed(params[:signed_blob_id]) - expires_in ActiveStorage::Blob.service.url_expires_in - redirect_to ActiveStorage::Variant.new(blob, params[:variation_key]).processed.service_url(disposition: params[:disposition]) - else - head :not_found - end + expires_in ActiveStorage::Blob.service.url_expires_in + redirect_to ActiveStorage::Variant.new(@blob, params[:variation_key]).processed.service_url(disposition: params[:disposition]) end end diff --git a/activestorage/app/controllers/concerns/active_storage/set_blob.rb b/activestorage/app/controllers/concerns/active_storage/set_blob.rb new file mode 100644 index 0000000000..b0f3d97a66 --- /dev/null +++ b/activestorage/app/controllers/concerns/active_storage/set_blob.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module ActiveStorage::SetBlob + extend ActiveSupport::Concern + + included do + before_action :set_blob + end + + private + def set_blob + @blob = ActiveStorage::Blob.find_signed(params[:signed_blob_id] || params[:signed_id]) + rescue ActiveSupport::MessageVerifier::InvalidSignature + head :not_found + end +end diff --git a/activestorage/test/controllers/blobs_controller_test.rb b/activestorage/test/controllers/blobs_controller_test.rb index 97177e64c2..9c811df895 100644 --- a/activestorage/test/controllers/blobs_controller_test.rb +++ b/activestorage/test/controllers/blobs_controller_test.rb @@ -8,6 +8,11 @@ class ActiveStorage::BlobsControllerTest < ActionDispatch::IntegrationTest @blob = create_file_blob filename: "racecar.jpg" end + test "showing blob with invalid signed ID" do + get rails_service_blob_url("invalid", "racecar.jpg") + assert_response :not_found + end + test "showing blob utilizes browser caching" do get rails_blob_url(@blob) diff --git a/activestorage/test/controllers/previews_controller_test.rb b/activestorage/test/controllers/previews_controller_test.rb index c3151a710e..704a466160 100644 --- a/activestorage/test/controllers/previews_controller_test.rb +++ b/activestorage/test/controllers/previews_controller_test.rb @@ -21,4 +21,13 @@ class ActiveStorage::PreviewsControllerTest < ActionDispatch::IntegrationTest assert_equal 77, image.width assert_equal 100, image.height end + + test "showing preview with invalid signed blob ID" do + get rails_blob_preview_url( + filename: @blob.filename, + signed_blob_id: "invalid", + variation_key: ActiveStorage::Variation.encode(resize: "100x100")) + + assert_response :not_found + end end diff --git a/activestorage/test/controllers/variants_controller_test.rb b/activestorage/test/controllers/variants_controller_test.rb index 6c70d73786..a0642f9bed 100644 --- a/activestorage/test/controllers/variants_controller_test.rb +++ b/activestorage/test/controllers/variants_controller_test.rb @@ -20,4 +20,13 @@ class ActiveStorage::VariantsControllerTest < ActionDispatch::IntegrationTest assert_equal 100, image.width assert_equal 67, image.height end + + test "showing variant with invalid signed blob ID" do + get rails_blob_variation_url( + filename: @blob.filename, + signed_blob_id: "invalid", + variation_key: ActiveStorage::Variation.encode(resize: "100x100")) + + assert_response :not_found + end end -- cgit v1.2.3