General improvements for donations handling

app/controllers/donations_controller.rb

@@ -1,15 +1,17 @@
 class DonationsController < ApplicationController
   layout 'stripe'
+
   skip_forgery_protection only: [:callback]
   skip_before_action :set_globals, only: [:callback]
   skip_before_action :check_if_warning_or_suspension_pending, only: [:callback]
   skip_before_action :stop_the_awful_troll, only: [:callback]
   skip_before_action :distinguish_fake_community, only: [:callback]
 
+  before_action :set_referrer, only: [:intent, :success]
+
   def index; end
 
   def intent
-    @referrer = params[:return_to]
     currencies = ['GBP', 'USD', 'EUR']
     @currency = currencies.include?(params[:currency]) ? params[:currency] : 'GBP'
     @symbol = { 'GBP' => '£', 'USD' => '$', 'EUR' => '€' }[@currency]
@@ -22,28 +24,34 @@ def intent
     end
 
     if amount < 0.50
-      flash[:danger] = "Sorry, we can't accept amounts below #{symbol}0.50. We appreciate your generosity, but the " \
+      flash[:danger] = "Sorry, we can't accept amounts below #{@symbol}0.50. We appreciate your generosity, but the " \
                        'processing fees make it prohibitive.'
       redirect_to donate_path
       return
     end
 
     # amount * 100 because Stripe takes amounts in pence
     @amount = amount
