diff --git a/app/controllers/donations_controller.rb b/app/controllers/donations_controller.rb index ec63fd3e9..3f05c1d98 100644 --- a/app/controllers/donations_controller.rb +++ b/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,7 +24,7 @@ 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 @@ -30,20 +32,26 @@ def intent # 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 diff --git a/app/helpers/uri_helper.rb b/app/helpers/uri_helper.rb new file mode 100644 index 000000000..9792da174 --- /dev/null +++ b/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 diff --git a/test/controllers/donations_controller_test.rb b/test/controllers/donations_controller_test.rb index e675e2f26..93fc88f19 100644 --- a/test/controllers/donations_controller_test.rb +++ b/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