-    @intent = Stripe::PaymentIntent.create({ amount: (amount * 100).to_i, currency: @currency,
-                                             metadata: { user_id: current_user&.id }, description: params[:desc] },
+    @intent = Stripe::PaymentIntent.create({ amount: (amount * 100).to_i,
+                                             currency: @currency,
+                                             metadata: { user_id: current_user&.id },
+                                             description: params[:desc] },
                                            { idempotency_key: params[:authenticity_token] })
   end
 
   def success
     @amount = params[:amount]
     @symbol = params[:currency]
-    @referrer = params[:return_to]
+
     Stripe::PaymentIntent.update(params[:intent], { metadata: { public_name: params[:public_name],
                                                                 public_comment: params[:public_comments] } })
-    DonationMailer.with(amount: @amount, currency: @symbol, email: params[:billing_email],
+
+    DonationMailer.with(amount: @amount,
+                        currency: @symbol,
+                        email: params[:billing_email],
                         name: current_user&.username || params[:billing_name])
-                  .donation_successful.deliver_now
+                  .donation_successful
+                  .deliver_now
   end
 
   def callback
@@ -54,36 +62,15 @@ def callback
     begin
       event = Stripe::Webhook.construct_event(payload, signature, secret)
     rescue JSON::ParserError
-      respond_to do |format|
-        format.json do
-          render status: 400, json: { error: 'Check yo JSON syntax. Fam.' }
-        end
-        format.any do
-          render status: 400, plain: 'Check yo JSON syntax. Fam.'
-        end
-      end
+      respond_to_invalid_json
       return
     rescue Stripe::SignatureVerificationError
-      respond_to do |format|
-        format.json do
-          render status: 400, json: { error: "You're not Stripe. Go away." }
-        end
-        format.any do
-          render status: 400, plain: "You're not Stripe. Go away."
-        end
-      end
+      respond_to_signature_error
       return
     end
 
     if event.nil?
-      respond_to do |format|
-        format.json do
-          render status: 500, json: { error: 'Webhook event not created. ???' }
-        end
-        format.any do
-          render status: 500, plain: 'Webhook event not created. ???'
-        end
-      end
+      respond_to_missing_event
       return
     end
 
@@ -92,16 +79,77 @@ def callback
     if StripeEventProcessor.respond_to?(method)
       begin
         result = StripeEventProcessor.send(method, object, event)
-        render status: 200, json: { status: 'Accepted for processing.', result: result }
+
+        render status: :success,
+               json: { status: 'Accepted for processing.', result: result }
       rescue Stripe::StripeError => e
         error_id = SecureRandom.uuid
-        ErrorLog.create(community: RequestContext.community, user: current_user, klass: e&.class,
-                        message: e&.message, backtrace: e&.backtrace&.join("\n"), request_uri: request.original_url,
-                        host: request.raw_host_with_port, uuid: error_id, user_agent: request.user_agent)
-        render status: 500, json: { error: "#{e&.class}: #{error_id} created." }
+
+        ErrorLog.create(community: RequestContext.community,
+                        user: current_user,
+                        klass: e&.class,
+                        message: e&.message,
+                        backtrace: e&.backtrace&.join("\n"),
+                        request_uri: request.original_url,
+                        host: request.raw_host_with_port,
+                        uuid: error_id,
+                        user_agent: request.user_agent)
+
+        render status: :internal_server_error,
+               json: { error: "#{e&.class}: #{error_id} created." }
       end
     else
-      render status: 202, json: { status: 'Accepted, not processed.' }
+      render status: :accepted,
+             json: { status: 'Accepted, not processed.' }
+    end
+  end
+
+  private
+
+  def respond_to_invalid_json
+    respond_to do |format|
+      format.json do
+        render status: :bad_request,
+               json: { error: 'Check your JSON syntax.' }
+      end
+      format.any do
+        render status: :bad_request,
+               plain: 'Check your JSON syntax.'
+      end
+    end
+  end
+
+  def respond_to_missing_event
+    respond_to do |format|
+      format.json do
+        render status: :internal_server_error,
+               json: { error: 'Webhook event not created.' }
+      end
+      format.any do
+        render status: :internal_server_error,
+               plain: 'Webhook event not created.'
+      end
     end
   end
+
+  def respond_to_signature_error
+    respond_to do |format|
+      format.json do
+        render status: :bad_request,
+               json: { error: "You're not Stripe. Go away." }
+      end
+      format.any do
+        render status: :bad_request,
+               plain: "You're not Stripe. Go away."
+      end
+    end
+  end
+
+  def set_referrer
+    return_to = params[:return_to]
+
+    return unless return_to.present? && helpers.safe_uri?(return_to)
+
+    @referrer = return_to
+  end
 end

app/helpers/uri_helper.rb

@@ -0,0 +1,24 @@
+module UriHelper
+  [:http, :https].each do |method|
+    # Does a given URI have the relevant scheme?
+    # @param {String} uri URI to check
+    # @return {Boolean} check result
+    define_method "#{method}_uri?" do |uri|
+      URI(uri).scheme.casecmp(method.to_s).zero?
+    end
+  end
+
+  # Is a given URI a relative one?
+  # @param {String} uri URI to check
+  # @return {Boolean} check result
+  def relative_uri?(uri)
+    URI(uri).relative?
+  end
+
+  # Is a given URI a safe one (absolute http://, https://, or relative)?
+  # @param {String} uri URI to check
+  # @return {Boolean} check result
+  def safe_uri?(uri)
+    relative_uri?(uri) || http_uri?(uri) || https_uri?(uri)
+  end
+end

test/controllers/donations_controller_test.rb

@@ -16,11 +16,71 @@ class DonationsControllerTest < ActionController::TestCase
     assert_response(:success)
   end
 
-  test 'should create PaymentIntent' do
+  test ':intent should safely handle referrer URIs' do
+    Stripe::PaymentIntent.stub(:create, nil) do
+      referrer_test_cases.each do |test_case|
+        get :intent, params: { return_to: test_case.first }
+        assert_equal test_case.second, assigns(:referrer).present?
+      end
+    end
+  end
+
+  test ':success should safely handle referrer URIs' do
+    Stripe::PaymentIntent.stub(:update, true) do
+      referrer_test_cases.each do |test_case|
+        get :success, params: {
+          billing_email: 'donator@example.com',
+          billing_name: 'donations_tester',
+          return_to: test_case.first
+        }
+        assert_equal test_case.second, assigns(:referrer).present?
+      end
+    end
+  end
+
+  test ':intent should correctly create PaymentIntent' do
     skip unless Stripe.api_key
     post :intent, params: { currency: 'EUR', amount: '24.99', desc: 'Created from Rails test' }
 
     assert_response(:success)
     assert_not_nil assigns(:intent)&.id
   end
+
+  test ':callback should correctly handle signature errors' do
+    @request.set_header('Stripe-Signature', 'invalid')
+
+    post :callback, params: { format: :json }
+
+    assert_response(:bad_request)
+    assert_nothing_raised do
+      parsed = JSON.parse(response.body)
+      assert_not_nil(parsed)
+      assert_equal "You're not Stripe. Go away.", parsed['error']
+    end
+  end
+
+  test ':callback should correctly handle missing events' do
+    Stripe::Webhook.stub(:construct_event, nil) do
+      post :callback, params: { format: :json }
+
+      assert_response(:internal_server_error)
+      assert_nothing_raised do
+        parsed = JSON.parse(response.body)
+        assert_not_nil(parsed)
+        assert_equal 'Webhook event not created.', parsed['error']
+      end
+    end
+  end
+
+  private
+
+  def referrer_test_cases
+    [
+      ['http://example.com/qa', true],
+      ['https://example.com/qa', true],
+      ['/relative_path', true],
+      ["javascript:alert('oops!')", false],
+      ['ftp://example.com/donwload', false]
+    ]
+  end
 end